Merge lp:~afrantzis/mir/fix-resources-release-on-shutdown-lp-1170643 into lp:~mir-team/mir/trunk

[Alexandros Frantzis]

shell: Close all open sessions when destroying the SessionManager

We need to do this manually to break the cyclic dependency between msh::Session
and msh::InputTargetListener, since our implementations of these interfaces
keep strong references to each other. This change fixes LP:#1170643.

shell: Close all open sessions when destroying the SessionManager

We need to do this manually to break the cyclic dependency between msh::Session
and msh::InputTargetListener, since our implementations of these interfaces
keep strong references to each other. This change fixes LP:#1170643.

The test code is complicated by the need to synchronize and relay results across processes. The Flag class could be made into a more general utility and used by other tests, but I left this for another MP.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:681
http://jenkins.qa.ubuntu.com/job/mir-ci/573/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/673
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/556
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-raring-amd64-ci/57/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=precise,flavor=amd64/305

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:681
http://jenkins.qa.ubuntu.com/job/mir-ci/580/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/684
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/567
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-raring-amd64-ci/64/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=precise,flavor=amd64/310

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:682
http://jenkins.qa.ubuntu.com/job/mir-ci/581/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/685
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/568
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-raring-amd64-ci/65
        deb: http://jenkins.qa.ubuntu.com/job/mir-raring-amd64-ci/65/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

LGTM

review: Approve

[Robert Carr (robertcarr)]

LGTM!

review: Approve

