Merge lp:~andreas-pokorny/unity-mir/fix-1281075 into lp:unity-mir

[Andreas Pokorny]

Use the process id found in the mir::shell::Session to match it with the Application object.

The issue is usually experienced when applications are started in parallel. As a result the notifications from mir/upstart do not happen in a fixed sequence. And since the application has to submit a surface before it leaves starting state the last start application will get all the sessions assigned to it.

The change ensures that mir sessions are assigned only to Applications with the same process id.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:188
http://jenkins.qa.ubuntu.com/job/unity-mir-ci/256/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-amd64-ci/119
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/120
        deb: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/120/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-i386-ci/119

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

review: Approve (continuous-integration)

[Andreas Pokorny (andreas-pokorny)]

* Are there any related MPs required for this MP to build/function as expected? Please list.
Only the prerequisite branch.

* Did you perform an exploratory manual test run of your code change and any related functionality?
I tested the change with unity8 on nexus4, and did not experience any bad behaviour.

* If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?
No, nothing changed there.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:189
http://jenkins.qa.ubuntu.com/job/unity-mir-ci/261/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-amd64-ci/124
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/125
        deb: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/125/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-i386-ci/124

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

review: Approve (continuous-integration)

[Gerry Boland (gerboland)]

review: Approve

[lp:~andreas-pokorny/unity-mir/fix-1281075]

181. By Alberto Aguirre on 2014-02-21

[cmake] Use XXX_LDFLAGS for libraries found with pkg_check_modules.

Using the pkg given LDFLAGS resolves linking issues when cross-compiling

182. By Alberto Aguirre on 2014-02-21

Add dev scripts to support cross-compilation

These scripts are adapted from the mir project.
The build dependencies are parsed from debian/control and
given to debootstrap to setup a basic armhf chroot environment for cross-compilation.

These scripts are intended for development purposes only

[Michał Sawicz (saviq)]

This is breaking web apps - see http://bazaar.launchpad.net/~unity-team/unity-mir/rotate-n7/revision/182 for a "fix" I applied for MWC.

review: Needs Fixing

[Andreas Pokorny (andreas-pokorny)]

Shall I turn the session member into a list - and have the session object that is returned by session() be the one with the last posted surface? ~> so that:

* screen shotting takes the right object
* closing one of the "other" sessions does not terminate the application

review: Needs Information

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:190
http://jenkins.qa.ubuntu.com/job/unity-mir-ci/264/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-amd64-ci/127
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/128
        deb: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/128/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-i386-ci/127

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

review: Approve (continuous-integration)

[Michał Sawicz (saviq)]

From what I know about this code, looks good.

review: Approve

[Gerry Boland (gerboland)]

review: Approve

[Gerry Boland (gerboland)]

• 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.
Was fine

[lp:~andreas-pokorny/unity-mir/fix-1281075]

183. By Andreas Pokorny on 2014-02-26

more log traces

184. By Andreas Pokorny on 2014-02-26

Adding Proc Info and Mock and a Mir Session Mock for unit testing

185. By Andreas Pokorny on 2014-02-26

Fixes Unity8 crash when two instances of the same application are launched
and the second one stops.

There is still misbehavior that cannot be resolved without further changes in
upstart-app-launch. I.e. when the second instance is shutdown the existing one
will be closed too, since they use the same appId.

The changes in the construction of ApplicationManager where neceessary to
replace the external parts of ApplicationManager with mock objects.

186. By Andreas Pokorny on 2014-02-26

review findings address

187. By Andreas Pokorny on 2014-02-27

Add failing test

188. By Andreas Pokorny on 2014-02-27

Fixing mismatched sessions - and hence surfaces - by using the recently introduced process id.

189. By Andreas Pokorny on 2014-02-27

Review findings adressed - two further test cases added.

Sessions can now be overridden as long as the first session has not yet provided a screenshot
(see the second test case). We could turn the Application session storage into a list though..
That would also improve the behavior during shutdown (when only of the session objects disappear).
But that is a bigger change in behavior, due to a different Session object life time.

[Andreas Pokorny (andreas-pokorny)]

I just re-based this branch on top of fix-1240400, and tested the result on N10. Looks good.

