Mir

Merge lp:~alan-griffiths/mir/SessionMediator-session-race into lp:~mir-team/mir/trunk

Proposed by Alan Griffiths on 2013-07-05
Status: Merged
Approved by: Alexandros Frantzis on 2013-07-08
Approved revision: 818
Merged at revision: 824
Proposed branch: lp:~alan-griffiths/mir/SessionMediator-session-race
Merge into: lp:~mir-team/mir/trunk
Diff against target: 274 lines (+110/-76)
4 files modified
include/server/mir/frontend/session_mediator.h (+3/-0)
src/server/frontend/session_mediator.cpp (+91/-68)
src/server/frontend/session_mediator_android.cpp (+8/-4)
src/server/frontend/session_mediator_gbm.cpp (+8/-4)
To merge this branch: bzr merge lp:~alan-griffiths/mir/SessionMediator-session-race
Reviewer Review Type Date Requested Status
Alexandros Frantzis (community) Approve on 2013-07-08
Robert Ancell Approve on 2013-07-07
Kevin DuBois (community) 2013-07-05 Approve on 2013-07-05
PS Jenkins bot (community) continuous-integration Approve on 2013-07-05
Review via email: mp+173247@code.launchpad.net

Commit message

frontend: Guard SessionMediator::session against race conditions

Description of the change

frontend: Guard SessionMediator::session against race conditions

