Merge lp:~robertcarr/mir/hold-surface-alive into lp:mir

[Robert Carr]

Hold ms::Surface alive from msh::Surface, and remove the explicit throw
calls. This way holding a shared_ptr to msh::Surface becomes safe (as long
as you drop it eventually!). (LP: #1234609)

End the weak_ptr madness. Retargeted to development-branch

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1064
http://jenkins.qa.ubuntu.com/job/mir-ci/1580/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2231
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/2116
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/824/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-armhf-ci/81
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-armhf-ci/81/artifact/work/output/*zip*/output.zip

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1065
http://jenkins.qa.ubuntu.com/job/mir-ci/1581/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2232
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/2117
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/825
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/825/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-armhf-ci/82
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-armhf-ci/82/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Kevin DuBois (kdub)]

How does the shell know when a surface has been destroyed? I'd still like to hedge against giving the shell a strong pointer (which leads to the temptation that the shell will just keep it around, when really, it might be destroyed at any time by mir)

In general, i'm okay with the shell having a strong pointer to the surfaces, provided the way they use it makes it clear that they should get the strong pointer, use it, and then release it.

Much of the functions in msh::Shell are now just passthrough.
perhaps we have something like:
std::shared_ptr<ms::Surface> msh::Shell::compositor_surface()
and then eliminate the passthrough functions

this part of the code needs some reworking, so if it solves a bug though and makes strange code less strange, i suppose that adds up to approve :)

review: Approve

[Kevin DuBois (kdub)]

actually, should be retargeted :)

should probably get a second look over from someone else before top approve as well

review: Needs Fixing

[Alexandros Frantzis (afrantzis)]

I can verify that this MP fixes the reported bug. However, it seems that the shell::Surface is never released and therefore the underlying ms::Surface is also never released. This is a pre-existing issue in unity8/mir, obscured before because the underlying ms::Surface was freed when the ApplicationSession was destroyed and called shell::Surface::destroy(). Before we land this, we need to find why shell::Surface is not released, and coordinate the fix with unity8 (if the problem is indeed outside Mir).

Marking "Needs fixing" to block this from landing before the other half of the fix is in place.

review: Needs Fixing

[Robert Carr (robertcarr)]

Thanks for checking Alexandros! I think I found out what was going on notably the mirsurfacemanager object in unity-mir is holding a map of shared_ptr<msh::Surface> to unity-mir MirSurface objects. It relies on the_session_listener to know when to clear these surfaces. This change sneakily changed the behavior such that this would not be emitted ons ession destruction anymore.

I have reinstated the behavior (and instated a test) in r1066. It will be a little bit before I can verify this is a full fix though.

[Alexandros Frantzis (afrantzis)]

>> This is a pre-existing issue in unity8/mir, obscured before because the underlying ms::Surface
>> was freed when the ApplicationSession was destroyed and called shell::Surface::destroy()

> This change sneakily changed the behavior such that this would not be emitted ons ession destruction anymore.

Perhaps what I am describing is a different (but related) issue then, since I don't see the shell::Surface dtor getting called when an application is closed [1] even with *current* lp:mir. I only see a call to shell::Surface::destroy() (invoked through the ApplicationSession dtor).

[1] By "closed" I mean pressing [X] on the app snapshot in the recent applications list.

[Robert Carr (robertcarr)]

With this branch and latest r's, I see a call to ms::Surface destroy on the phone (after closing app via X button)

[Robert Carr (robertcarr)]

This may be linked to: https://bugs.launchpad.net/ubuntu/+source/address-book-app/+bug/1235263

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1066
http://10.97.0.26:8080/job/mir-team-mir-development-branch-ci/114/
Executed test runs:
    SUCCESS: http://10.97.0.26:8080/job/mir-android-saucy-i386-build/2263
    SUCCESS: http://10.97.0.26:8080/job/mir-clang-saucy-amd64-build/2148
    FAILURE: http://10.97.0.26:8080/job/mir-team-mir-development-branch-saucy-amd64-ci/111/console

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/mir-team-mir-development-branch-ci/114/rebuild

