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

[Alan Griffiths]

frontend: Guard SessionMediator::session against race conditions

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.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:818
http://jenkins.qa.ubuntu.com/job/mir-ci/923/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1187
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1071
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/161
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/161/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=quantal,flavor=amd64/566

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

review: Approve (continuous-integration)

[Kevin DuBois (kdub)]

yeah, the unlocked session member needs locking!

review: Approve

[Robert Ancell (robert-ancell)]

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

review: Approve

[Daniel van Vugt (vanvugt)]

See also: valgrind --tool=helgrind

[Alexandros Frantzis (afrantzis)]

Looks good.

review: Approve

=== 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 @@
 
 #include <map>
 #include <memory>
+#include <mutex>
 
 namespace mir
 {
@@ -123,6 +124,8 @@
     std::shared_ptr<ClientBufferTracker> const client_tracker;
 
     std::shared_ptr<compositor::Buffer> client_buffer_resource;
+
+    std::mutex session_mutex;
     std::shared_ptr<Session> session;
 };
 
 
=== 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 @@
 {
     report->session_connect_called(request->application_name());
 
-    session = shell->open_session(request->application_name(), event_sink);
+    {
+        std::unique_lock<std::mutex> lock(session_mutex);
+        session = shell->open_session(request->application_name(), event_sink);
+    }
 
     auto ipc_package = graphics_platform->get_ipc_package();
     auto platform = response->mutable_platform();
@@ -109,42 +112,46 @@
     mir::protobuf::Surface* response,
     google::protobuf::Closure* done)
 {
-    if (session.get() == nullptr)
-        BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
-
-    report->session_create_surface_called(session->name());
-
-    auto const id = shell->create_surface_for(session,
-        msh::SurfaceCreationParameters()
-        .of_name(request->surface_name())
-        .of_size(request->width(), request->height())
-        .of_buffer_usage(static_cast<compositor::BufferUsage>(request->buffer_usage()))
-        .of_pixel_format(static_cast<geometry::PixelFormat>(request->pixel_format()))
-        );
-
     {
-        auto surface = session->get_surface(id);
-        response->mutable_id()->set_value(id.as_value());
-        response->set_width(surface->size().width.as_uint32_t());
-        response->set_height(surface->size().height.as_uint32_t());
-        response->set_pixel_format((int)surface->pixel_format());
-        response->set_buffer_usage(request->buffer_usage());
-
-        if (surface->supports_input())
-            response->add_fd(surface->client_input_fd());
-
-        client_buffer_resource = surface->advance_client_buffer();
-        auto const& id = client_buffer_resource->id();
-
-        auto buffer = response->mutable_buffer();
-        buffer->set_buffer_id(id.as_uint32_t());
-
-        if (!client_tracker->client_has(id))
+        std::unique_lock<std::mutex> lock(session_mutex);
+
+        if (session.get() == nullptr)
+            BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
+
+        report->session_create_surface_called(session->name());
+
+        auto const id = shell->create_surface_for(session,
+            msh::SurfaceCreationParameters()
+            .of_name(request->surface_name())
+            .of_size(request->width(), request->height())
+            .of_buffer_usage(static_cast<compositor::BufferUsage>(request->buffer_usage()))
+            .of_pixel_format(static_cast<geometry::PixelFormat>(request->pixel_format()))
+            );
+
         {
-            auto packer = std::make_shared<mfd::ProtobufBufferPacker>(buffer);
-            graphics_platform->fill_ipc_package(packer, client_buffer_resource);
+            auto surface = session->get_surface(id);
+            response->mutable_id()->set_value(id.as_value());
+            response->set_width(surface->size().width.as_uint32_t());
+            response->set_height(surface->size().height.as_uint32_t());
+            response->set_pixel_format((int)surface->pixel_format());
+            response->set_buffer_usage(request->buffer_usage());
+
+            if (surface->supports_input())
+                response->add_fd(surface->client_input_fd());
+
+            client_buffer_resource = surface->advance_client_buffer();
+            auto const& id = client_buffer_resource->id();
+
+            auto buffer = response->mutable_buffer();
+            buffer->set_buffer_id(id.as_uint32_t());
+
+            if (!client_tracker->client_has(id))
+            {
+                auto packer = std::make_shared<mfd::ProtobufBufferPacker>(buffer);
+                graphics_platform->fill_ipc_package(packer, client_buffer_resource);
+            }
+            client_tracker->add(id);
         }
-        client_tracker->add(id);
     }
 
     done->Run();
@@ -156,15 +163,19 @@
     ::mir::protobuf::Buffer* response,
     ::google::protobuf::Closure* done)
 {
-    if (session.get() == nullptr)
-        BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
-
-    report->session_next_buffer_called(session->name());
-
-    auto surface = session->get_surface(SurfaceId(request->value()));
-
-    client_buffer_resource.reset();
-    client_buffer_resource = surface->advance_client_buffer();
+    {
+        std::unique_lock<std::mutex> lock(session_mutex);
+
+        if (session.get() == nullptr)
+            BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
+
+        report->session_next_buffer_called(session->name());
+
+        auto surface = session->get_surface(SurfaceId(request->value()));
+
+        client_buffer_resource.reset();
+        client_buffer_resource = surface->advance_client_buffer();
+    }
 
     auto const& id = client_buffer_resource->id();
     response->set_buffer_id(id.as_uint32_t());
@@ -184,14 +195,18 @@
     mir::protobuf::Void*,
     google::protobuf::Closure* done)
 {
-    if (session.get() == nullptr)
-        BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
-
-    report->session_release_surface_called(session->name());
-
-    auto const id = SurfaceId(request->value());
-
-    session->destroy_surface(id);
+    {
+        std::unique_lock<std::mutex> lock(session_mutex);
+
+        if (session.get() == nullptr)
+            BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
+
+        report->session_release_surface_called(session->name());
+
+        auto const id = SurfaceId(request->value());
+
+        session->destroy_surface(id);
+    }
 
     done->Run();
 }
