Merge lp:~mterry/qtmir/fix-wakelocks into lp:qtmir

[Michael Terry]

Don't hold a wakelock on apps that are exempt from lifecycle management.

This supports the new exemptFromLifecycle flag for applications as well as bringing back the internal RunningInBackground state.

The tests were revived from the same branch that removed them when we removed RunningInBackground (revision 400)

Don't hold a wakelock on apps that are exempt from lifecycle management.

This supports the new exemptFromLifecycle flag for applications as well as bringing back the internal RunningInBackground state.

The tests were revived from the same branch that removed them when we removed RunningInBackground [1] (and in hindsight, I should have thought harder about the implications of simply removing a test that confirmed we were releasing wakelocks).

This branch is related to unity-api [2] and unity8 [3] MPS. All of which are in service of fixing bug 1518764.

[1] https://code.launchpad.net/~mterry/qtmir/no-touch-no-lifecycle/+merge/272791

[2]https://code.launchpad.net/~mterry/unity-api/fix-wakelocks/+merge/279499
[3] https://code.launchpad.net/~mterry/unity8/fix-wakelocks/+merge/279502

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:424
http://jenkins.qa.ubuntu.com/job/qtmir-ci/593/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-amd64-ci/289/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/289/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-i386-ci/171/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/326/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/326/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-i386-ci/171/console

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

review: Needs Fixing (continuous-integration)