review: Needs Fixing (continuous-integration)

[Daniel van Vugt (vanvugt)]

I think any approach that continues to expose the implementation class surfaces::Surface from an interface the shell uses (shell::Surface) is a mistake. Although the change has other benefits...

review: Abstain

[Daniel van Vugt (vanvugt)]

Sanity check: Is this proposal intended as a fix for bug 1234609?

[Alexandros Frantzis (afrantzis)]

> Sanity check: Is this proposal intended as a fix for bug 1234609?

Yes. When an app destroys itself there seems to be a race between its surface getting destroyed, and the shell needing to access at least some information about the surface. Letting the shell share ownership of the surface fixes this. Perhaps we could remove the race itself in the shell, but it's not clear what's going on exactly and if this is feasible, since the QML layer is involved.

[Alan Griffiths (alan-griffiths)]

If this really works, then msh::Surface is becoming a ghost we can likely refactor away.

Dredging my memory for the reasons that motivated msh::Surface I remember it had to do with race conditions where a surface was removed from compositing (a.k.a. "destroyed") while clients could be posting buffers - leading to threads stalling waiting for compositing that never happened. This led to resource leaks and shutdown problems.

In the current codebase I think that force_requests_to_complete() will avoid shutdown problems and release resources on exit. However, I've yet to convince myself that resources are released in a timely manner (i.e. shortly after the surface is destroyed by the client).

Marking as "Needs Information" until I've worked out an answer that satisfies me.

review: Needs Information

[Alexandros Frantzis (afrantzis)]

> With this branch and latest r's, I see a call to ms::Surface destroy on the phone (after closing app via X button)

Hmm, for me this works only for the first app I open/close. For subsequent apps I can see the surfaces (both ms:: and msh::) being created, but none is being destroyed after closing with X.

review: Needs Fixing

[Alexandros Frantzis (afrantzis)]

>> With this branch and latest r's, I see a call to ms::Surface destroy on the phone (after closing app via X button)
>
> Hmm, for me this works only for the first app I open/close. For subsequent apps I can
> see the surfaces (both ms:: > and msh::) being created, but none is being destroyed after closing with X.

Not a problem in Mir, see https://code.launchpad.net/~afrantzis/unity-mir/fix-1236898/+merge/189894

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1067
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/136/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2291
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/2176
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/133
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/133/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/mir-team-mir-development-branch-ci/136/rebuild

review: Approve (continuous-integration)

[Robert Carr (robertcarr)]

We need to coordinate the landing with the unity-mir fix.

[Robert Ancell (robert-ancell)]

It makes sense to me that the three concepts of a surface (shell, rendering, frontend) should have the same lifecycle and it should be ultimately be up to the shell to decide when a surface should be removed.

review: Approve

[Daniel van Vugt (vanvugt)]

This is likely a shell ABI break:

29 - std::weak_ptr<mir::surfaces::Surface> const surface;
30 + std::shared_ptr<mir::surfaces::Surface> const surface;

Are we supposed to ring a bell or something?

review: Needs Information

[Robert Ancell (robert-ancell)]

> This is likely a shell ABI break:
>
> 29 - std::weak_ptr<mir::surfaces::Surface> const surface;
> 30 + std::shared_ptr<mir::surfaces::Surface> const surface;
>
> Are we supposed to ring a bell or something?

We just have to bump ABI on development branch BEFORE merging to trunk and then ring a bell. If we can avoid breaking ABI much better, but will probably be too hard in this case.

[Robert Carr (robertcarr)]

>> If this really works, then msh::Surface is becoming a ghost we can likely refactor away.

Oi!

>> Dredging my memory for the reasons that motivated msh::Surface I remember it had to do with race conditions where a surface
>> was removed from compositing (a.k.a. "destroyed") while clients could be posting buffers - leading to threads stalling waiting >> for compositing that never happened. This led to resource leaks and shutdown problems.
>> In the current codebase I think that force_requests_to_complete() will avoid shutdown problems and release resources on exit.

Sure, we can see this through the tests.

