Merge lp:~dandrader/qtmir/bogusRespawn-lp1575577 into lp:qtmir

[Daniel d'Andrada]

Application: Don't respawn if closed while still starting up

+ refactoring of Application::onSessionStateChanged to explicitly cover every single internal state on session stopping.

* Are there any related MPs required for this MP to build/function as expected? Please list.
No

* Did you perform an exploratory manual test run of your code change and any related functionality?
Yes, although I couldn't reproduce the bug ever. But wrote a regression test that covers it well though.

* If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?
Not applicable.

[Unity8 CI Bot (unity8-ci-bot)]

FAILED: Continuous integration, rev:483
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/195/
Executed test runs:
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/1480
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/1445
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/1445
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=vivid+overlay/1445
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=vivid+overlay/1445/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/1445
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/1445/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=vivid+overlay/1445
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=vivid+overlay/1445/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/1445
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/1445/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=vivid+overlay/1445
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=vivid+overlay/1445/artifact/output/*zip*/output.zip
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/1445/console

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/195/rebuild

review: Needs Fixing (continuous-integration)

[Michael Terry (mterry)]

I am not qualified to review the refactor itself, but I tested the debs and I can't reproduce anymore (and it was very easy for me to reproduce before, so I believe it's fixed).

review: Approve (testing)

[Gerry Boland (gerboland)]