@@ -202,13 +217,17 @@
     mir::protobuf::Void* /*response*/,
     google::protobuf::Closure* done)
 {
-    if (session.get() == nullptr)
-        BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
-
-    report->session_disconnect_called(session->name());
-
-    shell->close_session(session);
-    session.reset();
+    {
+        std::unique_lock<std::mutex> lock(session_mutex);
+
+        if (session.get() == nullptr)
+            BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
+
+        report->session_disconnect_called(session->name());
+
+        shell->close_session(session);
+        session.reset();
+    }
 
     done->Run();
 }
@@ -225,16 +244,20 @@
     response->mutable_surfaceid()->CopyFrom(request->surfaceid());
     response->set_attrib(attrib);
 
-    if (session.get() == nullptr)
-        BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
-
-    report->session_configure_surface_called(session->name());
-
-    auto const id = frontend::SurfaceId(request->surfaceid().value());
-    int value = request->ivalue();
-    int newvalue = session->configure_surface(id, attrib, value);
-
-    response->set_ivalue(newvalue);
+    {
+        std::unique_lock<std::mutex> lock(session_mutex);
+
+        if (session.get() == nullptr)
+            BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
+
+        report->session_configure_surface_called(session->name());
+
+        auto const id = frontend::SurfaceId(request->surfaceid().value());
+        int value = request->ivalue();
+        int newvalue = session->configure_surface(id, attrib, value);
+
+        response->set_ivalue(newvalue);
+    }
 
     done->Run();
 }
 
=== 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 @@
     mir::protobuf::DRMAuthMagicStatus* /*response*/,
     google::protobuf::Closure* /*done*/)
 {
-    if (session.get() == nullptr)
-        BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
-
-    report->session_drm_auth_magic_called(session->name());
+    {
+        std::unique_lock<std::mutex> lock(session_mutex);
+
+        if (session.get() == nullptr)
+            BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
+
+        report->session_drm_auth_magic_called(session->name());
+    }
 
     BOOST_THROW_EXCEPTION(std::logic_error("drm_auth_magic request not supported by the Android platform"));
 }
 
=== 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 @@
     mir::protobuf::DRMAuthMagicStatus* response,
     google::protobuf::Closure* done)
 {
-    if (session.get() == nullptr)
-        BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
-
-    report->session_drm_auth_magic_called(session->name());
+    {
+        std::unique_lock<std::mutex> lock(session_mutex);
+
+        if (session.get() == nullptr)
+            BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
+
+        report->session_drm_auth_magic_called(session->name());
+    }
 
     auto const magic = static_cast<drm_magic_t>(request->magic());
     auto authenticator = std::dynamic_pointer_cast<mg::DRMAuthenticator>(graphics_platform);