Merge lp:~nick-dedekind/unity-mir/thready-screenshotting into lp:unity-mir

[Nick Dedekind]

Fixed threading issue when screen-shotting application about to be stopped.

 * 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

[lp:~nick-dedekind/unity-mir/thready-screenshotting]

243. By Nick Dedekind on 2014-07-10

use QMutex

244. By Nick Dedekind on 2014-07-10

use qt shared/weak ptr

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:243
http://jenkins.qa.ubuntu.com/job/unity-mir-ci/392/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-utopic-amd64-ci/56
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-utopic-armhf-ci/56
        deb: http://jenkins.qa.ubuntu.com/job/unity-mir-utopic-armhf-ci/56/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-utopic-i386-ci/56

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-mir-ci/392/rebuild

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:244
http://jenkins.qa.ubuntu.com/job/unity-mir-ci/393/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-utopic-amd64-ci/57
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-utopic-armhf-ci/57
        deb: http://jenkins.qa.ubuntu.com/job/unity-mir-utopic-armhf-ci/57/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-utopic-i386-ci/57

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-mir-ci/393/rebuild

review: Approve (continuous-integration)

[Gerry Boland (gerboland)]

+QMutex screenshotMutex;
does this need to live outside the namespace?

+ QMutexLocker lk(&screenshotMutex);
a quick 1 line comment explaining why we're doing this would help the casual reader see why we block in a deconstructor

Code looks good otherwise, am getting ready to test

review: Needs Fixing

[lp:~nick-dedekind/unity-mir/thready-screenshotting]

245. By Nick Dedekind on 2014-07-10

added comment

[Nick Dedekind (nick-dedekind)]

> +QMutex screenshotMutex;
> does this need to live outside the namespace?

Yes, so it can be used in the callback when the application is deleted (member would be deleted).

>
> + QMutexLocker lk(&screenshotMutex);
> a quick 1 line comment explaining why we're doing this would help the casual
> reader see why we block in a deconstructor

Done.

>
> Code looks good otherwise, am getting ready to test

[lp:~nick-dedekind/unity-mir/thready-screenshotting]

246. By Nick Dedekind on 2014-07-10

moved mutex into namespace

[Nick Dedekind (nick-dedekind)]

> > +QMutex screenshotMutex;
> > does this need to live outside the namespace?
>
> Yes, so it can be used in the callback when the application is deleted (member
> would be deleted).

Sorry, misunderstood. no, moved it inside.

>
> >
> > + QMutexLocker lk(&screenshotMutex);
> > a quick 1 line comment explaining why we're doing this would help the casual
> > reader see why we block in a deconstructor
>
> Done.
>
> >
> > Code looks good otherwise, am getting ready to test

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:246
http://jenkins.qa.ubuntu.com/job/unity-mir-ci/394/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-utopic-amd64-ci/58
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-utopic-armhf-ci/58
        deb: http://jenkins.qa.ubuntu.com/job/unity-mir-utopic-armhf-ci/58/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-utopic-i386-ci/58

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-mir-ci/394/rebuild

review: Approve (continuous-integration)

[Gerry Boland (gerboland)]

Testing this on a device, for each screenshot take, the UI freezes for 30+ seconds.

review: Needs Fixing

[Gerry Boland (gerboland)]

Tested just now on N4, worked fine. The UI freeze was a different issue

review: Approve

[Gerry Boland (gerboland)]

 * Did you perform an exploratory manual test run of the code change and any related functionality?
Y
 * Did CI run pass? If not, please explain why.
Y

