Merge lp:~nick-dedekind/qtmir/polite-close into lp:qtmir

[Nick Dedekind]

Politely asks processes to close before resorting to killing

Politely close applications.

 * Are there any related MPs required for this MP to build/function as expected? Please list.
https://code.launchpad.net/~nick-dedekind/platform-api/lp1351109.destroy-haptic-instance/+merge/273190
https://code.launchpad.net/~nick-dedekind/qtubuntu-sensors/lp1351109.destroy-haptic-instance/+merge/272755

 * Did you perform an exploratory manual test run of your code change and any related functionality?
Yes

 * If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?
N/A

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:340
http://jenkins.qa.ubuntu.com/job/qtmir-ci/282/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/15
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/15
        deb: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/15/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtmir-ci/282/rebuild

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:341
http://jenkins.qa.ubuntu.com/job/qtmir-ci/283/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/16
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/16
        deb: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/16/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtmir-ci/283/rebuild

review: Approve (continuous-integration)

[Gerry Boland (gerboland)]

Can I get you to rebase this on top of
https://code.launchpad.net/~dandrader/qtmir/detach-state-from-focus/+merge/258648
as I expect collisions. Daniel re-worked the Application class to more clearly manage process state, which this branch would benefit from.

Something to keep in mind is we may be treating applications which don't close when we ask them differently on desktop & mobile platforms. It's not uncommon for desktop apps to pop up a "Save" dialog to prevent user loosing their work. For mobile, we may have this issue too, though I've no idea how we'll present it to the user.

As a result I think the "kill after 3 seconds" logic should belong to unity8.

review: Needs Fixing

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:348
http://jenkins.qa.ubuntu.com/job/qtmir-ci/292/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/25/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/25/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtmir-ci/292/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:348
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~nick-dedekind/qtmir/polite-close/+merge/262188/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/qtmir-ci/295/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/28/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/28/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtmir-ci/295/rebuild

review: Needs Fixing (continuous-integration)

[Nick Dedekind (nick-dedekind)]

Logic for how a close request reacts (popup for confirm/error notification for hard exit/etc) will be dealt with by subsequent sprint cards. This is just a only a first pass.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:353
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~nick-dedekind/qtmir/polite-close/+merge/262188/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/qtmir-ci/299/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/32/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/32/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtmir-ci/299/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:354
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~nick-dedekind/qtmir/polite-close/+merge/262188/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/qtmir-ci/300/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/33/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/33/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtmir-ci/300/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:354
http://jenkins.qa.ubuntu.com/job/qtmir-ci/303/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/36/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/36/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtmir-ci/303/rebuild

review: Needs Fixing (continuous-integration)

[lp:~nick-dedekind/qtmir/polite-close]

346. By Daniel d'Andrada <dandrader@panzer> on 2015-06-30

merge detach-state-from-focus

347. By Daniel d'Andrada <dandrader@panzer> on 2015-06-30

Merge lp:~nick-dedekind/qtmir/polite-close

348. By Daniel d'Andrada <dandrader@panzer> on 2015-07-01

Modifications from review

349. By Daniel d'Andrada <dandrader@panzer> on 2015-07-01

Format is EXPECT_EQ(expected, actual)

350. By Daniel d'Andrada <dandrader@panzer> on 2015-07-01

Test change that makes it hang