>> However, I've yet to convince myself that resources are released in a timely manner (i.e. shortly after the surface is
>> destroyed by the client).

Why wouldn't they be? Do you mean you are concerned we hold references to the surface in Mir? The memcheck tests should verify this for us.

Resources shouldn't necessarily be released immediately after the client disconnects anyway (which I think was the 'flaw' of the explicit destroy method). In some cases: say the client unexpectedly disconnects, the shell wishes to keep the surface alive for a few seconds to perform an animation.

Marking as "Needs Information" until I've worked out an answer that satisfies me.

Reply

[kevin gunn (kgunn72)]

ok, time pressure.
after long discussino with racarr this seems like a good fix
alan_g & racarr to follow up later - but we're merging to hit the freeeze.

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-autolanding/39/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2308
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/2193
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-autolanding/38/console

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'include/server/mir/frontend/surface.h'
--- include/server/mir/frontend/surface.h	2013-08-28 03:41:48 +0000
+++ include/server/mir/frontend/surface.h	2013-10-08 18:20:54 +0000
@@ -50,7 +50,6 @@
 
     virtual ~Surface() {}
 
-    virtual void destroy() = 0;
     virtual void force_requests_to_complete() = 0;
 
     virtual geometry::Size size() const = 0;

=== modified file 'include/server/mir/shell/surface.h'
--- include/server/mir/shell/surface.h	2013-08-28 03:41:48 +0000
+++ include/server/mir/shell/surface.h	2013-10-08 18:20:54 +0000
@@ -60,8 +60,6 @@
     virtual void hide();
     virtual void show();
 
-    virtual void destroy();
-
     virtual void force_requests_to_complete();
 
     virtual std::string name() const;
@@ -97,7 +95,7 @@
 
     std::shared_ptr<SurfaceBuilder> const builder;
     std::shared_ptr<SurfaceConfigurator> const configurator;
-    std::weak_ptr<mir::surfaces::Surface> const surface;
+    std::shared_ptr<mir::surfaces::Surface> const surface;
 
     frontend::SurfaceId const id;
     std::shared_ptr<frontend::EventSink> const event_sink;

=== modified file 'include/test/mir_test_doubles/mock_surface.h'
--- include/test/mir_test_doubles/mock_surface.h	2013-08-28 03:41:48 +0000
+++ include/test/mir_test_doubles/mock_surface.h	2013-10-08 18:20:54 +0000
@@ -51,7 +51,6 @@
     MOCK_METHOD0(visible, bool());
     MOCK_METHOD1(raise, void(std::shared_ptr<shell::SurfaceController> const&));
 
-    MOCK_METHOD0(destroy, void());
     MOCK_METHOD0(force_requests_to_complete, void());
     MOCK_METHOD0(advance_client_buffer, std::shared_ptr<graphics::Buffer>());
 

=== modified file 'include/test/mir_test_doubles/stub_surface.h'
--- include/test/mir_test_doubles/stub_surface.h	2013-08-28 03:41:48 +0000
+++ include/test/mir_test_doubles/stub_surface.h	2013-10-08 18:20:54 +0000
@@ -35,7 +35,6 @@
 
     void hide() {}
     void show() {}
-    void destroy() {}
     void force_requests_to_complete() {}
 
     geometry::Size size() const

=== modified file 'src/server/shell/application_session.cpp'
--- src/server/shell/application_session.cpp	2013-08-28 03:41:48 +0000
+++ src/server/shell/application_session.cpp	2013-10-08 18:20:54 +0000
@@ -55,7 +55,7 @@
     std::unique_lock<std::mutex> lock(surfaces_mutex);
     for (auto const& pair_id_surface : surfaces)
     {
-        pair_id_surface.second->destroy();
+        session_listener->destroying_surface(*this, pair_id_surface.second);
     }
 }
 
@@ -116,7 +116,6 @@
     
     session_listener->destroying_surface(*this, p->second);
 
-    p->second->destroy();
     surfaces.erase(p);
 }
 