=== modified file 'src/modules/Unity/Application/application.cpp'
--- src/modules/Unity/Application/application.cpp	2014-05-26 15:54:44 +0000
+++ src/modules/Unity/Application/application.cpp	2014-07-10 13:29:20 +0000
@@ -30,6 +30,8 @@
 namespace unitymir
 {
 
+QMutex screenshotMutex;
+
 Application::Application(const QSharedPointer<TaskController>& taskController,
                          DesktopFileReader *desktopFileReader,
                          State state,
@@ -47,6 +49,7 @@
     , m_visible(false)
     , m_arguments(arguments)
     , m_suspendTimer(new QTimer(this))
+    , m_screenShotGuard(new Guard)
 {
     DLOG("Application::Application (this=%p, appId=%s, state=%d", this, qPrintable(desktopFileReader->appId()),
          static_cast<int>(state));
@@ -60,6 +63,12 @@
 
 Application::~Application()
 {
+    {
+        // In case we get a threaded screenshot callback once the application is deleted.
+        QMutexLocker lk(&screenshotMutex);
+        m_screenShotGuard.clear();
+    }
+
     DLOG("Application::~Application (this=%p)", this);
     delete m_desktopData;
 }
@@ -198,9 +207,16 @@
 
 void Application::updateScreenshot()
 {
+    QWeakPointer<Guard> wk(m_screenShotGuard.toWeakRef());
+
     session()->take_snapshot(
-        [&](mir::scene::Snapshot const& snapshot)
+        [&, wk](mir::scene::Snapshot const& snapshot)
         {
+            // In case we get a threaded screenshot callback once the application is deleted.
+            QMutexLocker lk(&screenshotMutex);
+            if (wk.isNull())
+                return;
+
             DLOG("ApplicationScreenshotProvider - Mir snapshot ready with size %d x %d",
                  snapshot.size.height.as_int(), snapshot.size.width.as_int());
 

=== modified file 'src/modules/Unity/Application/application.h'
--- src/modules/Unity/Application/application.h	2014-05-16 15:40:00 +0000
+++ src/modules/Unity/Application/application.h	2014-07-10 13:29:20 +0000
@@ -117,6 +117,9 @@
     QStringList m_arguments;
     QTimer* m_suspendTimer;
 
+    class Guard {};
+    QSharedPointer<Guard> m_screenShotGuard;
+
     friend class ApplicationManager;
     friend class MirSurfaceManager;
 };

=== modified file 'tests/application_manager_test.cpp'
--- tests/application_manager_test.cpp	2014-06-25 15:30:50 +0000
+++ tests/application_manager_test.cpp	2014-07-10 13:29:20 +0000
@@ -25,6 +25,8 @@
 
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
+#include <thread>
+#include <condition_variable>
 #include <QSignalSpy>
 
 #include "mock_application_controller.h"
@@ -70,6 +72,37 @@
         }
     {
     }
+
+    Application* startApplication(quint64 procId, QString const& appId)
+    {
+        using namespace testing;
+
+        ON_CALL(appController,appIdHasProcessId(procId, appId)).WillByDefault(Return(true));
+
+        // 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);
+
+        bool authed = false;
+        applicationManager.authorizeSession(procId, authed);
+        EXPECT_EQ(authed, true);
+
+        auto appSession = std::make_shared<MockSession>(appId.toStdString(), procId);
+
+        applicationManager.onSessionStarting(appSession);
+        return application;
+    }
+
     testing::NiceMock<testing::MockOomController> oomController;
     testing::NiceMock<testing::MockProcessController> processController;
     testing::NiceMock<testing::MockApplicationController> appController;
@@ -1879,3 +1912,95 @@
     EXPECT_EQ(countSpy.count(), 0);
     EXPECT_EQ(removedSpy.count(), 0);
 }
+
+/*
+ * Test that screenshotting callback works cross thread.
+ */
+TEST_F(ApplicationManagerTests, threadedScreenshot)
+{
+    using namespace testing;
+    quint64 procId1 = 5551;
+
+    std::mutex mutex;
+    std::condition_variable cv;
+    bool done = false;
+
+    auto application = startApplication(procId1, "webapp");
+    auto session = std::dynamic_pointer_cast<MockSession>(application->session());
+    ON_CALL(*session, take_snapshot(_)).WillByDefault(Invoke(
+        [&](mir::scene::SnapshotCallback const& callback)
+        {
+            std::thread ([&, callback]() {
+                std::unique_lock<std::mutex> lk(mutex);
+
+                mir::scene::Snapshot snapshot{mir::geometry::Size{0,0},
+                                          mir::geometry::Stride{0},
+                                          NULL};
+
+                callback(snapshot);
+
+                done = true;
+                lk.unlock();
+                cv.notify_one();
+            }).detach();
+        }));
+
+    application->updateScreenshot();
+
+    {
+        std::unique_lock<decltype(mutex)> lk(mutex);
+        cv.wait(lk, [&] { return done; } );
+        EXPECT_TRUE(done);
+    }
+
+    applicationManager.stopApplication(application->appId());
+}
+
+/*
+ * Test that screenshotting callback works when application has been deleted
+ */
+TEST_F(ApplicationManagerTests, threadedScreenshotAfterAppDelete)
+{
+    using namespace testing;
+    quint64 procId1 = 5551;
+
+    std::mutex mutex;
+    std::condition_variable cv;
+    bool ready = false;
+    bool done = false;
+
+    auto application = startApplication(procId1, "webapp");
+    auto session = std::dynamic_pointer_cast<MockSession>(application->session());
+    ON_CALL(*session, take_snapshot(_)).WillByDefault(Invoke(
+        [&](mir::scene::SnapshotCallback const& callback)
+        {
+            std::thread ([&, callback]() {
+
+                std::unique_lock<std::mutex> lk(mutex);
+                cv.wait(lk, [&]{ return ready; } );
+
+                mir::scene::Snapshot snapshot{mir::geometry::Size{0,0},
+                                          mir::geometry::Stride{0},
+                                          NULL};
+
+                callback(snapshot);
+
+                done = true;
+                lk.unlock();
+                cv.notify_one();
+            }).detach();
+        }));
+    application->updateScreenshot();
+
+    {
+        std::lock_guard<decltype(mutex)> lk(mutex);
+        applicationManager.stopApplication(application->appId());
+        ready = true;
+    }
+    cv.notify_one();
+
+    {
+        std::unique_lock<decltype(mutex)> lk(mutex);
+        cv.wait(lk, [&] { return done; } );
+    }
+}