Mir

Merge lp:~kdub/mir/sort-out-surface-ownership into lp:mir

sort-out-surface-ownership
Merge into development-branch

Proposed by Kevin DuBois on 2015-12-14

Status:	Merged
Approved by:	Chris Halse Rogers on 2016-01-14
Approved revision:	no longer in the source branch.
Merged at revision:	3240
Proposed branch:	lp:~kdub/mir/sort-out-surface-ownership
Merge into:	lp:mir
Diff against target:	1129 lines (+331/-299) 11 files modified src/client/connection_surface_map.h (+2/-2) src/client/mir_connection.cpp (+186/-18) src/client/mir_connection.h (+27/-4) src/client/mir_surface.cpp (+31/-179) src/client/mir_surface.h (+16/-14) src/client/mir_surface_api.cpp (+2/-4) src/client/surface_map.cpp (+4/-10) src/client/surface_map.h (+1/-0) tests/unit-tests/client/test_client_mir_surface.cpp (+20/-61) tests/unit-tests/client/test_connection_resource_map.cpp (+7/-6) tests/unit-tests/client/test_mir_connection.cpp (+35/-1)
To merge this branch:	bzr merge lp:~kdub/mir/sort-out-surface-ownership
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Chris Halse Rogers			Approve on 2016-01-14
Alberto Aguirre (community)			Approve on 2016-01-13
PS Jenkins bot (community)	continuous-integration		Approve on 2016-01-13
Mir CI Bot	continuous-integration		Approve on 2016-01-13
Alan Griffiths		2015-12-14	Approve on 2016-01-05
Review via email: mp+280481@code.launchpad.net

Commit message

client: sort out the lifetime and ownership of MirSurface. The MirSurface lifetimes are now managed by MirConnection, and SurfaceMap is where MirConnection stores the surfaces.

Previously, they were imperfectly managed by SurfaceMap, eg, MirSurface would call "delete this" on itself. This led to scenarios where SurfaceMap wasn't really sure if the surface was alive or dead. Since NBS increases the amount of Events being sent to the surface, this would tease out existing problems with this scenario around surface destruction more.

Description of the change

client: sort out the lifetime and ownership of MirSurface. The MirSurface lifetimes are now managed by MirConnection, and SurfaceMap is where MirConnection stores the surfaces.

Previously, they were imperfectly managed by SurfaceMap, eg, MirSurface would call "delete this" on itself sometimes. This led to scenarios where SurfaceMap wasn't really sure if the surface was alive or dead. Since NBS increases the amount of Events being sent to the surface, this would tease out existing problems with this scenario around surface destruction more.

I'll probably do something similar with BufferStream to reduce complexity even more (eg, SurfaceMap won't auto-add bufferstreams, and the funny deletion around l725 won't be needed anymore).

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-12-15:

FAILED: Continuous integration, rev:3197
http://jenkins.qa.ubuntu.com/job/mir-ci/5857/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5324
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/4230/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/5273/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/185/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/185/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5273/console

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

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-12-15:

FAILED: Continuous integration, rev:3197
http://jenkins.qa.ubuntu.com/job/mir-ci/5859/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5326
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/4232/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/5275/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/187/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/187/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5275/console

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

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-12-17:

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

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-12-18:

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Kevin DuBois (kdub) wrote on 2015-12-18:

^still a FD leak for me to track down, probably on monday

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-12-21:

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

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2016-01-04:

-TEST_F(MirClientSurfaceTest, create_wait_handle_really_blocks)

Is this test no longer relevant?

review: Needs Information

Revision history for this message

Kevin DuBois (kdub) wrote on 2016-01-04:

Its more of a test of the wait handle, and the logic for creating a surface has moved to the MirConnection. I suppose I can port the test over instead of just removing it

Revision history for this message

Kevin DuBois (kdub) wrote on 2016-01-04:

I think a case can be made for the removal of the test, but porting the test over was easy too.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2016-01-04:

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

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2016-01-05:

OK. (I'm not entirely convinced it is needed, but I don't see any harm.)

review: Approve

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-01-08:

PASSED: Continuous integration, rev:3205
https://mir-jenkins.ubuntu.com/job/mir-ci/18/
Executed test runs:
None: https://mir-jenkins.ubuntu.com/job/generic-update-mp/18/console

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/18/rebuild

review: Approve (continuous-integration)

Revision history for this message

Chris Halse Rogers (raof) wrote on 2016-01-10:

127 + std::lock_guard<decltype(mutex)> lock(mutex);
...
136 + callback(surf.get(), context);

You're calling into client code with an internal mutex called. That's generally a bad idea, and I think it will deadlock under plausible client usage - MirConnection::disconnect() takes the same mutex, and calling mir_connection_release() in the error case for surface construction doesn't seem too farfetched.

716 + //a bit batty, but the creation handle has to exist for as long as the MirSurface does,
717 + //as we don't really manage the lifetime of MirWaitHandle sensibly.
718 + std::shared_ptr<MirWaitHandle> const creation_handle;

I'd prefer a unique_ptr here, but that's stylistic rather than blocking.

Also, is this constraint sufficiently strong? Answer: Huh, that's the current behaviour. YAY MirWaitHandle!

review: Needs Fixing

Revision history for this message

Kevin DuBois (kdub) wrote on 2016-01-11:

@Chris, good catch, fixed the callback during the exception catching scenario. I kept the wait handle a shared ptr, was a bit easier to call at the appropriate time.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2016-01-11:

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

review: Approve (continuous-integration)

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-01-13:

PASSED: Continuous integration, rev:3209
https://mir-jenkins.ubuntu.com/job/mir-ci/35/
Executed test runs:
None: https://mir-jenkins.ubuntu.com/job/generic-update-mp/34/console

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/35/rebuild

review: Approve (continuous-integration)

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2016-01-13:

+ if (strncmp(surface->get_error_message(), "", 1))

Could just be if(!mir_surface_is_valid(surface))

review: Needs Fixing

Revision history for this message

Kevin DuBois (kdub) wrote on 2016-01-13:

@Alberto, fixed

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-01-13:

PASSED: Continuous integration, rev:3212
https://mir-jenkins.ubuntu.com/job/mir-ci/42/
Executed test runs:
None: https://mir-jenkins.ubuntu.com/job/generic-update-mp/42/console

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/42/rebuild

