Merge lp:~nick-dedekind/qtmir/fix-stopSessionWhileSuspending into lp:qtmir

[Nick Dedekind]

Fixed issue where stopping the session while suspending was causing app close to stall.

Fixed issue where stopping the session while suspending was causing app close to stall.

 * 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

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

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:435
http://jenkins.qa.ubuntu.com/job/qtmir-ci/657/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-amd64-ci/351
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/353
        deb: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/353/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-i386-ci/233
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/388/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/390/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-i386-ci/233/console

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

review: Needs Fixing (continuous-integration)

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

FAILED: Continuous integration, rev:435
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-1-ci/33/
Executed test runs:

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

review: Needs Fixing (continuous-integration)

=== modified file 'src/modules/Unity/Application/application.cpp'
--- src/modules/Unity/Application/application.cpp	2015-12-07 12:23:29 +0000
+++ src/modules/Unity/Application/application.cpp	2016-01-22 16:33:33 +0000
@@ -289,6 +289,7 @@
         return Running;
     case InternalState::Suspended:
         return Suspended;
+    case InternalState::StoppedResumable:
     case InternalState::Stopped:
     default:
         return Stopped;
@@ -428,7 +429,6 @@
         // too late
         break;
     }
-
 }
 
 void Application::doClose()
@@ -590,8 +590,12 @@
         }
         break;
     case ProcessSuspended:
-        Q_ASSERT(m_state == InternalState::SuspendingWaitProcess);
-        setInternalState(InternalState::Suspended);
+        if (m_state == InternalState::Closing) {
+            // If we get a process suspension event while we're closing, resume the process.
+            Q_EMIT resumeProcessRequested();
+        } else {
+            setInternalState(InternalState::Suspended);
+        }
         break;
     case ProcessFailed:
         // we assume the session always stop before the process