This may be related to intermittent failures - e.g. 1198238 and/or 1195089 (I can't reproduce either reliably)

But nothing stopped SessionMediator::session being updated and accessed on separate threads.

To post a comment you must log in.
Kevin DuBois (kdub) wrote :

yeah, the unlocked session member needs locking!

review: Approve
Robert Ancell (robert-ancell) wrote :

Confirmed bug 1195089 still occurs in this branch. Otherwise looks good.

review: Approve
Daniel van Vugt (vanvugt) wrote :

See also: valgrind --tool=helgrind

Alexandros Frantzis (afrantzis) wrote :

Looks good.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'include/server/mir/frontend/session_mediator.h'
2--- include/server/mir/frontend/session_mediator.h 2013-06-25 05:42:49 +0000
3+++ include/server/mir/frontend/session_mediator.h 2013-07-05 16:42:26 +0000
4@@ -24,6 +24,7 @@
5
6 #include <map>
7 #include <memory>
8+#include <mutex>
9
10 namespace mir
11 {
12@@ -123,6 +124,8 @@
13 std::shared_ptr<ClientBufferTracker> const client_tracker;
14
15 std::shared_ptr<compositor::Buffer> client_buffer_resource;
16+
17+ std::mutex session_mutex;
18 std::shared_ptr<Session> session;
19 };
20
21
22=== modified file 'src/server/frontend/session_mediator.cpp'
23--- src/server/frontend/session_mediator.cpp 2013-06-25 19:27:35 +0000
24+++ src/server/frontend/session_mediator.cpp 2013-07-05 16:42:26 +0000
25@@ -78,7 +78,10 @@
26 {
27 report->session_connect_called(request->application_name());
28
29- session = shell->open_session(request->application_name(), event_sink);
30+ {
31+ std::unique_lock<std::mutex> lock(session_mutex);
32+ session = shell->open_session(request->application_name(), event_sink);
33+ }
34
35 auto ipc_package = graphics_platform->get_ipc_package();
36 auto platform = response->mutable_platform();
37@@ -109,42 +112,46 @@
38 mir::protobuf::Surface* response,
39 google::protobuf::Closure* done)
40 {
41- if (session.get() == nullptr)
42- BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
43-
44- report->session_create_surface_called(session->name());
45-
46- auto const id = shell->create_surface_for(session,
47- msh::SurfaceCreationParameters()
48- .of_name(request->surface_name())
49- .of_size(request->width(), request->height())
50- .of_buffer_usage(static_cast<compositor::BufferUsage>(request->buffer_usage()))
51- .of_pixel_format(static_cast<geometry::PixelFormat>(request->pixel_format()))
52- );
53-
54 {
55- auto surface = session->get_surface(id);
56- response->mutable_id()->set_value(id.as_value());
57- response->set_width(surface->size().width.as_uint32_t());
58- response->set_height(surface->size().height.as_uint32_t());
59- response->set_pixel_format((int)surface->pixel_format());
60- response->set_buffer_usage(request->buffer_usage());
61-
62- if (surface->supports_input())
63- response->add_fd(surface->client_input_fd());
64-
65- client_buffer_resource = surface->advance_client_buffer();
66- auto const& id = client_buffer_resource->id();
67-
68- auto buffer = response->mutable_buffer();
69- buffer->set_buffer_id(id.as_uint32_t());
70-
71- if (!client_tracker->client_has(id))
72+ std::unique_lock<std::mutex> lock(session_mutex);
73+
74+ if (session.get() == nullptr)
75+ BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
76+
77+ report->session_create_surface_called(session->name());
78+
79+ auto const id = shell->create_surface_for(session,
80+ msh::SurfaceCreationParameters()
81+ .of_name(request->surface_name())
82+ .of_size(request->width(), request->height())
83+ .of_buffer_usage(static_cast<compositor::BufferUsage>(request->buffer_usage()))
84+ .of_pixel_format(static_cast<geometry::PixelFormat>(request->pixel_format()))
85+ );
86+
87 {
88- auto packer = std::make_shared<mfd::ProtobufBufferPacker>(buffer);
89- graphics_platform->fill_ipc_package(packer, client_buffer_resource);
90+ auto surface = session->get_surface(id);
91+ response->mutable_id()->set_value(id.as_value());
92+ response->set_width(surface->size().width.as_uint32_t());
93+ response->set_height(surface->size().height.as_uint32_t());
94+ response->set_pixel_format((int)surface->pixel_format());
95+ response->set_buffer_usage(request->buffer_usage());
96+
97+ if (surface->supports_input())
98+ response->add_fd(surface->client_input_fd());
99+
100+ client_buffer_resource = surface->advance_client_buffer();
101+ auto const& id = client_buffer_resource->id();
102+
103+ auto buffer = response->mutable_buffer();
104+ buffer->set_buffer_id(id.as_uint32_t());
105+
106+ if (!client_tracker->client_has(id))
107+ {
108+ auto packer = std::make_shared<mfd::ProtobufBufferPacker>(buffer);
109+ graphics_platform->fill_ipc_package(packer, client_buffer_resource);
110+ }
111+ client_tracker->add(id);
112 }
113- client_tracker->add(id);
114 }
115
116 done->Run();
117@@ -156,15 +163,19 @@
118 ::mir::protobuf::Buffer* response,
119 ::google::protobuf::Closure* done)
120 {
121- if (session.get() == nullptr)
122- BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
123-
124- report->session_next_buffer_called(session->name());
125-
126- auto surface = session->get_surface(SurfaceId(request->value()));
127-
128- client_buffer_resource.reset();
129- client_buffer_resource = surface->advance_client_buffer();
130+ {
131+ std::unique_lock<std::mutex> lock(session_mutex);
132+
133+ if (session.get() == nullptr)
134+ BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
135+
136+ report->session_next_buffer_called(session->name());
137+
138+ auto surface = session->get_surface(SurfaceId(request->value()));
139+
140+ client_buffer_resource.reset();
141+ client_buffer_resource = surface->advance_client_buffer();
142+ }
143
144 auto const& id = client_buffer_resource->id();
145 response->set_buffer_id(id.as_uint32_t());
146@@ -184,14 +195,18 @@
147 mir::protobuf::Void*,
148 google::protobuf::Closure* done)
149 {
150- if (session.get() == nullptr)
151- BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
152-
153- report->session_release_surface_called(session->name());
154-
155- auto const id = SurfaceId(request->value());
156-
157- session->destroy_surface(id);
158+ {
159+ std::unique_lock<std::mutex> lock(session_mutex);
160+
161+ if (session.get() == nullptr)
162+ BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
163+
164+ report->session_release_surface_called(session->name());
165+
166+ auto const id = SurfaceId(request->value());
167+
168+ session->destroy_surface(id);
169+ }
170
171 done->Run();
172 }
173@@ -202,13 +217,17 @@
174 mir::protobuf::Void* /*response*/,
175 google::protobuf::Closure* done)
176 {
177- if (session.get() == nullptr)
178- BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
179-
180- report->session_disconnect_called(session->name());
181-
182- shell->close_session(session);
183- session.reset();
184+ {
185+ std::unique_lock<std::mutex> lock(session_mutex);
186+
187+ if (session.get() == nullptr)
188+ BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
189+
190+ report->session_disconnect_called(session->name());
191+
192+ shell->close_session(session);
193+ session.reset();
194+ }
195
196 done->Run();
197 }
198@@ -225,16 +244,20 @@
199 response->mutable_surfaceid()->CopyFrom(request->surfaceid());
200 response->set_attrib(attrib);
201
202- if (session.get() == nullptr)
203- BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
204-
205- report->session_configure_surface_called(session->name());
206-
207- auto const id = frontend::SurfaceId(request->surfaceid().value());
208- int value = request->ivalue();
209- int newvalue = session->configure_surface(id, attrib, value);
210-
211- response->set_ivalue(newvalue);
212+ {
213+ std::unique_lock<std::mutex> lock(session_mutex);
214+
215+ if (session.get() == nullptr)
216+ BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
217+
218+ report->session_configure_surface_called(session->name());
219+
220+ auto const id = frontend::SurfaceId(request->surfaceid().value());
221+ int value = request->ivalue();
222+ int newvalue = session->configure_surface(id, attrib, value);
223+
224+ response->set_ivalue(newvalue);
225+ }
226
227 done->Run();
228 }
229
230=== modified file 'src/server/frontend/session_mediator_android.cpp'
231--- src/server/frontend/session_mediator_android.cpp 2013-05-30 03:50:54 +0000
232+++ src/server/frontend/session_mediator_android.cpp 2013-07-05 16:42:26 +0000
233@@ -29,10 +29,14 @@
234 mir::protobuf::DRMAuthMagicStatus* /*response*/,
235 google::protobuf::Closure* /*done*/)
236 {
237- if (session.get() == nullptr)
238- BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
239-
240- report->session_drm_auth_magic_called(session->name());
241+ {
242+ std::unique_lock<std::mutex> lock(session_mutex);
243+
244+ if (session.get() == nullptr)
245+ BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
246+
247+ report->session_drm_auth_magic_called(session->name());
248+ }
249
250 BOOST_THROW_EXCEPTION(std::logic_error("drm_auth_magic request not supported by the Android platform"));
251 }
252
253=== modified file 'src/server/frontend/session_mediator_gbm.cpp'
254--- src/server/frontend/session_mediator_gbm.cpp 2013-05-30 03:50:54 +0000
255+++ src/server/frontend/session_mediator_gbm.cpp 2013-07-05 16:42:26 +0000
256@@ -36,10 +36,14 @@
257 mir::protobuf::DRMAuthMagicStatus* response,
258 google::protobuf::Closure* done)
259 {
260- if (session.get() == nullptr)
261- BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
262-
263- report->session_drm_auth_magic_called(session->name());
264+ {
265+ std::unique_lock<std::mutex> lock(session_mutex);
266+
267+ if (session.get() == nullptr)
268+ BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
269+
270+ report->session_drm_auth_magic_called(session->name());
271+ }
272
273 auto const magic = static_cast<drm_magic_t>(request->magic());
274 auto authenticator = std::dynamic_pointer_cast<mg::DRMAuthenticator>(graphics_platform);

Subscribers

People subscribed via source and target branches