Merge lp:~alan-griffiths/mir/cleanup-some-locking into lp:mir

[Alan Griffiths]

client: rework dubious locking design in client rpc
(LP: #1243576) (LP: #1243584) (LP: #1243578) (LP: #1243575)

client: rework dubious locking design in client rpc

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1309
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/572/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/507
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/503
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/111/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/298
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/298/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/301
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/301/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/111
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/111/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/139/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/2586

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/572/rebuild

review: Needs Fixing (continuous-integration)

[Daniel van Vugt (vanvugt)]

Verified bug 1243584 fixed.
Bug 1243576 remains... unlinking it.

[Daniel van Vugt (vanvugt)]

To test and verify the bugs;
sudo valgrind --tool=helgrind --log-file=helgrind.log bin/mir_demo_client_egltriangle

The news is pretty good already as branch halves the size of the log.

[Daniel van Vugt (vanvugt)]

(1) Bug 1243576 is still happening. Please fix it or unlink.

(2) Adding error_message_mutex seems to not achieve anything. Because the string returned by MirConnection::get_error_message() remains unprotected.

(3) std::mutex mutex; // Protects all members of *this that don't have their own
Is there a logical or performance reason why the class has multiple mutexes? If neither, then it would be easier to follow using just one.

review: Needs Fixing

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1314
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/614/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/564
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/560
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/168/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/340
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/340/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/343
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/343/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/168
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/168/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/193/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/2723

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/614/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1317
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/616/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/566
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/562
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/170/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/342
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/342/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/345
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/345/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/170
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/170/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/194/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/2735

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/616/rebuild

review: Needs Fixing (continuous-integration)

[Daniel van Vugt (vanvugt)]

(1)-(3) Nice. All fixed, I think. As possibly more (linked 4 bugs now).

(4) surface_map is no longer protected by a mutex in MirConnection. But now I see it has its own built-in. That might be worth a comment where it's called by MirConnection.

(5) This is getting very complex and hard to review now. Not least because the diff doesn't show what's protected by each lock_guard. I suggest aiming to only fix one bug at a time in future :)

review: Approve

[Daniel van Vugt (vanvugt)]

I was able to reproduce Jenkins' failure of ServerDisconnect.client_detects_server_shutdown on mako. But only once and not repeatably, so can't prove it's due to this branch.

[Alan Griffiths (alan-griffiths)]

> I was able to reproduce Jenkins' failure of
> ServerDisconnect.client_detects_server_shutdown on mako. But only once and not
> repeatably, so can't prove it's due to this branch.

Thanks for trying - I've not been able to reproduce (yet) and your success gives me hope.

As it appears consistently on this branch in CI I'm assuming it is either due to the branch or exposed by it. Either way I need to track it down to land this.

[Alan Griffiths (alan-griffiths)]

> > I was able to reproduce Jenkins' failure of
> > ServerDisconnect.client_detects_server_shutdown on mako. But only once and
> not
> > repeatably, so can't prove it's due to this branch.
>
> Thanks for trying - I've not been able to reproduce (yet) and your success
> gives me hope.
>
> As it appears consistently on this branch in CI I'm assuming it is either due
> to the branch or exposed by it. Either way I need to track it down to land
> this.

Right, I've been able to reproduce and also on *development-branch && desktop*.

This seems to be a race condition in the client side error reporting - something like detecting the broken pipe on the sending thread "before" processing the disconnect event message on the receiving thread.

I've not yet figured out where it breaks, or why this MP is particularly susceptible on the phone. But look for a separate MP.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1320
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/624/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/576
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/572
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/180
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/350
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/350/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/353
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/353/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/180
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/180/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/203
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/2778

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/624/rebuild

review: Approve (continuous-integration)

=== modified file 'src/client/mir_client_library.cpp'
--- src/client/mir_client_library.cpp	2013-12-18 02:19:19 +0000
+++ src/client/mir_client_library.cpp	2014-01-07 15:59:47 +0000
@@ -108,9 +108,8 @@
     }
     catch (std::exception const& x)
     {
-        MirConnection* error_connection = new MirConnection();
+        MirConnection* error_connection = new MirConnection(x.what());
         error_connections.insert(error_connection);
-        error_connection->set_error_message(x.what());
         callback(error_connection, context);
         return nullptr;
     }