review: Approve (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2016-01-13:

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

review: Approve (continuous-integration)

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2016-01-13:

LGTM.

review: Approve

Revision history for this message

Chris Halse Rogers (raof) wrote on 2016-01-14:

LGTM now.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Emanuele Antonio Faraone

Gerry Boland

Kevin DuBois

Mir development team

 === modified file 'src/client/connection_surface_map.h'
 --- src/client/connection_surface_map.h	2015-12-15 12:24:10 +0000
 +++ src/client/connection_surface_map.h	2016-01-13 19:10:37 +0000
@@ -40,7 +40,7 @@
      ~ConnectionSurfaceMap() noexcept;
      void with_surface_do(frontend::SurfaceId surface_id, std::function<void(MirSurface*)> const& exec) const override;
--    void insert(frontend::SurfaceId surface_id, MirSurface* surface);
++    void insert(frontend::SurfaceId surface_id, std::shared_ptr<MirSurface> const& surface);
      void erase(frontend::SurfaceId surface_id);
      void with_stream_do(frontend::BufferStreamId stream_id, std::function<void(ClientBufferStream*)> const& exec) const override;
@@ -50,7 +50,7 @@
  private:
      std::shared_timed_mutex mutable guard;
--    std::unordered_map<frontend::SurfaceId, MirSurface*> surfaces;
++    std::unordered_map<frontend::SurfaceId, std::shared_ptr<MirSurface>> surfaces;
      std::unordered_map<frontend::BufferStreamId, StreamInfo> streams;
  };
 === modified file 'src/client/mir_connection.cpp'
 --- src/client/mir_connection.cpp	2016-01-04 07:26:59 +0000
 +++ src/client/mir_connection.cpp	2016-01-13 19:10:37 +0000
@@ -53,6 +53,76 @@
  namespace
+ {
++mir::protobuf::SurfaceParameters serialize_spec(MirSurfaceSpec const& spec)
++{
++    mp::SurfaceParameters message;
++
++#define SERIALIZE_OPTION_IF_SET(option) \
++    if (spec.option.is_set()) \
++        message.set_##option(spec.option.value());
++
++    SERIALIZE_OPTION_IF_SET(width);
++    SERIALIZE_OPTION_IF_SET(height);
++    SERIALIZE_OPTION_IF_SET(pixel_format);
++    SERIALIZE_OPTION_IF_SET(buffer_usage);
++    SERIALIZE_OPTION_IF_SET(surface_name);
++    SERIALIZE_OPTION_IF_SET(output_id);
++    SERIALIZE_OPTION_IF_SET(type);
++    SERIALIZE_OPTION_IF_SET(state);
++    SERIALIZE_OPTION_IF_SET(pref_orientation);
++    SERIALIZE_OPTION_IF_SET(edge_attachment);
++    SERIALIZE_OPTION_IF_SET(min_width);
++    SERIALIZE_OPTION_IF_SET(min_height);
++    SERIALIZE_OPTION_IF_SET(max_width);
++    SERIALIZE_OPTION_IF_SET(max_height);
++    SERIALIZE_OPTION_IF_SET(width_inc);
++    SERIALIZE_OPTION_IF_SET(height_inc);
++    // min_aspect is a special case (below)
++    // max_aspect is a special case (below)
++
++    if (spec.parent.is_set() && spec.parent.value() != nullptr)
++        message.set_parent_id(spec.parent.value()->id());
++
++    if (spec.parent_id)
++    {
++        auto id = message.mutable_parent_persistent_id();
++        id->set_value(spec.parent_id->as_string());
++    }
++
++    if (spec.aux_rect.is_set())
++    {
++        message.mutable_aux_rect()->set_left(spec.aux_rect.value().left);
++        message.mutable_aux_rect()->set_top(spec.aux_rect.value().top);
++        message.mutable_aux_rect()->set_width(spec.aux_rect.value().width);
++        message.mutable_aux_rect()->set_height(spec.aux_rect.value().height);
++    }
++
++    if (spec.min_aspect.is_set())
++    {
++        message.mutable_min_aspect()->set_width(spec.min_aspect.value().width);
++        message.mutable_min_aspect()->set_height(spec.min_aspect.value().height);
++    }
++
++    if (spec.max_aspect.is_set())
++    {
++        message.mutable_max_aspect()->set_width(spec.max_aspect.value().width);
++        message.mutable_max_aspect()->set_height(spec.max_aspect.value().height);
++    }
++
++    if (spec.input_shape.is_set())
++    {
++        for (auto const& rect : spec.input_shape.value())
++        {
++            auto const new_shape = message.add_input_shape();
++            new_shape->set_left(rect.left);
++            new_shape->set_top(rect.top);
++            new_shape->set_width(rect.width);
++            new_shape->set_height(rect.height);
++        }
++    }
++
++    return message;
++}
  class MirDisplayConfigurationStore
+ {
@@ -175,10 +245,106 @@
      mir_surface_callback callback,
      void * context)
+ {
--    auto surface = new MirSurface(this, server, &debug, buffer_stream_factory,
--        input_platform, spec, callback, context);
--
--    return surface->get_create_wait_handle();
++    auto response = std::make_shared<mp::Surface>();
++    auto c = std::make_shared<MirConnection::SurfaceCreationRequest>(callback, context, spec);
++    c->wh->expect_result();
++    auto const message = serialize_spec(spec);
++    {
++        std::lock_guard<decltype(mutex)> lock(mutex);
++        surface_requests.emplace_back(c);
++    }
++
++    try
++    {
++        server.create_surface(&message, c->response.get(),
++            gp::NewCallback(this, &MirConnection::surface_created, c.get()));
++    }
++    catch (std::exception const& ex)
++    {
++        std::unique_lock<decltype(mutex)> lock(mutex);
++        auto request = std::find_if(surface_requests.begin(), surface_requests.end(),
++            [&](std::shared_ptr<MirConnection::SurfaceCreationRequest> c2) { return c.get() == c2.get(); });
++        if (request != surface_requests.end())
++        {
++            auto id = next_error_id(lock);
++            auto surf = std::make_shared<MirSurface>(
++                std::string{"Error creating surface: "} + boost::diagnostic_information(ex), this, id, (*request)->wh);
++            surface_map->insert(id, surf);
++            auto wh = (*request)->wh;
++            surface_requests.erase(request);
++
++            lock.unlock();
++            callback(surf.get(), context);
++            wh->result_received();
++        }
++    }
++    return c->wh.get();
++}
++
++mf::SurfaceId MirConnection::next_error_id(std::unique_lock<std::mutex> const&)
++{
++    return mf::SurfaceId{surface_error_id--};
++}
++
++void MirConnection::surface_created(SurfaceCreationRequest* request)
++{
++    std::unique_lock<decltype(mutex)> lock(mutex);
++    std::shared_ptr<MirSurface> surf {nullptr};
++    std::shared_ptr<mcl::ClientBufferStream> stream {nullptr};
++    //make sure this request actually was made.
++    auto request_it = std::find_if(surface_requests.begin(), surface_requests.end(),
++        [&request](std::shared_ptr<MirConnection::SurfaceCreationRequest> r){ return request == r.get(); });
++    if (request_it == surface_requests.end())
++        return;
++
++    auto surface_proto = request->response;
++    auto callback = request->cb;
++    auto context = request->context;
++    auto const& spec = request->spec;
++
++    try
++    {
++        stream = buffer_stream_factory->make_producer_stream(
++            this, server, surface_proto->buffer_stream(), std::string{},
++            {surface_proto->width(), surface_proto->height()});
++    }
++    catch (std::exception const& error)
++    {
++        if (!surface_proto->has_error())
++            surface_proto->set_error(error.what());
++        // failed to create buffer_stream, so clean up FDs it doesn't own
++        for (auto i = 0; i < surface_proto->fd_size(); i++)
++            ::close(surface_proto->fd(i));
++        if (surface_proto->has_buffer_stream() && surface_proto->buffer_stream().has_buffer())
++            for (int i = 0; i < surface_proto->buffer_stream().buffer().fd_size(); i++)
++                ::close(surface_proto->buffer_stream().buffer().fd(i));
++    }
++
++    if (surface_proto->has_error() || !surface_proto->has_id())
++    {
++        std::string reason;
++        if (surface_proto->has_error())
++            reason += surface_proto->error();
++        if (surface_proto->has_error() && !surface_proto->has_id())
++            reason += " and ";
++        if (!surface_proto->has_id())
++            reason +=  "Server assigned surface no id";
++        auto id = next_error_id(lock);
++        surf = std::make_shared<MirSurface>(reason, this, id, request->wh);
++        surface_map->insert(id, surf);
++    }
++    else
++    {
++        surf = std::make_shared<MirSurface>(
++            this, server, &debug, stream, input_platform, spec, *surface_proto, request->wh);
++
++        surface_map->insert(mf::SurfaceId{surface_proto->id().value()}, surf);
++    }
++
++    callback(surf.get(), context);
++    request->wh->result_received();
++
++    surface_requests.erase(request_it);
+ }
  char const * MirConnection::get_error_message()
@@ -228,9 +394,7 @@
  void MirConnection::released(SurfaceRelease data)
+ {
--    surface_map->erase(mf::SurfaceId(data.surface->id()));
--
--    // Erasing this surface from surface_map means that it will no longer receive events
++    // releasing 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)
+     {
@@ -239,7 +403,7 @@
+     }
      data.callback(data.surface, data.context);
      data.handle->result_received();
--    delete data.surface;
++    surface_map->erase(mf::SurfaceId(data.surface->id()));
+ }
  MirWaitHandle* MirConnection::release_surface(
@@ -248,17 +412,26 @@
          void * context)
+ {
      auto new_wait_handle = new MirWaitHandle;
++    {
++        std::lock_guard<decltype(release_wait_handle_guard)> rel_lock(release_wait_handle_guard);
++        release_wait_handles.push_back(new_wait_handle);
++    }
++
++    if (!mir_surface_is_valid(surface))
++    {
++        new_wait_handle->expect_result();
++        new_wait_handle->result_received();
++        callback(surface, context);
++        auto id = surface->id();
++        surface_map->erase(mf::SurfaceId(id));
++        return new_wait_handle;
++    }
      SurfaceRelease surf_release{surface, new_wait_handle, callback, context};
      mp::SurfaceId message;
      message.set_value(surface->id());
--    {
--        std::lock_guard<decltype(release_wait_handle_guard)> rel_lock(release_wait_handle_guard);
--        release_wait_handles.push_back(new_wait_handle);
--    }
--
      new_wait_handle->expect_result();
      server.release_surface(&message, void_response.get(),
                             gp::NewCallback(this, &MirConnection::released, surf_release));
@@ -537,11 +710,6 @@
      surface_map->insert(mf::BufferStreamId(id), stream);
+ }
--void MirConnection::on_surface_created(int id, MirSurface* surface)
--{
--    surface_map->insert(mf::SurfaceId(id), surface);
--}
--
  void MirConnection::register_lifecycle_event_callback(mir_lifecycle_event_callback callback, void* context)
+ {
      lifecycle_control->set_callback(std::bind(callback, this, std::placeholders::_1, context));
 === modified file 'src/client/mir_connection.h'
 --- src/client/mir_connection.h	2016-01-04 21:41:00 +0000
 +++ src/client/mir_connection.h	2016-01-13 19:10:37 +0000
@@ -26,8 +26,10 @@
  #include "mir/geometry/size.h"
  #include "mir/client_platform.h"
++#include "mir/frontend/surface_id.h"
  #include "mir/client_context.h"
  #include "mir_toolkit/mir_client_library.h"
++#include "mir_surface.h"
  #include <atomic>
  #include <memory>
@@ -100,9 +102,9 @@
          mir_surface_callback callback,
          void * context);
      MirWaitHandle* release_surface(
--            MirSurface *surface,
--            mir_surface_callback callback,
--            void *context);
++        MirSurface *surface,
++        mir_surface_callback callback,
++        void *context);
      MirPromptSession* create_prompt_session();
