Merge lp:~gerboland/qtmir/dont-delete-qml-cache-on-good-stop into lp:qtmir

[Gerry Boland]

Add "Closing" state to Application, use it to distinguish user-induced close from app-induced close. Don't clear QML cache if user-induced.

Much code taken from a partially-related branch by Nick Dedekind:
https://code.launchpad.net/~nick-dedekind/qtmir/polite-close/+merge/262188

Note there is 1 disabled test, which needs additional work to fix.

 * Are there any related MPs required for this MP to build/function as expected? Please list.
N
 * Did you perform an exploratory manual test run of your code change and any related functionality?
Y
 * If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?
N/A

[Gerry Boland (gerboland)]

If app crashes, the QML cache may not be cleared. This requires additional work, which I'd prefer to consider in a later MR.

[Gerry Boland (gerboland)]

To test, get a shell on the phone and watch the contents of the ~/.cache/QML/Apps/ directory. Each app will get a directory with its appId name, which contains its cache. Deleting the cache means deleteing this directory.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:380
http://jenkins.qa.ubuntu.com/job/qtmir-ci/449/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-amd64-ci/145/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/145
        deb: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/145/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-i386-ci/27
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/182
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/182
        deb: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/182/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-wily-i386-ci/27

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

review: Needs Fixing (continuous-integration)

[Gerry Boland (gerboland)]

"virtual memory exhausted: Cannot allocate memory" - trying CI again

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:380
http://jenkins.qa.ubuntu.com/job/qtmir-ci/453/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-amd64-ci/149
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/149
        deb: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/149/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-i386-ci/31
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/186
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/186
        deb: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/186/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-wily-i386-ci/31

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

review: Approve (continuous-integration)

[Nick Dedekind (nick-dedekind)]

small comment.

review: Needs Fixing

[Nick Dedekind (nick-dedekind)]

small comment.

[Nick Dedekind (nick-dedekind)]

review: Needs Fixing

[Nick Dedekind (nick-dedekind)]

LGTM

review: Approve

[Nick Dedekind (nick-dedekind)]

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

 * Did CI run pass? If not, please explain why.
Yes

review: Approve

[Michał Sawicz (saviq)]

Approving the uncommit, sorry for the confusion.

review: Approve

=== modified file 'src/modules/Unity/Application/application.cpp'
--- src/modules/Unity/Application/application.cpp	2015-08-20 16:04:25 +0000
+++ src/modules/Unity/Application/application.cpp	2015-10-01 08:04:31 +0000
@@ -73,10 +73,27 @@
     qCDebug(QTMIR_APPLICATIONS) << "Application::~Application";
 
     // (ricmm) -- To be on the safe side, better wipe the application QML compile cache if it crashes on startup
