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
=== modified file 'include/server/mir/frontend/session_mediator.h'
--- include/server/mir/frontend/session_mediator.h 2013-06-25 05:42:49 +0000
+++ include/server/mir/frontend/session_mediator.h 2013-07-05 16:42:26 +0000
@@ -24,6 +24,7 @@
2424
25#include <map>25#include <map>
26#include <memory>26#include <memory>
27#include <mutex>
2728
28namespace mir29namespace mir
29{30{
@@ -123,6 +124,8 @@
123 std::shared_ptr<ClientBufferTracker> const client_tracker;124 std::shared_ptr<ClientBufferTracker> const client_tracker;
124125
125 std::shared_ptr<compositor::Buffer> client_buffer_resource;126 std::shared_ptr<compositor::Buffer> client_buffer_resource;
127
128 std::mutex session_mutex;
126 std::shared_ptr<Session> session;129 std::shared_ptr<Session> session;
127};130};
128131
129132
=== modified file 'src/server/frontend/session_mediator.cpp'
--- src/server/frontend/session_mediator.cpp 2013-06-25 19:27:35 +0000
+++ src/server/frontend/session_mediator.cpp 2013-07-05 16:42:26 +0000
@@ -78,7 +78,10 @@
78{78{
79 report->session_connect_called(request->application_name());79 report->session_connect_called(request->application_name());
8080
81 session = shell->open_session(request->application_name(), event_sink);81 {
82 std::unique_lock<std::mutex> lock(session_mutex);
83 session = shell->open_session(request->application_name(), event_sink);
84 }
8285
83 auto ipc_package = graphics_platform->get_ipc_package();86 auto ipc_package = graphics_platform->get_ipc_package();
84 auto platform = response->mutable_platform();87 auto platform = response->mutable_platform();
@@ -109,42 +112,46 @@
109 mir::protobuf::Surface* response,112 mir::protobuf::Surface* response,
110 google::protobuf::Closure* done)113 google::protobuf::Closure* done)
111{114{
112 if (session.get() == nullptr)
113 BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
114
115 report->session_create_surface_called(session->name());
116
117 auto const id = shell->create_surface_for(session,
118 msh::SurfaceCreationParameters()
119 .of_name(request->surface_name())
120 .of_size(request->width(), request->height())
121 .of_buffer_usage(static_cast<compositor::BufferUsage>(request->buffer_usage()))
122 .of_pixel_format(static_cast<geometry::PixelFormat>(request->pixel_format()))
123 );
124
125 {115 {
126 auto surface = session->get_surface(id);116 std::unique_lock<std::mutex> lock(session_mutex);
127 response->mutable_id()->set_value(id.as_value());117
128 response->set_width(surface->size().width.as_uint32_t());118 if (session.get() == nullptr)
129 response->set_height(surface->size().height.as_uint32_t());119 BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
130 response->set_pixel_format((int)surface->pixel_format());120
131 response->set_buffer_usage(request->buffer_usage());121 report->session_create_surface_called(session->name());
132122
133 if (surface->supports_input())123 auto const id = shell->create_surface_for(session,
134 response->add_fd(surface->client_input_fd());124 msh::SurfaceCreationParameters()
135125 .of_name(request->surface_name())
136 client_buffer_resource = surface->advance_client_buffer();126 .of_size(request->width(), request->height())
137 auto const& id = client_buffer_resource->id();127 .of_buffer_usage(static_cast<compositor::BufferUsage>(request->buffer_usage()))
138128 .of_pixel_format(static_cast<geometry::PixelFormat>(request->pixel_format()))
139 auto buffer = response->mutable_buffer();129 );
140 buffer->set_buffer_id(id.as_uint32_t());130
141
142 if (!client_tracker->client_has(id))
143 {131 {
144 auto packer = std::make_shared<mfd::ProtobufBufferPacker>(buffer);132 auto surface = session->get_surface(id);
145 graphics_platform->fill_ipc_package(packer, client_buffer_resource);133 response->mutable_id()->set_value(id.as_value());
134 response->set_width(surface->size().width.as_uint32_t());
135 response->set_height(surface->size().height.as_uint32_t());
136 response->set_pixel_format((int)surface->pixel_format());
137 response->set_buffer_usage(request->buffer_usage());
138
139 if (surface->supports_input())
140 response->add_fd(surface->client_input_fd());
141
142 client_buffer_resource = surface->advance_client_buffer();
143 auto const& id = client_buffer_resource->id();
144
145 auto buffer = response->mutable_buffer();
146 buffer->set_buffer_id(id.as_uint32_t());
147
148 if (!client_tracker->client_has(id))
149 {
150 auto packer = std::make_shared<mfd::ProtobufBufferPacker>(buffer);
151 graphics_platform->fill_ipc_package(packer, client_buffer_resource);
152 }
153 client_tracker->add(id);
146 }154 }
147 client_tracker->add(id);
148 }155 }
149156
150 done->Run();157 done->Run();
@@ -156,15 +163,19 @@
156 ::mir::protobuf::Buffer* response,163 ::mir::protobuf::Buffer* response,
157 ::google::protobuf::Closure* done)164 ::google::protobuf::Closure* done)
158{165{
159 if (session.get() == nullptr)166 {
160 BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));167 std::unique_lock<std::mutex> lock(session_mutex);
161168
162 report->session_next_buffer_called(session->name());169 if (session.get() == nullptr)
163170 BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
164 auto surface = session->get_surface(SurfaceId(request->value()));171
165172 report->session_next_buffer_called(session->name());
166 client_buffer_resource.reset();173
167 client_buffer_resource = surface->advance_client_buffer();174 auto surface = session->get_surface(SurfaceId(request->value()));
175
176 client_buffer_resource.reset();
177 client_buffer_resource = surface->advance_client_buffer();
178 }
168179
169 auto const& id = client_buffer_resource->id();180 auto const& id = client_buffer_resource->id();
170 response->set_buffer_id(id.as_uint32_t());181 response->set_buffer_id(id.as_uint32_t());
@@ -184,14 +195,18 @@
184 mir::protobuf::Void*,195 mir::protobuf::Void*,
185 google::protobuf::Closure* done)196 google::protobuf::Closure* done)
186{197{
187 if (session.get() == nullptr)198 {
188 BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));199 std::unique_lock<std::mutex> lock(session_mutex);
189200
190 report->session_release_surface_called(session->name());201 if (session.get() == nullptr)
191202 BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
192 auto const id = SurfaceId(request->value());203
193204 report->session_release_surface_called(session->name());
194 session->destroy_surface(id);205
206 auto const id = SurfaceId(request->value());
207
208 session->destroy_surface(id);
209 }
195210
196 done->Run();211 done->Run();
197}212}
@@ -202,13 +217,17 @@
202 mir::protobuf::Void* /*response*/,217 mir::protobuf::Void* /*response*/,
203 google::protobuf::Closure* done)218 google::protobuf::Closure* done)
204{219{
205 if (session.get() == nullptr)220 {
206 BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));221 std::unique_lock<std::mutex> lock(session_mutex);
207222
208 report->session_disconnect_called(session->name());223 if (session.get() == nullptr)
209224 BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
210 shell->close_session(session);225
211 session.reset();226 report->session_disconnect_called(session->name());
227
228 shell->close_session(session);
229 session.reset();
230 }
212231
213 done->Run();232 done->Run();
214}233}
@@ -225,16 +244,20 @@
225 response->mutable_surfaceid()->CopyFrom(request->surfaceid());244 response->mutable_surfaceid()->CopyFrom(request->surfaceid());
226 response->set_attrib(attrib);245 response->set_attrib(attrib);
227246
228 if (session.get() == nullptr)247 {
229 BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));248 std::unique_lock<std::mutex> lock(session_mutex);
230249
231 report->session_configure_surface_called(session->name());250 if (session.get() == nullptr)
232251 BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
233 auto const id = frontend::SurfaceId(request->surfaceid().value());252
234 int value = request->ivalue();253 report->session_configure_surface_called(session->name());
235 int newvalue = session->configure_surface(id, attrib, value);254
236255 auto const id = frontend::SurfaceId(request->surfaceid().value());
237 response->set_ivalue(newvalue);256 int value = request->ivalue();
257 int newvalue = session->configure_surface(id, attrib, value);
258
259 response->set_ivalue(newvalue);
260 }
238261
239 done->Run();262 done->Run();
240}263}
241264
=== modified file 'src/server/frontend/session_mediator_android.cpp'
--- src/server/frontend/session_mediator_android.cpp 2013-05-30 03:50:54 +0000
+++ src/server/frontend/session_mediator_android.cpp 2013-07-05 16:42:26 +0000
@@ -29,10 +29,14 @@
29 mir::protobuf::DRMAuthMagicStatus* /*response*/,29 mir::protobuf::DRMAuthMagicStatus* /*response*/,
30 google::protobuf::Closure* /*done*/)30 google::protobuf::Closure* /*done*/)
31{31{
32 if (session.get() == nullptr)32 {
33 BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));33 std::unique_lock<std::mutex> lock(session_mutex);
3434
35 report->session_drm_auth_magic_called(session->name());35 if (session.get() == nullptr)
36 BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
37
38 report->session_drm_auth_magic_called(session->name());
39 }
3640
37 BOOST_THROW_EXCEPTION(std::logic_error("drm_auth_magic request not supported by the Android platform"));41 BOOST_THROW_EXCEPTION(std::logic_error("drm_auth_magic request not supported by the Android platform"));
38}42}
3943
=== modified file 'src/server/frontend/session_mediator_gbm.cpp'
--- src/server/frontend/session_mediator_gbm.cpp 2013-05-30 03:50:54 +0000
+++ src/server/frontend/session_mediator_gbm.cpp 2013-07-05 16:42:26 +0000
@@ -36,10 +36,14 @@
36 mir::protobuf::DRMAuthMagicStatus* response,36 mir::protobuf::DRMAuthMagicStatus* response,
37 google::protobuf::Closure* done)37 google::protobuf::Closure* done)
38{38{
39 if (session.get() == nullptr)39 {
40 BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));40 std::unique_lock<std::mutex> lock(session_mutex);
4141
42 report->session_drm_auth_magic_called(session->name());42 if (session.get() == nullptr)
43 BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
44
45 report->session_drm_auth_magic_called(session->name());
46 }
4347
44 auto const magic = static_cast<drm_magic_t>(request->magic());48 auto const magic = static_cast<drm_magic_t>(request->magic());
45 auto authenticator = std::dynamic_pointer_cast<mg::DRMAuthenticator>(graphics_platform);49 auto authenticator = std::dynamic_pointer_cast<mg::DRMAuthenticator>(graphics_platform);

Subscribers

People subscribed via source and target branches