Merge lp:~afrantzis/mir/fix-1378740 into lp:mir

[Alexandros Frantzis]

input: Stop input threads properly when their controlling components get destroyed (LP: #1378740)

Otherwise we may destroy the thread objects without first having joined (or detached) them, leading to an abort.

input: Stop input threads properly when their controlling components get destroyed (LP: #1378740)

Otherwise we may destroy the thread objects without first having joined (or detached) them, leading to an abort.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1968
http://jenkins.qa.ubuntu.com/job/mir-ci/1756/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/2052
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/2059
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/1989
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-utopic-amd64-ci/94
        deb: http://jenkins.qa.ubuntu.com/job/mir-utopic-amd64-ci/94/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/926
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/926/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/2997
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/14330

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

LGTM

review: Approve

[Andreas Pokorny (andreas-pokorny)]

ok

review: Approve

=== modified file 'src/server/input/android/android_input_dispatcher.cpp'
--- src/server/input/android/android_input_dispatcher.cpp	2014-09-11 05:51:44 +0000
+++ src/server/input/android/android_input_dispatcher.cpp	2014-10-08 11:35:30 +0000
@@ -37,6 +37,13 @@
 {
 }
 
+mia::AndroidInputDispatcher::~AndroidInputDispatcher()
+{
+    // It is safe to call stop(), even if we haven't been started at all,
+    // or we have been previously started and stopped manually.
+    stop();
+}
+
 void mia::AndroidInputDispatcher::configuration_changed(nsecs_t when)
 {
     droidinput::NotifyConfigurationChangedArgs args(when);

=== modified file 'src/server/input/android/android_input_dispatcher.h'
--- src/server/input/android/android_input_dispatcher.h	2014-09-11 05:51:44 +0000
+++ src/server/input/android/android_input_dispatcher.h	2014-10-08 11:35:30 +0000
@@ -45,6 +45,7 @@
 public:
     AndroidInputDispatcher(std::shared_ptr<droidinput::InputDispatcherInterface> const& dispatcher,
                            std::shared_ptr<InputThread> const& thread);
+    ~AndroidInputDispatcher();
     void configuration_changed(nsecs_t when) override;
     void device_reset(int32_t device_id, nsecs_t when) override;
     void dispatch(MirEvent const& event) override;

=== modified file 'src/server/input/android/android_input_manager.cpp'
--- src/server/input/android/android_input_manager.cpp	2014-09-11 05:51:44 +0000
+++ src/server/input/android/android_input_manager.cpp	2014-10-08 11:35:30 +0000
@@ -36,6 +36,9 @@
 
 mia::InputManager::~InputManager()
 {
+    // It is safe to call stop(), even if we haven't been started at all,
+    // or we have been previously started and stopped manually.
+    stop();
 }
 
 void mia::InputManager::stop()

=== modified file 'tests/integration-tests/test_server_shutdown.cpp'
--- tests/integration-tests/test_server_shutdown.cpp	2014-09-30 13:50:09 +0000
+++ tests/integration-tests/test_server_shutdown.cpp	2014-10-08 11:35:30 +0000
@@ -22,6 +22,7 @@
 #include "mir/compositor/renderer_factory.h"
 
 #include "mir/run_mir.h"
+#include "mir/main_loop.h"
 
 #include "mir_toolkit/mir_client_library.h"
 
@@ -432,3 +433,40 @@
     EXPECT_EQ(0, connector.use_count());
     EXPECT_EQ(0, input_manager.use_count());
 }
+
+// This also acts as a regression test for LP: #1378740
+TEST(ServerShutdownWithThreadException,
+     server_releases_resources_on_abnormal_main_thread_termination)
+{
+    // Use the FakeEventHubServerConfig to get the production input components
+    // (with the exception of EventHub, of course).
+    auto server_config = std::make_shared<FakeEventHubServerConfig>();
+
+    std::thread server{
+        [&]
+        {
+            EXPECT_THROW(
+                mir::run_mir(*server_config,
+                    [server_config](mir::DisplayServer&)
+                    {
+                        server_config->the_main_loop()->enqueue(
+                            server_config.get(),
+                            [] { throw std::runtime_error(""); });
+                    }),
+                std::runtime_error);
+        }};
+
+    server.join();
+
+    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::Connector> connector = server_config->the_connector();
+    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, connector.use_count());
+    EXPECT_EQ(0, input_manager.use_count());
+}

=== modified file 'tests/unit-tests/input/android/test_android_input_dispatcher.cpp'
--- tests/unit-tests/input/android/test_android_input_dispatcher.cpp	2014-09-11 05:51:44 +0000
+++ tests/unit-tests/input/android/test_android_input_dispatcher.cpp	2014-10-08 11:35:30 +0000
@@ -57,6 +57,20 @@
 
 struct AndroidInputDispatcherTest : public testing::Test
 {
+    ~AndroidInputDispatcherTest()
+    {
+        using namespace ::testing;
+
+        Mock::VerifyAndClearExpectations(dispatcher.get());
+        Mock::VerifyAndClearExpectations(dispatcher_thread.get());
+
+        // mia::AndroidInputDispatcher destruction expectations
+        InSequence s;
+        EXPECT_CALL(*dispatcher_thread, request_stop());
+        EXPECT_CALL(*dispatcher, setInputDispatchMode(mia::DispatchDisabled, mia::DispatchFrozen));
+        EXPECT_CALL(*dispatcher_thread, join());
+    }
+
     std::shared_ptr<mtd::MockAndroidInputDispatcher> dispatcher = std::make_shared<mtd::MockAndroidInputDispatcher>();
     std::shared_ptr<MockInputThread> dispatcher_thread = std::make_shared<MockInputThread>();
     mia::AndroidInputDispatcher input_dispatcher{dispatcher,dispatcher_thread};
@@ -65,7 +79,7 @@
 
 }
 
-TEST_F(AndroidInputDispatcherTest, start_starts_dispathcer_and_thread)
+TEST_F(AndroidInputDispatcherTest, start_starts_dispatcher_and_thread)
 {
     using namespace ::testing;
 

=== modified file 'tests/unit-tests/input/android/test_android_input_manager.cpp'
--- tests/unit-tests/input/android/test_android_input_manager.cpp	2014-09-11 05:51:44 +0000
+++ tests/unit-tests/input/android/test_android_input_manager.cpp	2014-10-08 11:35:30 +0000
@@ -95,35 +95,56 @@
 {
     std::shared_ptr<MockEventHub> event_hub = std::make_shared<MockEventHub>();
     std::shared_ptr<MockInputThread> reader_thread = std::make_shared<MockInputThread>();
-};
-
-}
-
-TEST_F(AndroidInputManagerSetup, start_and_stop)
-{
-    using namespace ::testing;
-
-    ExpectationSet reader_setup;
-
-
-    reader_setup += EXPECT_CALL(*event_hub, flush()).Times(1);
-
-    EXPECT_CALL(*reader_thread, start())
-        .Times(1)
-        .After(reader_setup);
-
-    {
-        InSequence seq;
+
+    void setup_start_expectations()
+    {
+        testing::InSequence seq;
+
+        EXPECT_CALL(*event_hub, flush());
+        EXPECT_CALL(*reader_thread, start());
+    }
+
+    void setup_stop_expectations()
+    {
+        testing::InSequence seq;
 
         EXPECT_CALL(*reader_thread, request_stop());
         EXPECT_CALL(*event_hub, wake());
         EXPECT_CALL(*reader_thread, join());
     }
+};
+
+}
+
+TEST_F(AndroidInputManagerSetup, starts_and_stops_reader)
+{
+    using namespace ::testing;
+
+    setup_start_expectations();
+    setup_stop_expectations();
 
     mia::InputManager manager(event_hub, reader_thread);
 
     manager.start();
     manager.stop();
+
+    // The input manager is unconditionally stopped at destruction.
+    Mock::VerifyAndClearExpectations(reader_thread.get());
+    Mock::VerifyAndClearExpectations(event_hub.get());
+
+    setup_stop_expectations();
+}
+
+TEST_F(AndroidInputManagerSetup, stops_reader_at_destruction)
+{
+    using namespace ::testing;
+
+    setup_start_expectations();
+    setup_stop_expectations();
+
+    mia::InputManager manager(event_hub, reader_thread);
+
+    manager.start();
 }
 
 TEST_F(AndroidInputManagerSetup, channel_factory_returns_input_channel_with_fds)