Merge lp:~robertcarr/mir/fix-surface-stack-input-locking into lp:~mir-team/mir/trunk

[Robert Carr]

Fix locking around SurfaceStack::for_each(InputTargets)

Obviouly, ms::SurfaceStack::for_each (From the InputTargets) interface requires a lock. Now we must be wary of deadlock

In particular note the changes to ms::SurfaceStack::destroy_surface. The dead locking scenario fixed shows up moving the cursor over the stress test: InputDispatcher begins processing picking motion event target in response to cursor motion(taking lock). While the InputDispatcher is procesing this event but BEFORE it calls ms::Surfacetack::for_each through the target enumerator, the surface is destroyed. In ms::SurfaceStack::destroy_surface we take the surface stack lock, and call input_registrar->input_surface_closed. This attempts to call InputDispatcher::unregisterInputChannel, blocking on the lock from the in-procgress event targeting! However the event targeting can never finish as it will next have to call ms::SurfaceStack::lock (held by the blocked call to input_surface_closed).

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:713
http://jenkins.qa.ubuntu.com/job/mir-ci/656/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/803
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/685
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-raring-amd64-ci/141
        deb: http://jenkins.qa.ubuntu.com/job/mir-raring-amd64-ci/141/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

The point of the change to SurfaceStack::destroy_surface() is to re-order the input_registrar->input_surface_closed() and erase() calls?

Isn't that clearer with:

void ms::SurfaceStack::destroy_surface(std::weak_ptr<ms::Surface> const& surface)
{
    {
        std::lock_guard<std::mutex> lg(guard);

        auto const p = std::find(surfaces.begin(), surfaces.end(), surface.lock());

        if (p != surfaces.end())
        {
            auto keep_alive = *p;
            surfaces.erase(p);
            input_registrar->input_surface_closed(keep_alive);
        }
        // else; TODO error logging
    }

    emit_change_notification();
}

review: Approve

[Robert Carr (robertcarr)]

Not just to reorder. The call to the input registrar needs to be made outside of the surface stack lock.

[Alexandros Frantzis (afrantzis)]

Looks good. Perhaps a comment in ms::SurfaceStack::for_each(), that we are not allowed to directly or indirectly access the SurfaceStack in the callback.

review: Approve

=== modified file 'src/server/surfaces/surface_stack.cpp'
--- src/server/surfaces/surface_stack.cpp	2013-05-23 18:03:04 +0000
+++ src/server/surfaces/surface_stack.cpp	2013-05-31 16:42:24 +0000
@@ -96,6 +96,7 @@
 
 void ms::SurfaceStack::destroy_surface(std::weak_ptr<ms::Surface> const& surface)
 {
+    auto keep_alive = surface.lock();
     {
         std::lock_guard<std::mutex> lg(guard);
 
@@ -103,12 +104,15 @@
         
         if (p != surfaces.end())
         {
-            input_registrar->input_surface_closed(*p);
             surfaces.erase(p);
         }
-        // else; TODO error logging
+        else
+        {
+            // else; TODO error logging
+            return;
+        }
     }
-
+    input_registrar->input_surface_closed(keep_alive);
     emit_change_notification();
 }
 
@@ -120,6 +124,7 @@
 
 void ms::SurfaceStack::for_each(std::function<void(std::shared_ptr<mi::SurfaceTarget> const&)> const& callback)
 {
+    std::lock_guard<std::mutex> lg(guard);
     for (auto it = surfaces.rbegin(); it != surfaces.rend(); ++it)
         callback(*it);
 }