[Daniel d'Andrada (dandrader)]

I think you're missing a clear definition of the Closing state and of the transitions from and to it. In the code I see it can be in the Closing state even if the process is suspended. If so, you resume it. This leads to uncertainty in the code as it can be in Closing but you don't know if it's suspended or running. This compromises the idea of tracking the internal state through this variable as it no longer clearly reflects what's going on.

In my opinion, an app is in Closing state *only* when:
 - Its process is Running (or Unknown, which amounts to the same)
 - The closing timer is ticking

So if someone calls Application::close() while the app is Suspended. You should first resume() it and then set to Closing. Think of the state transitions. Suspended -> Running -> Closing. It can only enter Closing from Running. Not straight from Suspended.

The related code changes for this and some other small issues are here:
http://bazaar.launchpad.net/~dandrader/qtmir/polite-close/revision/348

review: Needs Fixing

[Daniel d'Andrada (dandrader)]

Format is EXPECT_EQ(expected, actual), not the other way around (yeah, it's the opposite of most other test APIs).

http://bazaar.launchpad.net/~dandrader/qtmir/polite-close/revision/349

review: Needs Fixing

[Daniel d'Andrada (dandrader)]

ApplicationManagerTests.applicationStartQueuedOnStartStopStart is stopping the app while is still in Starting state. The more common case, which is what we should be testing, is closing an app in Running state. I changed the test accordingly:

http://bazaar.launchpad.net/~dandrader/qtmir/polite-close/revision/350

It needs more work though, as you will have to mock/fake more stuff to make this code path function properly, like adding some fake implementation to the TaskController. so that AppMan calling TaskController->stopProcess() causes the app session to end and then TaskController::processStopped to get emited.

[Daniel d'Andrada (dandrader)]

1 - Launch some app (e.g. filemanager)
2 - $ sudo gdb -p $(pidof filemanager)
3 - (gdb) cont
4 - do right-edge drag to show the spread and swipe up filemanager to close it.

expected outcome:
You see in gdb that app exits nicely

Actual outcome:
You get: "Program received signal SIGSEGV, Segmentation fault."

Comments:
Without this branch I get "Program received signal SIGTERM, Terminated." instead.

It might not be our fault be we should at least know why it's crashing.

review: Needs Fixing

[lp:~nick-dedekind/qtmir/polite-close]

351. By Nick Dedekind on 2015-09-07

merged with trunk

352. By Nick Dedekind on 2015-09-07

fixed up a few resume on closing things

353. By Nick Dedekind on 2015-09-07

fixed up some logs

[Nick Dedekind (nick-dedekind)]

> 1 - Launch some app (e.g. filemanager)
> 2 - $ sudo gdb -p $(pidof filemanager)
> 3 - (gdb) cont
> 4 - do right-edge drag to show the spread and swipe up filemanager to close
> it.
>
> expected outcome:
> You see in gdb that app exits nicely
>
> Actual outcome:
> You get: "Program received signal SIGSEGV, Segmentation fault."
>
> Comments:
> Without this branch I get "Program received signal SIGTERM, Terminated."
> instead.
>
> It might not be our fault be we should at least know why it's crashing.

This can be dealt with by another branch. the problem occurs in trunk as well.
tested with messaging-app (crash), dialer-app (crash), camera-app (no crash)

[lp:~nick-dedekind/qtmir/polite-close]

354. By Nick Dedekind on 2015-09-07

fixed close for sync

355. By Nick Dedekind on 2015-09-07

updated tests

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:355
http://jenkins.qa.ubuntu.com/job/qtmir-ci/400/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/133/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/133/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtmir-ci/400/rebuild

review: Needs Fixing (continuous-integration)

[Nick Dedekind (nick-dedekind)]

> ApplicationManagerTests.applicationStartQueuedOnStartStopStart is stopping the
> app while is still in Starting state. The more common case, which is what we
> should be testing, is closing an app in Running state. I changed the test
> accordingly:
>
> http://bazaar.launchpad.net/~dandrader/qtmir/polite-close/revision/350
>
> It needs more work though, as you will have to mock/fake more stuff to make
> this code path function properly, like adding some fake implementation to the
> TaskController. so that AppMan calling TaskController->stopProcess() causes
> the app session to end and then TaskController::processStopped to get emited.

Thanks for the updates. I've merged in your work.
I've just called the appManager->onProcessStopped directly for now to get the cleanup working. Mocking the TaskController is quite a big job as most of the app manager tests rely the production task controller with a mocked app controller.

I've got another branch in the works for mocking the task controller. Can be merged separately.

[Nick Dedekind (nick-dedekind)]

> 1 - Launch some app (e.g. filemanager)
> 2 - $ sudo gdb -p $(pidof filemanager)
> 3 - (gdb) cont
> 4 - do right-edge drag to show the spread and swipe up filemanager to close
> it.
>
> expected outcome:
> You see in gdb that app exits nicely
>
> Actual outcome:
> You get: "Program received signal SIGSEGV, Segmentation fault."
>
> Comments:
> Without this branch I get "Program received signal SIGTERM, Terminated."
> instead.
>
> It might not be our fault be we should at least know why it's crashing.

I'll look into it when I get a minute. Seems to be crashing in libdbus-cpp.

[Daniel d'Andrada (dandrader)]

Code looks good. Didn't try it out on a device yet.

Two problems in the tests though:

1 - ApplicationManagerTests.shellStopsAppCorrectlyBeforeSurfaceCreated is failing:
http://paste.ubuntu.com/12418245/

2 - ApplicationManagerTests::failedApplicationCloseEventualyDeletesApplication test description (the comment above it) doesn't match with what the test does. Probably a copy-and-paste left over.

review: Needs Fixing

[Daniel d'Andrada (dandrader)]

> > 1 - Launch some app (e.g. filemanager)
> > 2 - $ sudo gdb -p $(pidof filemanager)
> > 3 - (gdb) cont
> > 4 - do right-edge drag to show the spread and swipe up filemanager to close
> > it.
> >
> > expected outcome:
> > You see in gdb that app exits nicely
> >
> > Actual outcome:
> > You get: "Program received signal SIGSEGV, Segmentation fault."
> >
> > Comments:
> > Without this branch I get "Program received signal SIGTERM, Terminated."
> > instead.
> >
> > It might not be our fault be we should at least know why it's crashing.
>
> This can be dealt with by another branch. the problem occurs in trunk as well.
> tested with messaging-app (crash), dialer-app (crash), camera-app (no crash)

Just tried three times with both messaging-app and dialer-app and didn't get any SIGSEGV with qtmir trunk.

[Gerry Boland (gerboland)]

> > 1 - Launch some app (e.g. filemanager)
> > 2 - $ sudo gdb -p $(pidof filemanager)
> > 3 - (gdb) cont
> > 4 - do right-edge drag to show the spread and swipe up filemanager to close
> > it.
> >
> > expected outcome:
> > You see in gdb that app exits nicely
> >
> > Actual outcome:
> > You get: "Program received signal SIGSEGV, Segmentation fault."
> >
> > Comments:
> > Without this branch I get "Program received signal SIGTERM, Terminated."
> > instead.
> >
> > It might not be our fault be we should at least know why it's crashing.
>
> This can be dealt with by another branch. the problem occurs in trunk as well.
> tested with messaging-app (crash), dialer-app (crash), camera-app (no crash)

Have you this branch prepared? We can't land this change if all apps will now crash on shutdown instead of being killed. It will cause apport to spin up for every app closed!

[lp:~nick-dedekind/qtmir/polite-close]

356. By Nick Dedekind on 2015-09-24

merged with trunk

357. By Nick Dedekind on 2015-09-25

version bump

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:357
http://jenkins.qa.ubuntu.com/job/qtmir-ci/437/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-amd64-ci/133/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/133/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-i386-ci/15/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/170
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/170
        deb: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/170/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-wily-i386-ci/15

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtmir-ci/437/rebuild

review: Needs Fixing (continuous-integration)

[Gerry Boland (gerboland)]

Nick found that this bug is causing the crash in clients when trying to shut down cleanly:
https://bugs.launchpad.net/platform-api/+bug/1351109

[Nick Dedekind (nick-dedekind)]

And fixed it here:
https://code.launchpad.net/~nick-dedekind/platform-api/lp1351109.destroy-haptic-instance
https://code.launchpad.net/~nick-dedekind/qtubuntu-sensors/lp1351109.destroy-haptic-instance

[lp:~nick-dedekind/qtmir/polite-close]

358. By Nick Dedekind on 2015-09-29

updated comment

359. By Nick Dedekind on 2015-09-29

merged with trunk

[Daniel d'Andrada (dandrader)]

I confirm that those branches do fix the crash.

Now the only issue left is that mysterious test failure...

[Daniel d'Andrada (dandrader)]

Well, Saviq and CI have the test passing fine as well.

review: Approve

[Gerry Boland (gerboland)]

Please strip tags! It's a qtmir problem now to

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:359
http://jenkins.qa.ubuntu.com/job/qtmir-ci/452/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-amd64-ci/148/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/148
        deb: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/148/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-i386-ci/30
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/185
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/185
        deb: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/185/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-wily-i386-ci/30

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtmir-ci/452/rebuild

review: Needs Fixing (continuous-integration)

[Gerry Boland (gerboland)]

Please merge qtmir trunk

[lp:~nick-dedekind/qtmir/polite-close]

360. By Nick Dedekind on 2015-11-02

merged with trunk

361. By Nick Dedekind on 2015-11-02

removed unnecessary cmake includes

362. By Nick Dedekind on 2015-11-09

merge with trunk

[Nick Dedekind (nick-dedekind)]

Fixe conflicts.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:364
http://jenkins.qa.ubuntu.com/job/qtmir-ci/596/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-amd64-ci/292/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/292/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-i386-ci/174/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/329/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/329/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-i386-ci/174/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtmir-ci/596/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:366
http://jenkins.qa.ubuntu.com/job/qtmir-ci/600/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-amd64-ci/296
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/296
        deb: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/296/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-i386-ci/178
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/333/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/333/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-i386-ci/178/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtmir-ci/600/rebuild

review: Needs Fixing (continuous-integration)

[lp:~nick-dedekind/qtmir/polite-close]

363. By Nick Dedekind on 2015-12-07

merged with trunk

=== modified file 'debian/changelog'
--- debian/changelog	2015-12-01 14:14:00 +0000
+++ debian/changelog	2015-12-07 10:51:38 +0000
@@ -1,3 +1,10 @@
+qtmir (0.4.7) UNRELEASED; urgency=medium
+
+  * [ Nick Dedekind ]
+  * Request app closing rather than killing.
+
+ -- Nick Dedekind <[e-mail]>  Fri, 25 Sep 2015 09:33:39 +0100
+
 qtmir (0.4.6+16.04.20151125-0ubuntu2) xenial; urgency=medium
 
   * Rebuild against Qt 5.5.1.

=== modified file 'src/modules/Unity/Application/application.cpp'
--- src/modules/Unity/Application/application.cpp	2015-10-01 17:20:40 +0000
+++ src/modules/Unity/Application/application.cpp	2015-12-07 10:51:38 +0000
@@ -45,13 +45,14 @@
     , m_sharedWakelock(sharedWakelock)
     , m_desktopData(desktopFileReader)
     , m_pid(0)
-    , m_stage((m_desktopData->stageHint() == "SideStage") ? Application::SideStage : Application::MainStage)
+    , m_stage((desktopFileReader->stageHint() == "SideStage") ? Application::SideStage : Application::MainStage)
     , m_state(InternalState::Starting)
     , m_focused(false)
     , m_arguments(arguments)
     , m_session(nullptr)
     , m_requestedState(RequestedRunning)
     , m_processState(ProcessUnknown)
+    , m_closeTimer(0)
 {
     qCDebug(QTMIR_APPLICATIONS) << "Application::Application - appId=" << desktopFileReader->appId();
 
@@ -394,25 +395,22 @@
     return m_pid;
 }
 
-void Application::setPid(pid_t pid)
-{
-    m_pid = pid;
-}
-
 void Application::close()
 {
     qCDebug(QTMIR_APPLICATIONS) << "Application::close - appId=" << appId();
 
     switch (m_state) {
     case InternalState::Starting:
+        stop();
+        break;
     case InternalState::Running:
-        setInternalState(InternalState::Closing);
+        doClose();
         break;
     case InternalState::SuspendingWaitSession:
     case InternalState::SuspendingWaitProcess:
     case InternalState::Suspended:
         setRequestedState(RequestedRunning);
-        setInternalState(InternalState::Closing);
+        doClose();
         break;
     case InternalState::Closing:
         // already on the way
@@ -422,6 +420,22 @@
         // too late
         break;
     }
+
+}
+
+void Application::doClose()
+{
+    Q_ASSERT(m_closeTimer == 0);
+    Q_ASSERT(m_session != nullptr);
+
+    m_session->close();
+    m_closeTimer = startTimer(3000);
+    setInternalState(InternalState::Closing);
+}
+
+void Application::setPid(pid_t pid)
+{
+    m_pid = pid;
 }
 
 void Application::setArguments(const QStringList arguments)
@@ -603,6 +617,8 @@
 
 void Application::suspend()
 {
+    qCDebug(QTMIR_APPLICATIONS) << "Application::suspend - appId=" << appId();
+
     Q_ASSERT(m_state == InternalState::Running);
     Q_ASSERT(m_session != nullptr);
 
@@ -612,6 +628,8 @@
 
 void Application::resume()
 {
+    qCDebug(QTMIR_APPLICATIONS) << "Application::resume - appId=" << appId();
+
     if (m_state == InternalState::Suspended) {
         setInternalState(InternalState::Running);
         Q_EMIT resumeProcessRequested();
@@ -634,6 +652,21 @@
     Q_EMIT startProcessRequested();
 }
 
+void Application::stop()
+{
+    qCDebug(QTMIR_APPLICATIONS) << "Application::stop - appId=" << appId();
+
+    Q_EMIT stopProcessRequested();
+}
+
+void Application::timerEvent(QTimerEvent *event)
+{
+    if (event->timerId() == m_closeTimer) {
+        m_closeTimer = 0;
+        stop();
+    }
+}
+
 bool Application::isTouchApp() const
 {
     return m_desktopData->isTouchApp();
@@ -704,7 +737,7 @@
              *  3. application is managed by upstart and is in foreground (i.e. has
              *     Running state), if Mir reports the application disconnects, it
              *     either crashed or stopped itself.
-             * 4. We're expecting the application to stop after a close request
+             *  4. We're expecting the application to stop after a close request
              */
             setInternalState(InternalState::Stopped);
         } else {

=== modified file 'src/modules/Unity/Application/application.h'
--- src/modules/Unity/Application/application.h	2015-11-17 14:16:22 +0000
+++ src/modules/Unity/Application/application.h	2015-12-07 10:51:38 +0000
@@ -137,6 +137,7 @@
     void sessionChanged(SessionInterface *session);
 
     void startProcessRequested();
+    void stopProcessRequested();
     void suspendProcessRequested();
     void resumeProcessRequested();
     void stopped();
@@ -146,6 +147,9 @@
 
     void respawn();
 
+protected:
+    void timerEvent(QTimerEvent *event);
+
 private:
 
     QString longAppId() const;
@@ -158,11 +162,13 @@
     void wipeQMLCache();
     void suspend();
     void resume();
+    void stop();
     QColor colorFromString(const QString &colorString, const char *colorName) const;
     static const char* internalStateToStr(InternalState state);
     void applyRequestedState();
     void applyRequestedRunning();
     void applyRequestedSuspended();
+    void doClose();
 
     QSharedPointer<SharedWakelock> m_sharedWakelock;
     DesktopFileReader* m_desktopData;
@@ -178,6 +184,7 @@
     SessionInterface *m_session;
     RequestedState m_requestedState;
     ProcessState m_processState;
+    int m_closeTimer;
 
     friend class ApplicationManager;
     friend class SessionManager;

=== modified file 'src/modules/Unity/Application/application_manager.cpp'
--- src/modules/Unity/Application/application_manager.cpp	2015-11-17 14:16:22 +0000
+++ src/modules/Unity/Application/application_manager.cpp	2015-12-07 10:51:38 +0000
@@ -353,6 +353,23 @@
         return nullptr;
     }
 
+    if (m_queuedStartApplications.contains(inputAppId)) {
+        qWarning() << "ApplicationManager::startApplication - application appId=" << appId << " is queued to start";
+        return nullptr;
+    } else {
+        application = findClosingApplication(inputAppId);
+        if (application) {
+            m_queuedStartApplications.append(inputAppId);
+            qWarning() << "ApplicationManager::startApplication - application appId=" << appId << " is closing. Queuing start";
+            connect(application, &QObject::destroyed, this, [this, application, inputAppId, flags, arguments]() {
+                m_queuedStartApplications.removeAll(inputAppId);
+                // start the app.
+                startApplication(inputAppId, flags, arguments);
+            }, Qt::QueuedConnection); // Queued so that we finish the app removal before starting again.
+            return nullptr;
+        }
+    }
+
     if (!m_taskController->start(appId, arguments)) {
         qWarning() << "Upstart failed to start application with appId" << appId;
         return nullptr;
@@ -438,17 +455,12 @@
     application->close();
     remove(application);
 
-    bool result = m_taskController->stop(application->longAppId());
-
-    if (!result && application->pid() > 0) {
-        qWarning() << "FAILED to ask Upstart to stop application with appId" << appId
-                   << "Sending SIGTERM to process:" << application->pid();
-        kill(application->pid(), SIGTERM);
-        result = true;
-    }
-
-    delete application;
-    return result;
+    connect(application, &QObject::destroyed, this, [this, application](QObject*) {
+        m_closingApplications.removeAll(application);
+    });
+    m_closingApplications.append(application);
+    application->close();
+    return true;
 }
 
 void ApplicationManager::onProcessFailed(const QString &appId, const bool duringStartup)
@@ -473,7 +485,11 @@
 {
     tracepoint(qtmir, onProcessStopped);
     qCDebug(QTMIR_APPLICATIONS) << "ApplicationManager::onProcessStopped - appId=" << appId;
+
     Application *application = findApplication(appId);
+    if (!application) {
+        application = findClosingApplication(appId);
+    }
 
     if (!application) {
         qDebug() << "ApplicationManager::onProcessStopped reports stop of appId=" << appId
@@ -729,6 +745,15 @@
             this, [=]() { m_taskController->start(appId, arguments); },
             Qt::QueuedConnection);
 
+    connect(application, &Application::stopProcessRequested, this, [=]() {
+        if (!m_taskController->stop(application->longAppId()) && application->pid() > 0) {
+            qWarning() << "FAILED to ask Upstart to stop application with appId" << appId
+                       << "Sending SIGTERM to process:" << appId;
+            kill(application->pid(), SIGTERM);
+            application->setProcessState(Application::ProcessStopped);
+        }
+    });
+
     connect(application, &Application::suspendProcessRequested, this, [=]() { m_taskController->suspend(longAppId); } );
     connect(application, &Application::resumeProcessRequested, this, [=]() { m_taskController->resume(longAppId); } );
 
@@ -753,7 +778,10 @@
     Q_ASSERT(application != nullptr);
     qCDebug(QTMIR_APPLICATIONS) << "ApplicationManager::remove - appId=" << application->appId();
 
-    application->disconnect(this);
+    disconnect(application, &Application::fullscreenChanged, this, 0);
+    disconnect(application, &Application::focusedChanged, this, 0);
+    disconnect(application, &Application::stateChanged, this, 0);
+    disconnect(application, &Application::stageChanged, this, 0);
 
     int i = m_applications.indexOf(application);
     if (i != -1) {
@@ -813,4 +841,16 @@
     return result;
 }
 
+Application *ApplicationManager::findClosingApplication(const QString &inputAppId) const
+{
+    const QString appId = toShortAppIdIfPossible(inputAppId);
+
+    for (Application *app : m_closingApplications) {
+        if (app->appId() == appId) {
+            return app;
+        }
+    }
+    return nullptr;
+}
+
 } // namespace qtmir

=== modified file 'src/modules/Unity/Application/application_manager.h'
--- src/modules/Unity/Application/application_manager.h	2015-11-17 14:16:22 +0000
+++ src/modules/Unity/Application/application_manager.h	2015-12-07 10:51:38 +0000
@@ -149,6 +149,7 @@
     QString toString() const;
 
     Application* findApplicationWithPromptSession(const mir::scene::PromptSession* promptSession);
+    Application *findClosingApplication(const QString &inputAppId) const;
 
     QSharedPointer<MirServer> m_mirServer;
 
@@ -160,6 +161,8 @@
     QSharedPointer<ProcInfo> m_procInfo;
     QSharedPointer<SharedWakelock> m_sharedWakelock;
     QSharedPointer<SettingsInterface> m_settings;
+    QList<Application*> m_closingApplications;
+    QList<QString> m_queuedStartApplications;
     static ApplicationManager* the_application_manager;
 
     friend class Application;

=== modified file 'src/modules/Unity/Application/mirsurface.cpp'
--- src/modules/Unity/Application/mirsurface.cpp	2015-11-25 15:38:39 +0000
+++ src/modules/Unity/Application/mirsurface.cpp	2015-12-07 10:51:38 +0000
@@ -420,6 +420,13 @@
     }
 }
 
+void MirSurface::close()
+{
+    if (m_surface) {
+        m_surface->request_client_surface_close();
+    }
+}
+
 void MirSurface::resize(int width, int height)
 {
     int mirWidth = m_surface->size().width.as_int();

=== modified file 'src/modules/Unity/Application/mirsurface.h'
--- src/modules/Unity/Application/mirsurface.h	2015-11-25 15:38:39 +0000
+++ src/modules/Unity/Application/mirsurface.h	2015-12-07 10:51:38 +0000
@@ -99,6 +99,8 @@
 
     void setFocus(bool focus) override;
 
+    void close() override;
+
     void mousePressEvent(QMouseEvent *event) override;
     void mouseMoveEvent(QMouseEvent *event) override;
     void mouseReleaseEvent(QMouseEvent *event) override;

=== modified file 'src/modules/Unity/Application/mirsurfaceinterface.h'
--- src/modules/Unity/Application/mirsurfaceinterface.h	2015-11-25 15:38:39 +0000
+++ src/modules/Unity/Application/mirsurfaceinterface.h	2015-12-07 10:51:38 +0000
@@ -64,6 +64,8 @@
 
     virtual void setFocus(bool focus) = 0;
 
+    virtual void close() = 0;
+
     virtual void mousePressEvent(QMouseEvent *event) = 0;
     virtual void mouseMoveEvent(QMouseEvent *event) = 0;
     virtual void mouseReleaseEvent(QMouseEvent *event) = 0;

=== modified file 'src/modules/Unity/Application/session.cpp'
--- src/modules/Unity/Application/session.cpp	2015-08-31 09:51:28 +0000
+++ src/modules/Unity/Application/session.cpp	2015-12-07 10:51:38 +0000
@@ -40,6 +40,28 @@
 namespace qtmir
 {
 
+namespace {
+
+const char *sessionStateToString(SessionInterface::State state)
+{
+    switch (state) {
+    case SessionInterface::Starting:
+        return "starting";
+    case SessionInterface::Running:
+        return "running";
+    case SessionInterface::Suspending:
+        return "suspending";
+    case SessionInterface::Suspended:
+        return "suspended";
+    case SessionInterface::Stopped:
+        return "stopped";
+    default:
+        return "???";
+    }
+}
+
+}
+
 Session::Session(const std::shared_ptr<ms::Session>& session,
                  const std::shared_ptr<ms::PromptSessionManager>& promptSessionManager,
                  QObject *parent)
@@ -142,6 +164,9 @@
 
 void Session::setState(State state) {
     if (state != m_state) {
+        qCDebug(QTMIR_SESSIONS) << "Session::setState - session=" << name()
+            << "state=" << sessionStateToString(state);
+
         m_state = state;
         Q_EMIT stateChanged(m_state);
     }
@@ -232,7 +257,7 @@
 
 void Session::suspend()
 {
-    qCDebug(QTMIR_SESSIONS) << "Session::suspend - session=" << this << "state=" << applicationStateToStr(m_state);
+    qCDebug(QTMIR_SESSIONS) << "Session::suspend - session=" << this << "state=" << sessionStateToString(m_state);
     if (m_state == Running) {
         session()->set_lifecycle_state(mir_lifecycle_state_will_suspend);
         m_suspendTimer->start(1500);
@@ -251,7 +276,7 @@
 
 void Session::resume()
 {
-    qCDebug(QTMIR_SESSIONS) << "Session::resume - session=" << this << "state=" << applicationStateToStr(m_state);
+    qCDebug(QTMIR_SESSIONS) << "Session::resume - session=" << this << "state=" << sessionStateToString(m_state);
 
     if (m_state == Suspending || m_state == Suspended) {
         doResume();
@@ -281,9 +306,20 @@
     setState(Running);
 }
 
+void Session::close()
+{
+    qCDebug(QTMIR_SESSIONS) << "Session::close - " << name() << m_surface;
+    if (m_surface) {
+        m_surface->close();
+    }
+}
+
 void Session::stop()
 {
+    qCDebug(QTMIR_SESSIONS) << "Session::stop - " << name();
+
     if (m_state != Stopped) {
+
         stopPromptSessions();
         if (m_suspendTimer->isActive())
             m_suspendTimer->stop();
@@ -304,6 +340,8 @@
 void Session::setLive(const bool live)
 {
     if (m_live != live) {
+        qCDebug(QTMIR_SESSIONS) << "Session::setLive - " << name() << "live=" << live;
+
         m_live = live;
         Q_EMIT liveChanged(m_live);
         if (!live) {

=== modified file 'src/modules/Unity/Application/session.h'
--- src/modules/Unity/Application/session.h	2015-08-31 09:51:28 +0000
+++ src/modules/Unity/Application/session.h	2015-12-07 10:51:38 +0000
@@ -63,6 +63,7 @@
 
     void suspend() override;
     void resume() override;
+    void close() override;
     void stop() override;
 
     void addChildSession(SessionInterface* session) override;

=== modified file 'src/modules/Unity/Application/session_interface.h'
--- src/modules/Unity/Application/session_interface.h	2015-08-31 09:51:28 +0000
+++ src/modules/Unity/Application/session_interface.h	2015-12-07 10:51:38 +0000
@@ -80,6 +80,7 @@
     virtual void setApplication(unity::shell::application::ApplicationInfoInterface* item) = 0;
     virtual void suspend() = 0;
     virtual void resume() = 0;
+    virtual void close() = 0;
     virtual void stop() = 0;
 
     // For SessionManager use

=== modified file 'tests/modules/Application/CMakeLists.txt'
--- tests/modules/Application/CMakeLists.txt	2015-08-19 20:17:56 +0000
+++ tests/modules/Application/CMakeLists.txt	2015-12-07 10:51:38 +0000
@@ -11,6 +11,11 @@
   ${CMAKE_SOURCE_DIR}/src/modules
   ${CMAKE_SOURCE_DIR}/tests/modules/common
   ${MIRSERVER_INCLUDE_DIRS}
+
+  ${Qt5Core_INCLUDE_DIRS}
+  ${Qt5GUI_INCLUDE_DIRS}
+  ${Qt5Quick_INCLUDE_DIRS}
+  ${Qt5DBus_INCLUDE_DIRS}
 )
 
 add_executable(application_test ${APPLICATION_TEST_SOURCES})

=== modified file 'tests/modules/ApplicationManager/application_manager_test.cpp'
--- tests/modules/ApplicationManager/application_manager_test.cpp	2015-11-17 14:16:22 +0000
+++ tests/modules/ApplicationManager/application_manager_test.cpp	2015-12-07 10:51:38 +0000
@@ -1870,3 +1870,158 @@
     EXPECT_EQ(0, applicationManager.count());
     EXPECT_TRUE(dir.exists());
 }
+
+/*
+ * Test that there is an attempt at polite exiting of the app by requesting closure of the surface.
+ */
+TEST_F(ApplicationManagerTests,requestSurfaceCloseOnStop)
+{
+    using namespace ::testing;
+
+    const QString appId("testAppId");
+    quint64 procId = 5551;
+    Application* app = startApplication(procId, appId);
+    std::shared_ptr<mir::scene::Session> session = app->session()->session();
+
+    FakeMirSurface *surface = new FakeMirSurface;
+    onSessionCreatedSurface(session.get(), surface);
+    surface->drawFirstFrame();
+
+    QSignalSpy spy(surface, SIGNAL(closeRequested()));
+
+    // Stop app
+    applicationManager.stopApplication(appId);
+
+    EXPECT_EQ(1, spy.count());
+}
+
+
+//  * Test that if there is no surface available to the app when it is stopped, that it is forced to close.
+TEST_F(ApplicationManagerTests,forceAppDeleteWhenRemovedWithMissingSurface)
+{
+    using namespace ::testing;
+
+    int argc = 0;
+    char* argv[0];
+    QCoreApplication qtApp(argc, argv); // app for deleteLater event
+
+    const QString appId("testAppId");
+    quint64 procId = 5551;
+    Application* app = startApplication(procId, appId);
+
+    QSignalSpy spy(app, SIGNAL(destroyed(QObject*)));
+    QObject::connect(app, &QObject::destroyed, app, [&qtApp](){ qtApp.quit(); });
+
+    // Stop app
+    applicationManager.stopApplication(appId);
+    qtApp.exec();
+    EXPECT_EQ(1, spy.count());
+}
+
+/*
+ * Test that if an application is started while it is still attempting to close, it is queued to start again.
+ */
+TEST_F(ApplicationManagerTests,applicationStartQueuedOnStartStopStart)
+{
+    using namespace ::testing;
+
+    int argc = 0;
+    char* argv[0];
+    QCoreApplication qtApp(argc, argv); // app for deleteLater event
+
+    const QString appId("testAppId");
+    quint64 procId = 5551;
+    Application* app = startApplication(procId, appId);
+    std::shared_ptr<mir::scene::Session> session = app->session()->session();
+
+    FakeMirSurface *surface = new FakeMirSurface;
+    onSessionCreatedSurface(session.get(), surface);
+    surface->drawFirstFrame();
+
+    EXPECT_EQ(Application::InternalState::Running, app->internalState());
+
+    // Stop app
+    applicationManager.stopApplication(appId);
+
+    EXPECT_EQ(Application::InternalState::Closing, app->internalState());
+
+    // // Set up Mocks & signal watcher
+    auto mockDesktopFileReader = new NiceMock<MockDesktopFileReader>(appId, QFileInfo());
+    ON_CALL(*mockDesktopFileReader, loaded()).WillByDefault(Return(true));
+    ON_CALL(*mockDesktopFileReader, appId()).WillByDefault(Return(appId));
+    ON_CALL(desktopFileReaderFactory, createInstance(appId, _)).WillByDefault(Return(mockDesktopFileReader));
+
+    EXPECT_EQ(nullptr, applicationManager.startApplication(appId, ApplicationManager::NoFlag));
+
+    QSignalSpy spy(&applicationManager, SIGNAL(applicationAdded(const QString&)));
+    applicationManager.onProcessStopped(appId);
+
+    QObject::connect(&applicationManager, &ApplicationManager::applicationAdded,
+                     &applicationManager, [&qtApp, appId](const QString& startedApp) {
+        if (startedApp == appId) {
+            qtApp.quit();
+        }
+    });
+
+    qtApp.exec();
+    EXPECT_EQ(1, spy.count());
+}
+
+/*
+ * Test that there is an attempt at polite exiting of the app by requesting closure of the surface.
+ */
+TEST_F(ApplicationManagerTests,suspendedApplicationResumesClosesAndDeletes)
+{
+    using namespace ::testing;
+
+    const QString appId("testAppId");
+    quint64 procId = 5551;
+    Application* app = startApplication(procId, appId);
+    std::shared_ptr<mir::scene::Session> session = app->session()->session();
+
+    FakeMirSurface *surface = new FakeMirSurface;
+    onSessionCreatedSurface(session.get(), surface);
+    surface->drawFirstFrame();
+    EXPECT_EQ(Application::InternalState::Running, app->internalState());
+    EXPECT_EQ(SessionInterface::Running,  app->session()->state());
+
+    // Suspend the application.
+    suspend(app);
+    EXPECT_EQ(Application::InternalState::Suspended, app->internalState());
+
+    // Stop app
+    applicationManager.stopApplication(appId);
+    EXPECT_EQ(Application::InternalState::Closing, app->internalState());
+    EXPECT_EQ(SessionInterface::Running,  app->session()->state());
+}
+
+/*
+ * Test that a application which fails to close will eventually be forceable closed.
+ */
+TEST_F(ApplicationManagerTests,failedApplicationCloseEventualyDeletesApplication)
+{
+    using namespace ::testing;
+
+    int argc = 0;
+    char* argv[0];
+    QCoreApplication qtApp(argc, argv); // app for deleteLater event
+
+    const QString appId("testAppId");
+    quint64 procId = 5551;
+    Application* app = startApplication(procId, appId);
+    std::shared_ptr<mir::scene::Session> session = app->session()->session();
+
+    FakeMirSurface *surface = new FakeMirSurface;
+    onSessionCreatedSurface(session.get(), surface);
+    surface->drawFirstFrame();
+
+    EXPECT_EQ(Application::InternalState::Running, app->internalState());
+
+    QSignalSpy spy(app, SIGNAL(destroyed(QObject*)));
+    QObject::connect(app, &QObject::destroyed, app, [&qtApp](){ qtApp.quit(); });
+
+    // Stop app
+    applicationManager.stopApplication(appId);
+    qtApp.exec();
+    EXPECT_EQ(1, spy.count());
+}

=== modified file 'tests/modules/common/fake_mirsurface.h'
--- tests/modules/common/fake_mirsurface.h	2015-11-25 15:38:39 +0000
+++ tests/modules/common/fake_mirsurface.h	2015-12-07 10:51:38 +0000
@@ -174,6 +174,13 @@
 
     QCursor cursor() const override { return QCursor(); }
 
+    void close() override {
+        Q_EMIT closeRequested();
+    }
+
+Q_SIGNALS:
+    void closeRequested();
+
 public Q_SLOTS:
     void onCompositorSwappedBuffers() override {}
 

=== modified file 'tests/modules/common/fake_session.h'
--- tests/modules/common/fake_session.h	2015-10-20 09:57:17 +0000
+++ tests/modules/common/fake_session.h	2015-12-07 10:51:38 +0000
@@ -15,6 +15,7 @@
  */
 
 #include <Unity/Application/session_interface.h>
+#include <QDebug>
 
 #ifndef QTMIR_FAKE_SESSION_H
 #define QTMIR_FAKE_SESSION_H
@@ -73,6 +74,9 @@
         setState(Stopped);
     }
 
+    void close() override {
+    }
+
     // For SessionManager use
 
     void addChildSession(SessionInterface*) override {}

=== modified file 'tests/modules/common/mock_session.h'
--- tests/modules/common/mock_session.h	2015-08-31 09:51:28 +0000
+++ tests/modules/common/mock_session.h	2015-12-07 10:51:38 +0000
@@ -49,6 +49,7 @@
 
     MOCK_METHOD0(suspend, void());
     MOCK_METHOD0(resume, void());
+    MOCK_METHOD0(close, void());
     MOCK_METHOD0(stop, void());
 
     MOCK_METHOD1(addChildSession, void(SessionInterface* session));

=== modified file 'tests/modules/common/qtmir_test.h'
--- tests/modules/common/qtmir_test.h	2015-11-19 12:56:02 +0000
+++ tests/modules/common/qtmir_test.h	2015-12-07 10:51:38 +0000
@@ -139,7 +139,9 @@
         ON_CALL(*mockDesktopFileReader, loaded()).WillByDefault(Return(true));
         ON_CALL(*mockDesktopFileReader, appId()).WillByDefault(Return(appId));
 
-        ON_CALL(desktopFileReaderFactory, createInstance(appId, _)).WillByDefault(Return(mockDesktopFileReader));
+        EXPECT_CALL(desktopFileReaderFactory, createInstance(appId, _))
+                .Times(1)
+                .WillOnce(Return(mockDesktopFileReader));
 
         EXPECT_CALL(appController, startApplicationWithAppIdAndArgs(appId, _))
                 .Times(1)
@@ -153,7 +155,11 @@
         EXPECT_EQ(authed, true);
 
         auto appSession = std::make_shared<mir::scene::MockSession>(appId.toStdString(), procId);
+        applicationManager.onSessionStarting(appSession);
         sessionManager.onSessionStarting(appSession);
+        
+        Mock::VerifyAndClearExpectations(&appController);
+        Mock::VerifyAndClearExpectations(&desktopFileReaderFactory);
         return application;
     }