@@ -152,7 +154,6 @@
      EGLNativeDisplayType egl_native_display();
      MirPixelFormat       egl_pixel_format(EGLDisplay, EGLConfig) const;
--    void on_surface_created(int id, MirSurface* surface);
      void on_stream_created(int id, mir::client::ClientBufferStream* stream);
      MirWaitHandle* configure_display(MirDisplayConfiguration* configuration);
@@ -167,9 +168,28 @@
+     }
      mir::client::rpc::DisplayServer& display_server();
++    mir::client::rpc::DisplayServerDebug& debug_display_server();
      std::shared_ptr<mir::logging::Logger> const& the_logger() const;
  private:
++    //google cant have callbacks with more than 2 args
++    struct SurfaceCreationRequest
++    {
++        SurfaceCreationRequest(mir_surface_callback cb, void* context,  MirSurfaceSpec const& spec) :
++            cb(cb), context(context), spec(spec),
++            response(std::make_shared<mir::protobuf::Surface>()),
++            wh(std::make_shared<MirWaitHandle>())
++        {
++        }
++        mir_surface_callback cb;
++        void* context;
++        MirSurfaceSpec const spec;
++        std::shared_ptr<mir::protobuf::Surface> response;
++        std::shared_ptr<MirWaitHandle> wh;
++    };
++    std::vector<std::shared_ptr<SurfaceCreationRequest>> surface_requests;
++    void surface_created(SurfaceCreationRequest*);
++
      void populate_server_package(MirPlatformPackage& platform_package) override;
      // MUST be first data member so it is destroyed last.
      struct Deregisterer