=== modified file 'src/server/shell/surface.cpp'
--- src/server/shell/surface.cpp	2013-08-28 03:41:48 +0000
+++ src/server/shell/surface.cpp	2013-10-08 18:20:54 +0000
@@ -58,165 +58,74 @@
 
 msh::Surface::~Surface() noexcept
 {
-    if (surface.lock())
-    {
-        destroy();
-    }
+    builder->destroy_surface(surface);
 }
 
 void msh::Surface::hide()
 {
-    if (auto const& s = surface.lock())
-    {
-        s->set_hidden(true);
-    }
-    else
-    {
-        BOOST_THROW_EXCEPTION(std::runtime_error("Invalid surface"));
-    }
+    surface->set_hidden(true);
 }
 
 void msh::Surface::show()
 {
-    if (auto const& s = surface.lock())
-    {
-        s->set_hidden(false);
-    }
-    else
-    {
-        BOOST_THROW_EXCEPTION(std::runtime_error("Invalid surface"));
-    }
-}
-
-void msh::Surface::destroy()
-{
-    builder->destroy_surface(surface);
+    surface->set_hidden(false);
 }
 
 void msh::Surface::force_requests_to_complete()
 {
-    if (auto const& s = surface.lock())
-    {
-        s->force_requests_to_complete();
-    }
+    surface->force_requests_to_complete();
 }
 
 mir::geometry::Size msh::Surface::size() const
 {
-    if (auto const& s = surface.lock())
-    {
-        return s->size();
-    }
-    else
-    {
-        BOOST_THROW_EXCEPTION(std::runtime_error("Invalid surface"));
-    }
+    return surface->size();
 }
 
 void msh::Surface::move_to(geometry::Point const& p)
 {
-    if (auto const& s = surface.lock())
-    {
-        s->move_to(p);
-    }
-    else
-    {
-        BOOST_THROW_EXCEPTION(std::runtime_error("Invalid surface"));
-    }
+    surface->move_to(p);
 }
 
 mir::geometry::Point msh::Surface::top_left() const
 {
-    if (auto const& s = surface.lock())
-    {
-        return s->top_left();
-    }
-    else
-    {
-        BOOST_THROW_EXCEPTION(std::runtime_error("Invalid surface"));
-    }
+    return surface->top_left();
 }
 
 std::string msh::Surface::name() const
 {
-    if (auto const& s = surface.lock())
-    {
-        return s->name();
-    }
-    else
-    {
-        BOOST_THROW_EXCEPTION(std::runtime_error("Invalid surface"));
-    }
+    return surface->name();
 }
 
 mir::geometry::PixelFormat msh::Surface::pixel_format() const
 {
-    if (auto const& s = surface.lock())
-    {
-        return s->pixel_format();
-    }
-    else
-    {
-        BOOST_THROW_EXCEPTION(std::runtime_error("Invalid surface"));
-    }
+    return surface->pixel_format();
 }
 
 std::shared_ptr<mg::Buffer> msh::Surface::advance_client_buffer()
 {
-    if (auto const& s = surface.lock())
-    {
-        return s->advance_client_buffer();
-    }
-    else
-    {
-        BOOST_THROW_EXCEPTION(std::runtime_error("Invalid surface"));
-    }
+    return surface->advance_client_buffer();
 }
 
 void msh::Surface::allow_framedropping(bool allow)
 {
-    if (auto const& s = surface.lock())
-    {
-        s->allow_framedropping(allow);
-    }
+    surface->allow_framedropping(allow);
 }
  
 void msh::Surface::with_most_recent_buffer_do(
     std::function<void(mg::Buffer&)> const& exec)
 {
-    if (auto const& s = surface.lock())
-    {
-        auto buf = s->snapshot_buffer();
-        exec(*buf);
-    }
-    else
-    {
-        BOOST_THROW_EXCEPTION(std::runtime_error("Invalid surface"));
-    }
+    auto buf = surface->snapshot_buffer();
+    exec(*buf);
 }
 
 bool msh::Surface::supports_input() const
 {
-    if (auto const& s = surface.lock())
-    {
-        return s->supports_input();
-    }
-    else
-    {
-        BOOST_THROW_EXCEPTION(std::runtime_error("Invalid surface"));
-    }
+    return surface->supports_input();
 }
 
 int msh::Surface::client_input_fd() const
 {
-    if (auto const& s = surface.lock())
-    {
-        return s->client_input_fd();
-    }
-    else
-    {
-        BOOST_THROW_EXCEPTION(std::runtime_error("Invalid surface"));
-    }
+    return surface->client_input_fd();
 }
 
 int msh::Surface::configure(MirSurfaceAttrib attrib, int value)
