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

[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!).

End the weak_ptr madness.

[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

=== 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-04 18:28:48 +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-04 18:28:48 +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-04 18:28:48 +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-04 18:28:48 +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-04 18:28:48 +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-04 18:28:48 +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-04 18:28:48 +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-04 18:28:48 +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(