Yesterday I also played around with keeping all session objects around. But that changed life time in a way that a deadlock is triggered, between thread locking QtCoreApplication or stuff in QtMetaObject and SessionContainers in mir...

[Andreas Pokorny (andreas-pokorny)]

review: Approve

[kevin gunn (kgunn72)]

so, i was just trying to land this however it failed to compile when merge together (standalone was fine).
i was able to replicate locally too.
note:
start with trunk, merge on top in order the following
lp:~albaguirre/unity-mir/cross-compile-link-fix
lp:~albaguirre/unity-mir/add-cross-compile-scripts
lp:~andreas-pokorny/unity-mir/fix-1240400
then merge this on top...
the failure comes in the form of a weird complaint about the tests added below, claiming it can't find declarations of command_line_

review: Needs Fixing

[lp:~andreas-pokorny/unity-mir/fix-1281075]

190. By Andreas Pokorny on 2014-03-06

merged review findings from fix-1240400

[Andreas Pokorny (andreas-pokorny)]

After applying the last coding style fixes to:

> lp:~andreas-pokorny/unity-mir/fix-1240400

.. I forgot to update that branch again - should work now.

=== modified file 'src/modules/Unity/Application/application_manager.cpp'
--- src/modules/Unity/Application/application_manager.cpp	2014-03-06 19:38:45 +0000
+++ src/modules/Unity/Application/application_manager.cpp	2014-03-06 19:38:45 +0000
@@ -634,9 +634,7 @@
         return;
     }
 
-    //FIXME(greyback) Mir not supplying any identifier that we can use to link the PID to the session
-    // so am assuming that the *most recently* launched application session is the one that connects
-    Application* application = findLastExecutedApplication();
+    Application* application = findApplicationWithPid(session->process_id());
     if (application && application->state() != Application::Running) {
         application->setSession(session);
         if (application->stage() == Application::MainStage)
@@ -793,15 +791,6 @@
     return nullptr;
 }
 
-Application* ApplicationManager::findLastExecutedApplication()
-{
-    if (m_applications.length() > 0) {
-        return m_applications.last();
-    } else {
-        return nullptr;
-    }
-}
-
 Application* ApplicationManager::applicationForStage(Application::Stage stage)
 {
     DLOG("ApplicationManager::focusedApplicationForStage(this=%p)", this);

=== modified file 'src/modules/Unity/Application/application_manager.h'
--- src/modules/Unity/Application/application_manager.h	2014-03-06 19:38:45 +0000
+++ src/modules/Unity/Application/application_manager.h	2014-03-06 19:38:45 +0000
@@ -129,7 +129,6 @@
     void remove(Application* application);
     Application* findApplicationWithSession(const std::shared_ptr<mir::shell::Session> &session);
     Application* findApplicationWithSession(const mir::shell::Session *session);
-    Application* findLastExecutedApplication();
     Application* applicationForStage(Application::Stage stage);
     QModelIndex findIndex(Application* application);
     bool checkFocusOnRemovedApplication(Application* application);

=== modified file 'tests/application_manager_test.cpp'
--- tests/application_manager_test.cpp	2014-03-06 19:38:45 +0000
+++ tests/application_manager_test.cpp	2014-03-06 19:38:45 +0000
@@ -222,3 +222,107 @@
     EXPECT_NE(nullptr, beforeFailure);
     EXPECT_EQ(beforeFailure, afterFailure);
 }
