Merge lp:~alan-griffiths/mir/fix-1522105 into lp:mir

[Alan Griffiths]

scene: fix the locking in SurfaceStack

scene: fix the locking in SurfaceStack

The primary motivation is add_observer() using a reference to an unlocked data structure (lp:1522105) but there were also issues with create_rendering_tracker_for() and update_rendering_tracker_compositors() which access rendering_trackers without any locking.

[Alan Griffiths (alan-griffiths)]

An alternative solution to ~albaguirre/mir/fix-1522105/+merge/279393

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3168
http://jenkins.qa.ubuntu.com/job/mir-ci/5762/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5179
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/4085
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/5127
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-xenial-touch/90/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/91
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/91/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/91
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/91/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5127
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5127/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/7657
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/25763
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/88/console

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

review: Approve (continuous-integration)

[Alberto Aguirre (albaguirre)]

I vote for this one :)

review: Approve

[Kevin DuBois (kdub)]

looks good to me

review: Approve

[Kevin DuBois (kdub)]

> looks good to me
At Alberto's request in standup today, will cherry-pick this fix into 0.18 once this MP lands to lp:mir

=== modified file 'src/server/scene/surface_stack.cpp'
--- src/server/scene/surface_stack.cpp	2015-11-05 11:51:04 +0000
+++ src/server/scene/surface_stack.cpp	2015-12-03 10:31:29 +0000
@@ -127,7 +127,7 @@
 
 mc::SceneElementSequence ms::SurfaceStack::scene_elements_for(mc::CompositorID id)
 {
-    std::lock_guard<decltype(guard)> lg(guard);
+    RecursiveReadLock lg(guard);
 
     scene_changed = false;
     mc::SceneElementSequence elements;
@@ -155,7 +155,7 @@
 
 int ms::SurfaceStack::frames_pending(mc::CompositorID id) const
 {
-    std::lock_guard<decltype(guard)> lg(guard);
+    RecursiveReadLock lg(guard);
 
     int result = scene_changed ? 1 : 0;
     for (auto const& surface : surfaces)
@@ -182,7 +182,7 @@
 
 void ms::SurfaceStack::register_compositor(mc::CompositorID cid)
 {
-    std::lock_guard<decltype(guard)> lg(guard);
+    RecursiveWriteLock lg(guard);
 
     registered_compositors.insert(cid);
 
@@ -191,7 +191,7 @@
 
 void ms::SurfaceStack::unregister_compositor(mc::CompositorID cid)
 {
-    std::lock_guard<decltype(guard)> lg(guard);
+    RecursiveWriteLock lg(guard);
 
     registered_compositors.erase(cid);
 
@@ -202,7 +202,7 @@
     std::shared_ptr<mg::Renderable> const& overlay)
 {
     {
-        std::lock_guard<decltype(guard)> lg(guard);
+        RecursiveWriteLock lg(guard);
         overlays.push_back(overlay);
     }
     emit_scene_changed();
@@ -213,7 +213,7 @@
 {
     auto overlay = weak_overlay.lock();
     {
-        std::lock_guard<decltype(guard)> lg(guard);
+        RecursiveWriteLock lg(guard);
         auto const p = std::find(overlays.begin(), overlays.end(), overlay);
         if (p == overlays.end())
         {
@@ -228,7 +228,7 @@
 void ms::SurfaceStack::emit_scene_changed()
 {
     {
-        std::lock_guard<decltype(guard)> lg(guard);
+        RecursiveWriteLock lg(guard);
         scene_changed = true;
     }
     observers.scene_changed();
@@ -239,7 +239,7 @@
     mi::InputReceptionMode input_mode)
 {
     {
-        std::lock_guard<decltype(guard)> lg(guard);
+        RecursiveWriteLock lg(guard);
         surfaces.push_back(surface);
         create_rendering_tracker_for(surface);
     }
@@ -255,7 +255,7 @@
 
     bool found_surface = false;
     {
-        std::lock_guard<decltype(guard)> lg(guard);
+        RecursiveWriteLock lg(guard);
 
         auto const surface = std::find(surfaces.begin(), surfaces.end(), keep_alive);
 
@@ -292,7 +292,7 @@
 auto ms::SurfaceStack::surface_at(geometry::Point cursor) const
 -> std::shared_ptr<Surface>
 {
-    std::lock_guard<decltype(guard)> lg(guard);
+    RecursiveReadLock lg(guard);
     for (auto const& surface : in_reverse(surfaces))
     {
         // TODO There's a lack of clarity about how the input area will
@@ -308,7 +308,7 @@
 
 void ms::SurfaceStack::for_each(std::function<void(std::shared_ptr<mi::Surface> const&)> const& callback)
 {
-    std::lock_guard<decltype(guard)> lg(guard);
+    RecursiveReadLock lg(guard);
     for (auto &surface : surfaces)
     {
         if (surface->query(mir_surface_attrib_visibility) ==
@@ -321,32 +321,34 @@
 
 void ms::SurfaceStack::raise(std::weak_ptr<Surface> const& s)
 {
-    auto surface = s.lock();
+    bool surfaces_reordered{false};
 
     {
-        std::unique_lock<decltype(guard)> ul(guard);
+        auto const surface = s.lock();
+
+        RecursiveWriteLock ul(guard);
         auto const p = std::find(surfaces.begin(), surfaces.end(), surface);
 
         if (p != surfaces.end())
         {
             surfaces.erase(p);
             surfaces.push_back(surface);
-
-            ul.unlock();
-            observers.surfaces_reordered();
-
-            return;
+            surfaces_reordered = true;
         }
     }
 
-    BOOST_THROW_EXCEPTION(std::runtime_error("Invalid surface"));
+    if (!surfaces_reordered)
+        BOOST_THROW_EXCEPTION(std::runtime_error("Invalid surface"));
+
+    observers.surfaces_reordered();
+    return;
 }
 
 void ms::SurfaceStack::raise(SurfaceSet const& ss)
 {
     bool surfaces_reordered{false};
     {
-        std::lock_guard<decltype(guard)> ul(guard);
+        RecursiveWriteLock ul(guard);
 
         auto const old_surfaces = surfaces;
         std::stable_partition(
@@ -364,12 +366,16 @@
 void ms::SurfaceStack::create_rendering_tracker_for(std::shared_ptr<Surface> const& surface)
 {
     auto const tracker = std::make_shared<RenderingTracker>(surface);
+
+    RecursiveWriteLock ul(guard);
     tracker->active_compositors(registered_compositors);
     rendering_trackers[surface.get()] = tracker;
 }
 
 void ms::SurfaceStack::update_rendering_tracker_compositors()
 {
+    RecursiveReadLock ul(guard);
+
     for (auto const& pair : rendering_trackers)
         pair.second->active_compositors(registered_compositors);
 }
@@ -379,12 +385,10 @@
     observers.add(observer);
 
     // Notify observer of existing surfaces
-    std::unique_lock<decltype(guard)> lk(guard);
+    RecursiveReadLock lk(guard);
     for (auto &surface : surfaces)
     {
-        lk.unlock();
         observer->surface_exists(surface.get());
-        lk.lock();
     }
 }
 
 
=== modified file 'src/server/scene/surface_stack.h'
--- src/server/scene/surface_stack.h	2015-11-25 16:32:07 +0000
+++ src/server/scene/surface_stack.h	2015-12-03 10:31:29 +0000
@@ -24,6 +24,7 @@
 #include "mir/compositor/scene.h"
 #include "mir/scene/observer.h"
 #include "mir/input/scene.h"
+#include "mir/recursive_read_write_mutex.h"
 
 #include "mir/basic_observers.h"
 
@@ -106,7 +107,7 @@
     void create_rendering_tracker_for(std::shared_ptr<Surface> const&);
     void update_rendering_tracker_compositors();
 
-    std::mutex mutable guard;
+    RecursiveReadWriteMutex mutable guard;
 
     std::shared_ptr<InputRegistrar> const input_registrar;
     std::shared_ptr<SceneReport> const report;
 
=== modified file 'tests/unit-tests/scene/test_surface_stack.cpp'
--- tests/unit-tests/scene/test_surface_stack.cpp	2015-11-05 11:51:04 +0000
+++ tests/unit-tests/scene/test_surface_stack.cpp	2015-12-03 10:31:29 +0000
@@ -349,7 +349,7 @@
         .WillByDefault(InvokeWithoutArgs([&]{ready++;}));
     ON_CALL(*mock_queue, compositor_acquire(_))
         .WillByDefault(InvokeWithoutArgs([&]{ready--; return mt::fake_shared(stub_buffer); }));
- 
+
     auto surface = std::make_shared<ms::BasicSurface>(
         std::string("stub"),
         geom::Rectangle{{},{}},
@@ -620,20 +620,76 @@
 {
     using namespace ::testing;
 
-    using namespace ::testing;
-
     MockSceneObserver observer;
 
     InSequence seq;
     EXPECT_CALL(observer, surface_exists(stub_surface1.get())).Times(1);
     EXPECT_CALL(observer, surface_exists(stub_surface2.get())).Times(1);
-    
+
     stack.add_surface(stub_surface1, default_params.input_mode);
     stack.add_surface(stub_surface2, default_params.input_mode);
 
     stack.add_observer(mt::fake_shared(observer));
 }
 
+TEST_F(SurfaceStack, scene_observer_can_query_scene_within_surface_exists_notification)
+{
+    using namespace ::testing;
+
+    MockSceneObserver observer;
+
+    auto const scene_query = [&]{
+        stack.for_each([&](std::shared_ptr<mi::Surface> const& surface){
+            EXPECT_THAT(surface.get(), Eq(stub_surface1.get()));
+        });
+    };
+    EXPECT_CALL(observer, surface_exists(stub_surface1.get())).Times(1)
+        .WillOnce(InvokeWithoutArgs(scene_query));
+
+    stack.add_surface(stub_surface1, default_params.input_mode);
+    stack.add_observer(mt::fake_shared(observer));
+}
+
+TEST_F(SurfaceStack, scene_observer_can_async_query_scene_within_surface_exists_notification)
+{
+    using namespace ::testing;
+
+    MockSceneObserver observer;
+
+    auto const scene_query = [&]{
+        stack.for_each([&](std::shared_ptr<mi::Surface> const& surface){
+            EXPECT_THAT(surface.get(), Eq(stub_surface1.get()));
+        });
+    };
+
+    auto const async_scene_query = [&]{
+        std::async(std::launch::async, scene_query);
+    };
+
+    EXPECT_CALL(observer, surface_exists(stub_surface1.get())).Times(1)
+        .WillOnce(InvokeWithoutArgs(async_scene_query));
+
+    stack.add_surface(stub_surface1, default_params.input_mode);
+    stack.add_observer(mt::fake_shared(observer));
+}
+
+
+TEST_F(SurfaceStack, scene_observer_can_remove_surface_from_scene_within_surface_exists_notification)
+{
+    using namespace ::testing;
+
+    MockSceneObserver observer;
+
+    auto const surface_removal = [&]{
+        stack.remove_surface(stub_surface1);
+    };
+    EXPECT_CALL(observer, surface_exists(stub_surface1.get())).Times(1)
+        .WillOnce(InvokeWithoutArgs(surface_removal));
+
+    stack.add_surface(stub_surface1, default_params.input_mode);
+    stack.add_observer(mt::fake_shared(observer));
+}
+
 TEST_F(SurfaceStack, surfaces_reordered)
 {
     using namespace ::testing;
@@ -923,7 +979,7 @@
 TEST_F(SurfaceStack, overlays_do_not_appear_in_input_enumeration)
 {
     mtd::StubRenderable r;
-    
+
     stack.add_surface(stub_surface1, default_params.input_mode);
     stack.add_surface(stub_surface2, default_params.input_mode);
 
@@ -946,7 +1002,7 @@
     using namespace ::testing;
 
     mtd::StubRenderable r;
-    
+
     stack.add_surface(stub_surface1, default_params.input_mode);
     stack.add_input_visualization(mt::fake_shared(r));
     stack.add_surface(stub_surface2, default_params.input_mode);
@@ -956,7 +1012,7 @@
         ElementsAre(
             SceneElementForSurface(stub_surface1),
             SceneElementForSurface(stub_surface2),
-            SceneElementForStream(mt::fake_shared(r))));    
+            SceneElementForStream(mt::fake_shared(r))));
 }
 
 TEST_F(SurfaceStack, removed_overlays_are_removed)
@@ -964,7 +1020,7 @@
     using namespace ::testing;
 
     mtd::StubRenderable r;
-    
+
     stack.add_surface(stub_surface1, default_params.input_mode);
     stack.add_input_visualization(mt::fake_shared(r));
     stack.add_surface(stub_surface2, default_params.input_mode);
@@ -975,7 +1031,7 @@
             SceneElementForSurface(stub_surface1),
             SceneElementForSurface(stub_surface2),
             SceneElementForStream(mt::fake_shared(r))));
-    
+
     stack.remove_input_visualization(mt::fake_shared(r));
 
     EXPECT_THAT(
@@ -991,10 +1047,10 @@
 
     EXPECT_CALL(o1, scene_changed()).Times(1);
     EXPECT_CALL(o2, scene_changed()).Times(1);
-    
+
     stack.add_observer(mt::fake_shared(o1));
     stack.add_observer(mt::fake_shared(o2));
-    
+
     stack.emit_scene_changed();
 }