Mir

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

  1. fix-1522105
  2. Merge into development-branch
Proposed by Alan Griffiths
Status: Merged
Approved by: Alberto Aguirre
Approved revision: no longer in the source branch.
Merged at revision: 3172
Proposed branch: lp:~alan-griffiths/mir/fix-1522105
Merge into: lp:mir
Diff against target: 348 lines (+96/-35)
3 files modified
src/server/scene/surface_stack.cpp (+27/-23)
src/server/scene/surface_stack.h (+2/-1)
tests/unit-tests/scene/test_surface_stack.cpp (+67/-11)
To merge this branch: bzr merge lp:~alan-griffiths/mir/fix-1522105
Reviewer Review Type Date Requested Status
Kevin DuBois (community) Approve
Alberto Aguirre (community) Approve
PS Jenkins bot (community) continuous-integration Approve
Review via email: mp+279418@code.launchpad.net

Commit message

scene: fix the locking in SurfaceStack

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

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

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

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)
Revision history for this message
Alberto Aguirre (albaguirre) wrote :

I vote for this one :)

review: Approve
Revision history for this message
Kevin DuBois (kdub) wrote :

looks good to me

review: Approve
Revision history for this message
Kevin DuBois (kdub) wrote :

> 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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/server/scene/surface_stack.cpp'
2--- src/server/scene/surface_stack.cpp 2015-11-05 11:51:04 +0000
3+++ src/server/scene/surface_stack.cpp 2015-12-03 10:31:29 +0000
4@@ -127,7 +127,7 @@
5
6 mc::SceneElementSequence ms::SurfaceStack::scene_elements_for(mc::CompositorID id)
7 {
8- std::lock_guard<decltype(guard)> lg(guard);
9+ RecursiveReadLock lg(guard);
10
11 scene_changed = false;
12 mc::SceneElementSequence elements;
13@@ -155,7 +155,7 @@
14
15 int ms::SurfaceStack::frames_pending(mc::CompositorID id) const
16 {
17- std::lock_guard<decltype(guard)> lg(guard);
18+ RecursiveReadLock lg(guard);
19
20 int result = scene_changed ? 1 : 0;
21 for (auto const& surface : surfaces)
22@@ -182,7 +182,7 @@
23
24 void ms::SurfaceStack::register_compositor(mc::CompositorID cid)
25 {
26- std::lock_guard<decltype(guard)> lg(guard);
27+ RecursiveWriteLock lg(guard);
28
29 registered_compositors.insert(cid);
30
31@@ -191,7 +191,7 @@
32
33 void ms::SurfaceStack::unregister_compositor(mc::CompositorID cid)
34 {
35- std::lock_guard<decltype(guard)> lg(guard);
36+ RecursiveWriteLock lg(guard);
37
38 registered_compositors.erase(cid);
39
40@@ -202,7 +202,7 @@
41 std::shared_ptr<mg::Renderable> const& overlay)
42 {
43 {
44- std::lock_guard<decltype(guard)> lg(guard);
45+ RecursiveWriteLock lg(guard);
46 overlays.push_back(overlay);
47 }
48 emit_scene_changed();
49@@ -213,7 +213,7 @@
50 {
51 auto overlay = weak_overlay.lock();
52 {
53- std::lock_guard<decltype(guard)> lg(guard);
54+ RecursiveWriteLock lg(guard);
55 auto const p = std::find(overlays.begin(), overlays.end(), overlay);
56 if (p == overlays.end())
57 {
58@@ -228,7 +228,7 @@
59 void ms::SurfaceStack::emit_scene_changed()
60 {
61 {
62- std::lock_guard<decltype(guard)> lg(guard);
63+ RecursiveWriteLock lg(guard);
64 scene_changed = true;
65 }
66 observers.scene_changed();
67@@ -239,7 +239,7 @@
68 mi::InputReceptionMode input_mode)
69 {
70 {
71- std::lock_guard<decltype(guard)> lg(guard);
72+ RecursiveWriteLock lg(guard);
73 surfaces.push_back(surface);
74 create_rendering_tracker_for(surface);
75 }
76@@ -255,7 +255,7 @@
77
78 bool found_surface = false;
79 {
80- std::lock_guard<decltype(guard)> lg(guard);
81+ RecursiveWriteLock lg(guard);
82
83 auto const surface = std::find(surfaces.begin(), surfaces.end(), keep_alive);
84
85@@ -292,7 +292,7 @@
86 auto ms::SurfaceStack::surface_at(geometry::Point cursor) const
87 -> std::shared_ptr<Surface>
88 {
89- std::lock_guard<decltype(guard)> lg(guard);
90+ RecursiveReadLock lg(guard);
91 for (auto const& surface : in_reverse(surfaces))
92 {
93 // TODO There's a lack of clarity about how the input area will
94@@ -308,7 +308,7 @@
95
96 void ms::SurfaceStack::for_each(std::function<void(std::shared_ptr<mi::Surface> const&)> const& callback)
97 {
98- std::lock_guard<decltype(guard)> lg(guard);
99+ RecursiveReadLock lg(guard);
100 for (auto &surface : surfaces)
101 {
102 if (surface->query(mir_surface_attrib_visibility) ==
103@@ -321,32 +321,34 @@
104
105 void ms::SurfaceStack::raise(std::weak_ptr<Surface> const& s)
106 {
107- auto surface = s.lock();
108+ bool surfaces_reordered{false};
109
110 {
111- std::unique_lock<decltype(guard)> ul(guard);
112+ auto const surface = s.lock();
113+
114+ RecursiveWriteLock ul(guard);
115 auto const p = std::find(surfaces.begin(), surfaces.end(), surface);
116
117 if (p != surfaces.end())
118 {
119 surfaces.erase(p);
120 surfaces.push_back(surface);
121-
122- ul.unlock();
123- observers.surfaces_reordered();
124-
125- return;
126+ surfaces_reordered = true;
127 }
128 }
129
130- BOOST_THROW_EXCEPTION(std::runtime_error("Invalid surface"));
131+ if (!surfaces_reordered)
132+ BOOST_THROW_EXCEPTION(std::runtime_error("Invalid surface"));
133+
134+ observers.surfaces_reordered();
135+ return;
136 }
137
138 void ms::SurfaceStack::raise(SurfaceSet const& ss)
139 {
140 bool surfaces_reordered{false};
141 {
142- std::lock_guard<decltype(guard)> ul(guard);
143+ RecursiveWriteLock ul(guard);
144
145 auto const old_surfaces = surfaces;
146 std::stable_partition(
147@@ -364,12 +366,16 @@
148 void ms::SurfaceStack::create_rendering_tracker_for(std::shared_ptr<Surface> const& surface)
149 {
150 auto const tracker = std::make_shared<RenderingTracker>(surface);
151+
152+ RecursiveWriteLock ul(guard);
153 tracker->active_compositors(registered_compositors);
154 rendering_trackers[surface.get()] = tracker;
155 }
156
157 void ms::SurfaceStack::update_rendering_tracker_compositors()
158 {
159+ RecursiveReadLock ul(guard);
160+
161 for (auto const& pair : rendering_trackers)
162 pair.second->active_compositors(registered_compositors);
163 }
164@@ -379,12 +385,10 @@
165 observers.add(observer);
166
167 // Notify observer of existing surfaces
168- std::unique_lock<decltype(guard)> lk(guard);
169+ RecursiveReadLock lk(guard);
170 for (auto &surface : surfaces)
171 {
172- lk.unlock();
173 observer->surface_exists(surface.get());
174- lk.lock();
175 }
176 }
177
178
179=== modified file 'src/server/scene/surface_stack.h'
180--- src/server/scene/surface_stack.h 2015-11-25 16:32:07 +0000
181+++ src/server/scene/surface_stack.h 2015-12-03 10:31:29 +0000
182@@ -24,6 +24,7 @@
183 #include "mir/compositor/scene.h"
184 #include "mir/scene/observer.h"
185 #include "mir/input/scene.h"
186+#include "mir/recursive_read_write_mutex.h"
187
188 #include "mir/basic_observers.h"
189
190@@ -106,7 +107,7 @@
191 void create_rendering_tracker_for(std::shared_ptr<Surface> const&);
192 void update_rendering_tracker_compositors();
193
194- std::mutex mutable guard;
195+ RecursiveReadWriteMutex mutable guard;
196
197 std::shared_ptr<InputRegistrar> const input_registrar;
198 std::shared_ptr<SceneReport> const report;
199
200=== modified file 'tests/unit-tests/scene/test_surface_stack.cpp'
201--- tests/unit-tests/scene/test_surface_stack.cpp 2015-11-05 11:51:04 +0000
202+++ tests/unit-tests/scene/test_surface_stack.cpp 2015-12-03 10:31:29 +0000
203@@ -349,7 +349,7 @@
204 .WillByDefault(InvokeWithoutArgs([&]{ready++;}));
205 ON_CALL(*mock_queue, compositor_acquire(_))
206 .WillByDefault(InvokeWithoutArgs([&]{ready--; return mt::fake_shared(stub_buffer); }));
207-
208+
209 auto surface = std::make_shared<ms::BasicSurface>(
210 std::string("stub"),
211 geom::Rectangle{{},{}},
212@@ -620,20 +620,76 @@
213 {
214 using namespace ::testing;
215
216- using namespace ::testing;
217-
218 MockSceneObserver observer;
219
220 InSequence seq;
221 EXPECT_CALL(observer, surface_exists(stub_surface1.get())).Times(1);
222 EXPECT_CALL(observer, surface_exists(stub_surface2.get())).Times(1);
223-
224+
225 stack.add_surface(stub_surface1, default_params.input_mode);
226 stack.add_surface(stub_surface2, default_params.input_mode);
227
228 stack.add_observer(mt::fake_shared(observer));
229 }
230
231+TEST_F(SurfaceStack, scene_observer_can_query_scene_within_surface_exists_notification)
232+{
233+ using namespace ::testing;
234+
235+ MockSceneObserver observer;
236+
237+ auto const scene_query = [&]{
238+ stack.for_each([&](std::shared_ptr<mi::Surface> const& surface){
239+ EXPECT_THAT(surface.get(), Eq(stub_surface1.get()));
240+ });
241+ };
242+ EXPECT_CALL(observer, surface_exists(stub_surface1.get())).Times(1)
243+ .WillOnce(InvokeWithoutArgs(scene_query));
244+
245+ stack.add_surface(stub_surface1, default_params.input_mode);
246+ stack.add_observer(mt::fake_shared(observer));
247+}
248+
249+TEST_F(SurfaceStack, scene_observer_can_async_query_scene_within_surface_exists_notification)
250+{
251+ using namespace ::testing;
252+
253+ MockSceneObserver observer;
254+
255+ auto const scene_query = [&]{
256+ stack.for_each([&](std::shared_ptr<mi::Surface> const& surface){
257+ EXPECT_THAT(surface.get(), Eq(stub_surface1.get()));
258+ });
259+ };
260+
261+ auto const async_scene_query = [&]{
262+ std::async(std::launch::async, scene_query);
263+ };
264+
265+ EXPECT_CALL(observer, surface_exists(stub_surface1.get())).Times(1)
266+ .WillOnce(InvokeWithoutArgs(async_scene_query));
267+
268+ stack.add_surface(stub_surface1, default_params.input_mode);
269+ stack.add_observer(mt::fake_shared(observer));
270+}
271+
272+
273+TEST_F(SurfaceStack, scene_observer_can_remove_surface_from_scene_within_surface_exists_notification)
274+{
275+ using namespace ::testing;
276+
277+ MockSceneObserver observer;
278+
279+ auto const surface_removal = [&]{
280+ stack.remove_surface(stub_surface1);
281+ };
282+ EXPECT_CALL(observer, surface_exists(stub_surface1.get())).Times(1)
283+ .WillOnce(InvokeWithoutArgs(surface_removal));
284+
285+ stack.add_surface(stub_surface1, default_params.input_mode);
286+ stack.add_observer(mt::fake_shared(observer));
287+}
288+
289 TEST_F(SurfaceStack, surfaces_reordered)
290 {
291 using namespace ::testing;
292@@ -923,7 +979,7 @@
293 TEST_F(SurfaceStack, overlays_do_not_appear_in_input_enumeration)
294 {
295 mtd::StubRenderable r;
296-
297+
298 stack.add_surface(stub_surface1, default_params.input_mode);
299 stack.add_surface(stub_surface2, default_params.input_mode);
300
301@@ -946,7 +1002,7 @@
302 using namespace ::testing;
303
304 mtd::StubRenderable r;
305-
306+
307 stack.add_surface(stub_surface1, default_params.input_mode);
308 stack.add_input_visualization(mt::fake_shared(r));
309 stack.add_surface(stub_surface2, default_params.input_mode);
310@@ -956,7 +1012,7 @@
311 ElementsAre(
312 SceneElementForSurface(stub_surface1),
313 SceneElementForSurface(stub_surface2),
314- SceneElementForStream(mt::fake_shared(r))));
315+ SceneElementForStream(mt::fake_shared(r))));
316 }
317
318 TEST_F(SurfaceStack, removed_overlays_are_removed)
319@@ -964,7 +1020,7 @@
320 using namespace ::testing;
321
322 mtd::StubRenderable r;
323-
324+
325 stack.add_surface(stub_surface1, default_params.input_mode);
326 stack.add_input_visualization(mt::fake_shared(r));
327 stack.add_surface(stub_surface2, default_params.input_mode);
328@@ -975,7 +1031,7 @@
329 SceneElementForSurface(stub_surface1),
330 SceneElementForSurface(stub_surface2),
331 SceneElementForStream(mt::fake_shared(r))));
332-
333+
334 stack.remove_input_visualization(mt::fake_shared(r));
335
336 EXPECT_THAT(
337@@ -991,10 +1047,10 @@
338
339 EXPECT_CALL(o1, scene_changed()).Times(1);
340 EXPECT_CALL(o2, scene_changed()).Times(1);
341-
342+
343 stack.add_observer(mt::fake_shared(o1));
344 stack.add_observer(mt::fake_shared(o2));
345-
346+
347 stack.emit_scene_changed();
348 }
349

Subscribers

People subscribed via source and target branches