Merge lp:~aacid/qtmir/fixAuthorizeDeadlock into lp:qtmir

[Albert Astals Cid]

Run ApplicationManager::authorizeSession in the calling thread

Instead of using a BlockingQueuedConnection

This fixes the deadlock we have when we are creating a player that uses gstreamer and
gstreamer needs to update its registry (which blocks waiting for an external process to finish)
and that external process tries to connect to mir.

Adds a QMutex in ApplicationManager that protects the public functions and slots

The mutex needs to be recursive since for example beginInsertRows call in add() will call rowCount()

The setPid/addApp functions are still run through a queued signal so that the model/QObjects are still all handled in the main thread.

findApplicationMutexHeld is not really necessary since the mutex is recursive but thought it's better to leave the recursive use for when it's really needed

 * Are there any related MPs required for this MP to build/function as expected?
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

[Gerry Boland (gerboland)]

There is one code path I'm concerned about here, which is when an app is launched by UAL directly (say via command line, or url dispatcher) - so AppMan::startApplication wasn't called.

Ubuntu-app-launch library causes the TaskController::processStarting signal to be emitted. I *think* this happens on an internal UAL thread. So the slot ApplicationManager::onProcessStarting will be called as a queued event.

Then before this MP, TaskController::authorizationRequestedForSession signal is fired from a Mir thread, causing the slot AppMan::authorizeSession to also be queued event.

This ensures the order of execution in AppMan is always first onProcessStarting, second authorizeSession. If the order is wrong, authoriseSession will reject the app (because UAL hadn't told it to expect it yet).

My worry is if UAL launched a super fast app, it would be possible for this sequence of events to occur:
- TaskController::processStarting signal emitted
- TaskController::authorizationRequestedForSession signal emitted
- authorizeSession (called Direct)
- onProcessStarting (called Queued)
causing the app to be rejected.

Unfortunately I can't point you to an easy way to reproduce this. I can try constructing a test to prove/disprove my worry here, if you'd like.

review: Needs Fixing

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

FAILED: Continuous integration, rev:620
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/607/
Executed test runs:
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build/4607/console
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/4635
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4462/console
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4462/console
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4462/console
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4462/console
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4462/console
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4462/console

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

review: Needs Fixing (continuous-integration)

[lp:~aacid/qtmir/fixAuthorizeDeadlock]

621. By Albert Astals Cid on 2017-03-22

Move pid storing to ApplicationManager

Instead of application since the only thing that needs it is the ApplicationManager
this way instead of queuing its setting we can just set it directly

Fixes crash we had on a test

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

PASSED: Continuous integration, rev:621
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/608/
Executed test runs:
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build/4610
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/4638
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4465
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4465/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4465
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4465/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4465
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4465/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4465
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4465/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4465
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4465/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4465
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4465/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Albert Astals Cid (aacid)]

> Ubuntu-app-launch library causes the TaskController::processStarting signal to
> be emitted. I *think* this happens on an internal UAL thread. So the slot
> ApplicationManager::onProcessStarting will be called as a queued event.

Can not reproduce that, added QThread::currentThread logging to the preStartCallback lambda and to ApplicationManager::onProcessStarting and they are both run on the same thread when doing
   ubuntu-app-launch webbrowser-app
from a terminal.

[lp:~aacid/qtmir/fixAuthorizeDeadlock]

622. By Albert Astals Cid on 2017-03-22

spacing

[Albert Astals Cid (aacid)]

> > Ubuntu-app-launch library causes the TaskController::processStarting signal
> to
> > be emitted. I *think* this happens on an internal UAL thread. So the slot
> > ApplicationManager::onProcessStarting will be called as a queued event.
>
> Can not reproduce that, added QThread::currentThread logging to the
> preStartCallback lambda and to ApplicationManager::onProcessStarting and they
> are both run on the same thread when doing
> ubuntu-app-launch webbrowser-app
> from a terminal.

Ted confirmed: "The C API will always callback on the thread of the caller if it has a GMainContext set. Otherwise the default thread context."

So i guess we're fine?

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

