Mir

Merge lp:~vanvugt/mir/fix-racy-BufferMap-operator into lp:mir

  1. fix-racy-BufferMap-operator
  2. Merge into development-branch
Proposed by Daniel van Vugt
Status: Merged
Approved by: Daniel van Vugt
Approved revision: no longer in the source branch.
Merged at revision: 4064
Proposed branch: lp:~vanvugt/mir/fix-racy-BufferMap-operator
Merge into: lp:mir
Diff against target: 230 lines (+28/-20)
13 files modified
include/server/mir/frontend/client_buffers.h (+1/-1)
src/server/compositor/buffer_map.cpp (+10/-1)
src/server/compositor/buffer_map.h (+2/-1)
src/server/compositor/stream.cpp (+2/-2)
src/server/scene/application_session.cpp (+1/-1)
tests/include/mir/test/doubles/stub_buffer_stream_factory.h (+1/-1)
tests/integration-tests/test_buffer_scheduling.cpp (+1/-1)
tests/integration-tests/test_submit_buffer.cpp (+1/-1)
tests/unit-tests/compositor/test_client_buffers.cpp (+4/-4)
tests/unit-tests/compositor/test_dropping_schedule.cpp (+1/-2)
tests/unit-tests/compositor/test_multi_monitor_arbiter.cpp (+1/-2)
tests/unit-tests/compositor/test_stream.cpp (+2/-2)
tests/unit-tests/scene/test_surface_stack.cpp (+1/-1)
To merge this branch: bzr merge lp:~vanvugt/mir/fix-racy-BufferMap-operator
Reviewer Review Type Date Requested Status
Mir CI Bot continuous-integration Approve
Alan Griffiths Approve
Review via email: mp+318751@code.launchpad.net

Commit message

Fix racy compositor::BufferMap::operator[] returning a reference from inside a lock.

To post a comment you must log in.
Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

FAILED: Continuous integration, rev:4067
https://mir-jenkins.ubuntu.com/job/mir-ci/3095/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/4144/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/4231
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/4221
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/4221
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/4221
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4171
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4171/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/4171
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/4171/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4171
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4171/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/4171
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/4171/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/4171
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/4171/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/4171/console

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/3095/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

^^^
Bug 1613523

Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:4068
https://mir-jenkins.ubuntu.com/job/mir-ci/3096/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/4145
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/4232
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/4222
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/4222
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/4222
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4172
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4172/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/4172
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/4172/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4172
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4172/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/4172
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/4172/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/4172
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/4172/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/4172
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/4172/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/3096/rebuild