@@ -319,33 +228,15 @@
 
 void msh::Surface::take_input_focus(std::shared_ptr<msh::InputTargeter> const& targeter)
 {
-    auto s = surface.lock();
-    if (s)
-        targeter->focus_changed(s->input_channel());
-    else
-        BOOST_THROW_EXCEPTION(std::runtime_error("Invalid surface"));
+    targeter->focus_changed(surface->input_channel());
 }
 
 void msh::Surface::set_input_region(std::vector<geom::Rectangle> const& region)
 {
-    if (auto const& s = surface.lock())
-    {
-        s->set_input_region(region);
-    }
-    else
-    {
-        BOOST_THROW_EXCEPTION(std::runtime_error("Invalid surface"));
-    }
+    surface->set_input_region(region);
 }
 
 void msh::Surface::raise(std::shared_ptr<msh::SurfaceController> const& controller)
 {
-    if (auto const& s = surface.lock())
-    {
-        controller->raise(s);
-    }
-    else
-    {
-        BOOST_THROW_EXCEPTION(std::runtime_error("Invalid surface"));
-    }
+    controller->raise(surface);
 }

=== modified file 'tests/unit-tests/shell/test_application_session.cpp'
--- tests/unit-tests/shell/test_application_session.cpp	2013-08-28 03:41:48 +0000
+++ tests/unit-tests/shell/test_application_session.cpp	2013-10-08 18:20:54 +0000
@@ -65,7 +65,6 @@
     ON_CALL(surface_factory, create_surface(_,_,_,_)).WillByDefault(Return(mock_surface));
 
     EXPECT_CALL(surface_factory, create_surface(_, _, _, _));
-    EXPECT_CALL(*mock_surface, destroy());
     
     mtd::MockSessionListener listener;
     EXPECT_CALL(listener, surface_created(_, _)).Times(1);
@@ -82,6 +81,32 @@
     session.destroy_surface(surf);
 }
 
+TEST(ApplicationSession, listener_notified_of_surface_destruction_on_session_destruction)
+{
+    using namespace ::testing;
+
+    auto mock_surface = make_mock_surface();
+
+    mtd::NullEventSink sender;
+    mtd::MockSurfaceFactory surface_factory;
+    ON_CALL(surface_factory, create_surface(_,_,_,_)).WillByDefault(Return(mock_surface));
+
+    EXPECT_CALL(surface_factory, create_surface(_, _, _, _));
+    
+    mtd::MockSessionListener listener;
+    EXPECT_CALL(listener, surface_created(_, _)).Times(1);
+    EXPECT_CALL(listener, destroying_surface(_, _)).Times(1);
+
+    {
+        msh::ApplicationSession session(mt::fake_shared(surface_factory), "Foo",
+            std::make_shared<mtd::NullSnapshotStrategy>(), mt::fake_shared(listener),
+                mt::fake_shared(sender));
+
+        msh::SurfaceCreationParameters params;
+        session.create_surface(params);
+    }
+}
+
 TEST(ApplicationSession, default_surface_is_first_surface)
 {
     using namespace ::testing;
@@ -142,7 +167,6 @@
         InSequence seq;
         EXPECT_CALL(*mock_surface, hide()).Times(1);
         EXPECT_CALL(*mock_surface, show()).Times(1);
-        EXPECT_CALL(*mock_surface, destroy()).Times(1);
     }
 
     msh::SurfaceCreationParameters params;

