Merge lp:~albaguirre/mir/fix-1522105 into lp:mir

[Alberto Aguirre]

Fix SurfaceStack::add_observer iterating over surface list while dropping/reacquiring its mutex. (LP: #1522105)

Another thread could call SurfaceStack::surface_removed while the lock is dropped, invalidating the iterator, leading to runtime errors.

A secondary lock is used to ensure surface_removed and add_observer are mutually exclusive thus ensuring the order of calls to Observer::surface_exists and Observer::surface_removed is as expected and so that Observer::surface_exists is never called with a stale surface pointer.

A bit ugly but fixes the bug. Note that a surface_exists handler which triggers another thread to remove a surface from the scene could potentially deadlock - but I fail to a reason we should support that.

[Alan Griffiths (alan-griffiths)]

Clearly the "before" is worse than the proposal - it holds (and uses) a reference to an element that needs to be locked to avoid invalidation.

While there are no obvious problems with the proposed approach it isn't obvious that there are no problems. Maybe this is a case for RecursiveReadWriteMutex?

review: Needs Information

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3165
http://jenkins.qa.ubuntu.com/job/mir-ci/5757/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5173
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/4079
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/5121
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-xenial-touch/85/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/86
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/86/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/86
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/86/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5121
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5121/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/7651
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/25754
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/83/console

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

I've put up an alternative using RecursiveReadWriteMutex:

    lp:~alan-griffiths/mir/fix-1522105/+merge/279418

I'll let the voters choose.

review: Abstain

=== 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 07:23:10 +0000
@@ -221,7 +221,7 @@
         }
         overlays.erase(p);
     }
-    
+
     emit_scene_changed();
 }
 
@@ -252,7 +252,7 @@
 void ms::SurfaceStack::remove_surface(std::weak_ptr<Surface> const& surface)
 {
     auto const keep_alive = surface.lock();
-
+    std::lock_guard<decltype(remove_guard)> lg{remove_guard};
     bool found_surface = false;
     {
         std::lock_guard<decltype(guard)> lg(guard);
@@ -378,13 +378,19 @@
 {
     observers.add(observer);
 
-    // Notify observer of existing surfaces
+    // NOTE: Guarantee that surface_exists will not be called with stale surfaces
+    // by preventing surface_removed from removing surfaces before we are done here
+    std::lock_guard<decltype(remove_guard)> rlk{remove_guard};
+
+    // A copy of the surfaces is taken, as raise and add_surface can still invalidate
+    // the iterators and surface_exists should not be called with the surface list lock held.
     std::unique_lock<decltype(guard)> lk(guard);
-    for (auto &surface : surfaces)
+    auto const surfaces_copy = surfaces;
+    lk.unlock();
+
+    for (auto &surface : surfaces_copy)
     {
-        lk.unlock();
         observer->surface_exists(surface.get());
-        lk.lock();
     }
 }
 
@@ -393,13 +399,13 @@
     auto o = observer.lock();
     if (!o)
         BOOST_THROW_EXCEPTION(std::logic_error("Invalid observer (destroyed)"));
-    
+
     o->end_observation();
-    
+
     observers.remove(o);
 }
 
-void ms::Observers::surface_added(ms::Surface* surface) 
+void ms::Observers::surface_added(ms::Surface* surface)
 {
     for_each([&](std::shared_ptr<Observer> const& observer)
         { observer->surface_added(surface); });

=== 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 07:23:10 +0000
@@ -88,16 +88,16 @@
     void add_surface(
         std::shared_ptr<Surface> const& surface,
         input::InputReceptionMode input_mode) override;
-    
+
     auto surface_at(geometry::Point) const -> std::shared_ptr<Surface> override;
 
     void add_observer(std::shared_ptr<Observer> const& observer) override;
     void remove_observer(std::weak_ptr<Observer> const& observer) override;
-    
+
     // Intended for input overlays, as described in mir::input::Scene documentation.
     void add_input_visualization(std::shared_ptr<graphics::Renderable> const& overlay) override;
     void remove_input_visualization(std::weak_ptr<graphics::Renderable> const& overlay) override;
-    
+
     void emit_scene_changed() override;
 
 private:
@@ -107,6 +107,7 @@
     void update_rendering_tracker_compositors();
 
     std::mutex mutable guard;
+    std::recursive_mutex mutable remove_guard;
 
     std::shared_ptr<InputRegistrar> const input_registrar;
     std::shared_ptr<SceneReport> const report;
@@ -114,7 +115,7 @@
     std::vector<std::shared_ptr<Surface>> surfaces;
     std::map<Surface*,std::shared_ptr<RenderingTracker>> rendering_trackers;
     std::set<compositor::CompositorID> registered_compositors;
-    
+
     std::vector<std::shared_ptr<graphics::Renderable>> overlays;
 
     Observers observers;

=== 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 07:23:10 +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();
 }