@@ -191,6 +211,9 @@
      std::unique_ptr<mir::protobuf::Void> set_base_display_configuration_response;
      std::atomic<bool> disconnecting{false};
++    mir::frontend::SurfaceId next_error_id(std::unique_lock<std::mutex> const&);
++    int surface_error_id{-1};
++
      std::shared_ptr<mir::client::ClientPlatformFactory> const client_platform_factory;
      std::shared_ptr<mir::client::ClientPlatform> platform;
      std::shared_ptr<EGLNativeDisplayType> native_display;
 === modified file 'src/client/mir_surface.cpp'
 --- src/client/mir_surface.cpp	2015-12-17 15:58:42 +0000
 +++ src/client/mir_surface.cpp	2016-01-13 19:10:37 +0000
@@ -49,77 +49,6 @@
+ {
  std::mutex handle_mutex;
  std::unordered_set<MirSurface*> valid_surfaces;
--
--mir::protobuf::SurfaceParameters serialize_spec(MirSurfaceSpec const& spec)
--{
--    mp::SurfaceParameters message;
--
--#define SERIALIZE_OPTION_IF_SET(option) \
--    if (spec.option.is_set()) \
--        message.set_##option(spec.option.value());
--
--    SERIALIZE_OPTION_IF_SET(width);
--    SERIALIZE_OPTION_IF_SET(height);
--    SERIALIZE_OPTION_IF_SET(pixel_format);
--    SERIALIZE_OPTION_IF_SET(buffer_usage);
--    SERIALIZE_OPTION_IF_SET(surface_name);
--    SERIALIZE_OPTION_IF_SET(output_id);
--    SERIALIZE_OPTION_IF_SET(type);
--    SERIALIZE_OPTION_IF_SET(state);
--    SERIALIZE_OPTION_IF_SET(pref_orientation);
--    SERIALIZE_OPTION_IF_SET(edge_attachment);
--    SERIALIZE_OPTION_IF_SET(min_width);
--    SERIALIZE_OPTION_IF_SET(min_height);
--    SERIALIZE_OPTION_IF_SET(max_width);
--    SERIALIZE_OPTION_IF_SET(max_height);
--    SERIALIZE_OPTION_IF_SET(width_inc);
--    SERIALIZE_OPTION_IF_SET(height_inc);
--    // min_aspect is a special case (below)
--    // max_aspect is a special case (below)
--
--    if (spec.parent.is_set() && spec.parent.value() != nullptr)
--        message.set_parent_id(spec.parent.value()->id());
--
--    if (spec.parent_id)
--    {
--        auto id = message.mutable_parent_persistent_id();
--        id->set_value(spec.parent_id->as_string());
--    }
--
--    if (spec.aux_rect.is_set())
--    {
--        message.mutable_aux_rect()->set_left(spec.aux_rect.value().left);
--        message.mutable_aux_rect()->set_top(spec.aux_rect.value().top);
--        message.mutable_aux_rect()->set_width(spec.aux_rect.value().width);
--        message.mutable_aux_rect()->set_height(spec.aux_rect.value().height);
--    }
--
--    if (spec.min_aspect.is_set())
--    {
--        message.mutable_min_aspect()->set_width(spec.min_aspect.value().width);
--        message.mutable_min_aspect()->set_height(spec.min_aspect.value().height);
--    }
--
--    if (spec.max_aspect.is_set())
--    {
--        message.mutable_max_aspect()->set_width(spec.max_aspect.value().width);
--        message.mutable_max_aspect()->set_height(spec.max_aspect.value().height);
--    }
--
--    if (spec.input_shape.is_set())
--    {
--        for (auto const& rect : spec.input_shape.value())
--        {
--            auto const new_shape = message.add_input_shape();
--            new_shape->set_left(rect.left);
--            new_shape->set_top(rect.top);
--            new_shape->set_width(rect.width);
--            new_shape->set_height(rect.height);
--        }
--    }
--
--    return message;
--}
+ }
  MirSurfaceSpec::MirSurfaceSpec(
@@ -159,10 +88,17 @@
      return string_id;
+ }
--MirSurface::MirSurface(std::string const& error)
--    : surface{mcl::make_protobuf_object<mir::protobuf::Surface>()}
++MirSurface::MirSurface(
++    std::string const& error,
++    MirConnection* conn,
++    mir::frontend::SurfaceId id,
++    std::shared_ptr<MirWaitHandle> const& handle) :
++    surface{mcl::make_protobuf_object<mir::protobuf::Surface>()},
++    connection_(conn),
++    creation_handle(handle)
+ {
      surface->set_error(error);
++    surface->mutable_id()->set_value(id.as_value());
      std::lock_guard<decltype(handle_mutex)> lock(handle_mutex);
      valid_surfaces.insert(this);
@@ -172,25 +108,30 @@
      MirConnection *allocating_connection,
      mclr::DisplayServer& the_server,
      mclr::DisplayServerDebug* debug,
--    std::shared_ptr<mcl::ClientBufferStreamFactory> const& buffer_stream_factory,
++    std::shared_ptr<mcl::ClientBufferStream> const& buffer_stream,
      std::shared_ptr<mircv::InputPlatform> const& input_platform,
      MirSurfaceSpec const& spec,
--    mir_surface_callback callback, void * context)
++    mir::protobuf::Surface const& surface_proto,
++    std::shared_ptr<MirWaitHandle> const& handle)
      : server{&the_server},
        debug{debug},
--      surface{mcl::make_protobuf_object<mir::protobuf::Surface>()},
++      surface{mcl::make_protobuf_object<mir::protobuf::Surface>(surface_proto)},
        persistent_id{mcl::make_protobuf_object<mir::protobuf::PersistentSurfaceId>()},
        name{spec.surface_name.is_set() ? spec.surface_name.value() : ""},
        void_response{mcl::make_protobuf_object<mir::protobuf::Void>()},
        modify_result{mcl::make_protobuf_object<mir::protobuf::Void>()},
--      connection(allocating_connection),
--      buffer_stream_factory(buffer_stream_factory),
++      connection_(allocating_connection),
++      buffer_stream(buffer_stream),
        input_platform(input_platform),
        keymapper(std::make_shared<mircv::XKBMapper>()),