=== modified file 'tests/unit-tests/shell/test_surface.cpp'
--- tests/unit-tests/shell/test_surface.cpp	2013-08-28 03:41:48 +0000
+++ tests/unit-tests/shell/test_surface.cpp	2013-10-08 18:20:54 +0000
@@ -191,197 +191,20 @@
     EXPECT_CALL(surface_builder, create_surface(nullptr, _)).Times(AnyNumber());
     EXPECT_CALL(surface_builder, destroy_surface(_)).Times(0);
 
-    msh::Surface test(
-            nullptr,
-            mt::fake_shared(surface_builder), std::make_shared<mtd::NullSurfaceConfigurator>(),
-            msh::a_surface(), stub_id, stub_sender);
-
-    Mock::VerifyAndClearExpectations(&test);
-    EXPECT_CALL(surface_builder, destroy_surface(_)).Times(1);
-
-    // Check destroy_surface is called immediately
-    test.destroy();
-
-    Mock::VerifyAndClearExpectations(&test);
+    {
+        msh::Surface test(
+                          nullptr,
+                          mt::fake_shared(surface_builder), std::make_shared<mtd::NullSurfaceConfigurator>(),
+                          msh::a_surface(), stub_id, stub_sender);
+        
+        Mock::VerifyAndClearExpectations(&test);
+        EXPECT_CALL(surface_builder, destroy_surface(_)).Times(1);
+        Mock::VerifyAndClearExpectations(&test);
+    }
+
     EXPECT_CALL(surface_builder, destroy_surface(_)).Times(0);
 }
 
