Merge lp:~robertcarr/mir/fix-shell-focus-locking into lp:~mir-team/mir/trunk

[Robert Carr]

Fix multiple race conditions in SessionManager

Fix multiple race conditions in SessionManager

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:552
http://jenkins.qa.ubuntu.com/job/mir-ci/284/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/239
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/122
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/289
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/289/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=precise,flavor=amd64/95

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/284/rebuild

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

Would it be worth giving set_focus_to_locked() a "std::unique_lock<std::mutex> const&" parameter so that it explicitly requires the surrounding lock to make the call? (No need for the parameter in the function body of course.)

Hmm...then we wouldn't need two functions - a temporary could be used for the call sites that don't have an existing lock.

[Alan Griffiths (alan-griffiths)]

> Would it be worth giving set_focus_to_locked() a "std::unique_lock<std::mutex>
> const&" parameter so that it explicitly requires the surrounding lock to make
> the call? (No need for the parameter in the function body of course.)

The idea is that then the lock can't be accidentally deleted in maintenance.

[Robert Carr (robertcarr)]

Didn't realize there were comments here! Nice idea. Iterated!

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:554
http://jenkins.qa.ubuntu.com/job/mir-ci/338/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/321
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/204
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/343
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/343/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/338/rebuild

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

Looks good.

review: Approve

[Alan Griffiths (alan-griffiths)]

LGTM

review: Approve

=== modified file 'include/server/mir/shell/session_manager.h'
--- include/server/mir/shell/session_manager.h	2013-03-29 16:51:35 +0000
+++ include/server/mir/shell/session_manager.h	2013-04-09 16:57:22 +0000
@@ -77,7 +77,7 @@
     typedef std::vector<std::pair<int, std::shared_ptr<frontend::Session>>> Tags;
     Tags tags;
 
-    void set_focus_to(std::shared_ptr<frontend::Session> const& next_focus);
+    void set_focus_to_locked(std::unique_lock<std::mutex> const& lock, std::shared_ptr<frontend::Session> const& next_focus);
 };
 
 }

=== modified file 'src/server/shell/session_manager.cpp'
--- src/server/shell/session_manager.cpp	2013-03-29 16:51:35 +0000
+++ src/server/shell/session_manager.cpp	2013-04-09 16:57:22 +0000
@@ -57,28 +57,25 @@
 
     app_container->insert_session(new_session);
 
-    {
-        std::unique_lock<std::mutex> lock(mutex);
-        focus_application = new_session;
-    }
-    focus_setter->set_focus_to(new_session);
+    set_focus_to_locked(std::unique_lock<std::mutex>(mutex), new_session);
 
     return new_session;
 }
 
-inline void msh::SessionManager::set_focus_to(std::shared_ptr<mf::Session> const& next_focus)
+inline void msh::SessionManager::set_focus_to_locked(std::unique_lock<std::mutex> const&, std::shared_ptr<mf::Session> const& next_focus)
 {
     auto shell_session = std::dynamic_pointer_cast<msh::Session>(next_focus);
+
     focus_application = shell_session;
     focus_setter->set_focus_to(shell_session);
 }
 
 void msh::SessionManager::close_session(std::shared_ptr<mf::Session> const& session)
 {
-    std::unique_lock<std::mutex> lock(mutex);
-
     app_container->remove_session(session);
-    set_focus_to(focus_sequence->default_focus());
+
+    std::unique_lock<std::mutex> lock(mutex);
+    set_focus_to_locked(lock, focus_sequence->default_focus());
 
     typedef Tags::value_type Pair;
 
@@ -100,7 +97,7 @@
     {
         focus = focus_sequence->successor_of(focus);
     }
-    set_focus_to(focus);
+    set_focus_to_locked(lock, focus);
 }
 
 void msh::SessionManager::shutdown()
@@ -134,7 +131,7 @@
 
     if (tags.end() != match)
     {
-        set_focus_to(match->second);
+        set_focus_to_locked(lock, match->second);
     }
 }
 
@@ -142,7 +139,7 @@
     mf::SurfaceCreationParameters const& params)
 {
     auto id = session->create_surface(params);
-    set_focus_to(session);
+    set_focus_to_locked(std::unique_lock<std::mutex>(mutex), session);
 
     return id;
 }