=== modified file 'src/client/mir_connection.cpp'
--- src/client/mir_connection.cpp	2013-12-20 05:06:28 +0000
+++ src/client/mir_connection.cpp	2014-01-07 15:59:47 +0000
@@ -66,10 +66,10 @@
 std::unordered_set<MirConnection*> valid_connections;
 }
 
-MirConnection::MirConnection() :
+MirConnection::MirConnection(std::string const& error_message) :
     channel(),
     server(0),
-    error_message("ERROR")
+    error_message(error_message)
 {
 }
 
@@ -84,11 +84,11 @@
         lifecycle_control(conf.the_lifecycle_control()),
         surface_map(conf.the_surface_map())
 {
+    connect_result.set_error("connect not called");
     {
         std::lock_guard<std::mutex> lock(connection_guard);
         valid_connections.insert(this);
     }
-    connect_result.set_error("connect not called");
 }
 
 MirConnection::~MirConnection() noexcept
@@ -97,9 +97,12 @@
     // But, if after 500ms we don't get a call, assume it won't happen.
     connect_wait_handle.wait_for_pending(std::chrono::milliseconds(500));
 
-    std::lock_guard<std::mutex> lock(connection_guard);
-    valid_connections.erase(this);
+    {
+        std::lock_guard<std::mutex> lock(connection_guard);
+        valid_connections.erase(this);
+    }
 