-TEST_F(ShellSurface, size_throw_behavior)
-{
-    msh::Surface test(
-            nullptr,
-            mt::fake_shared(surface_builder), std::make_shared<mtd::NullSurfaceConfigurator>(),
-            msh::a_surface(), stub_id, stub_sender);
-
-    EXPECT_NO_THROW({
-        test.size();
-    });
-
-    surface_builder.reset_surface();
-
-    EXPECT_THROW({
-        test.size();
-    }, std::runtime_error);
-}
-
-TEST_F(ShellSurface, top_left_throw_behavior)
-{
-    msh::Surface test(
-            nullptr,
-            mt::fake_shared(surface_builder), std::make_shared<mtd::NullSurfaceConfigurator>(),
-            msh::a_surface(), stub_id, stub_sender);
-
-    EXPECT_NO_THROW({
-        test.top_left();
-    });
-
-    surface_builder.reset_surface();
-
-    EXPECT_THROW({
-        test.top_left();
-    }, std::runtime_error);
-}
-
-TEST_F(ShellSurface, name_throw_behavior)
-{
-    msh::Surface test(
-            nullptr,
-            mt::fake_shared(surface_builder), std::make_shared<mtd::NullSurfaceConfigurator>(),
-            msh::a_surface(), stub_id, stub_sender);
-
-    EXPECT_NO_THROW({
-        test.name();
-    });
-
-    surface_builder.reset_surface();
-
-    EXPECT_THROW({
-        test.name();
-    }, std::runtime_error);
-}
-
-TEST_F(ShellSurface, pixel_format_throw_behavior)
-{
-    msh::Surface test(
-            nullptr,
-            mt::fake_shared(surface_builder), std::make_shared<mtd::NullSurfaceConfigurator>(),
-            msh::a_surface(), stub_id, stub_sender);
-
-    EXPECT_NO_THROW({
-        test.pixel_format();
-    });
-
-    surface_builder.reset_surface();
-
-    EXPECT_THROW({
-        test.pixel_format();
-    }, std::runtime_error);
-}
-
-TEST_F(ShellSurface, hide_throw_behavior)
-{
-    msh::Surface test(
-            nullptr,
-            mt::fake_shared(surface_builder), std::make_shared<mtd::NullSurfaceConfigurator>(),
-            msh::a_surface(), stub_id, stub_sender);
-
-    EXPECT_NO_THROW({
-        test.hide();
-    });
-
-    surface_builder.reset_surface();
-
-    EXPECT_THROW({
-        test.hide();
-    }, std::runtime_error);
-}
-
-TEST_F(ShellSurface, show_throw_behavior)
-{
-    msh::Surface test(
-            nullptr,
-            mt::fake_shared(surface_builder), std::make_shared<mtd::NullSurfaceConfigurator>(),
-            msh::a_surface(), stub_id, stub_sender);
-
-    EXPECT_NO_THROW({
-        test.show();
-    });
-
-    surface_builder.reset_surface();
-
-    EXPECT_THROW({
-        test.show();
-    }, std::runtime_error);
-}
-
-TEST_F(ShellSurface, destroy_throw_behavior)
-{
-    msh::Surface test(
-            nullptr,
-            mt::fake_shared(surface_builder), std::make_shared<mtd::NullSurfaceConfigurator>(),
-            msh::a_surface(), stub_id, stub_sender);
-
-    EXPECT_NO_THROW({
-        test.destroy();
-    });
-
-    surface_builder.reset_surface();
-
-    EXPECT_NO_THROW({
-        test.destroy();
-    });
-}
-
-TEST_F(ShellSurface, force_request_to_complete_throw_behavior)
-{
-    msh::Surface test(
-            nullptr,
-            mt::fake_shared(surface_builder), std::make_shared<mtd::NullSurfaceConfigurator>(),
-            msh::a_surface(), stub_id, stub_sender);
-
-    EXPECT_NO_THROW({
-        test.force_requests_to_complete();
-    });
-
-    surface_builder.reset_surface();
-
-    EXPECT_NO_THROW({
-        test.force_requests_to_complete();
-    });
-}
-
-TEST_F(ShellSurface, advance_client_buffer_throw_behavior)
-{
-    msh::Surface test(
-            nullptr,
-            mt::fake_shared(surface_builder), std::make_shared<mtd::NullSurfaceConfigurator>(),
-            msh::a_surface(), stub_id, stub_sender);
-
-    EXPECT_NO_THROW({
-        test.advance_client_buffer();
-    });
-
-    surface_builder.reset_surface();
-
-    EXPECT_THROW({
-        test.advance_client_buffer();
-    }, std::runtime_error);
-}
-
-TEST_F(ShellSurface, input_fds_throw_behavior)
-{
-    msh::Surface test(
-            nullptr,
-            mt::fake_shared(surface_builder), std::make_shared<mtd::NullSurfaceConfigurator>(),
-            msh::a_surface(), stub_id, stub_sender);
-
-    surface_builder.reset_surface();
-
-    EXPECT_THROW({
-            test.client_input_fd();
-    }, std::runtime_error);
-}
-
 TEST_F(ShellSurface, attributes)
 {
     using namespace testing;
@@ -522,43 +345,6 @@
     test.take_input_focus(mt::fake_shared(targeter));
 }
 
-TEST_F(ShellSurface, take_input_focus_throw_behavior)
-{
-    using namespace ::testing;
-
-    msh::Surface test(
-        nullptr,
-        mt::fake_shared(surface_builder), std::make_shared<mtd::NullSurfaceConfigurator>(),
-        msh::a_surface(), stub_id, stub_sender);
-    surface_builder.reset_surface();
-
-    mtd::StubInputTargeter targeter;
-    
-    EXPECT_THROW({
-            test.take_input_focus(mt::fake_shared(targeter));
-    }, std::runtime_error);
-}
-
-TEST_F(ShellSurface, set_input_region_throw_behavior)
-{
-    using namespace ::testing;
-    
-    msh::Surface test(
-        nullptr,
-        mt::fake_shared(surface_builder), std::make_shared<mtd::NullSurfaceConfigurator>(),
-        msh::a_surface(), stub_id, stub_sender);
-    
-    EXPECT_NO_THROW({
-            test.set_input_region(std::vector<geom::Rectangle>{});
-    });
-
-    surface_builder.reset_surface();
-
-    EXPECT_THROW({
-            test.set_input_region(std::vector<geom::Rectangle>{});
-    }, std::runtime_error);
-}
-
 TEST_F(ShellSurface, with_most_recent_buffer_do_uses_compositor_buffer)
 {
     msh::Surface test(