--      configure_result{mcl::make_protobuf_object<mir::protobuf::SurfaceSetting>()}
++      configure_result{mcl::make_protobuf_object<mir::protobuf::SurfaceSetting>()},
++      creation_handle(handle)
+ {
--    for (int i = 0; i < mir_surface_attribs; i++)
--        attrib_cache[i] = -1;
++    for(int i = 0; i < surface_proto.attributes_size(); i++)
++    {
++        auto const& attrib = surface_proto.attributes(i);
++        attrib_cache[attrib.attrib()] = attrib.ivalue();
++    }
      if (spec.event_handler.is_set())
+     {
@@ -201,16 +142,10 @@
              spec.event_handler.value().context);
+     }
--    auto const message = serialize_spec(spec);
--    create_wait_handle.expect_result();
--    try
--    {
--        server->create_surface(&message, surface.get(), gp::NewCallback(this, &MirSurface::created, callback, context));
--    }
--    catch (std::exception const& ex)
--    {
--        surface->set_error(std::string{"Error invoking create surface: "} +
--                          boost::diagnostic_information(ex));
++    if (surface_proto.fd_size() > 0 && handle_event_callback)
++    {
++        input_thread = std::make_shared<md::ThreadedDispatcher>("Input dispatch",
++            input_platform->create_input_receiver( surface_proto.fd(0), keymapper, handle_event_callback));
+     }
      std::lock_guard<decltype(handle_mutex)> lock(handle_mutex);
@@ -303,11 +238,6 @@
      return &persistent_id_wait_handle;
+ }
--MirWaitHandle* MirSurface::get_create_wait_handle()
--{
--    return &create_wait_handle;
--}
--
  /* todo: all these conversion functions are a bit of a kludge, probably
           better to have a more developed MirPixelFormat that can handle this */
  MirPixelFormat MirSurface::convert_ipc_pf_to_geometry(gp::int32 pf) const
@@ -315,89 +245,6 @@
      return static_cast<MirPixelFormat>(pf);
+ }
--void MirSurface::created(mir_surface_callback callback, void * context)
--{
--    {
--    std::lock_guard<decltype(mutex)> lock(mutex);
--    if (!surface->has_id())
--    {
--        if (!surface->has_error())
--            surface->set_error("Error processing surface create response, no ID (disconnected?)");
--
--        callback(this, context);
--        create_wait_handle.result_received();
--        return;
--    }
--    }
--    try
--    {
--        {
--            geom::Size size{surface->width(), surface->height()};
--            auto stream = buffer_stream_factory->
--                make_producer_stream(connection, *server, surface->buffer_stream(), name, size);
--
--            std::lock_guard<decltype(mutex)> lock(mutex);
--            buffer_stream = std::move(stream);
--            for(int i = 0; i < surface->attributes_size(); i++)
--            {
--                auto const& attrib = surface->attributes(i);
--                attrib_cache[attrib.attrib()] = attrib.ivalue();
--            }
--        }
--
--        connection->on_surface_created(id(), this);
--    }
--    catch (std::exception const& error)
--    {
--        // failed to create buffer_stream, so clean up FDs it doesn't own
--        if (!buffer_stream)
--            for (int i = 0; i < surface->buffer_stream().buffer().fd_size(); i++)
--                ::close(surface->buffer_stream().buffer().fd(i));
--
--        surface->set_error(std::string{"Error processing Surface creating response:"} +
--                          boost::diagnostic_information(error));
--    }
--
--    if (surface->fd_size() > 0 && handle_event_callback)
--    {
--        auto input_dispatcher = input_platform->create_input_receiver(surface->fd(0),
--                                                                      keymapper,
--                                                                      handle_event_callback);
--        input_thread = std::make_shared<md::ThreadedDispatcher>("Input dispatch", input_dispatcher);
--    }
--
--    callback(this, context);
--    create_wait_handle.result_received();
--}
--
--MirWaitHandle* MirSurface::release_surface(
--        mir_surface_callback callback,
--        void * context)
--{
--    bool was_valid = false;
--    {
--        std::lock_guard<decltype(handle_mutex)> lock(handle_mutex);
--        if (valid_surfaces.count(this))
--            was_valid = true;
--        valid_surfaces.erase(this);
--    }
--    if (this->surface->has_error())
--        was_valid = false;
--
--    MirWaitHandle* wait_handle{nullptr};
--    if (connection && was_valid)
--    {
--        wait_handle = connection->release_surface(this, callback, context);
--    }
--    else
--    {
--        callback(this, context);
--        delete this;
--    }
--
--    return wait_handle;
--}
--
  MirWaitHandle* MirSurface::configure_cursor(MirCursorConfiguration const* cursor)
+ {
      mp::CursorSetting setting;
@@ -779,3 +626,8 @@
      return &modify_wait_handle;
+ }
++
++MirConnection* MirSurface::connection() const
++{
++    return connection_;
++}
 === modified file 'src/client/mir_surface.h'
 --- src/client/mir_surface.h	2015-12-17 15:51:08 +0000
 +++ src/client/mir_surface.h	2016-01-13 19:10:37 +0000
@@ -25,6 +25,7 @@
  #include "rpc/mir_display_server_debug.h"
  #include "mir/client_platform.h"
++#include "mir/frontend/surface_id.h"
  #include "mir/optional_value.h"
  #include "mir/geometry/dimensions.h"
  #include "mir_toolkit/common.h"
@@ -100,7 +101,7 @@
      mir::optional_value<MirOrientationMode> pref_orientation;
      mir::optional_value<MirSurface*> parent;
--    std::unique_ptr<MirPersistentId> parent_id;
++    std::shared_ptr<MirPersistentId> parent_id;
      mir::optional_value<MirRectangle> aux_rect;
      mir::optional_value<MirEdgeAttachment> edge_attachment;
@@ -140,28 +141,27 @@
      MirSurface(MirSurface const &) = delete;
      MirSurface& operator=(MirSurface const &) = delete;
--    MirSurface(std::string const& error);
++    MirSurface(
++        std::string const& error,
++        MirConnection *allocating_connection,
++        mir::frontend::SurfaceId surface_id,
++        std::shared_ptr<MirWaitHandle> const& handle);
      MirSurface(
          MirConnection *allocating_connection,
          mir::client::rpc::DisplayServer& server,
          mir::client::rpc::DisplayServerDebug* debug,
--        std::shared_ptr<mir::client::ClientBufferStreamFactory> const& buffer_stream_factory,
++        std::shared_ptr<mir::client::ClientBufferStream> const& buffer_stream,
          std::shared_ptr<mir::input::receiver::InputPlatform> const& input_platform,
--        MirSurfaceSpec const& spec,
--        mir_surface_callback callback, void * context);
++        MirSurfaceSpec const& spec, mir::protobuf::Surface const& surface_proto,
++        std::shared_ptr<MirWaitHandle> const& handle);
      ~MirSurface();
--    MirWaitHandle* release_surface(
--        mir_surface_callback callback,
--        void *context);
--
      MirSurfaceParameters get_parameters() const;
      char const * get_error_message();
      int id() const;