PASSED: Continuous integration, rev:622
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/610/
Executed test runs:
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build/4613
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/4641
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4467
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4467/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4467
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4467/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4467
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4467/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4467
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4467/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4467
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4467/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4467
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4467/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Gerry Boland (gerboland)]

> Ted confirmed: "The C API will always callback on the thread of the caller if
> it has a GMainContext set. Otherwise the default thread context."
>
> So i guess we're fine?

Ok, that's news to me. Then yes, I don't see any big problem with this. Will test & review tomorrow.

[lp:~aacid/qtmir/fixAuthorizeDeadlock]

623. By Albert Astals Cid on 2017-03-24

Merge

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

PASSED: Continuous integration, rev:623
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/622/
Executed test runs:
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build/4656
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/4684
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4507
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4507/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4507
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4507/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4507
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4507/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4507
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4507/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4507
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4507/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4507
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4507/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Gerry Boland (gerboland)]

Yep, tested and the order of events is as you say. Works reliably, thank you

review: Approve

=== modified file 'src/modules/Unity/Application/application.cpp'
--- src/modules/Unity/Application/application.cpp	2017-02-07 20:59:34 +0000
+++ src/modules/Unity/Application/application.cpp	2017-03-24 09:01:47 +0000
@@ -28,7 +28,6 @@
 
 // QPA mirserver
 #include "logging.h"
-#include "initialsurfacesizes.h"
 
 // Unity API
 #include <unity/shell/application/MirSurfaceInterface.h>
@@ -48,7 +47,6 @@
     : ApplicationInfoInterface(appInfo->appId(), parent)
     , m_sharedWakelock(sharedWakelock)
     , m_appInfo(appInfo)
-    , m_pid(0)
     , m_supportedStages(Application::MainStage|Application::SideStage)
     , m_state(InternalState::Starting)
     , m_arguments(arguments)
@@ -107,10 +105,6 @@
         delete m_session;
     }
 
-    if (m_pid != 0) {
-        InitialSurfaceSizes::remove(m_pid);
-    }
-
     delete m_stopTimer;
 }
 
@@ -424,11 +418,6 @@
     return m_session ? m_session->fullscreen() : false;
 }
 
-pid_t Application::pid() const
-{
-    return m_pid;
-}
-
 void Application::close()
 {
     INFO_MSG << "()";
@@ -459,19 +448,6 @@
     }
 }
 
-void Application::setPid(pid_t pid)
-{
-    if (m_pid != 0) {
-        InitialSurfaceSizes::remove(m_pid);
-    }
-
-    m_pid = pid;
-
-    if (m_initialSurfaceSize.isValid() && m_pid != 0) {
-        InitialSurfaceSizes::set(m_pid, m_initialSurfaceSize);
-    }
-}
-
 void Application::setArguments(const QStringList &arguments)
 {
     m_arguments = arguments;
@@ -849,9 +825,6 @@
 
     if (size != m_initialSurfaceSize) {
         m_initialSurfaceSize = size;
-        if (m_pid != 0 && m_initialSurfaceSize.isValid()) {
-            InitialSurfaceSizes::set(m_pid, size);
-        }
         Q_EMIT initialSurfaceSizeChanged(m_initialSurfaceSize);
     }
 }

=== modified file 'src/modules/Unity/Application/application.h'
--- src/modules/Unity/Application/application.h	2016-11-15 17:54:03 +0000
+++ src/modules/Unity/Application/application.h	2017-03-24 09:01:47 +0000
@@ -118,8 +118,6 @@
 
     Stages supportedStages() const;
 
-    pid_t pid() const;
-
     // internal as in "not exposed in unity-api", so qtmir-internal.
     InternalState internalState() const { return m_state; }
 
@@ -149,7 +147,6 @@
 
     void acquireWakelock() const;
     void releaseWakelock() const;
-    void setPid(pid_t pid);
     void setArguments(const QStringList &arguments);
     void setInternalState(InternalState state);
     void wipeQMLCache();
@@ -166,7 +163,6 @@
 
     QSharedPointer<SharedWakelock> m_sharedWakelock;
     QSharedPointer<ApplicationInfo> m_appInfo;