+    std::lock_guard<decltype(mutex)> lock(mutex);
     if (connect_result.has_platform())
     {
         auto const& platform = connect_result.platform();
@@ -113,8 +116,6 @@
     mir_surface_callback callback,
     void * context)
 {
-    std::lock_guard<std::recursive_mutex> lock(mutex);
-
     auto surface = new MirSurface(this, server, platform->create_buffer_factory(), input_platform, params, callback, context);
 
     return surface->get_create_wait_handle();
@@ -122,22 +123,18 @@
 
 char const * MirConnection::get_error_message()
 {
-    std::lock_guard<std::recursive_mutex> lock(mutex);
+    std::lock_guard<decltype(mutex)> lock(mutex);
 
     if (connect_result.has_error())
     {
         return connect_result.error().c_str();
     }
-    else
-    {
+
     return error_message.c_str();
-    }
 }
 
 void MirConnection::set_error_message(std::string const& error)
 {
-    std::lock_guard<std::recursive_mutex> lock(mutex);
-
     error_message = error;
 }
 
@@ -154,10 +151,8 @@
 
 void MirConnection::released(SurfaceRelease data)
 {
-    {
-        std::lock_guard<std::recursive_mutex> lock(mutex);
-        surface_map->erase(data.surface->id());
-    }
+    surface_map->erase(data.surface->id());
+
     // Erasing this surface from surface_map means that it will no longer receive events
     // If it's still focused, send an unfocused event before we kill it entirely
     if (data.surface->attrib(mir_surface_attrib_focus) == mir_surface_focused)
@@ -179,8 +174,6 @@
         mir_surface_callback callback,
         void * context)
 {
-    std::lock_guard<std::recursive_mutex> lock(mutex);
-
     auto new_wait_handle = new MirWaitHandle;
 
     SurfaceRelease surf_release{surface, new_wait_handle, callback, context};
@@ -189,7 +182,7 @@
     message.set_value(surface->id());
 
     {
-        std::lock_guard<std::mutex> rel_lock(release_wait_handle_guard);
+        std::lock_guard<decltype(release_wait_handle_guard)> rel_lock(release_wait_handle_guard);
         release_wait_handles.push_back(new_wait_handle);
     }
 
@@ -216,7 +209,7 @@
 {
     bool safe_to_callback = true;
     {
-        std::lock_guard<std::recursive_mutex> lock(mutex);
+        std::lock_guard<decltype(mutex)> lock(mutex);
 
         if (!connect_result.has_platform() || !connect_result.has_display_configuration())
         {
@@ -248,10 +241,13 @@
     mir_connected_callback callback,
     void * context)
 {
-    std::lock_guard<std::recursive_mutex> lock(mutex);
-
-    connect_parameters.set_application_name(app_name);
-    connect_wait_handle.expect_result();
+    {
+        std::lock_guard<decltype(mutex)> lock(mutex);
+
+        connect_parameters.set_application_name(app_name);
+        connect_wait_handle.expect_result();
+    }
+
     server.connect(
         0,
         &connect_parameters,
@@ -266,7 +262,7 @@
     /* todo: keeping all MirWaitHandles from a release surface until the end of the connection
        is a kludge until we have a better story about the lifetime of MirWaitHandles */
     {
-        std::lock_guard<std::mutex> lock(release_wait_handle_guard);
+        std::lock_guard<decltype(release_wait_handle_guard)> lock(release_wait_handle_guard);
         for (auto handle : release_wait_handles)
             delete handle;
     }
@@ -276,8 +272,6 @@
 
 MirWaitHandle* MirConnection::disconnect()
 {
-    std::lock_guard<std::recursive_mutex> lock(mutex);
-
     server.disconnect(0, &ignored, &ignored,
                       google::protobuf::NewCallback(this, &MirConnection::done_disconnect));
 
@@ -297,8 +291,6 @@
                                              mir_drm_auth_magic_callback callback,
                                              void* context)
 {
-    std::lock_guard<std::recursive_mutex> lock(mutex);
-
     mir::protobuf::DRMMagic request;
     request.set_magic(magic);
 
@@ -320,12 +312,13 @@
            return false;
     }
 
+    std::lock_guard<decltype(connection->mutex)> lock(connection->mutex);
     return !connection->connect_result.has_error();
 }
 
 void MirConnection::populate(MirPlatformPackage& platform_package)
 {
-    std::lock_guard<std::recursive_mutex> lock(mutex);
+    std::lock_guard<decltype(mutex)> lock(mutex);
 
     if (!connect_result.has_error() && connect_result.has_platform())
     {
@@ -351,7 +344,7 @@
 
 MirDisplayConfiguration* MirConnection::create_copy_of_display_config()
 {
-    std::lock_guard<std::recursive_mutex> lock(mutex);
+    std::lock_guard<decltype(mutex)> lock(mutex);
     return display_configuration->copy_to_client();
 }
 
@@ -361,6 +354,8 @@
     unsigned int& valid_formats)
 {
     valid_formats = 0;
+
+    std::lock_guard<decltype(mutex)> lock(mutex);
     if (!connect_result.has_error())
     {
         valid_formats = std::min(
@@ -376,7 +371,7 @@
 
 std::shared_ptr<mir::client::ClientPlatform> MirConnection::get_client_platform()
 {
-    std::lock_guard<std::recursive_mutex> lock(mutex);
+    std::lock_guard<decltype(mutex)> lock(mutex);
 
     return platform;
 }
@@ -388,7 +383,7 @@
 
 EGLNativeDisplayType MirConnection::egl_native_display()
 {
-    std::lock_guard<std::recursive_mutex> lock(mutex);
+    std::lock_guard<decltype(mutex)> lock(mutex);
 
     return *native_display;
 }
@@ -410,8 +405,6 @@
 
 bool MirConnection::validate_user_display_config(MirDisplayConfiguration* config)
 {
-    std::lock_guard<std::recursive_mutex> lock(mutex);
-
     MirDisplayConfigurationStore orig_config{display_configuration->copy_to_client()};
 
     if ((!config) || (config->num_outputs == 0) || (config->outputs == NULL) ||
@@ -437,6 +430,8 @@
 
 void MirConnection::done_display_configure()
 {
+    std::lock_guard<decltype(mutex)> lock(mutex);
+
     set_error_message(display_configuration_response.error());
 
     if (!display_configuration_response.has_error())
@@ -447,23 +442,26 @@
 
 MirWaitHandle* MirConnection::configure_display(MirDisplayConfiguration* config)
 {
-    std::lock_guard<std::recursive_mutex> lock(mutex);
     if (!validate_user_display_config(config))
     {
         return NULL;
     }
 
     mir::protobuf::DisplayConfiguration request;
-    for (auto i=0u; i < config->num_outputs; i++)
     {
-        auto output = config->outputs[i];
-        auto display_request = request.add_display_output();
-        display_request->set_output_id(output.output_id);
-        display_request->set_used(output.used);
-        display_request->set_current_mode(output.current_mode);
-        display_request->set_position_x(output.position_x);
-        display_request->set_position_y(output.position_y);
-        display_request->set_power_mode(output.power_mode);
+        std::lock_guard<decltype(mutex)> lock(mutex);
+
+        for (auto i=0u; i < config->num_outputs; i++)
+        {
+            auto output = config->outputs[i];
+            auto display_request = request.add_display_output();
+            display_request->set_output_id(output.output_id);
+            display_request->set_used(output.used);
+            display_request->set_current_mode(output.current_mode);
+            display_request->set_position_x(output.position_x);
+            display_request->set_position_y(output.position_y);
+            display_request->set_power_mode(output.power_mode);
+        }
     }
 
     server.configure_display(0, &request, &display_configuration_response,
@@ -475,6 +473,8 @@
 bool MirConnection::set_extra_platform_data(
     std::vector<int> const& extra_platform_data_arg)
 {
+    std::lock_guard<decltype(mutex)> lock(mutex);
+
     auto const total_data_size =
         connect_result.platform().data_size() + extra_platform_data_arg.size();
 

=== modified file 'src/client/mir_connection.h'
--- src/client/mir_connection.h	2013-12-20 05:06:28 +0000
+++ src/client/mir_connection.h	2014-01-07 15:59:47 +0000
@@ -69,7 +69,7 @@
 struct MirConnection : mir::client::ClientContext
 {
 public:
-    MirConnection();
+    MirConnection(std::string const& error_message);
 
     MirConnection(mir::client::ConnectionConfiguration& conf);
     ~MirConnection() noexcept;
@@ -87,7 +87,6 @@
             void *context);
 
     char const * get_error_message();
-    void set_error_message(std::string const& error);
 
     MirWaitHandle* connect(
         const char* app_name,
@@ -125,7 +124,7 @@
     bool set_extra_platform_data(std::vector<int> const& extra_platform_data);
 
 private:
-    std::recursive_mutex mutex; // Protects all members of *this
+    std::mutex mutex; // Protects all members of *this (except release_wait_handles)
 
     std::shared_ptr<mir::client::rpc::MirBasicRpcChannel> channel;
     mir::protobuf::DisplayServer::Stub server;
@@ -163,6 +162,7 @@
 
     struct SurfaceRelease;
 
+    void set_error_message(std::string const& error);
     void done_disconnect();
     void connected(mir_connected_callback callback, void * context);
     void released(SurfaceRelease );

=== modified file 'src/client/mir_surface.cpp'
--- src/client/mir_surface.cpp	2014-01-03 05:37:54 +0000
+++ src/client/mir_surface.cpp	2014-01-07 15:59:47 +0000
@@ -64,7 +64,7 @@
 
 MirSurface::~MirSurface()
 {
-    std::lock_guard<std::recursive_mutex> lock(mutex);
+    std::lock_guard<decltype(mutex)> lock(mutex);
 
     if (input_thread)
     {
@@ -80,7 +80,7 @@
 
 MirSurfaceParameters MirSurface::get_parameters() const
 {
-    std::lock_guard<std::recursive_mutex> lock(mutex);
+    std::lock_guard<decltype(mutex)> lock(mutex);
 
     return MirSurfaceParameters {
         0,
@@ -93,7 +93,7 @@
 
 char const * MirSurface::get_error_message()
 {
-    std::lock_guard<std::recursive_mutex> lock(mutex);
+    std::lock_guard<decltype(mutex)> lock(mutex);
 
     if (surface.has_error())
     {
@@ -104,21 +104,21 @@
 
 int MirSurface::id() const
 {
-    std::lock_guard<std::recursive_mutex> lock(mutex);
+    std::lock_guard<decltype(mutex)> lock(mutex);
 
     return surface.id().value();
 }
 
 bool MirSurface::is_valid() const
 {
-    std::lock_guard<std::recursive_mutex> lock(mutex);
+    std::lock_guard<decltype(mutex)> lock(mutex);
 
     return !surface.has_error();
 }
 
 void MirSurface::get_cpu_region(MirGraphicsRegion& region_out)
 {
-    std::lock_guard<std::recursive_mutex> lock(mutex);
+    std::lock_guard<decltype(mutex)> lock(mutex);
 
     auto buffer = buffer_depository->current_buffer();
 
@@ -132,21 +132,21 @@
 
 void MirSurface::release_cpu_region()
 {
-    std::lock_guard<std::recursive_mutex> lock(mutex);
-
     secured_region.reset();
 }
 
 MirWaitHandle* MirSurface::next_buffer(mir_surface_callback callback, void * context)
 {
-    std::lock_guard<std::recursive_mutex> lock(mutex);
-
+    std::unique_lock<decltype(mutex)> lock(mutex);
     release_cpu_region();
+    auto const id = &surface.id();
+    auto const mutable_buffer = surface.mutable_buffer();
+    lock.unlock();
 
     server.next_buffer(
         0,
-        &surface.id(),
-        surface.mutable_buffer(),
+        id,
+        mutable_buffer,
         google::protobuf::NewCallback(this, &MirSurface::new_buffer, callback, context));
 
     return &next_buffer_wait_handle;
@@ -166,8 +166,6 @@
 
 void MirSurface::process_incoming_buffer()
 {
-    std::lock_guard<std::recursive_mutex> lock(mutex);
-
     auto const& buffer = surface.buffer();
 
     /*
@@ -201,17 +199,16 @@
 
 void MirSurface::created(mir_surface_callback callback, void * context)
 {
+    auto platform = connection->get_client_platform();
+
     {
-        std::lock_guard<std::recursive_mutex> lock(mutex);
+        std::lock_guard<decltype(mutex)> lock(mutex);
 
         process_incoming_buffer();
-
-        auto platform = connection->get_client_platform();
         accelerated_window = platform->create_egl_native_window(this);
-
-        connection->on_surface_created(id(), this);
     }
 
+    connection->on_surface_created(id(), this);
     callback(this, context);
     create_wait_handle.result_received();
 }
@@ -219,7 +216,7 @@
 void MirSurface::new_buffer(mir_surface_callback callback, void * context)
 {
     {
-        std::lock_guard<std::recursive_mutex> lock(mutex);
+        std::lock_guard<decltype(mutex)> lock(mutex);
         process_incoming_buffer();
     }
 
@@ -244,23 +241,21 @@
 
 std::shared_ptr<mcl::ClientBuffer> MirSurface::get_current_buffer()
 {
-    std::lock_guard<std::recursive_mutex> lock(mutex);
+    std::lock_guard<decltype(mutex)> lock(mutex);
 
     return buffer_depository->current_buffer();
 }
 
 uint32_t MirSurface::get_current_buffer_id() const
 {
-    std::lock_guard<std::recursive_mutex> lock(mutex);
+    std::lock_guard<decltype(mutex)> lock(mutex);
 
     return buffer_depository->current_buffer_id();
 }
 
 void MirSurface::populate(MirBufferPackage& buffer_package)
 {
-    std::lock_guard<std::recursive_mutex> lock(mutex);
-
-    if (is_valid() && surface.has_buffer())
+    if (!surface.has_error() && surface.has_buffer())
     {
         auto const& buffer = surface.buffer();
 
@@ -292,19 +287,19 @@
 
 EGLNativeWindowType MirSurface::generate_native_window()
 {
-    std::lock_guard<std::recursive_mutex> lock(mutex);
+    std::lock_guard<decltype(mutex)> lock(mutex);
 
     return *accelerated_window;
 }
 
 MirWaitHandle* MirSurface::configure(MirSurfaceAttrib at, int value)
 {
-    std::lock_guard<std::recursive_mutex> lock(mutex);
-
+    std::unique_lock<decltype(mutex)> lock(mutex);
     mp::SurfaceSetting setting;
     setting.mutable_surfaceid()->CopyFrom(surface.id());
     setting.set_attrib(at);
     setting.set_ivalue(value);
+    lock.unlock();
 
     configure_wait_handle.expect_result();
     server.configure_surface(0, &setting, &configure_result,
@@ -315,7 +310,7 @@
 
 void MirSurface::on_configured()
 {
-    std::lock_guard<std::recursive_mutex> lock(mutex);
+    std::lock_guard<decltype(mutex)> lock(mutex);
 
     if (configure_result.has_surfaceid() &&
         configure_result.surfaceid().value() == surface.id().value() &&
@@ -345,14 +340,14 @@
 
 int MirSurface::attrib(MirSurfaceAttrib at) const
 {
-    std::lock_guard<std::recursive_mutex> lock(mutex);
+    std::lock_guard<decltype(mutex)> lock(mutex);
 
     return attrib_cache[at];
 }
 
 void MirSurface::set_event_handler(MirEventDelegate const* delegate)
 {
-    std::lock_guard<std::recursive_mutex> lock(mutex);
+    std::lock_guard<decltype(mutex)> lock(mutex);
 
     if (input_thread)
     {
@@ -378,7 +373,7 @@
 
 void MirSurface::handle_event(MirEvent const& e)
 {
-    std::unique_lock<std::recursive_mutex> lock(mutex);
+    std::unique_lock<decltype(mutex)> lock(mutex);
 
     if (e.type == mir_event_type_surface)
     {
@@ -397,7 +392,7 @@
 
 MirPlatformType MirSurface::platform_type()
 {
-    std::lock_guard<std::recursive_mutex> lock(mutex);
+    std::lock_guard<decltype(mutex)> lock(mutex);
 
     auto platform = connection->get_client_platform();
     return platform->platform_type();

=== modified file 'src/client/mir_surface.h'
--- src/client/mir_surface.h	2013-12-18 02:19:19 +0000
+++ src/client/mir_surface.h	2014-01-07 15:59:47 +0000
@@ -83,7 +83,6 @@
     std::shared_ptr<mir::client::ClientBuffer> get_current_buffer();
     uint32_t get_current_buffer_id() const;
     void get_cpu_region(MirGraphicsRegion& region);
-    void release_cpu_region();
     EGLNativeWindowType generate_native_window();
 
     MirWaitHandle* configure(MirSurfaceAttrib a, int value);
@@ -93,7 +92,7 @@
     void handle_event(MirEvent const& e);
 
 private:
-    mutable std::recursive_mutex mutex; // Protects all members of *this
+    mutable std::mutex mutex; // Protects all members of *this
 
     void on_configured();
     void process_incoming_buffer();
@@ -101,6 +100,7 @@
     void created(mir_surface_callback callback, void * context);
     void new_buffer(mir_surface_callback callback, void * context);
     MirPixelFormat convert_ipc_pf_to_geometry(google::protobuf::int32 pf);
+    void release_cpu_region();
 
     /* todo: race condition. protobuf does not guarantee that callbacks will be synchronized. potential
              race in surface, last_buffer_id */
@@ -108,7 +108,7 @@
     mir::protobuf::Surface surface;
     std::string error_message;
 
-    MirConnection *connection;
+    MirConnection* const connection;
     MirWaitHandle create_wait_handle;
     MirWaitHandle next_buffer_wait_handle;
     MirWaitHandle configure_wait_handle;

=== modified file 'src/client/rpc/mir_basic_rpc_channel.cpp'
--- src/client/rpc/mir_basic_rpc_channel.cpp	2013-09-26 13:23:44 +0000
+++ src/client/rpc/mir_basic_rpc_channel.cpp	2014-01-07 15:59:47 +0000
@@ -33,40 +33,39 @@
 {
 }
 
-mclrd::SendBuffer& mclrd::PendingCallCache::save_completion_details(
+void mclrd::PendingCallCache::save_completion_details(
     mir::protobuf::wire::Invocation& invoke,
     google::protobuf::Message* response,
     std::shared_ptr<google::protobuf::Closure> const& complete)
 {
     std::unique_lock<std::mutex> lock(mutex);
 
-    auto& current = pending_calls[invoke.id()] = PendingCall(response, complete);
-    return current.send_buffer;
+    pending_calls[invoke.id()] = PendingCall(response, complete);
 }
 
 void mclrd::PendingCallCache::complete_response(mir::protobuf::wire::Result& result)
 {
-    std::unique_lock<std::mutex> lock(mutex);
-    auto call = pending_calls.find(result.id());
-    if (call == pending_calls.end())
+    PendingCall completion;
+
+    {
+        std::unique_lock<std::mutex> lock(mutex);
+        auto call = pending_calls.find(result.id());
+        if (call != pending_calls.end())
+        {
+            completion = call->second;
+            pending_calls.erase(call);
+        }
+    }
+
+    if (!completion.complete)
     {
         rpc_report->orphaned_result(result);
     }
     else
     {
-        auto& completion = call->second;
-
         rpc_report->complete_response(result);
-
         completion.response->ParseFromString(result.response());
-
-        // Let the completion closure live a bit longer than our lock...
-        std::shared_ptr<google::protobuf::Closure> complete =
-            completion.complete;
-        pending_calls.erase(call);
-
-        lock.unlock();  // Avoid deadlocks in callbacks
-        complete->Run();
+        completion.complete->Run();
     }
 }
 

=== modified file 'src/client/rpc/mir_basic_rpc_channel.h'
--- src/client/rpc/mir_basic_rpc_channel.h	2013-09-26 13:23:44 +0000
+++ src/client/rpc/mir_basic_rpc_channel.h	2014-01-07 15:59:47 +0000
@@ -54,7 +54,7 @@
 public:
     PendingCallCache(std::shared_ptr<RpcReport> const& rpc_report);
 
-    SendBuffer& save_completion_details(
+    void save_completion_details(
         mir::protobuf::wire::Invocation& invoke,
         google::protobuf::Message* response,
         std::shared_ptr<google::protobuf::Closure> const& complete);
@@ -78,7 +78,6 @@
         PendingCall()
         : response(0), complete() {}
 
-        SendBuffer send_buffer;
         google::protobuf::Message* response;
         std::shared_ptr<google::protobuf::Closure> complete;
     };

=== modified file 'src/client/rpc/mir_socket_rpc_channel.cpp'
--- src/client/rpc/mir_socket_rpc_channel.cpp	2013-12-18 02:19:19 +0000
+++ src/client/rpc/mir_socket_rpc_channel.cpp	2014-01-07 15:59:47 +0000
@@ -267,15 +267,14 @@
         google::protobuf::NewPermanentCallback(this, &MirSocketRpcChannel::receive_file_descriptors, response, complete));
 
     // Only save details after serialization succeeds
-    auto& send_buffer = pending_calls.save_completion_details(invocation, response, callback);
+    pending_calls.save_completion_details(invocation, response, callback);
 
     // Only send message when details saved for handling response
-    send_message(invocation, send_buffer, invocation);
+    send_message(invocation, invocation);
 }
 
 void mclr::MirSocketRpcChannel::send_message(
     mir::protobuf::wire::Invocation const& body,
-    detail::SendBuffer& send_buffer,
     mir::protobuf::wire::Invocation const& invocation)
 {
     const size_t size = body.ByteSize();
@@ -285,7 +284,7 @@
         static_cast<unsigned char>((size >> 0) & 0xff)
     };
 
-    send_buffer.resize(sizeof header_bytes + size);
+    detail::SendBuffer send_buffer(sizeof header_bytes + size);
     std::copy(header_bytes, header_bytes + sizeof header_bytes, send_buffer.begin());
     body.SerializeToArray(send_buffer.data() + sizeof header_bytes, size);
 

=== modified file 'src/client/rpc/mir_socket_rpc_channel.h'
--- src/client/rpc/mir_socket_rpc_channel.h	2013-12-18 02:19:19 +0000
+++ src/client/rpc/mir_socket_rpc_channel.h	2014-01-07 15:59:47 +0000
@@ -83,7 +83,7 @@
 
     void receive_file_descriptors(google::protobuf::Message* response, google::protobuf::Closure* complete);
     void receive_file_descriptors(std::vector<int> &fds);
-    void send_message(mir::protobuf::wire::Invocation const& body, detail::SendBuffer& buffer,
+    void send_message(mir::protobuf::wire::Invocation const& body,
                       mir::protobuf::wire::Invocation const& invocation);
     void on_header_read(const boost::system::error_code& error);
 

=== modified file 'tests/acceptance-tests/test_client_focus_notification.cpp'
--- tests/acceptance-tests/test_client_focus_notification.cpp	2013-12-18 02:19:19 +0000
+++ tests/acceptance-tests/test_client_focus_notification.cpp	2014-01-07 15:59:47 +0000
@@ -136,7 +136,7 @@
 
 }
 
-TEST_F(BespokeDisplayServerTestFixture, two_scene_are_notified_of_gaining_and_losing_focus)
+TEST_F(BespokeDisplayServerTestFixture, two_surfaces_are_notified_of_gaining_and_losing_focus)
 {
     using namespace ::testing;