--    MirWaitHandle* get_create_wait_handle();
      MirWaitHandle* configure(MirSurfaceAttrib a, int value);
      // TODO: Some sort of extension mechanism so that this can be moved
@@ -192,12 +192,12 @@
      static bool is_valid(MirSurface* query);
      MirWaitHandle* request_persistent_id(mir_surface_id_callback callback, void* context);
++    MirConnection* connection() const;
  private:
      std::mutex mutable mutex; // Protects all members of *this
      void on_configured();
      void on_cursor_configured();
--    void created(mir_surface_callback callback, void* context);
      void acquired_persistent_id(mir_surface_id_callback callback, void* context);
      MirPixelFormat convert_ipc_pf_to_geometry(google::protobuf::int32 pf) const;
@@ -213,14 +213,12 @@
      MirWaitHandle modify_wait_handle;
      std::unique_ptr<mir::protobuf::Void> const modify_result;
--    MirConnection* const connection{nullptr};
++    MirConnection* const connection_;
--    MirWaitHandle create_wait_handle;
      MirWaitHandle configure_wait_handle;
      MirWaitHandle configure_cursor_wait_handle;
      MirWaitHandle persistent_id_wait_handle;
--    std::shared_ptr<mir::client::ClientBufferStreamFactory> const buffer_stream_factory;
      std::shared_ptr<mir::client::ClientBufferStream> buffer_stream;
      std::shared_ptr<mir::input::receiver::InputPlatform> const input_platform;
      std::shared_ptr<mir::input::receiver::XKBMapper> const keymapper;
@@ -234,6 +232,10 @@
      std::function<void(MirEvent const*)> handle_event_callback;
      std::shared_ptr<mir::dispatch::ThreadedDispatcher> input_thread;
      bool auto_resize_stream{true};
++
++    //a bit batty, but the creation handle has to exist for as long as the MirSurface does,
++    //as we don't really manage the lifetime of MirWaitHandle sensibly.
++    std::shared_ptr<MirWaitHandle> const creation_handle;
  };
  #endif /* MIR_CLIENT_PRIVATE_MIR_WAIT_HANDLE_H_ */
 === modified file 'src/client/mir_surface_api.cpp'
 --- src/client/mir_surface_api.cpp	2015-10-14 16:16:08 +0000
 +++ src/client/mir_surface_api.cpp	2016-01-13 19:10:37 +0000
@@ -152,9 +152,6 @@
+     }
      catch (std::exception const& error)
+     {
--        auto error_surf = new MirSurface{std::string{"Failed to create surface: "} +
--                                         boost::diagnostic_information(error)};
--        (*callback)(error_surf, context);
          return nullptr;
+     }
+ }
@@ -258,9 +255,10 @@
      MirSurface* surface,
      mir_surface_callback callback, void* context)
+ {
++    auto connection = surface->connection();
      try
+     {
--        return surface->release_surface(callback, context);
++        return connection->release_surface(surface, callback, context);
+     }
      catch (std::exception const& ex)
+     {
 === modified file 'src/client/surface_map.cpp'
 --- src/client/surface_map.cpp	2015-12-15 15:42:37 +0000
 +++ src/client/surface_map.cpp	2016-01-13 19:10:37 +0000
@@ -31,7 +31,7 @@
  mcl::ConnectionSurfaceMap::~ConnectionSurfaceMap() noexcept
+ {
--    std::unordered_map<frontend::SurfaceId, MirSurface*> surface_map;
++    std::unordered_map<frontend::SurfaceId, std::shared_ptr<MirSurface>> surface_map;
      std::unordered_map<frontend::BufferStreamId, StreamInfo> stream_map;
+     {
          //Prevent TSAN from flagging lock ordering issues
@@ -42,14 +42,8 @@
          stream_map = std::move(streams);
+     }
--    // Unless the client has screwed up there should be no surfaces left
++    // Unless the client has screwed up there should be no streams left
      // here. (OTOH *we* don't need to leak memory when clients screw up.)
--    for (auto const& surface : surface_map)
--    {
--        if (MirSurface::is_valid(surface.second))
--            delete surface.second;
--    }
--
      for (auto const& info : stream_map)
+     {
          if (info.second.owned)
@@ -65,7 +59,7 @@
      if (it != surfaces.end())
+     {
          auto const surface = it->second;
--        exec(surface);
++        exec(surface.get());
+     }
      else
+     {
@@ -75,7 +69,7 @@
+     }
+ }
--void mcl::ConnectionSurfaceMap::insert(mf::SurfaceId surface_id, MirSurface* surface)
++void mcl::ConnectionSurfaceMap::insert(mf::SurfaceId surface_id, std::shared_ptr<MirSurface> const& surface)
+ {
      // get_buffer_stream has internal locks - call before locking mutex to
      // avoid locking ordering issues
 === modified file 'src/client/surface_map.h'
 --- src/client/surface_map.h	2015-07-16 07:03:19 +0000
 +++ src/client/surface_map.h	2016-01-13 19:10:37 +0000
@@ -22,6 +22,7 @@
  #include "mir/frontend/surface_id.h"
  #include "mir/frontend/buffer_stream_id.h"
  #include <functional>
++#include <memory>
  struct MirSurface;
 === modified file 'tests/unit-tests/client/test_client_mir_surface.cpp'
 --- tests/unit-tests/client/test_client_mir_surface.cpp	2015-12-16 20:01:21 +0000
 +++ tests/unit-tests/client/test_client_mir_surface.cpp	2016-01-13 19:10:37 +0000
@@ -51,8 +51,7 @@
  #include <boost/throw_exception.hpp>
  #include "mir/test/doubles/stub_client_buffer_factory.h"
--#include "mir/test/doubles/stub_client_buffer_stream_factory.h"
--#include "mir/test/doubles/mock_client_buffer_stream_factory.h"
++#include "mir/test/doubles/stub_buffer_stream.h"
  #include "mir/test/doubles/mock_client_buffer_stream.h"
  #include "mir_test_framework/stub_client_platform_factory.h"
@@ -139,7 +138,6 @@
      static std::map<int, int> sent_surface_attributes;
--private:
      void generate_unique_buffer()
+     {
          global_buffer_id++;
@@ -278,10 +276,6 @@
+ {
+ }
--void null_surface_callback(MirSurface*, void*)
--{
--}
--
  void null_event_callback(MirSurface*, MirEvent const*, void*)
+ {
+ }
@@ -294,6 +288,8 @@
+ {
      MirClientSurfaceTest()
+     {
++        mock_server_tool->create_surface_response(&surface_proto);
++        ON_CALL(*stub_buffer_stream, swap_interval()).WillByDefault(testing::Return(1));
          start_test_server();
          connect_to_test_server();
+     }
@@ -333,50 +329,47 @@
              connection.get(),
              server_stub,
              nullptr,
--            stub_buffer_stream_factory,
++            stub_buffer_stream,
              input_platform,
              spec,
--            &null_surface_callback,
--            nullptr);
++            surface_proto,
++            wh);
+     }
      std::shared_ptr<MirSurface> create_surface_with(
          mclr::DisplayServer& server_stub,
--        std::shared_ptr<mcl::ClientBufferStreamFactory> const& buffer_stream_factory)
++        std::shared_ptr<mcl::ClientBufferStream> const& buffer_stream)
+     {
          return std::make_shared<MirSurface>(
              connection.get(),
              server_stub,
              nullptr,
--            buffer_stream_factory,
++            buffer_stream,
              input_platform,
              spec,
--            &null_surface_callback,
--            nullptr);
++            surface_proto,
++            wh);
+     }
      std::shared_ptr<MirSurface> create_and_wait_for_surface_with(
          mclr::DisplayServer& server_stub)
+     {
          auto surface = create_surface_with(server_stub);
--        surface->get_create_wait_handle()->wait_for_all();
          return surface;
+     }
      std::shared_ptr<MirSurface> create_and_wait_for_surface_with(
          mclr::DisplayServer& server_stub,
--        std::shared_ptr<mcl::ClientBufferStreamFactory> const& buffer_stream_factory)
++        std::shared_ptr<mcl::ClientBufferStream> const& buffer_stream)
+     {
--        auto surface = create_surface_with(server_stub, buffer_stream_factory);
--        surface->get_create_wait_handle()->wait_for_all();
++        auto surface = create_surface_with(server_stub, buffer_stream);
          return surface;
+     }
      std::shared_ptr<MirConnection> connection;
      MirSurfaceSpec const spec{nullptr, 33, 45, mir_pixel_format_abgr_8888};