+ switch(m_state) {
Missing space between "switch" and the bracket.

+ /* killed by upstart while in background. Keep it in the window list
upstart doesn't kill apps in background, the kernel OOM killer does.

+ /* we're not able to respawn this application */
I think it's a good idea to explain some reasons why, e.g. "not managed by upstart (launched via cmd line by user)"

+void Application::onSessionStopped()
name sounds like a slot. Not a complaint, just initial thought on reading the code.

Nothing (except existing tests) is using the canBeResumed() method now. Please either use it in this code, or remove it completely. I would lean toward keeping it, as it interprets the meaning of "processState == ProcessUnknown" slightly more clearly in the code. But I'll not argue with your decision.

I'm going to run some UITK AP tests, since they're great at exercising AppMan code.

review: Needs Fixing

[lp:~dandrader/qtmir/bogusRespawn-lp1575577]

484. By Daniel d'Andrada on 2016-05-03

Address review comments

[Daniel d'Andrada (dandrader)]

On 03/05/2016 11:44, Gerry Boland wrote:
> Review: Needs Fixing
>
> + switch(m_state) {
> Missing space between "switch" and the bracket.

Fixed.

>
> + /* killed by upstart while in background. Keep it in the window list
> upstart doesn't kill apps in background, the kernel OOM killer does.

Fixed.

> + /* we're not able to respawn this application */
> I think it's a good idea to explain some reasons why, e.g. "not managed by upstart (launched via cmd line by user)"

Done.

> +void Application::onSessionStopped()
> name sounds like a slot. Not a complaint, just initial thought on reading the code.

Suggestions?

> Nothing (except existing tests) is using the canBeResumed() method now. Please either use it in this code, or remove it completely. I would lean toward keeping it, as it interprets the meaning of "processState == ProcessUnknown" slightly more clearly in the code. But I'll not argue with your decision.

Removed canBeResumed() and updated tests.

[Unity8 CI Bot (unity8-ci-bot)]

PASSED: Continuous integration, rev:484
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/198/
Executed test runs:
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/1496
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/1460
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/1460
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=vivid+overlay/1460
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=vivid+overlay/1460/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/1460
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/1460/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=vivid+overlay/1460
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=vivid+overlay/1460/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/1460
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/1460/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=vivid+overlay/1460
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=vivid+overlay/1460/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/1460
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/1460/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/198/rebuild

review: Approve (continuous-integration)

[Gerry Boland (gerboland)]

I tested with ubuntu-ui-toolkit AP tests - always a good tester - and any issues that appeared with due to other reasons. This looks good

review: Approve

=== modified file 'src/modules/Unity/Application/application.cpp'
--- src/modules/Unity/Application/application.cpp	2016-04-26 08:56:36 +0000
+++ src/modules/Unity/Application/application.cpp	2016-05-03 15:28:09 +0000
@@ -427,11 +427,6 @@
     return m_session ? m_session->fullscreen() : false;
 }
 
-bool Application::canBeResumed() const
-{
-    return m_processState != ProcessUnknown;
-}
-
 pid_t Application::pid() const
 {
     return m_pid;
@@ -774,28 +769,64 @@
         Q_EMIT suspendProcessRequested();
         break;
     case Session::Stopped:
-        if ((m_state == InternalState::SuspendingWaitSession || m_state == InternalState::SuspendingWaitProcess)
-                && m_processState != Application::ProcessFailed) {
-            // Session stopped normally while we're waiting for suspension
+        onSessionStopped();
+    }
+}
+
+void Application::onSessionStopped()
+{
+    switch (m_state) {
+    case InternalState::Starting:
+        /* application has stopped before it managed to create a surface, we can
+           assume it crashed on startup, and thus cannot be resumed */
+        setInternalState(InternalState::Stopped);
+        break;
+    case InternalState::Running:
+        /* application is on foreground, if Mir reports the application disconnects,
+           it either crashed or stopped itself. Either way, it must go away. */
+        setInternalState(InternalState::Stopped);
+        break;
+    case InternalState::RunningInBackground:
+        if (m_processState == Application::ProcessFailed) {
+            /* killed by the Out-Of-Memory killer while in background. Keep it in the window list
+               as the user didn't want it to go away */
+            setInternalState(InternalState::StoppedResumable);
+        } else {
+            /* the application closed itself while in the background. Let it go away */
+            setInternalState(InternalState::Stopped);
+        }
+        break;
+    case InternalState::SuspendingWaitSession:
+    case InternalState::SuspendingWaitProcess:
+        if (m_processState == Application::ProcessFailed) {
+            /* killed by the Out-Of-Memory killer while suspended (or getting there), keep it around as the user
+               doesn't expect it to disappear */
+            setInternalState(InternalState::StoppedResumable);
+        } else {
+            /* Session stopped normally while we're waiting for suspension */
             stop();
             setInternalState(InternalState::Stopped);
-        } else if (!canBeResumed()
-                || m_state == InternalState::Starting
-                || m_state == InternalState::Running
-                || m_state == InternalState::Closing) {
-            /*  1. application is not managed by upstart
-             *  2. application is managed by upstart, but has stopped before it managed
-             *     to create a surface, we can assume it crashed on startup, and thus
-             *     cannot be resumed
-             *  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
-             */
-            setInternalState(InternalState::Stopped);
+        }
+        break;
+    case InternalState::Suspended:
+        if (m_processState != ProcessUnknown) {
+            /* If the user explicly closed this application, we would have it resumed before doing so.
+               Since this is not the case, keep it around. */
+            setInternalState(InternalState::StoppedResumable);
         } else {
-            setInternalState(InternalState::StoppedResumable);
+            /* We're not able to respawn this application because it's not managed by upstart
+               (probably was launched via cmd line by user) */
+            setInternalState(InternalState::Stopped);
         }
+        break;
+    case InternalState::Closing:
+        /* We're expecting the application to stop after a close request */
+        setInternalState(InternalState::Stopped);
+        break;
+    case InternalState::StoppedResumable:
+    case InternalState::Stopped:
+        /* NOOP */
+        break;
     }
 }
 

=== modified file 'src/modules/Unity/Application/application.h'
--- src/modules/Unity/Application/application.h	2016-04-26 08:56:36 +0000
+++ src/modules/Unity/Application/application.h	2016-05-03 15:28:09 +0000
@@ -56,7 +56,7 @@
     Q_DECLARE_FLAGS(Stages, Stage)
 
     enum ProcessState {
-        ProcessUnknown,
+        ProcessUnknown, // not managed by upstart, so we can't respawn that application
         ProcessRunning,
         ProcessSuspended,
         ProcessFailed, // it stopped, but because it was killed or because it crashed
@@ -118,8 +118,6 @@
     SessionInterface* session() const;
     void setSession(SessionInterface *session);
 
-    bool canBeResumed() const;
-
     bool isValid() const;
     bool fullscreen() const;
 
@@ -172,6 +170,7 @@
     void applyRequestedRunning();
     void applyRequestedSuspended();
     void applyClosing();
+    void onSessionStopped();
 
     QSharedPointer<SharedWakelock> m_sharedWakelock;
     QSharedPointer<ApplicationInfo> m_appInfo;

=== modified file 'tests/modules/Application/application_test.cpp'
--- tests/modules/Application/application_test.cpp	2016-04-26 08:56:36 +0000
+++ tests/modules/Application/application_test.cpp	2016-05-03 15:28:09 +0000
@@ -589,3 +589,30 @@
 
     EXPECT_EQ(Application::InternalState::StoppedResumable, application->internalState());
 }
+
+/*
+   Regression test for bug "App respawns if manually closed while it's launching"
+   https://bugs.launchpad.net/ubuntu/+source/qtmir/+bug/1575577
+ */
+TEST_F(ApplicationTests, dontRespawnIfClosedWhileStillStartingUp)
+{
+    using namespace ::testing;
+
+    QScopedPointer<Application> application(createApplicationWithFakes());
+
+    application->setProcessState(Application::ProcessRunning);
+
+    FakeSession *session = new FakeSession;
+
+    application->setSession(session);
+
+    QSignalSpy spyStartProcess(application.data(), SIGNAL(startProcessRequested()));
+
+    // Close the application before it even gets a surface (it's still in "starting" state)
+    application->close();
+
+    session->setState(SessionInterface::Stopped);
+
+    EXPECT_EQ(Application::InternalState::Stopped, application->internalState());
+    EXPECT_EQ(0, spyStartProcess.count());
+}

=== modified file 'tests/modules/ApplicationManager/application_manager_test.cpp'
--- tests/modules/ApplicationManager/application_manager_test.cpp	2016-04-26 08:56:36 +0000
+++ tests/modules/ApplicationManager/application_manager_test.cpp	2016-05-03 15:28:09 +0000
@@ -437,7 +437,7 @@
     EXPECT_EQ(Application::Starting, theApp->state());
     EXPECT_EQ(appId, theApp->appId());
     EXPECT_EQ(name, theApp->name());
-    EXPECT_FALSE(theApp->canBeResumed());
+    EXPECT_EQ(Application::ProcessUnknown, theApp->processState());
 
     // check signals were emitted
     EXPECT_EQ(2, countSpy.count()); //FIXME(greyback)
@@ -478,7 +478,7 @@
     EXPECT_EQ(Application::Starting, theApp->state());
     EXPECT_EQ(appId, theApp->appId());
     EXPECT_EQ(name, theApp->name());
-    EXPECT_EQ(true, theApp->canBeResumed());
+    EXPECT_NE(Application::ProcessUnknown, theApp->processState());
 
     // check signals were emitted
     EXPECT_EQ(2, countSpy.count()); //FIXME(greyback)
@@ -525,7 +525,7 @@
     EXPECT_EQ(theApp->state(), Application::Starting);
     EXPECT_EQ(theApp->appId(), appId);
     EXPECT_EQ(theApp->name(), name);
-    EXPECT_EQ(theApp->canBeResumed(), false);
+    EXPECT_EQ(theApp->processState(), Application::ProcessUnknown);
 
     // check signals were emitted
     EXPECT_EQ(countSpy.count(), 2); //FIXME(greyback)
@@ -597,7 +597,7 @@
     EXPECT_EQ(theApp->state(), Application::Starting);
     EXPECT_EQ(theApp->appId(), appId);
     EXPECT_EQ(theApp->name(), name);
-    EXPECT_EQ(theApp->canBeResumed(), true);
+    EXPECT_NE(Application::ProcessUnknown, theApp->processState());
 
     // check only once instance in the model
     EXPECT_EQ(applicationManager.count(), 1);