-    pid_t m_pid;
     Stages m_supportedStages;
     InternalState m_state;
     QStringList m_arguments;

=== modified file 'src/modules/Unity/Application/application_manager.cpp'
--- src/modules/Unity/Application/application_manager.cpp	2017-03-24 09:01:47 +0000
+++ src/modules/Unity/Application/application_manager.cpp	2017-03-24 09:01:47 +0000
@@ -28,6 +28,7 @@
 #include "settings.h"
 
 // mirserver
+#include "initialsurfacesizes.h"
 #include "nativeinterface.h"
 #include "logging.h"
 
@@ -84,6 +85,9 @@
         return nullptr;
     }
 
+    qRegisterMetaType<QSharedPointer<qtmir::ApplicationInfo>>("QSharedPointer<qtmir::ApplicationInfo>");
+    qRegisterMetaType<pid_t>("pid_t");
+
     QSharedPointer<TaskController> taskController(new upstart::TaskController());
     QSharedPointer<ProcInfo> procInfo(new ProcInfo());
     QSharedPointer<SharedWakelock> sharedWakelock(new SharedWakelock);
@@ -135,6 +139,7 @@
     , m_procInfo(procInfo)
     , m_sharedWakelock(sharedWakelock)
     , m_settings(settings)
+    , m_mutex(QMutex::Recursive) // Needs to be recursive since e.g. beginInsertRows will call rowCount
 {
     qCDebug(QTMIR_APPLICATIONS) << "ApplicationManager::ApplicationManager (this=%p)" << this;
     setObjectName(QStringLiteral("qtmir::ApplicationManager"));
@@ -152,9 +157,11 @@
     QObject::connect(m_taskController.data(), &TaskController::resumeRequested,
                      this, &ApplicationManager::onResumeRequested);
     QObject::connect(m_taskController.data(), &TaskController::authorizationRequestedForSession,
-                     this, &ApplicationManager::authorizeSession);
+                     this, &ApplicationManager::authorizeSession, Qt::DirectConnection);
     QObject::connect(m_taskController.data(), &TaskController::sessionStarting,
                      this, &ApplicationManager::onSessionStarting);
+
+    connect(this, &ApplicationManager::queuedAddApp, this, &ApplicationManager::addApp);
 }
 
 ApplicationManager::~ApplicationManager()