--    std::shared_ptr<mtd::StubClientBufferStreamFactory> const stub_buffer_stream_factory =
--        std::make_shared<mtd::StubClientBufferStreamFactory>();
++    std::shared_ptr<mtd::MockClientBufferStream> stub_buffer_stream{std::make_shared<mtd::MockClientBufferStream>()};
      std::shared_ptr<StubClientInputPlatform> const input_platform =
          std::make_shared<StubClientInputPlatform>();
      std::shared_ptr<MockServerPackageGenerator> const mock_server_tool =
@@ -385,6 +378,8 @@
      std::shared_ptr<mcl::ConnectionSurfaceMap> surface_map;
      std::shared_ptr<mt::TestProtobufServer> test_server;
      std::shared_ptr<mclr::DisplayServer> client_comm_channel;
++    mir::protobuf::Surface surface_proto;
++    std::shared_ptr<MirWaitHandle> const wh{std::make_shared<MirWaitHandle>()};
      std::chrono::milliseconds const pause_time{10};
  };
@@ -403,22 +398,6 @@
+     }
+ }
--TEST_F(MirClientSurfaceTest, create_wait_handle_really_blocks)
--{
--    using namespace testing;
--
--    FakeRpcChannel fake_channel;
--    mclr::DisplayServer unresponsive_server{mt::fake_shared(fake_channel)};
--
--    auto const surface = create_surface_with(unresponsive_server);
--    auto wait_handle = surface->get_create_wait_handle();
--
--    auto expected_end = std::chrono::steady_clock::now() + pause_time;
--    wait_handle->wait_for_pending(pause_time);
--
--    EXPECT_GE(std::chrono::steady_clock::now(), expected_end);
--}
--
  TEST_F(MirClientSurfaceTest, creates_input_thread_with_input_dispatcher_when_delegate_specified)
+ {
      using namespace ::testing;
@@ -432,9 +411,7 @@
          .WillOnce(Return(mock_input_dispatcher));
      MirSurface surface{connection.get(), *client_comm_channel, nullptr,
--        stub_buffer_stream_factory, mock_input_platform, spec, &null_surface_callback, nullptr};
--    auto wait_handle = surface.get_create_wait_handle();
--    wait_handle->wait_for_all();
++        stub_buffer_stream, mock_input_platform, spec, surface_proto, wh};
      surface.set_event_handler(null_event_callback, nullptr);
      mock_input_dispatcher->trigger();
@@ -455,9 +432,7 @@
          .WillOnce(Return(mock_input_dispatcher));
      MirSurface surface{connection.get(), *client_comm_channel, nullptr,
--        stub_buffer_stream_factory, mock_input_platform, spec, &null_surface_callback, nullptr};
--    auto wait_handle = surface.get_create_wait_handle();
--    wait_handle->wait_for_all();
++        stub_buffer_stream, mock_input_platform, spec, surface_proto, wh};
      surface.set_event_handler(null_event_callback, nullptr);
      // Should now not get dispatched.
@@ -478,9 +453,7 @@
      EXPECT_CALL(*mock_input_platform, create_input_receiver(_, _, _)).Times(0);
      MirSurface surface{connection.get(), *client_comm_channel, nullptr,
--        stub_buffer_stream_factory, mock_input_platform, spec, &null_surface_callback, nullptr};
--    auto wait_handle = surface.get_create_wait_handle();
--    wait_handle->wait_for_all();
++        stub_buffer_stream, mock_input_platform, spec, surface_proto, wh};
+ }
  TEST_F(MirClientSurfaceTest, valid_surface_is_valid)
@@ -531,12 +504,7 @@
+ {
      using namespace testing;
      auto mock_stream = std::make_shared<mtd::MockClientBufferStream>();
--    auto mock_stream_factory = std::make_shared<NiceMock<mtd::MockClientBufferStreamFactory>>();
      auto mock_input_platform = std::make_shared<NiceMock<MockClientInputPlatform>>();
--    ON_CALL(*mock_stream_factory, make_producer_stream(_,_,_,_,An<void*>()))
--        .WillByDefault(Return(mock_stream.get()));
--    ON_CALL(*mock_stream_factory, make_producer_stream(_,_,_,_,An<geom::Size>()))
--        .WillByDefault(Return(mock_stream));
      ON_CALL(*mock_input_platform, create_input_receiver(_,_,_))
          .WillByDefault(Return(std::make_shared<mt::TestDispatchable>([]{})));
      ON_CALL(*mock_stream, rpc_id()).WillByDefault(Return(mir::frontend::BufferStreamId(2)));
@@ -545,11 +513,8 @@
      EXPECT_CALL(*mock_stream, set_size(size));
      auto ev = mir::events::make_event(mir::frontend::SurfaceId(2), size);
--    //FIXME: difficult construction
      MirSurface surface{connection.get(), *client_comm_channel, nullptr,
--        mock_stream_factory, mock_input_platform, spec, &null_surface_callback, nullptr};
--    auto wait_handle = surface.get_create_wait_handle();
--    wait_handle->wait_for_all();
++        mock_stream, mock_input_platform, spec, surface_proto, wh};
      surface.handle_event(*ev);
      surface_map->erase(mir::frontend::BufferStreamId(2));
