QtMir

src/modules/Unity/Application/application.cpp (+0/-27)
src/modules/Unity/Application/application.h (+0/-4)
src/modules/Unity/Application/application_manager.cpp (+120/-36)
src/modules/Unity/Application/application_manager.h (+12/-0)
src/modules/Unity/Application/taskcontroller.cpp (+1/-6)
src/modules/Unity/Application/taskcontroller.h (+0/-2)

To merge this branch:

bzr merge lp://staging/~aacid/qtmir/fixAuthorizeDeadlock

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Gerry Boland (community)		2017-03-22	Approve on 2017-03-24
Unity8 CI Bot (community)	continuous-integration		Approve on 2017-03-24
Review via email: mp+320614@code.staging.launchpad.net

Commit message

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

Description of the change

* 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

Revision history for this message

Gerry Boland (gerboland) wrote on 2017-03-22:

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

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-03-22:

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

review: Needs Fixing (continuous-integration)

lp://staging/~aacid/qtmir/fixAuthorizeDeadlock updated on 2017-03-22

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

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-03-22:

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

review: Approve (continuous-integration)

Revision history for this message

Albert Astals Cid (aacid) wrote on 2017-03-22:

> 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://staging/~aacid/qtmir/fixAuthorizeDeadlock updated on 2017-03-22

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

Revision history for this message

Albert Astals Cid (aacid) wrote on 2017-03-22:

> > 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?

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-03-22:

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

review: Approve (continuous-integration)

Revision history for this message

Gerry Boland (gerboland) wrote on 2017-03-22:

> 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://staging/~aacid/qtmir/fixAuthorizeDeadlock updated on 2017-03-24