+
+TEST_F(ApplicationManagerTests,bug_case_1281075_session_ptrs_always_distributed_to_last_started_app)
+{
+    using namespace ::testing;
+    quint64 first_procId = 5921;
+    quint64 second_procId = 5922;
+    quint64 third_procId = 5923;
+    std::shared_ptr<mir::shell::Surface> aSurface(nullptr);
+    const char first_app_id[] = "app1";
+    QByteArray first_cmdLine( "/usr/bin/app1 --desktop_file_hint=app1");
+    const char second_app_id[] = "app2";
+    QByteArray second_cmdLine( "/usr/bin/app2--desktop_file_hint=app2");
+    const char third_app_id[] = "app3";
+    QByteArray third_cmdLine( "/usr/bin/app3 --desktop_file_hint=app3");
+
+    EXPECT_CALL(procInfo,command_line(first_procId))
+        .Times(1)
+        .WillOnce(Return(first_cmdLine));
+
+    ON_CALL(appController,appIdHasProcessId(_,_)).WillByDefault(Return(false));
+
+    EXPECT_CALL(procInfo,command_line(second_procId))
+        .Times(1)
+        .WillOnce(Return(second_cmdLine));
+
+    EXPECT_CALL(procInfo,command_line(third_procId))
+        .Times(1)
+        .WillOnce(Return(third_cmdLine));
+
+    bool authed = true;
+
+    std::shared_ptr<mir::shell::Session> first_session = std::make_shared<MockSession>("Oo", first_procId);
+    std::shared_ptr<mir::shell::Session> second_session = std::make_shared<MockSession>("oO", second_procId);
+    std::shared_ptr<mir::shell::Session> third_session = std::make_shared<MockSession>("OO", third_procId);
+    applicationManager.authorizeSession(first_procId, authed);
+    applicationManager.authorizeSession(second_procId, authed);
+    applicationManager.authorizeSession(third_procId, authed);
+    applicationManager.onSessionStarting(first_session);
+    applicationManager.onSessionStarting(third_session);
+    applicationManager.onSessionStarting(second_session);
+
+    Application * firstApp = applicationManager.findApplication(first_app_id);
+    Application * secondApp = applicationManager.findApplication(second_app_id);
+    Application * thirdApp = applicationManager.findApplication(third_app_id);
+
+    EXPECT_EQ(first_session, firstApp->session());
+    EXPECT_EQ(second_session, secondApp->session());
+    EXPECT_EQ(third_session, thirdApp->session());
+}
+
+TEST_F(ApplicationManagerTests,two_session_on_one_application)
+{
+    using namespace ::testing;
+    quint64 a_procId = 5921;
+    const char an_app_id[] = "some_app";
+    QByteArray a_cmd( "/usr/bin/app1 --desktop_file_hint=some_app");
+
+    ON_CALL(procInfo,command_line(_)).WillByDefault(Return(a_cmd));
+
+    ON_CALL(appController,appIdHasProcessId(_,_)).WillByDefault(Return(false));
+
+    bool authed = true;
+
+    std::shared_ptr<mir::shell::Session> first_session = std::make_shared<MockSession>("Oo", a_procId);
+    std::shared_ptr<mir::shell::Session> second_session = std::make_shared<MockSession>("oO", a_procId);
+    applicationManager.authorizeSession(a_procId, authed);
+
+    applicationManager.onSessionStarting(first_session);
+    applicationManager.onSessionStarting(second_session);
+
+    Application * the_app = applicationManager.findApplication(an_app_id);
+
+    EXPECT_EQ(true, authed);
+    EXPECT_EQ(second_session, the_app->session());
+}
+
+TEST_F(ApplicationManagerTests,two_session_on_one_application_after_starting)
+{
+    using namespace ::testing;
+    quint64 a_procId = 5921;
+    const char an_app_id[] = "some_app";
+    QByteArray a_cmd( "/usr/bin/app1 --desktop_file_hint=some_app");
+    std::shared_ptr<mir::shell::Surface> aSurface(nullptr);
+
+    ON_CALL(procInfo,command_line(_)).WillByDefault(Return(a_cmd));
+
+    ON_CALL(appController,appIdHasProcessId(_,_)).WillByDefault(Return(false));
+
+    bool authed = true;
+
+    std::shared_ptr<mir::shell::Session> first_session = std::make_shared<MockSession>("Oo", a_procId);
+    std::shared_ptr<mir::shell::Session> second_session = std::make_shared<MockSession>("oO", a_procId);
+    applicationManager.authorizeSession(a_procId, authed);
+
+    applicationManager.onSessionStarting(first_session);
+    applicationManager.onSessionCreatedSurface(first_session.get(), aSurface);
+    applicationManager.onSessionStarting(second_session);
+
+    Application * the_app = applicationManager.findApplication(an_app_id);
+
+    EXPECT_EQ(true, authed);
+    EXPECT_EQ(Application::Running, the_app->state());
+    EXPECT_EQ(first_session, the_app->session());
+}