Merge lp:~robertcarr/mir/weak-session-for-session-mediator into lp:~mir-team/mir/trunk

[Robert Carr]

SessionMediator must hold only a weak reference to the session.

This is a stab at fixing the stress test, as discussed here: https://code.launchpad.net/~robertcarr/mir/session-transactions/+merge/175669 there are several ways open_session/close_session can throw (the get_surface + hold shared_ptr to msh::Surface 'race') leaving the session mediator in an invalid state, causing a fatal exception when it tries to say repeatedly close the session (from ~SessionMediator).

The SessionMediator now holds a weak reference to the session, with only the session container holding the strong reference. I think this is in general just a more consistent application of shared_ptrs, however it also fixes the double call to close_session which is throwing from ~SessionMediator.

[Robert Carr (robertcarr)]

So far I've been able to run the stress test for 30 minutes.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1007
http://jenkins.qa.ubuntu.com/job/mir-ci/1375/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1844
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1729
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/613/console

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

review: Needs Fixing (continuous-integration)

[Robert Carr (robertcarr)]

So excited about the stress tests I forgot to run the unit tests ;)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1008
http://jenkins.qa.ubuntu.com/job/mir-ci/1376/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1845
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1730
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/614
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/614/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Linked the bug which I assume is the one you're trying to fix...

[Daniel van Vugt (vanvugt)]

Tested. The bug is half solved, so long as you don't use the mouse much.

Fixed:
terminate called after throwing an instance of 'boost::exception_detail::clone_impl<boost::exception_detail::error_info_injector<std::logic_error> >'
  what(): Invalid Session

Still reproduceable with mir_stress and some input on other clients:

terminate called after throwing an instance of 'boost::exception_detail::clone_impl<boost::exception_detail::error_info_injector<std::logic_error> >'
  what(): Requesting handle for an unregistered channel

Perhaps we need to take this as a half fix? Or divide the bug itself in two?

review: Needs Information

[Daniel van Vugt (vanvugt)]

This is a rather important fix, in theory. And it works for me... We can log a new bug for the remaining input-related crash.

I'm also ignoring the minor indentation and whitespace mistakes.

review: Approve

[Thomas Voß (thomas-voss)]

LGTM. As Daniel, I'm ignoring the whitespace and indentation noise for now.

review: Approve

=== modified file 'include/server/mir/frontend/session_mediator.h'
--- include/server/mir/frontend/session_mediator.h	2013-08-23 15:27:41 +0000
+++ include/server/mir/frontend/session_mediator.h	2013-08-23 22:22:27 +0000
@@ -123,7 +123,7 @@
     std::shared_ptr<graphics::Buffer> client_buffer_resource;
 
     std::mutex session_mutex;
-    std::shared_ptr<Session> session;
+    std::weak_ptr<Session> weak_session;
 };
 
 }