@@ -165,11 +172,13 @@
 
 int ApplicationManager::rowCount(const QModelIndex &parent) const
 {
+    QMutexLocker locker(&m_mutex);
     return !parent.isValid() ? m_applications.size() : 0;
 }
 
 QVariant ApplicationManager::data(const QModelIndex &index, int role) const
 {
+    QMutexLocker locker(&m_mutex);
     if (index.row() >= 0 && index.row() < m_applications.size()) {
         Application *application = m_applications.at(index.row());
         switch (role) {
@@ -201,6 +210,7 @@
 
 Application* ApplicationManager::get(int index) const
 {
+    QMutexLocker locker(&m_mutex);
     if (index < 0 || index >= m_applications.count()) {
         return nullptr;
     }
@@ -209,6 +219,14 @@
 
 Application* ApplicationManager::findApplication(const QString &inputAppId) const
 {
+    QMutexLocker locker(&m_mutex);
+    return findApplicationMutexHeld(inputAppId);
+}
+
+Application* ApplicationManager::findApplicationMutexHeld(const QString &inputAppId) const
+{
+    // We don't really need this function since the mutex is recursive but is a bit
+    // better if we just lock the mutex recursively when really need it
     const QString appId = toShortAppIdIfPossible(inputAppId);
 
     for (Application *app : m_applications) {
@@ -221,10 +239,12 @@
 
 bool ApplicationManager::requestFocusApplication(const QString &inputAppId)
 {
+    QMutexLocker locker(&m_mutex);
+
     const QString appId = toShortAppIdIfPossible(inputAppId);
 
     qCDebug(QTMIR_APPLICATIONS) << "ApplicationManager::requestFocusApplication - appId=" << appId;
-    Application *application = findApplication(appId);
+    Application *application = findApplicationMutexHeld(appId);
 
     if (!application) {
         qDebug() << "No such running application with appId=" << appId;
@@ -238,6 +258,7 @@
 
 QString ApplicationManager::focusedApplicationId() const
 {
+    QMutexLocker locker(&m_mutex);
     for (const auto application : m_applications) {
         if (application->focused()) {
             return application->appId();
@@ -264,11 +285,13 @@
 Application* ApplicationManager::startApplication(const QString &inputAppId,
                                                   const QStringList &arguments)
 {
+    QMutexLocker locker(&m_mutex);
+
     tracepoint(qtmir, startApplication);
     QString appId = toShortAppIdIfPossible(inputAppId);
     qCDebug(QTMIR_APPLICATIONS) << "ApplicationManager::startApplication - this=" << this << "appId" << qPrintable(appId);
 
-    Application *application = findApplication(appId);
+    Application *application = findApplicationMutexHeld(appId);
     if (application) {
         qWarning() << "ApplicationManager::startApplication - application appId=" << appId << " already exists";
         return nullptr;
@@ -297,7 +320,7 @@
     }
 
     // The TaskController may synchroneously callback onProcessStarting, so check if application already added
-    application = findApplication(appId);
+    application = findApplicationMutexHeld(appId);
     if (application) {
         application->setArguments(arguments);
     } else {
@@ -320,10 +343,12 @@
 
 void ApplicationManager::onProcessStarting(const QString &appId)
 {
+    QMutexLocker locker(&m_mutex);
+
     tracepoint(qtmir, onProcessStarting);
     qCDebug(QTMIR_APPLICATIONS) << "ApplicationManager::onProcessStarting - appId=" << appId;
 
-    Application *application = findApplication(appId);
+    Application *application = findApplicationMutexHeld(appId);
     if (!application) { // then shell did not start this application, so ubuntu-app-launch must have - add to list
         auto appInfo = m_taskController->getInfoForApp(appId);
         if (!appInfo) {
@@ -360,10 +385,12 @@
  */
 bool ApplicationManager::stopApplication(const QString &inputAppId)
 {
+    QMutexLocker locker(&m_mutex);
+
     const QString appId = toShortAppIdIfPossible(inputAppId);
     qCDebug(QTMIR_APPLICATIONS) << "ApplicationManager::stopApplication - appId=" << appId;
 
-    Application *application = findApplication(appId);
+    Application *application = findApplicationMutexHeld(appId);
     if (!application) {
         qCritical() << "No such running application with appId" << appId;
         return false;
@@ -376,21 +403,20 @@
 
 void ApplicationManager::onApplicationClosing(Application *application)
 {
+    QMutexLocker locker(&m_mutex);
     remove(application);
 
-    connect(application, &QObject::destroyed, this, [this, application](QObject*) {
-        m_closingApplications.removeAll(application);
-    });
     m_closingApplications.append(application);
 }
 
 void ApplicationManager::onProcessFailed(const QString &appId, TaskController::Error error)
 {
+    QMutexLocker locker(&m_mutex);
     // Applications fail if they fail to launch, crash or are killed.
 
     qCDebug(QTMIR_APPLICATIONS) << "ApplicationManager::onProcessFailed - appId=" << appId;
 
-    Application *application = findApplication(appId);
+    Application *application = findApplicationMutexHeld(appId);
     if (!application) {
         qWarning() << "ApplicationManager::onProcessFailed - upstart reports failure of application" << appId
                    << "that AppManager is not managing";
@@ -399,15 +425,17 @@
 
     Q_UNUSED(error); // FIXME(greyback) upstart reports app that fully started up & crashes as failing during startup??
     application->setProcessState(Application::ProcessFailed);
-    application->setPid(0);
+    setApplicationPid(application, 0);
 }
 
 void ApplicationManager::onProcessStopped(const QString &appId)
 {
+    QMutexLocker locker(&m_mutex);
+
     tracepoint(qtmir, onProcessStopped);
     qCDebug(QTMIR_APPLICATIONS) << "ApplicationManager::onProcessStopped - appId=" << appId;
 
-    Application *application = findApplication(appId);
+    Application *application = findApplicationMutexHeld(appId);
     if (!application) {
         application = findClosingApplication(appId);
     }
@@ -422,15 +450,17 @@
     // we don't want to override what onProcessFailed already set.
     if (application->processState() != Application::ProcessFailed) {
         application->setProcessState(Application::ProcessStopped);
-        application->setPid(0);
+        setApplicationPid(application, 0);
     }
 }
 
 void ApplicationManager::onProcessSuspended(const QString &appId)
 {
+    QMutexLocker locker(&m_mutex);
+
     qCDebug(QTMIR_APPLICATIONS) << "ApplicationManager::onProcessSuspended - appId=" << appId;
 
-    Application *application = findApplication(appId);
+    Application *application = findApplicationMutexHeld(appId);
 
     if (!application) {
         qDebug() << "ApplicationManager::onProcessSuspended reports stop of appId=" << appId
@@ -443,9 +473,10 @@
 
 void ApplicationManager::onFocusRequested(const QString& appId)
 {
+    QMutexLocker locker(&m_mutex);
     qCDebug(QTMIR_APPLICATIONS) << "ApplicationManager::onFocusRequested - appId=" << appId;
 
-    Application *application = findApplication(appId);
+    Application *application = findApplicationMutexHeld(appId);
     if (application) {
         application->requestFocus();
     }
@@ -453,9 +484,10 @@
 
 void ApplicationManager::onResumeRequested(const QString& appId)
 {
+    QMutexLocker locker(&m_mutex);
     qCDebug(QTMIR_APPLICATIONS) << "ApplicationManager::onResumeRequested - appId=" << appId;
 
-    Application *application = findApplication(appId);
+    Application *application = findApplicationMutexHeld(appId);
 
     if (!application) {
         qCritical() << "ApplicationManager::onResumeRequested: No such running application" << appId;
@@ -471,6 +503,7 @@
 
 void ApplicationManager::onAppDataChanged(const int role)
 {
+    QMutexLocker locker(&m_mutex);
     if (sender()) {
         Application *application = static_cast<Application*>(sender());
         QModelIndex appIndex = findIndex(application);
@@ -480,6 +513,11 @@
 
 void ApplicationManager::authorizeSession(const pid_t pid, bool &authorized)
 {
+    // This is the only function that is called from a different thread than the one
+    // in which the object lives, that's why we use queuedAddApp
+
+    QMutexLocker locker(&m_mutex);
+
     tracepoint(qtmir, authorizeSession);
     authorized = false; //to be proven wrong
 
@@ -489,7 +527,7 @@
         if (app->state() == Application::Starting) {
             tracepoint(qtmir, appIdHasProcessId_start);
             if (m_taskController->appIdHasProcessId(app->appId(), pid)) {
-                app->setPid(pid);
+                setApplicationPid(app, pid);
                 authorized = true;
                 tracepoint(qtmir, appIdHasProcessId_end, 1); //found
                 return;
@@ -538,26 +576,17 @@
 
     // some naughty applications use a script to launch the actual application. Check for the
     // case where shell actually launched the script.
-    Application *application = findApplication(appInfo->appId());
+    Application *application = findApplicationMutexHeld(appInfo->appId());
     if (application && application->state() == Application::Starting) {
         qCDebug(QTMIR_APPLICATIONS) << "Process with pid" << pid << "appeared, attaching to existing entry"
                                     << "in application list with appId:" << application->appId();
-        application->setPid(pid);
+        setApplicationPid(application, pid);
         authorized = true;
         return;
     }
 
-    qCDebug(QTMIR_APPLICATIONS) << "New process with pid" << pid << "appeared, adding new application to the"
-                                << "application list with appId:" << appInfo->appId();
-
-    QStringList arguments(info->asStringList());
-    application = new Application(
-        m_sharedWakelock,
-        appInfo,
-        arguments,
-        this);
-    application->setPid(pid);
-    add(application);
+    const QStringList arguments(info->asStringList());
+    queuedAddApp(appInfo, arguments, pid);
     authorized = true;
 }
 
@@ -569,6 +598,7 @@
 
     auto qtmirSurface = static_cast<qtmir::MirSurfaceInterface*>(surface);
 
+    QMutexLocker locker(&m_mutex);
     return findApplicationWithPid(miral::pid_of(qtmirSurface->session()->session()));
 }
 
@@ -585,13 +615,29 @@
         return nullptr;
 
     for (Application *app : m_applications) {
-        if (app->pid() == pid) {
+        if (m_applicationsPid.value(app) == pid) {
             return app;
         }
     }
     return nullptr;
 }
 
+void ApplicationManager::addApp(const QSharedPointer<qtmir::ApplicationInfo> &appInfo, const QStringList &arguments, const pid_t pid)
+{
+    QMutexLocker locker(&m_mutex);
+
+    qCDebug(QTMIR_APPLICATIONS) << "New process with pid" << pid << "appeared, adding new application to the"
+                                << "application list with appId:" << appInfo->appId();
+
+    Application *application = new Application(
+        m_sharedWakelock,
+        appInfo,
+        arguments,
+        this);
+    setApplicationPid(application, pid);
+    add(application);
+}
+
 void ApplicationManager::add(Application* application)
 {
     Q_ASSERT(application != nullptr);
@@ -602,6 +648,24 @@
     }
     DEBUG_MSG << "(appId=" << application->appId() << ")";
 
+    connect(application, &QObject::destroyed, this, [this, application] {
+        const pid_t pid = m_applicationsPid.value(application);
+        if (pid != 0) {
+            InitialSurfaceSizes::remove(pid);
+            m_applicationsPid.remove(application);
+        }
+        m_closingApplications.removeAll(application);
+    });
+    connect(application, &Application::initialSurfaceSizeChanged, this, [this, application] {
+        const pid_t pid = m_applicationsPid.value(application);
+        if (pid != 0) {
+            const QSize size = application->initialSurfaceSize();
+            if (size.isValid()) {
+                InitialSurfaceSizes::set(pid, size);
+            }
+        }
+    });
+
     Q_ASSERT(!m_modelUnderChange);
     m_modelUnderChange = true;
 
@@ -638,11 +702,14 @@
             Qt::QueuedConnection);
 
     connect(application, &Application::stopProcessRequested, this, [=]() {
-        if (!m_taskController->stop(appId) && application->pid() > 0) {
-            qWarning() << "FAILED to ask Upstart to stop application with appId" << appId
-                       << "Sending SIGTERM to process:" << appId;
-            kill(application->pid(), SIGTERM);
-            application->setProcessState(Application::ProcessStopped);
+        if (!m_taskController->stop(appId)) {
+            const pid_t pid = m_applicationsPid.value(application);
+            if (pid > 0) {
+                qWarning() << "FAILED to ask Upstart to stop application with appId" << appId
+                        << "Sending SIGTERM to process:" << appId;
+                kill(pid, SIGTERM);
+                application->setProcessState(Application::ProcessStopped);
+            }
         }
     });
 
@@ -734,8 +801,24 @@
     return nullptr;
 }
 
+void ApplicationManager::setApplicationPid(Application *app, pid_t pid)
+{
+    const pid_t oldPid = m_applicationsPid.value(app);
+    if (oldPid != 0) {
+        InitialSurfaceSizes::remove(oldPid);
+    }
+
+    m_applicationsPid.insert(app, pid);
+
+    if (app->initialSurfaceSize().isValid() && pid != 0) {
+        InitialSurfaceSizes::set(pid, app->initialSurfaceSize());
+    }
+}
+
 void ApplicationManager::onSessionStarting(SessionInterface *qmlSession)
 {
+    QMutexLocker locker(&m_mutex);
+
     Application* application = findApplicationWithSession(qmlSession->session());
     if (application && application->state() != Application::Running) {
         application->setSession(qmlSession);
@@ -744,6 +827,7 @@
 
 SessionInterface *ApplicationManager::findSession(const mir::scene::Session* session) const
 {
+    QMutexLocker locker(&m_mutex);
     return m_taskController->findSession(session);
 }
 

=== modified file 'src/modules/Unity/Application/application_manager.h'
--- src/modules/Unity/Application/application_manager.h	2017-03-24 09:01:47 +0000
+++ src/modules/Unity/Application/application_manager.h	2017-03-24 09:01:47 +0000
@@ -65,6 +65,7 @@
     static ApplicationManager* create();
     static ApplicationManager* singleton();
 
+    // Noone else will use the objects passed in this contructor
     explicit ApplicationManager(
             const QSharedPointer<TaskController> &taskController,
             const QSharedPointer<SharedWakelock> &sharedWakelock,
@@ -102,9 +103,16 @@
 private Q_SLOTS:
     void onAppDataChanged(const int role);
     void onApplicationClosing(Application *application);
+    void addApp(const QSharedPointer<qtmir::ApplicationInfo> &appInfo, const QStringList &arguments, const pid_t pid);
+
+Q_SIGNALS:
+    void queuedAddApp(const QSharedPointer<qtmir::ApplicationInfo> &appInfo, const QStringList &arguments, const pid_t pid);
 
 private:
+    // All calls to private functions happen with the mutex held
     qtmir::Application* findApplicationWithPid(const pid_t pid) const;
+    Application* findApplicationMutexHeld(const QString &inputAppId) const;
+
     Application* findApplicationWithSession(const std::shared_ptr<mir::scene::Session> &session);
     void setFocused(Application *application);
     void add(Application *application);
@@ -117,7 +125,10 @@
     Application* findApplicationWithPromptSession(const mir::scene::PromptSession* promptSession);
     Application *findClosingApplication(const QString &inputAppId) const;
 
+    void setApplicationPid(Application *application, pid_t pid);
+
     QList<Application*> m_applications;
+    QHash<Application*, pid_t> m_applicationsPid;
     DBusFocusInfo *m_dbusFocusInfo;
     QSharedPointer<TaskController> m_taskController;
     QSharedPointer<ProcInfo> m_procInfo;
@@ -130,6 +141,7 @@
 
     friend class Application;
     friend class DBusWindowStack;
+    mutable QMutex m_mutex;
 };
 
 } // namespace qtmir

=== modified file 'src/modules/Unity/Application/taskcontroller.cpp'
--- src/modules/Unity/Application/taskcontroller.cpp	2017-02-21 18:46:30 +0000
+++ src/modules/Unity/Application/taskcontroller.cpp	2017-03-24 09:01:47 +0000
@@ -53,7 +53,7 @@
 
     auto sessionAuthorizer = static_cast<SessionAuthorizer*>(nativeInterface->nativeResourceForIntegration("SessionAuthorizer"));
     QObject::connect(sessionAuthorizer, &SessionAuthorizer::requestAuthorizationForSession,
-                     this, &TaskController::onAuthorizationForSessionRequested, Qt::BlockingQueuedConnection);
+                     this, &TaskController::authorizationRequestedForSession, Qt::DirectConnection);
 }
 
 TaskController::TaskController(std::shared_ptr<PromptSessionManager> &promptSessionManager, QObject *parent)
@@ -159,11 +159,6 @@
     return nullptr;
 }
 
-void TaskController::onAuthorizationForSessionRequested(const pid_t &pid, bool &authorized)
-{
-    Q_EMIT authorizationRequestedForSession(pid, authorized);
-}
-
 void TaskController::connectToAppNotifier(AppNotifier *appNotifier)
 {
     QObject::connect(appNotifier, &AppNotifier::appAdded,

=== modified file 'src/modules/Unity/Application/taskcontroller.h'
--- src/modules/Unity/Application/taskcontroller.h	2017-02-21 18:46:30 +0000
+++ src/modules/Unity/Application/taskcontroller.h	2017-03-24 09:01:47 +0000
@@ -104,8 +104,6 @@
     void onPromptProviderAdded(const qtmir::PromptSession &promptSession, const miral::Application &);
     void onPromptProviderRemoved(const qtmir::PromptSession &promptSession, const miral::Application &);
 
-    void onAuthorizationForSessionRequested(const pid_t &pid, bool &authorized);
-
 protected:
     TaskController(QObject *parent = nullptr);
     TaskController(std::shared_ptr<PromptSessionManager>&, QObject *parent = nullptr);