@@ -647,7 +651,7 @@
 {
     qCDebug(QTMIR_APPLICATIONS) << "Application::resume - appId=" << appId();
 
-    if (m_state == InternalState::Suspended) {
+    if (m_state == InternalState::Suspended || m_state == InternalState::SuspendingWaitProcess) {
         setInternalState(InternalState::Running);
         Q_EMIT resumeProcessRequested();
         if (m_processState == ProcessSuspended) {
@@ -761,7 +765,12 @@
         Q_EMIT suspendProcessRequested();
         break;
     case Session::Stopped:
-        if (!canBeResumed()
+        if ((m_state == InternalState::SuspendingWaitProcess || m_state == InternalState::SuspendingWaitProcess) &&
+             m_processState != Application::ProcessFailed) {
+            // Session stopped normally while we're waiting for suspension.
+            doClose();
+            Q_EMIT resumeProcessRequested();
+        } else if (!canBeResumed()
                 || m_state == InternalState::Starting
                 || m_state == InternalState::Running
                 || m_state == InternalState::Closing) {

=== modified file 'src/modules/Unity/Application/application_manager.cpp'
--- src/modules/Unity/Application/application_manager.cpp	2015-12-07 12:23:29 +0000
+++ src/modules/Unity/Application/application_manager.cpp	2016-01-22 16:33:33 +0000
@@ -509,10 +509,17 @@
 
 void ApplicationManager::onProcessSuspended(const QString &appId)
 {
+    qCDebug(QTMIR_APPLICATIONS) << "ApplicationManager::onProcessSuspended - appId=" << appId;
+
     Application *application = findApplication(appId);
-    if (application) {
-        application->setProcessState(Application::ProcessSuspended);
+
+    if (!application) {
+        qDebug() << "ApplicationManager::onProcessSuspended reports stop of appId=" << appId
+                 << "which AppMan is not managing, ignoring the event";
+        return;
     }
+
+    application->setProcessState(Application::ProcessSuspended);
 }
 
 void ApplicationManager::onFocusRequested(const QString& appId)

=== modified file 'src/modules/Unity/Application/session.cpp'
--- src/modules/Unity/Application/session.cpp	2015-12-14 17:45:28 +0000
+++ src/modules/Unity/Application/session.cpp	2016-01-22 16:33:33 +0000
@@ -108,7 +108,6 @@
 {
     Q_ASSERT(m_state == Session::Suspending);
 
-
     auto surfaceList = m_surfaces.list();
     if (surfaceList.empty()) {
         qCDebug(QTMIR_SESSIONS) << "Application::suspend - no surface to call stopFrameDropper() on!";
@@ -161,14 +160,34 @@
     return m_state;
 }
 
-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);
-    }
+void Session::setState(State state)
+{
+    if (m_state == state) {
+        return;
+    }
+
+    qCDebug(QTMIR_SESSIONS) << "Session::setState - session=" << name()
+                            << "state=" << sessionStateToString(state);
+
+    if (m_state == Suspending) {
+        m_suspendTimer->stop();
+    }
+
+    m_state = state;
+
+    switch (m_state) {
+        case Starting:
+        case Running:
+            break;
+        case Suspending:
+            m_suspendTimer->start(1500);
+            break;
+        case Suspended:
+        case Stopped:
+            break;
+    }
+
+    Q_EMIT stateChanged(m_state);
 }
 
 bool Session::fullscreen() const
@@ -286,10 +305,7 @@
 
 void Session::doResume()
 {
-    if (m_state == Suspending) {
-        Q_ASSERT(m_suspendTimer->isActive());
-        m_suspendTimer->stop();
-    } else if (m_state == Suspended) {
+    if (m_state == Suspended) {
         Q_ASSERT(m_surfaces.rowCount() > 0);
         auto surfaceList = m_surfaces.list();
         for (int i = 0; i < surfaceList.count(); ++i) {
@@ -314,6 +330,8 @@
 {
     qCDebug(QTMIR_SESSIONS) << "Session::close - " << name();
 
+    if (m_state == Stopped) return;
+
     auto surfaceList = m_surfaces.list();
     for (int i = 0; i < surfaceList.count(); ++i) {
         surfaceList[i]->close();
@@ -328,9 +346,6 @@
 
         stopPromptSessions();
 
-        if (m_suspendTimer->isActive())
-            m_suspendTimer->stop();
-
         {
             auto surfaceList = m_surfaces.list();
             for (int i = 0; i < surfaceList.count(); ++i) {

=== modified file 'src/modules/Unity/Application/session.h'
--- src/modules/Unity/Application/session.h	2015-12-07 12:02:48 +0000
+++ src/modules/Unity/Application/session.h	2016-01-22 16:33:33 +0000
@@ -93,7 +93,7 @@
 private Q_SLOTS:
     void updateFullscreenProperty();
 
-private:
+protected:
     void setParentSession(Session* session);
     void setState(State state);
     void doResume();

=== modified file 'tests/modules/ApplicationManager/application_manager_test.cpp'
--- tests/modules/ApplicationManager/application_manager_test.cpp	2015-12-07 12:23:29 +0000
+++ tests/modules/ApplicationManager/application_manager_test.cpp	2016-01-22 16:33:33 +0000
@@ -2133,3 +2133,40 @@
     qtApp.exec();
     EXPECT_EQ(1, spy.count());
 }
+
+/*
+ * Test that an application that is suspended after its session is stopped is closed
+ */
+TEST_F(ApplicationManagerTests,CloseWhenSuspendedAfterSessionStopped)
+{
+    using namespace ::testing;
+
+    const QString appId("testAppId");
+    quint64 procId = 5551;
+
+    auto application = startApplication(procId, "testAppId");
+
+    qtmir::Session* session(static_cast<qtmir::Session*>(application->session()));
+
+    FakeMirSurface *surface = new FakeMirSurface;
+    onSessionCreatedSurface(session->session().get(), surface);
+    surface->drawFirstFrame();
+    EXPECT_EQ(Application::InternalState::Running, application->internalState());
+
+    // session is suspended
+    application->setRequestedState(Application::RequestedSuspended);
+    EXPECT_EQ(Application::InternalState::SuspendingWaitSession, application->internalState());
+    session->doSuspend();
+    EXPECT_EQ(Application::InternalState::SuspendingWaitProcess, application->internalState());
+    session->setLive(false);
+    EXPECT_EQ(Application::InternalState::Closing, application->internalState());
+
+    // The process can be suspended after the session has dissapeared.
+    applicationManager.onProcessSuspended(application->appId());
+    EXPECT_EQ(Application::InternalState::Closing, application->internalState());
+
+    QSignalSpy spy(application, SIGNAL(stopped()));
+    applicationManager.onProcessStopped(application->appId());
+    EXPECT_EQ(Application::Stopped, application->state());
+    EXPECT_EQ(spy.count(), 1);
+}

=== modified file 'tests/modules/SessionManager/session_test.cpp'
--- tests/modules/SessionManager/session_test.cpp	2015-12-07 12:02:48 +0000
+++ tests/modules/SessionManager/session_test.cpp	2016-01-22 16:33:33 +0000
@@ -281,3 +281,40 @@
 
     Mock::VerifyAndClear(mirServer->the_mock_prompt_session_manager().get());
 }
+
+TEST_F(SessionTests, SessionStopsWhileSuspendingDoesntSuspend)
+{
+    using namespace testing;
+
+    class SessionTestClass : public qtmir::Session
+    {
+    public:
+        SessionTestClass(const std::shared_ptr<mir::scene::Session>& session,
+                         const std::shared_ptr<mir::scene::PromptSessionManager>& promptSessionManager)
+            : Session(session, promptSessionManager) {}
+
+        using Session::m_suspendTimer;
+    };
+
+    const QString appId("test-app");
+    const pid_t procId = 5551;
+
+    auto mirSession = std::make_shared<MockSession>(appId.toStdString(), procId);
+
+    auto session = std::make_shared<SessionTestClass>(mirSession, mirServer->the_prompt_session_manager());
+    {
+        FakeMirSurface *surface = new FakeMirSurface;
+        session->registerSurface(surface);
+        surface->drawFirstFrame();
+    }
+    EXPECT_EQ(Session::Running, session->state());
+
+    EXPECT_CALL(*mirSession, set_lifecycle_state(mir_lifecycle_state_will_suspend));
+    session->suspend();
+    EXPECT_EQ(Session::Suspending, session->state());
+    EXPECT_TRUE(session->m_suspendTimer->isActive());
+
+    session->setLive(false);
+    EXPECT_EQ(Session::Stopped, session->state());
+    EXPECT_FALSE(session->m_suspendTimer->isActive());
+}

=== modified file 'tests/modules/common/qtmir_test.cpp'
--- tests/modules/common/qtmir_test.cpp	2015-12-03 17:26:31 +0000
+++ tests/modules/common/qtmir_test.cpp	2016-01-22 16:33:33 +0000
@@ -41,6 +41,9 @@
     case Application::InternalState::StoppedResumable:
         *os << "StoppedResumable";
         break;
+    case Application::InternalState::Closing:
+        *os << "Closing";
+        break;
     case Application::InternalState::Stopped:
         *os << "Stopped";
         break;