=== modified file 'src/server/shell/session_manager.cpp'
--- src/server/shell/session_manager.cpp	2013-05-02 00:11:18 +0000
+++ src/server/shell/session_manager.cpp	2013-05-14 09:55:30 +0000
@@ -57,6 +57,22 @@
 
 msh::SessionManager::~SessionManager()
 {
+    /*
+     * Close all open sessions. We need to do this manually here
+     * to break the cyclic dependency between msh::Session
+     * and msh::InputTargetListener, since our implementations
+     * of these interfaces keep strong references to each other.
+     * TODO: Investigate other solutions (e.g. weak_ptr)
+     */
+    std::vector<std::shared_ptr<msh::Session>> sessions;
+
+    app_container->for_each([&](std::shared_ptr<Session> const& session)
+    {
+        sessions.push_back(session);
+    });
+
+    for (auto& session : sessions)
+        close_session(session);
 }
 
 std::shared_ptr<mf::Session> msh::SessionManager::open_session(

=== modified file 'tests/acceptance-tests/test_server_shutdown.cpp'
--- tests/acceptance-tests/test_server_shutdown.cpp	2013-04-25 21:17:37 +0000
+++ tests/acceptance-tests/test_server_shutdown.cpp	2013-05-14 09:55:30 +0000
@@ -20,6 +20,7 @@
 #include "mir/graphics/renderer.h"
 
 #include "mir_test_framework/display_server_test_fixture.h"
+#include "mir_test/fake_event_hub_input_configuration.h"
 
 #include <gtest/gtest.h>
 
@@ -28,7 +29,10 @@
 #include <fcntl.h>
 
 namespace mg = mir::graphics;
+namespace mi = mir::input;
+namespace mia = mir::input::android;
 namespace mtf = mir_test_framework;
+namespace mtd = mir::test::doubles;
 
 namespace
 {
@@ -56,19 +60,49 @@
 {
 }
 
+class Flag
+{
+public:
+    explicit Flag(std::string const& flag_file)
+        : flag_file{flag_file}
+    {
+        std::remove(flag_file.c_str());
+    }
+
+    void set()
+    {
+        close(open(flag_file.c_str(), O_CREAT, S_IWUSR | S_IRUSR));
+    }
+
+    bool is_set()
+    {
+        int fd = -1;
+        if ((fd = open(flag_file.c_str(), O_RDONLY, S_IWUSR | S_IRUSR)) != -1)
+        {
+            close(fd);
+            return true;
+        }
+        return false;
+    }
+
+    void wait()
+    {
+        while (!is_set())
+            std::this_thread::sleep_for(std::chrono::milliseconds(1));
+    }
+
+private:
+    std::string const flag_file;
+};
+
 }
 
 TEST_F(BespokeDisplayServerTestFixture, server_can_shut_down_when_clients_are_blocked)
 {
-    static std::string const next_buffer_done1{"next_buffer_done1_c5d49978.tmp"};
-    static std::string const next_buffer_done2{"next_buffer_done2_c5d49978.tmp"};
-    static std::string const next_buffer_done3{"next_buffer_done3_c5d49978.tmp"};
-    static std::string const server_done{"server_done_c5d49978.tmp"};
-
-    std::remove(next_buffer_done1.c_str());
-    std::remove(next_buffer_done2.c_str());
-    std::remove(next_buffer_done3.c_str());
-    std::remove(server_done.c_str());
+    Flag next_buffer_done1{"next_buffer_done1_c5d49978.tmp"};
+    Flag next_buffer_done2{"next_buffer_done2_c5d49978.tmp"};
+    Flag next_buffer_done3{"next_buffer_done3_c5d49978.tmp"};
+    Flag server_done{"server_done_c5d49978.tmp"};
 
     struct ServerConfig : TestingServerConfiguration
     {
@@ -82,10 +116,10 @@
 
     struct ClientConfig : TestingClientConfiguration
     {
-        ClientConfig(std::string const& next_buffer_done,
-                     std::string const& server_done)
-            : next_buffer_done{next_buffer_done},
-              server_done{server_done}
+        ClientConfig(Flag& next_buffer_done,
+                     Flag& server_done)
+            : next_buffer_done(next_buffer_done),
+              server_done(server_done)
         {
         }
 
@@ -110,8 +144,8 @@
             /* Ask for the first second buffer (should block) */
             mir_surface_next_buffer(surf, null_surface_callback, nullptr);
 
-            set_flag(next_buffer_done);
-            wait_for(server_done);
+            next_buffer_done.set();
+            server_done.wait();
 
             /* 
              * TODO: Releasing the connection to a shut down server blocks
@@ -122,25 +156,10 @@
              */
         }
 
-        void set_flag(std::string const& flag_file)
-        {
-            close(open(flag_file.c_str(), O_CREAT, S_IWUSR | S_IRUSR));
-        }
-
-        void wait_for(std::string const& flag_file)
-        {
-            int fd = -1;
-            while ((fd = open(flag_file.c_str(), O_RDONLY, S_IWUSR | S_IRUSR)) == -1)
-            {
-                std::this_thread::sleep_for(std::chrono::milliseconds(1));
-            }
-            close(fd);
-        }
-
-        std::string const next_buffer_done;
-        std::string const server_done;
+        Flag& next_buffer_done;
+        Flag& server_done;
     };
-    
+
     ClientConfig client_config1{next_buffer_done1, server_done};
     ClientConfig client_config2{next_buffer_done2, server_done};
     ClientConfig client_config3{next_buffer_done3, server_done};
@@ -152,14 +171,151 @@
     run_in_test_process([&]
     {
         /* Wait until the clients are blocked on getting the second buffer */
-        client_config1.wait_for(next_buffer_done1);
-        client_config2.wait_for(next_buffer_done2);
-        client_config3.wait_for(next_buffer_done3);
+        next_buffer_done1.wait();
+        next_buffer_done2.wait();
+        next_buffer_done3.wait();
 
         /* Shutting down the server should not block */
         shutdown_server_process();
 
         /* Notify the clients that we are done (we only need to set the flag once) */
-        client_config1.set_flag(server_done);
-    });
+        server_done.set();
+    });
+}
+
+TEST_F(BespokeDisplayServerTestFixture, server_releases_resources_on_shutdown_with_connected_clients)
+{
+    Flag surface_created1{"surface_created1_7e9c69fc.tmp"};
+    Flag surface_created2{"surface_created2_7e9c69fc.tmp"};
+    Flag surface_created3{"surface_created3_7e9c69fc.tmp"};
+    Flag server_done{"server_done_7e9c69fc.tmp"};
+    Flag resources_freed_success{"resources_free_success_7e9c69fc.tmp"};
+    Flag resources_freed_failure{"resources_free_failure_7e9c69fc.tmp"};
+
+    /* Use the real input manager, but with a fake event hub */
+    struct ServerConfig : TestingServerConfiguration
+    {
+        std::shared_ptr<mia::InputConfiguration> the_input_configuration() override
+        {
+            if (!input_configuration)
+            {
+                input_configuration =
+                    std::make_shared<mtd::FakeEventHubInputConfiguration>(
+                        std::initializer_list<std::shared_ptr<mi::EventFilter> const>{},
+                        the_viewable_area(),
+                        std::shared_ptr<mi::CursorListener>());
+            }
+
+            return input_configuration;
+        }
+
+        std::shared_ptr<mi::InputManager> the_input_manager() override
+        {
+            return DefaultServerConfiguration::the_input_manager();
+        }
+
+        std::shared_ptr<mir::shell::InputTargetListener> the_input_target_listener() override
+        {
+            return DefaultServerConfiguration::the_input_target_listener();
+        }
+
+        std::shared_ptr<mia::InputConfiguration> input_configuration;
+    };
+
+    auto server_config = std::make_shared<ServerConfig>();
+    launch_server_process(*server_config);
+
+    struct ClientConfig : TestingClientConfiguration
+    {
+        ClientConfig(Flag& surface_created,
+                     Flag& server_done)
+            : surface_created(surface_created),
+              server_done(server_done)
+        {
+        }
+
+        void exec()
+        {
+            MirConnection* connection = mir_connect_sync(mir_test_socket, __PRETTY_FUNCTION__);
+
+            ASSERT_TRUE(connection != NULL);
+
+            MirSurfaceParameters const request_params =
+            {
+                __PRETTY_FUNCTION__,
+                640, 480,
+                mir_pixel_format_abgr_8888,
+                mir_buffer_usage_hardware
+            };
+
+            mir_connection_create_surface_sync(connection, &request_params);
+
+            surface_created.set();
+            server_done.wait();
+        }
+
+        Flag& surface_created;
+        Flag& server_done;
+    };
+
+    ClientConfig client_config1{surface_created1, server_done};
+    ClientConfig client_config2{surface_created2, server_done};
+    ClientConfig client_config3{surface_created3, server_done};
+
+    launch_client_process(client_config1);
+    launch_client_process(client_config2);
+    launch_client_process(client_config3);
+
+    run_in_test_process([&]
+    {
+        /* Wait until the clients have created a surface */
+        surface_created1.wait();
+        surface_created2.wait();
+        surface_created3.wait();
+
+        /* Shut down the server */
+        shutdown_server_process();
+
+        /* Wait until we have checked the resources in the server process */
+        while (!resources_freed_failure.is_set() && !resources_freed_success.is_set())
+            std::this_thread::sleep_for(std::chrono::milliseconds(1));
+
+        /* Fail if the resources have not been freed */
+        if (resources_freed_failure.is_set())
+            ADD_FAILURE();
+
+        /* Notify the clients that we are done (we only need to set the flag once) */
+        server_done.set();
+    });
+
+    /*
+     * Check that all resources are freed after destroying the server config.
+     * Note that these checks are run multiple times: in the server process,
+     * in each of the client processes and in the test process. We only care about
+     * the results in the server process (in the other cases the checks will
+     * succeed anyway, since we are not using the config object).
+     */
+    std::weak_ptr<mir::graphics::Display> display = server_config->the_display();
+    std::weak_ptr<mir::compositor::Compositor> compositor = server_config->the_compositor();
+    std::weak_ptr<mir::frontend::Communicator> communicator = server_config->the_communicator();
+    std::weak_ptr<mir::input::InputManager> input_manager = server_config->the_input_manager();
+
+    server_config.reset();
+
+    EXPECT_EQ(0, display.use_count());
+    EXPECT_EQ(0, compositor.use_count());
+    EXPECT_EQ(0, communicator.use_count());
+    EXPECT_EQ(0, input_manager.use_count());
+
+    if (display.use_count() != 0 ||
+        compositor.use_count() != 0 ||
+        communicator.use_count() != 0 ||
+        input_manager.use_count() != 0)
+    {
+        resources_freed_failure.set();
+    }
+    else
+    {
+        resources_freed_success.set();
+    }
 }