review: Approve (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

Looks sensible

review: Approve
Revision history for this message
Mir CI Bot (mir-ci-bot) :
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'include/server/mir/frontend/client_buffers.h'
2--- include/server/mir/frontend/client_buffers.h 2017-01-30 08:13:20 +0000
3+++ include/server/mir/frontend/client_buffers.h 2017-03-02 12:43:44 +0000
4@@ -32,7 +32,7 @@
5 public:
6 virtual graphics::BufferID add_buffer(std::shared_ptr<graphics::Buffer> const& properties) = 0;
7 virtual void remove_buffer(graphics::BufferID id) = 0;
8- virtual std::shared_ptr<graphics::Buffer>& operator[](graphics::BufferID) = 0;
9+ virtual std::shared_ptr<graphics::Buffer> get(graphics::BufferID) const = 0;
10 virtual void send_buffer(graphics::BufferID id) = 0;
11 virtual void receive_buffer(graphics::BufferID id) = 0;
12
13
14=== modified file 'src/server/compositor/buffer_map.cpp'
15--- src/server/compositor/buffer_map.cpp 2017-01-30 08:13:20 +0000
16+++ src/server/compositor/buffer_map.cpp 2017-03-02 12:43:44 +0000
17@@ -91,7 +91,7 @@
18 it->second.owner = Owner::server;
19 }
20
21-std::shared_ptr<mg::Buffer>& mc::BufferMap::operator[](mg::BufferID id)
22+std::shared_ptr<mg::Buffer> mc::BufferMap::get(mg::BufferID id) const
23 {
24 std::unique_lock<decltype(mutex)> lk(mutex);
25 return checked_buffers_find(id, lk)->second.buffer;
26@@ -105,3 +105,12 @@
27 BOOST_THROW_EXCEPTION(std::logic_error("cannot find buffer by id"));
28 return it;
29 }
30+
31+mc::BufferMap::Map::const_iterator mc::BufferMap::checked_buffers_find(
32+ mg::BufferID id, std::unique_lock<std::mutex> const&) const
33+{
34+ auto it = buffers.find(id);
35+ if (it == buffers.end())
36+ BOOST_THROW_EXCEPTION(std::logic_error("cannot find buffer by id"));
37+ return it;
38+}
39
40=== modified file 'src/server/compositor/buffer_map.h'
41--- src/server/compositor/buffer_map.h 2017-01-30 08:13:20 +0000
42+++ src/server/compositor/buffer_map.h 2017-03-02 12:43:44 +0000
43@@ -39,7 +39,7 @@
44 void receive_buffer(graphics::BufferID id) override;
45 void send_buffer(graphics::BufferID id) override;
46
47- std::shared_ptr<graphics::Buffer>& operator[](graphics::BufferID) override;
48+ std::shared_ptr<graphics::Buffer> get(graphics::BufferID) const override;
49
50 private:
51 std::mutex mutable mutex;
52@@ -54,6 +54,7 @@
53 //used to keep strong reference
54 Map buffers;
55 Map::iterator checked_buffers_find(graphics::BufferID, std::unique_lock<std::mutex> const&);
56+ Map::const_iterator checked_buffers_find(graphics::BufferID, std::unique_lock<std::mutex> const&) const;
57
58 //would be better to schedule the async buffer callbacks in the ipc subsystem,
59 //instead of driving from within the compositor threads (LP: #1395421)
60
61=== modified file 'src/server/compositor/stream.cpp'
62--- src/server/compositor/stream.cpp 2017-02-21 21:43:12 +0000
63+++ src/server/compositor/stream.cpp 2017-03-02 12:43:44 +0000
64@@ -97,7 +97,7 @@
65 std::lock_guard<decltype(mutex)> lk(mutex);
66 first_frame_posted = true;
67 buffers->receive_buffer(buffer->id());
68- schedule->schedule((*buffers)[buffer->id()]);
69+ schedule->schedule(buffers->get(buffer->id()));
70 if (!associated_buffers.empty() && (client_owned_buffer_count(lk) == 0))
71 drop_policy->swap_now_blocking();
72 }
73@@ -247,7 +247,7 @@
74 else
75 {
76 for (auto it : associated_buffers)
77- if ((*buffers)[it]->pixel_format() != mir_pixel_format_argb_8888)
78+ if (buffers->get(it)->pixel_format() != mir_pixel_format_argb_8888)
79 return false;
80 }
81 return true;
82
83=== modified file 'src/server/scene/application_session.cpp'
84--- src/server/scene/application_session.cpp 2017-02-24 12:21:55 +0000
85+++ src/server/scene/application_session.cpp 2017-03-02 12:43:44 +0000
86@@ -476,7 +476,7 @@
87
88 std::shared_ptr<mg::Buffer> ms::ApplicationSession::get_buffer(mg::BufferID id)
89 {
90- return (*buffers)[id];
91+ return buffers->get(id);
92 }
93
94 void ms::ApplicationSession::send_error(mir::ClientVisibleError const& error)
95
96=== modified file 'tests/include/mir/test/doubles/stub_buffer_stream_factory.h'
97--- tests/include/mir/test/doubles/stub_buffer_stream_factory.h 2017-01-18 02:29:37 +0000
98+++ tests/include/mir/test/doubles/stub_buffer_stream_factory.h 2017-03-02 12:43:44 +0000
99@@ -39,7 +39,7 @@
100 void remove_buffer(graphics::BufferID) override
101 {
102 }
103- std::shared_ptr<graphics::Buffer>& operator[](graphics::BufferID) override
104+ std::shared_ptr<graphics::Buffer> get(graphics::BufferID) const override
105 {
106 return buffer;
107 }
108
109=== modified file 'tests/integration-tests/test_buffer_scheduling.cpp'
110--- tests/integration-tests/test_buffer_scheduling.cpp 2017-01-30 05:18:36 +0000
111+++ tests/integration-tests/test_buffer_scheduling.cpp 2017-03-02 12:43:44 +0000
112@@ -485,7 +485,7 @@
113 if (!submit_stream)
114 return;
115 mg::BufferID id{static_cast<unsigned int>(buffer.buffer_id())};
116- submit_stream->submit_buffer((*map)[id]);
117+ submit_stream->submit_buffer(map->get(id));
118 });
119 ipc->on_allocate(
120 [this](geom::Size sz)
121
122=== modified file 'tests/integration-tests/test_submit_buffer.cpp'
123--- tests/integration-tests/test_submit_buffer.cpp 2017-01-30 08:13:20 +0000
124+++ tests/integration-tests/test_submit_buffer.cpp 2017-03-02 12:43:44 +0000
125@@ -142,7 +142,7 @@
126 {
127 }
128
129- std::shared_ptr<mg::Buffer>& operator[](mg::BufferID) override
130+ std::shared_ptr<mg::Buffer> get(mg::BufferID) const override
131 {
132 return b;
133 }
134
135=== modified file 'tests/unit-tests/compositor/test_client_buffers.cpp'
136--- tests/unit-tests/compositor/test_client_buffers.cpp 2017-01-30 08:13:20 +0000
137+++ tests/unit-tests/compositor/test_client_buffers.cpp 2017-03-02 12:43:44 +0000
138@@ -49,7 +49,7 @@
139 TEST_F(ClientBuffers, access_of_nonexistent_buffer_throws)
140 {
141 EXPECT_THROW({
142- auto buffer = map[stub_buffer.id()];
143+ auto buffer = map.get(stub_buffer.id());
144 }, std::logic_error);
145 }
146
147@@ -63,13 +63,13 @@
148 TEST_F(ClientBuffers, can_access_once_added)
149 {
150 auto id = map.add_buffer(mt::fake_shared(stub_buffer));
151- EXPECT_THAT(map[id].get(), Eq(&stub_buffer));
152+ EXPECT_THAT(map.get(id).get(), Eq(&stub_buffer));
153 }
154
155 TEST_F(ClientBuffers, sends_update_msg_to_send_buffer)
156 {
157 auto id = map.add_buffer(mt::fake_shared(stub_buffer));
158- auto buffer = map[id];
159+ auto buffer = map.get(id);
160 EXPECT_CALL(*mock_sink, update_buffer(Ref(*buffer)));
161 map.send_buffer(id);
162 }
163@@ -77,7 +77,7 @@
164 TEST_F(ClientBuffers, sends_no_update_msg_if_buffer_is_not_around)
165 {
166 auto id = map.add_buffer(mt::fake_shared(stub_buffer));
167- auto buffer = map[id];
168+ auto buffer = map.get(id);
169
170 EXPECT_CALL(*mock_sink, remove_buffer(Ref(*buffer)));
171 map.remove_buffer(id);
172
173=== modified file 'tests/unit-tests/compositor/test_dropping_schedule.cpp'
174--- tests/unit-tests/compositor/test_dropping_schedule.cpp 2017-01-18 02:29:37 +0000
175+++ tests/unit-tests/compositor/test_dropping_schedule.cpp 2017-03-02 12:43:44 +0000
176@@ -38,9 +38,8 @@
177 MOCK_METHOD1(remove_buffer, void(mg::BufferID id));
178 MOCK_METHOD1(send_buffer, void(mg::BufferID id));
179 MOCK_METHOD1(receive_buffer, void(mg::BufferID id));
180- MOCK_METHOD1(at, std::shared_ptr<mg::Buffer>&(mg::BufferID));
181 MOCK_CONST_METHOD0(client_owned_buffer_count, size_t());
182- std::shared_ptr<mg::Buffer>& operator[](mg::BufferID id) { return at(id); }
183+ MOCK_CONST_METHOD1(get, std::shared_ptr<mg::Buffer>(mg::BufferID));
184 };
185
186 struct DroppingSchedule : Test
187
188=== modified file 'tests/unit-tests/compositor/test_multi_monitor_arbiter.cpp'
189--- tests/unit-tests/compositor/test_multi_monitor_arbiter.cpp 2017-02-15 19:14:27 +0000
190+++ tests/unit-tests/compositor/test_multi_monitor_arbiter.cpp 2017-03-02 12:43:44 +0000
191@@ -39,9 +39,8 @@
192 MOCK_METHOD1(remove_buffer, void(mg::BufferID id));
193 MOCK_METHOD1(receive_buffer, void(mg::BufferID id));
194 MOCK_METHOD1(send_buffer, void(mg::BufferID id));
195- MOCK_METHOD1(at, std::shared_ptr<mg::Buffer>&(mg::BufferID));
196 MOCK_CONST_METHOD0(client_owned_buffer_count, size_t());
197- std::shared_ptr<mg::Buffer>& operator[](mg::BufferID id) { return at(id); }
198+ MOCK_CONST_METHOD1(get, std::shared_ptr<mg::Buffer>(mg::BufferID));
199 };
200
201 struct FixedSchedule : mc::Schedule
202
203=== modified file 'tests/unit-tests/compositor/test_stream.cpp'
204--- tests/unit-tests/compositor/test_stream.cpp 2017-01-18 02:29:37 +0000
205+++ tests/unit-tests/compositor/test_stream.cpp 2017-03-02 12:43:44 +0000
206@@ -64,9 +64,9 @@
207 }
208 void send_buffer(mg::BufferID id)
209 {
210- sink.send_buffer(mf::BufferStreamId{33}, *operator[](id), mg::BufferIpcMsgType::update_msg);
211+ sink.send_buffer(mf::BufferStreamId{33}, *get(id), mg::BufferIpcMsgType::update_msg);
212 }
213- std::shared_ptr<mg::Buffer>& operator[](mg::BufferID id)
214+ std::shared_ptr<mg::Buffer> get(mg::BufferID id) const
215 {
216 auto it = std::find_if(buffers.begin(), buffers.end(),
217 [id](std::shared_ptr<mg::Buffer> const& b)
218
219=== modified file 'tests/unit-tests/scene/test_surface_stack.cpp'
220--- tests/unit-tests/scene/test_surface_stack.cpp 2017-02-15 14:45:41 +0000
221+++ tests/unit-tests/scene/test_surface_stack.cpp 2017-03-02 12:43:44 +0000
222@@ -292,7 +292,7 @@
223
224 struct StubBuffers : mtd::StubClientBuffers
225 {
226- std::shared_ptr<mg::Buffer>& operator[](mg::BufferID) override
227+ std::shared_ptr<mg::Buffer> get(mg::BufferID) const override
228 {
229 return buffer;
230 }

Subscribers

People subscribed via source and target branches