-    if (m_processState == Application::ProcessUnknown
-            || state() == Application::Starting
-            || state() == Application::Running) {
-        wipeQMLCache();
+    if (m_processState == Application::ProcessUnknown) {
+        wipeQMLCache();
+    }
+
+    switch (m_state) {
+    case InternalState::Starting:
+    case InternalState::Running:
+    case InternalState::RunningInBackground:
+    case InternalState::SuspendingWaitSession:
+    case InternalState::SuspendingWaitProcess:
+        wipeQMLCache();
+        break;
+    case InternalState::Closing:
+    case InternalState::Suspended:
+    case InternalState::StoppedResumable:
+        break;
+    case InternalState::Stopped:
+        if (m_processState == Application::ProcessFailed) { // process crashed
+            wipeQMLCache();
+        }
+        break;
     }
 
     if (m_session) {
@@ -203,6 +220,8 @@
         return "SuspendingWaitProcess";
     case InternalState::Suspended:
         return "Suspended";
+    case InternalState::Closing:
+        return "Closing";
     case InternalState::StoppedResumable:
         return "StoppedResumable";
     case InternalState::Stopped:
@@ -266,6 +285,7 @@
     case InternalState::RunningInBackground:
     case InternalState::SuspendingWaitSession:
     case InternalState::SuspendingWaitProcess:
+    case InternalState::Closing:
         return Running;
     case InternalState::Suspended:
         return Suspended;
@@ -321,6 +341,8 @@
     case InternalState::SuspendingWaitProcess:
         // should leave the app alone until it reaches Suspended state
         break;
+    case InternalState::Closing:
+        break;
     case InternalState::StoppedResumable:
         respawn();
         break;
@@ -350,6 +372,9 @@
     case InternalState::Suspended:
         // it's already going where we it's wanted
         break;
+    case InternalState::Closing:
+        // don't suspend while it is closing
+        break;
     case InternalState::StoppedResumable:
     case InternalState::Stopped:
         // the app doesn't have a process in the first place, so there's nothing to suspend
@@ -382,6 +407,32 @@
     m_pid = pid;
 }
 
+void Application::close()
+{
+    qCDebug(QTMIR_APPLICATIONS) << "Application::close - appId=" << appId();
+
+    switch (m_state) {
+    case InternalState::Starting:
+    case InternalState::Running:
+        setInternalState(InternalState::Closing);
+        break;
+    case InternalState::RunningInBackground:
+    case InternalState::SuspendingWaitSession:
+    case InternalState::SuspendingWaitProcess:
+    case InternalState::Suspended:
+        setRequestedState(RequestedRunning);
+        setInternalState(InternalState::Closing);
+        break;
+    case InternalState::Closing:
+        // already on the way
+        break;
+    case InternalState::StoppedResumable:
+    case InternalState::Stopped:
+        // too late
+        break;
+    }
+}
+
 void Application::setArguments(const QStringList arguments)
 {
     m_arguments = arguments;
@@ -411,6 +462,7 @@
         case InternalState::Starting:
         case InternalState::Running:
         case InternalState::RunningInBackground:
+        case InternalState::Closing:
             m_session->resume();
             break;
         case InternalState::SuspendingWaitSession:
@@ -474,6 +526,9 @@
         case InternalState::Suspended:
             releaseWakelock();
             break;
+        case InternalState::Closing:
+            acquireWakelock();
+            break;
         case InternalState::StoppedResumable:
             releaseWakelock();
             break;
@@ -546,7 +601,8 @@
         if (m_state == InternalState::Starting) {
             // that was way too soon. let it go away
             setInternalState(InternalState::Stopped);
-        } else if (m_state == InternalState::StoppedResumable) {
+        } else if (m_state == InternalState::StoppedResumable ||
+                   m_state == InternalState::Closing) {
             // The application stopped nicely, likely closed itself. Thus not meant to be resumed later.
             setInternalState(InternalState::Stopped);
         } else {
@@ -657,7 +713,8 @@
     case Session::Stopped:
         if (!canBeResumed()
                 || m_state == InternalState::Starting
-                || m_state == InternalState::Running) {
+                || 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
@@ -665,6 +722,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
              */
             setInternalState(InternalState::Stopped);
         } else {

=== modified file 'src/modules/Unity/Application/application.h'
--- src/modules/Unity/Application/application.h	2015-08-20 16:10:50 +0000
+++ src/modules/Unity/Application/application.h	2015-10-01 08:04:31 +0000
@@ -72,6 +72,7 @@
         SuspendingWaitSession,
         SuspendingWaitProcess,
         Suspended,
+        Closing, // The user has requested the app be closed
         StoppedResumable, // The process stopped but we want to keep the Application object around
                           // so it can be respawned as if it never stopped running in the first place.
         Stopped // It closed itself, crashed or it stopped and we can't respawn it
@@ -125,6 +126,8 @@
 
     pid_t pid() const;
 
+    void close();
+
     // for tests
     InternalState internalState() const { return m_state; }
 

=== modified file 'src/modules/Unity/Application/application_manager.cpp'
--- src/modules/Unity/Application/application_manager.cpp	2015-08-21 00:00:16 +0000
+++ src/modules/Unity/Application/application_manager.cpp	2015-10-01 08:04:31 +0000
@@ -438,6 +438,7 @@
         return false;
     }
 
+    application->close();
     remove(application);
 
     bool result = m_taskController->stop(application->longAppId());

=== modified file 'tests/modules/ApplicationManager/application_manager_test.cpp'
--- tests/modules/ApplicationManager/application_manager_test.cpp	2015-08-31 09:51:28 +0000
+++ tests/modules/ApplicationManager/application_manager_test.cpp	2015-10-01 08:04:31 +0000
@@ -1853,3 +1853,137 @@
     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
+ */
+TEST_F(ApplicationManagerTests,QMLcacheRetainedOnAppStop)
+{
+    using namespace ::testing;
+    const QString appId("testAppId1234");
+    quint64 procId = 5551;
+
+    // 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));
+
+    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);
+
+    // Create fake QML cache for this app
+    QString path(QDir::homePath() + QStringLiteral("/.cache/QML/Apps/") + appId);
+    QDir dir(path);
+    dir.mkpath(path);
+
+    // Stop app
+    applicationManager.stopApplication(appId);
+
+    EXPECT_EQ(0, applicationManager.count());
+    EXPECT_TRUE(dir.exists());
+}
+
+/*
+ * Test that if running application stops unexpectedly, AppMan deletes the QML cache
+ */
+TEST_F(ApplicationManagerTests,DISABLED_QMLcacheDeletedOnAppCrash)
+{
+    using namespace ::testing;
+    const QString appId("testAppId12345");
+    quint64 procId = 5551;
+
+    // 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));
+
+    Application *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);
+
+    // Have app in fully Running state
+    FakeMirSurface *aSurface = new FakeMirSurface;
+    onSessionCreatedSurface(session.get(), aSurface);
+    aSurface->drawFirstFrame();
+    ASSERT_EQ(Application::InternalState::Running, the_app->internalState());
+
+    // Create fake QML cache for this app
+    QString path(QDir::homePath() + QStringLiteral("/.cache/QML/Apps/") + appId);
+    QDir dir(path);
+    dir.mkpath(path);
+
+    // Report app crash
+    onSessionStopping(session);
+    // Upstart notifies of **crashing** app
+    applicationManager.onProcessFailed(appId, true);
+    applicationManager.onProcessStopped(appId);
+
+    EXPECT_EQ(0, applicationManager.count());
+    EXPECT_FALSE(dir.exists());
+}
+
+/*
+ * Test that if running application stops itself cleanly, AppMan retains the QML cache
+ */
+TEST_F(ApplicationManagerTests,QMLcacheRetainedOnAppShutdown)
+{
+    using namespace ::testing;
+    const QString appId("testAppId123456");
+    quint64 procId = 5551;
+
+    // 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));
+
+    Application *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);
+
+    // Have app in fully Running state
+    FakeMirSurface *aSurface = new FakeMirSurface;
+    onSessionCreatedSurface(session.get(), aSurface);
+    aSurface->drawFirstFrame();
+    ASSERT_EQ(Application::InternalState::Running, the_app->internalState());
+
+    // Create fake QML cache for this app
+    QString path(QDir::homePath() + QStringLiteral("/.cache/QML/Apps/") + appId);
+    QDir dir(path);
+    dir.mkpath(path);
+
+    // Report app stop
+    onSessionStopping(session);
+    // Upstart notifies of stopping app
+    applicationManager.onProcessStopped(appId);
+
+    EXPECT_EQ(0, applicationManager.count());
+    EXPECT_TRUE(dir.exists());
+}