=== modified file 'tests/integration-tests/shell/test_session_manager.cpp'
--- tests/integration-tests/shell/test_session_manager.cpp	2013-05-02 00:11:18 +0000
+++ tests/integration-tests/shell/test_session_manager.cpp	2013-05-14 09:55:30 +0000
@@ -70,6 +70,8 @@
     auto session2 = session_manager.open_session("Microsoft Access", std::shared_ptr<me::EventSink>());
     auto session3 = session_manager.open_session("WordPerfect", std::shared_ptr<me::EventSink>());
 
+    Mock::VerifyAndClearExpectations(&focus_setter);
+
     {
       InSequence seq;
       EXPECT_CALL(focus_setter, set_focus_to(Eq(session1))).Times(1);
@@ -80,6 +82,11 @@
     session_manager.focus_next();
     session_manager.focus_next();
     session_manager.focus_next();
+
+    Mock::VerifyAndClearExpectations(&focus_setter);
+
+    // Possible change of focus while sessions are closed on shutdown
+    EXPECT_CALL(focus_setter, set_focus_to(_)).Times(AtLeast(0));
 }
 
 TEST(TestSessionManagerAndFocusSelectionStrategy, closing_applications_transfers_focus)
@@ -108,6 +115,8 @@
     auto session2 = session_manager.open_session("Microsoft Access", std::shared_ptr<me::EventSink>());
     auto session3 = session_manager.open_session("WordPerfect", std::shared_ptr<me::EventSink>());
 
+    Mock::VerifyAndClearExpectations(&focus_setter);
+
     {
       InSequence seq;
       EXPECT_CALL(focus_setter, set_focus_to(Eq(session2))).Times(1);
@@ -116,4 +125,9 @@
 
     session_manager.close_session(session3);
     session_manager.close_session(session2);
+
+    Mock::VerifyAndClearExpectations(&focus_setter);
+
+    // Possible change of focus while sessions are closed on shutdown
+    EXPECT_CALL(focus_setter, set_focus_to(_)).Times(AtLeast(0));
 }