=== modified file 'include/test/mir_test_doubles/stub_shell.h'
--- include/test/mir_test_doubles/stub_shell.h	2013-08-13 22:13:41 +0000
+++ include/test/mir_test_doubles/stub_shell.h	2013-08-23 22:22:27 +0000
@@ -29,11 +29,14 @@
 namespace doubles
 {
 
-class StubShell : public frontend::Shell
+struct StubShell : public frontend::Shell
 {
+    StubShell() : stub_session(std::make_shared<StubSession>())
+    {
+    }
     std::shared_ptr<frontend::Session> open_session(std::string const& /* name */, std::shared_ptr<frontend::EventSink> const& /* sink */) override
     {
-        return std::make_shared<StubSession>();
+        return stub_session;
     }
     void close_session(std::shared_ptr<frontend::Session> const& /* session */) override
     {
@@ -46,6 +49,7 @@
     void handle_surface_created(std::shared_ptr<frontend::Session> const& /* session */) override
     {
     }
+    std::shared_ptr<StubSession> const stub_session;
 };
 
 }

=== modified file 'src/server/frontend/session_mediator.cpp'
--- src/server/frontend/session_mediator.cpp	2013-08-23 15:27:41 +0000
+++ src/server/frontend/session_mediator.cpp	2013-08-23 22:22:27 +0000
@@ -73,6 +73,7 @@
 
 mf::SessionMediator::~SessionMediator() noexcept
 {
+    auto session = weak_session.lock();
     if (session)
     {
         report->session_error(session->name(), __PRETTY_FUNCTION__, "connection dropped without disconnect");
@@ -90,9 +91,9 @@
 
     {
         std::unique_lock<std::mutex> lock(session_mutex);
-        session = shell->open_session(request->application_name(), event_sink);
+        weak_session = shell->open_session(request->application_name(), event_sink);
     }
-
+    
     auto ipc_package = graphics_platform->get_ipc_package();
     auto platform = response->mutable_platform();
 
@@ -117,42 +118,40 @@
     mir::protobuf::Surface* response,
     google::protobuf::Closure* done)
 {
+    std::unique_lock<std::mutex> lock(session_mutex);
+
+    auto session = weak_session.lock();
+    
+    if (session.get() == nullptr)
+        BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
+    
+    report->session_create_surface_called(session->name());
+
+    auto const id = session->create_surface(msh::SurfaceCreationParameters()
+        .of_name(request->surface_name())
+        .of_size(request->width(), request->height())
+        .of_buffer_usage(static_cast<graphics::BufferUsage>(request->buffer_usage()))
+        .of_pixel_format(static_cast<geometry::PixelFormat>(request->pixel_format()))
+        .with_output_id(graphics::DisplayConfigurationOutputId(request->output_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 = session->create_surface(
-            msh::SurfaceCreationParameters()
-            .of_name(request->surface_name())
-            .of_size(request->width(), request->height())
-            .of_buffer_usage(static_cast<graphics::BufferUsage>(request->buffer_usage()))
-            .of_pixel_format(static_cast<geometry::PixelFormat>(request->pixel_format()))
-            .with_output_id(graphics::DisplayConfigurationOutputId(request->output_id()))
-            );
-         {
-             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);
-         }
+        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);
     }
 
     // TODO: NOTE: We use the ordering here to ensure the shell acts on the surface after the surface ID is sent over the wire.
@@ -171,6 +170,8 @@
 {
     {
         std::unique_lock<std::mutex> lock(session_mutex);
+        
+        auto session = weak_session.lock();
 
         if (session.get() == nullptr)
             BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
@@ -204,6 +205,8 @@
     {
         std::unique_lock<std::mutex> lock(session_mutex);
 
+        auto session = weak_session.lock();
+
         if (session.get() == nullptr)
             BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
 
@@ -227,13 +230,15 @@
     {
         std::unique_lock<std::mutex> lock(session_mutex);
 
+        auto session = weak_session.lock();
+
         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();
+        weak_session.reset();
     }
 
     done->Run();
@@ -254,6 +259,8 @@
     {
         std::unique_lock<std::mutex> lock(session_mutex);
 
+        auto session = weak_session.lock();
+
         if (session.get() == nullptr)
             BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
 
@@ -277,6 +284,11 @@
 {
     {
         std::unique_lock<std::mutex> lock(session_mutex);
+        auto session = weak_session.lock();
+
+        if (session.get() == nullptr)
+            BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
+
         auto config = display_changer->active_configuration();
         for (auto i=0; i < request->display_output_size(); i++)
         {   

=== modified file 'src/server/frontend/session_mediator_android.cpp'
--- src/server/frontend/session_mediator_android.cpp	2013-07-05 16:40:27 +0000
+++ src/server/frontend/session_mediator_android.cpp	2013-08-23 22:22:27 +0000
@@ -31,6 +31,8 @@
 {
     {
         std::unique_lock<std::mutex> lock(session_mutex);
+        
+        auto session = weak_session.lock();
 
         if (session.get() == nullptr)
             BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));

=== modified file 'src/server/frontend/session_mediator_gbm.cpp'
--- src/server/frontend/session_mediator_gbm.cpp	2013-07-05 16:40:27 +0000
+++ src/server/frontend/session_mediator_gbm.cpp	2013-08-23 22:22:27 +0000
@@ -38,6 +38,7 @@
 {
     {
         std::unique_lock<std::mutex> lock(session_mutex);
+        auto session = weak_session.lock();
 
         if (session.get() == nullptr)
             BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));