+ }
@@ -558,13 +523,8 @@
+ {
      using namespace testing;
      auto mock_stream = std::make_shared<NiceMock<mtd::MockClientBufferStream>>();
--    auto mock_stream_factory = std::make_shared<NiceMock<mtd::MockClientBufferStreamFactory>>();
      auto mock_input_platform = std::make_shared<NiceMock<MockClientInputPlatform>>();
      ON_CALL(*mock_stream, rpc_id()).WillByDefault(Return(mir::frontend::BufferStreamId(2)));
--    ON_CALL(*mock_stream_factory, make_producer_stream(_,_,_,_,An<void*>()))
--        .WillByDefault(Return(mock_stream.get()));
--    ON_CALL(*mock_stream_factory, make_producer_stream(_,_,_,_,An<geom::Size>()))
--        .WillByDefault(Return(mock_stream));
      ON_CALL(*mock_input_platform, create_input_receiver(_,_,_))
          .WillByDefault(Return(std::make_shared<mt::TestDispatchable>([]{})));
@@ -572,8 +532,7 @@
      EXPECT_CALL(*mock_stream, set_size(size)).Times(0);
      auto ev = mir::events::make_event(mir::frontend::SurfaceId(2), size);
      MirSurface surface{connection.get(), *client_comm_channel, nullptr,
--        mock_stream_factory, mock_input_platform, spec, &null_surface_callback, nullptr};
--    surface.get_create_wait_handle()->wait_for_all();
++        mock_stream, mock_input_platform, spec, surface_proto, wh};
      MirSurfaceSpec spec;
      std::vector<MirBufferStreamInfo> info =
 === modified file 'tests/unit-tests/client/test_connection_resource_map.cpp'
 --- tests/unit-tests/client/test_connection_resource_map.cpp	2015-07-16 07:03:19 +0000
 +++ tests/unit-tests/client/test_connection_resource_map.cpp	2016-01-13 19:10:37 +0000
@@ -27,7 +27,8 @@
  struct ConnectionResourceMap : testing::Test
+ {
--    MirSurface surface{"a string"};
++    std::shared_ptr<MirWaitHandle> wh { std::make_shared<MirWaitHandle>() };
++    std::shared_ptr<MirSurface> surface{std::make_shared<MirSurface>("a string", nullptr, mf::SurfaceId{2}, wh)};
      mtd::MockClientBufferStream stream;
      mf::SurfaceId const surface_id{43};
      mf::BufferStreamId const stream_id{43};
@@ -39,14 +40,14 @@
      auto surface_called = false;
      auto stream_called = false;
      mcl::ConnectionSurfaceMap map;
--    map.insert(surface_id, &surface);
++    map.insert(surface_id, surface);
      map.with_surface_do(surface_id, [&](MirSurface* surf) {
--        EXPECT_THAT(surf, Eq(&surface));
++        EXPECT_THAT(surf, Eq(surface.get()));
          surface_called = true;
      });
      map.with_stream_do(stream_id, [&](mcl::ClientBufferStream* stream) {
--        EXPECT_THAT(stream, Eq(surface.get_buffer_stream()));
++        EXPECT_THAT(stream, Eq(surface->get_buffer_stream()));
          stream_called = true;
      });
@@ -59,9 +60,9 @@
      using namespace testing;
      auto stream_called = false;
      mcl::ConnectionSurfaceMap map;
--    map.insert(surface_id, &surface);
++    map.insert(surface_id, surface);
      map.with_stream_do(stream_id, [&](mcl::ClientBufferStream* stream) {
--        EXPECT_THAT(stream, Eq(surface.get_buffer_stream()));
++        EXPECT_THAT(stream, Eq(surface->get_buffer_stream()));
          stream_called = true;
      });
      EXPECT_TRUE(stream_called);
 === modified file 'tests/unit-tests/client/test_mir_connection.cpp'
 --- tests/unit-tests/client/test_mir_connection.cpp	2015-07-29 01:39:52 +0000
 +++ tests/unit-tests/client/test_mir_connection.cpp	2016-01-13 19:10:37 +0000
@@ -63,7 +63,7 @@
          ON_CALL(*this, watch_fd()).WillByDefault(testing::Return(pollable_fd));
+     }
--    void call_method(std::string const& name,
++    virtual void call_method(std::string const& name,
                      google::protobuf::MessageLite const* parameters,
                      google::protobuf::MessageLite* response,
                      google::protobuf::Closure* complete)
@@ -83,6 +83,13 @@
              platform_operation(static_cast<mp::PlatformOperationMessage const*>(parameters),
                                 static_cast<mp::PlatformOperationMessage*>(response));
+         }
++        else if (name == "create_surface")
++        {
++            auto response_message = static_cast<mp::Surface*>(response);
++            response_message->mutable_id()->set_value(33);
++            response_message->mutable_buffer_stream()->mutable_id()->set_value(33);
++        }
++
          complete->Run();
+     }
@@ -638,3 +645,30 @@
      EXPECT_THAT(mir_platform_message_get_opcode(returned_response), Eq(opcode));
      mir_platform_message_release(returned_response);
+ }
++
++TEST_F(MirConnectionTest, create_wait_handle_really_blocks)
++{
++    using namespace testing;
++
++    std::chrono::milliseconds const pause_time{10};
++    struct FakeRpcChannel : public MockRpcChannel
++    {
++        void call_method(
++            std::string const&,
++            google::protobuf::MessageLite const*,
++            google::protobuf::MessageLite*,
++            google::protobuf::Closure* closure) override
++        {
++            delete closure;
++        }
++    };
++    TestConnectionConfiguration conf{mock_platform, std::make_shared<NiceMock<FakeRpcChannel>>()};
++    MirConnection connection(conf);
++    MirSurfaceSpec const spec{&connection, 33, 45, mir_pixel_format_abgr_8888};
++
++    auto wait_handle = connection.create_surface(spec, nullptr, nullptr);
++    auto expected_end = std::chrono::steady_clock::now() + pause_time;
++    wait_handle->wait_for_pending(pause_time);
++
++    EXPECT_GE(std::chrono::steady_clock::now(), expected_end);
++}