[Daniel d'Andrada (dandrader)]

In Application::setInternalState(Application::InternalState state):

"""
        case InternalState::RunningInBackground:
            releaseWakelock();
            break;
"""

could be simply:

"""
        case InternalState::RunningInBackground:

"""

as the next case does the same.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:425
http://jenkins.qa.ubuntu.com/job/qtmir-ci/594/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-amd64-ci/290/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/290/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-i386-ci/172/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/327/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/327/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-i386-ci/172/console

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

review: Needs Fixing (continuous-integration)

[Michael Terry (mterry)]

> In Application::setInternalState(Application::InternalState state):
> [snip]

True. That whole switch statement could use a lot of cleanup as there are several duplicate entries. But it looks like an intentional choice on Gerry's part, and this change in this branch is really just a revert to bring this code back to what it looked like.

I can clean up if you really want it more slim. But I'd prefer to either (a) clean up the whole switch or (b) leave this code as it was. With a preference for (b).

[Daniel d'Andrada (dandrader)]

On 03/12/2015 18:22, Michael Terry wrote:
>> In Application::setInternalState(Application::InternalState state):
>> [snip]
> True. That whole switch statement could use a lot of cleanup as there are several duplicate entries. But it looks like an intentional choice on Gerry's part, and this change in this branch is really just a revert to bring this code back to what it looked like.
>

I wrote that code, not Gerry.

What I do want to keep is the order in which the enum items appear in
the switch: They should be the same as in the enum declaration.

And there's already a precedent for that: the Starting case and the
SuspendingWait* ones.

[lp:~mterry/qtmir/fix-wakelocks]

426. By Michael Terry on 2015-12-03

Collapse a switch statement

[Michael Terry (mterry)]

> I wrote that code, not Gerry.

Whoops, sorry!

> What I do want to keep is the order in which the enum items appear in
> the switch: They should be the same as in the enum declaration.
>
> And there's already a precedent for that: the Starting case and the
> SuspendingWait* ones.

That switch isn't in enum declaration order. But I gotcha. I condensed the new switch statement.

[Daniel d'Andrada (dandrader)]

Code looks ok. Didn't test yet

review: Approve (code)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:426
http://jenkins.qa.ubuntu.com/job/qtmir-ci/595/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-amd64-ci/291/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/291/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-i386-ci/173/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/328/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/328/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-i386-ci/173/console

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

review: Needs Fixing (continuous-integration)

[Daniel d'Andrada (dandrader)]

And fixes the bug

review: Approve

Unmerged revisions

=== 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-03 20:39:24 +0000
@@ -52,6 +52,7 @@
     , m_session(nullptr)
     , m_requestedState(RequestedRunning)
     , m_processState(ProcessUnknown)
+    , m_exemptFromLifecycle(false)
 {
     qCDebug(QTMIR_APPLICATIONS) << "Application::Application - appId=" << desktopFileReader->appId();
 
@@ -78,6 +79,7 @@
     switch (m_state) {
     case InternalState::Starting:
     case InternalState::Running:
+    case InternalState::RunningInBackground:
     case InternalState::SuspendingWaitSession:
     case InternalState::SuspendingWaitProcess:
         wipeQMLCache();
@@ -209,6 +211,8 @@
         return "Starting";
     case InternalState::Running:
         return "Running";
+    case InternalState::RunningInBackground:
+        return "RunningInBackground";
     case InternalState::SuspendingWaitSession:
         return "SuspendingWaitSession";
     case InternalState::SuspendingWaitProcess:
@@ -277,6 +281,7 @@
     case InternalState::Starting:
         return Starting;
     case InternalState::Running:
+    case InternalState::RunningInBackground:
     case InternalState::SuspendingWaitSession:
     case InternalState::SuspendingWaitProcess:
     case InternalState::Closing:
@@ -327,6 +332,7 @@
     case InternalState::Running:
         // already where it's wanted to be
         break;
+    case InternalState::RunningInBackground:
     case InternalState::SuspendingWaitSession:
     case InternalState::Suspended:
         resume();
@@ -359,6 +365,7 @@
             Q_ASSERT(m_processState == ProcessUnknown);
         }
         break;
+    case InternalState::RunningInBackground:
     case InternalState::SuspendingWaitSession:
     case InternalState::SuspendingWaitProcess:
     case InternalState::Suspended:
@@ -408,6 +415,7 @@
     case InternalState::Running:
         setInternalState(InternalState::Closing);
         break;
+    case InternalState::RunningInBackground:
     case InternalState::SuspendingWaitSession:
     case InternalState::SuspendingWaitProcess:
     case InternalState::Suspended:
@@ -452,6 +460,7 @@
         switch (m_state) {
         case InternalState::Starting:
         case InternalState::Running:
+        case InternalState::RunningInBackground:
         case InternalState::Closing:
             m_session->resume();
             break;
@@ -510,6 +519,7 @@
         case InternalState::Running:
             acquireWakelock();
             break;
+        case InternalState::RunningInBackground:
         case InternalState::Suspended:
             releaseWakelock();
             break;
@@ -606,8 +616,15 @@
     Q_ASSERT(m_state == InternalState::Running);
     Q_ASSERT(m_session != nullptr);
 
-    setInternalState(InternalState::SuspendingWaitSession);
-    m_session->suspend();
+    if (exemptFromLifecycle()) {
+        // There's no need to keep the wakelock as the process is never suspended
+        // and thus has no cleanup to perform when (for example) the display is
+        // blanked.
+        setInternalState(InternalState::RunningInBackground);
+    } else {
+        setInternalState(InternalState::SuspendingWaitSession);
+        m_session->suspend();
+    }
 }
 
 void Application::resume()
@@ -622,6 +639,8 @@
     } else if (m_state == InternalState::SuspendingWaitSession) {
         setInternalState(InternalState::Running);
         m_session->resume();
+    } else if (m_state == InternalState::RunningInBackground) {
+        setInternalState(InternalState::Running);
     }
 }
 
@@ -639,6 +658,22 @@
     return m_desktopData->isTouchApp();
 }
 
+bool Application::exemptFromLifecycle() const
+{
+    return m_exemptFromLifecycle;
+}
+
+void Application::setExemptFromLifecycle(bool exemptFromLifecycle)
+{
+    if (m_exemptFromLifecycle != exemptFromLifecycle)
+    {
+        // We don't adjust current suspension state, we only care about exempt
+        // status going into a suspend.
+        m_exemptFromLifecycle = exemptFromLifecycle;
+        Q_EMIT exemptFromLifecycleChanged(m_exemptFromLifecycle);
+    }
+}
+
 QString Application::longAppId() const
 {
     return m_longAppId;

=== 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-03 20:39:24 +0000
@@ -68,6 +68,7 @@
     enum class InternalState {
         Starting,
         Running,
+        RunningInBackground,
         SuspendingWaitSession,
         SuspendingWaitProcess,
         Suspended,
@@ -104,6 +105,8 @@
     Qt::ScreenOrientations supportedOrientations() const override;
     bool rotatesWindowContents() const override;
     bool isTouchApp() const override;
+    bool exemptFromLifecycle() const override;
+    void setExemptFromLifecycle(bool) override;
 
     void setStage(Stage stage);
 
@@ -178,6 +181,7 @@
     SessionInterface *m_session;
     RequestedState m_requestedState;
     ProcessState m_processState;
+    bool m_exemptFromLifecycle;
 
     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-03 20:39:24 +0000
@@ -229,6 +229,8 @@
                 return QVariant::fromValue(application->focused());
             case RoleIsTouchApp:
                 return QVariant::fromValue(application->isTouchApp());
+            case RoleExemptFromLifecycle:
+                return QVariant::fromValue(application->exemptFromLifecycle());
             case RoleSession:
                 return QVariant::fromValue(application->session());
             case RoleFullscreen:

=== 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-03 20:39:24 +0000
@@ -68,7 +68,7 @@
 
     // FIXME: these roles should be added to unity-api and removed from here
     enum MoreRoles {
-        RoleSession = RoleIsTouchApp+1,
+        RoleSession = RoleExemptFromLifecycle+1,
         RoleFullscreen,
     };
 

=== 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-03 20:39:24 +0000
@@ -1737,6 +1737,114 @@
     }
 }
 
+TEST_F(ApplicationManagerTests, lifecycleExemptAppIsNotSuspended)
+{
+    using namespace ::testing;
+
+    const QString appId("testAppId");
+    const quint64 procId = 12345;
+
+    // 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_CALL(appController, startApplicationWithAppIdAndArgs(appId, _))
+            .Times(1)
+            .WillOnce(Return(true));
+
+    auto the_app = applicationManager.startApplication(appId, ApplicationManager::NoFlag);
+    applicationManager.onProcessStarting(appId);
+    std::shared_ptr<mir::scene::Session> session = std::make_shared<MockSession>("", procId);
+    bool authed = true;
+    applicationManager.authorizeSession(procId, authed);
+    onSessionStarting(session);
+
+    // App creates surface, focuses it so state is running
+    FakeMirSurface *surface = new FakeMirSurface;
+    onSessionCreatedSurface(session.get(), surface);
+    surface->drawFirstFrame();
+
+    // Test normal lifecycle management as a control group
+    ASSERT_EQ(Application::InternalState::Running, the_app->internalState());
+    ASSERT_EQ(Application::ProcessState::ProcessRunning, the_app->processState());
+
+    EXPECT_CALL(*(mir::scene::MockSession*)session.get(), set_lifecycle_state(mir_lifecycle_state_will_suspend));
+    the_app->setRequestedState(Application::RequestedSuspended);
+    ASSERT_EQ(Application::InternalState::SuspendingWaitSession, the_app->internalState());
+
+    static_cast<qtmir::Session*>(the_app->session())->doSuspend();
+    ASSERT_EQ(Application::InternalState::SuspendingWaitProcess, the_app->internalState());
+    applicationManager.onProcessSuspended(the_app->appId());
+    ASSERT_EQ(Application::InternalState::Suspended, the_app->internalState());
+
+    EXPECT_CALL(*(mir::scene::MockSession*)session.get(), set_lifecycle_state(mir_lifecycle_state_resumed));
+    the_app->setRequestedState(Application::RequestedRunning);
+
+    EXPECT_EQ(Application::Running, the_app->state());
+
+    // Now mark the app as exempt from lifecycle management and retest
+    the_app->setExemptFromLifecycle(true);
+
+    EXPECT_EQ(Application::Running, the_app->state());
+
+    EXPECT_CALL(*(mir::scene::MockSession*)session.get(), set_lifecycle_state(_)).Times(0);
+    the_app->setRequestedState(Application::RequestedSuspended);
+
+    // And expect it to be running still
+    ASSERT_EQ(Application::InternalState::RunningInBackground, the_app->internalState());
+
+    the_app->setRequestedState(Application::RequestedRunning);
+
+    EXPECT_EQ(Application::Running, the_app->state());
+    ASSERT_EQ(Application::InternalState::Running, the_app->internalState());
+}
+
+/*
+ * Test lifecycle exempt applications have their wakelocks released when shell tries to suspend them
+ */
+TEST_F(ApplicationManagerTests, lifecycleExemptAppHasWakelockReleasedOnAttemptedSuspend)
+{
+    using namespace ::testing;
+
+    const QString appId("testAppId");
+    const quint64 procId = 12345;
+
+    // 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_CALL(appController, startApplicationWithAppIdAndArgs(appId, _))
+            .Times(1)
+            .WillOnce(Return(true));
+
+    auto application = applicationManager.startApplication(appId, ApplicationManager::NoFlag);
+    applicationManager.onProcessStarting(appId);
+    std::shared_ptr<mir::scene::Session> session = std::make_shared<MockSession>("", procId);
+    bool authed = true;
+    applicationManager.authorizeSession(procId, authed);
+    onSessionStarting(session);
+
+    // App creates surface, focuses it so state is running
+    FakeMirSurface *surface = new FakeMirSurface;
+    onSessionCreatedSurface(session.get(), surface);
+    surface->drawFirstFrame();
+
+    // Mark app as exempt
+    application->setExemptFromLifecycle(true);
+
+    application->setRequestedState(Application::RequestedSuspended);
+
+    EXPECT_FALSE(sharedWakelock.enabled());
+    ASSERT_EQ(Application::InternalState::RunningInBackground, application->internalState());
+    EXPECT_EQ(Application::Running, application->state());
+}
+
 /*
  * Test that when user stops an application, application does not delete QML cache
  */

=== modified file 'tests/modules/common/qtmir_test.cpp'
--- tests/modules/common/qtmir_test.cpp	2015-10-01 16:48:38 +0000
+++ tests/modules/common/qtmir_test.cpp	2015-12-03 20:39:24 +0000
@@ -26,6 +26,9 @@
     case Application::InternalState::Running:
         *os << "Running";
         break;
+    case Application::InternalState::RunningInBackground:
+        *os << "RunningInBackground";
+        break;
     case Application::InternalState::SuspendingWaitSession:
         *os << "SuspendingWaitSession";
         break;