Merge lp:~afrantzis/mir/fix-1513901-shutdown-crash-with-mesa-kms into lp:mir

[Alexandros Frantzis]

platform: Platform emergency cleanup handlers should keep platform modules alive

Otherwise, the cleanup handler destructor (which is part of the platform shared
object code) may be called after the platform shared object has been unloaded,
leading to a crash.

platform: Platform emergency cleanup handlers should keep platform modules alive

Otherwise, the cleanup handler destructor (which is part of the platform shared object code) may be called after the platform shared object has been unloaded, leading to a crash.

Note that the changes in this MP break the platform ABI, but the ABI has already been bumped after 0.17.

[Alan Griffiths (alan-griffiths)]

Fixes the problem.

It seems a shame to add yet a further level of indirection (and memory allocation) to std::function<> but this only costs during startup/shutdown, so OK.

review: Approve

[Andreas Pokorny (andreas-pokorny)]

> Fixes the problem.
>
> It seems a shame to add yet a further level of indirection (and memory
> allocation) to std::function<> but this only costs during startup/shutdown, so
> OK.

Hm there is a proposal for a std::function with allocator..

Couldnt we store alive-keeping inside the bound functor. (Downside: Moves responsibility of being aware of the problem and dealing with it properly to the caller/platform author.. ).

review: Needs Information

[Andreas Pokorny (andreas-pokorny)]

I meant:
     emergency_cleanup_registry.add(
        [weak_vt, weak_drm, keep_library_alive=mir::detail::RefCountedLibrary(reinterpret_cast<void*>(&create_host_platform))]
        {
            if (auto const vt = weak_vt.lock())
                try { vt->restore(); } catch (...) {}

            if (auto const drm = weak_drm.lock())
                try { drm->drop_master(); } catch (...) {}
 });

[Alexandros Frantzis (afrantzis)]

> keep_library_alive=mir::detail::RefCountedLibrary(reinterpret_cast<void*>(&create_host_platform))]

This what I tried first (although I used a shared_ptr<> backed by mir::UniqueModulePtr<int> as the keep_alive object) but it didn't have the desired result. I also tried your proposal (RefCountedLibrary) a moment ago without success either.

[Andreas Pokorny (andreas-pokorny)]

oh you might then have the problem that the destructor code is unloaded while being executed.. since it is part of the lambda object inside the platform library...

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3093
http://jenkins.qa.ubuntu.com/job/mir-ci/5527/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/4833
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/3740
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/4775
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-wily-touch/733/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1681
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1681/artifact/work/output/*zip*/output.zip
    None: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/733/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4776
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4776/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/7376
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/25013
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-wily-armhf/734/console

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

review: Approve (continuous-integration)

=== modified file 'src/include/platform/mir/emergency_cleanup_registry.h'
--- src/include/platform/mir/emergency_cleanup_registry.h	2015-02-22 07:46:25 +0000
+++ src/include/platform/mir/emergency_cleanup_registry.h	2015-11-09 11:17:37 +0000
@@ -21,11 +21,13 @@
 #define MIR_EMERGENCY_CLEANUP_REGISTRY_H_
 
 #include <functional>
+#include "mir/module_deleter.h"
 
 namespace mir
 {
 
-typedef std::function<void()> EmergencyCleanupHandler;
+using EmergencyCleanupHandler = std::function<void()>;
+using ModuleEmergencyCleanupHandler = mir::UniqueModulePtr<std::function<void()>>;
 
 class EmergencyCleanupRegistry
 {
@@ -33,6 +35,7 @@
     virtual ~EmergencyCleanupRegistry() = default;
 
     virtual void add(EmergencyCleanupHandler const& handler) = 0;
+    virtual void add(ModuleEmergencyCleanupHandler handler) = 0;
 
 protected:
     EmergencyCleanupRegistry() = default;

=== modified file 'src/platforms/mesa/server/kms/platform.cpp'
--- src/platforms/mesa/server/kms/platform.cpp	2015-11-05 18:24:29 +0000
+++ src/platforms/mesa/server/kms/platform.cpp	2015-11-09 11:17:37 +0000
@@ -51,14 +51,16 @@
     std::weak_ptr<VirtualTerminal> weak_vt = vt;
     std::weak_ptr<mgmh::DRMHelper> weak_drm = drm;
     emergency_cleanup_registry.add(
-        [weak_vt,weak_drm]
-        {
-            if (auto const vt = weak_vt.lock())
-                try { vt->restore(); } catch (...) {}
-
-            if (auto const drm = weak_drm.lock())
-                try { drm->drop_master(); } catch (...) {}
-        });
+        make_module_ptr<EmergencyCleanupHandler>(
+            [weak_vt,weak_drm]
+            {
+                if (auto const vt = weak_vt.lock())
+                    try { vt->restore(); } catch (...) {}
+
+                if (auto const drm = weak_drm.lock())
+                    try { drm->drop_master(); } catch (...) {}
+            }));
+
 }
 
 mir::UniqueModulePtr<mg::GraphicBufferAllocator> mgm::Platform::create_buffer_allocator()

=== modified file 'src/server/default_emergency_cleanup.cpp'
--- src/server/default_emergency_cleanup.cpp	2014-07-03 09:46:40 +0000
+++ src/server/default_emergency_cleanup.cpp	2015-11-09 11:17:37 +0000
@@ -22,7 +22,16 @@
 {
     std::lock_guard<std::mutex> lock{handlers_mutex};
 
-    last_item()->next.reset(new ListItem{handler, nullptr});
+    last_item()->next.reset(
+        new ListItem{std::make_shared<EmergencyCleanupHandler>(handler), nullptr});
+    ++num_handlers;
+}
+
+void mir::DefaultEmergencyCleanup::add(ModuleEmergencyCleanupHandler handler)
+{
+    std::lock_guard<std::mutex> lock{handlers_mutex};
+
+    last_item()->next.reset(new ListItem{std::move(handler), nullptr});
     ++num_handlers;
 }
 
@@ -33,7 +42,7 @@
     while (handlers_left-- > 0)
     {
         item = item->next.get();
-        item->handler();
+        (*item->handler)();
     }
 }
 

=== modified file 'src/server/default_emergency_cleanup.h'
--- src/server/default_emergency_cleanup.h	2015-02-22 07:46:25 +0000
+++ src/server/default_emergency_cleanup.h	2015-11-09 11:17:37 +0000
@@ -32,12 +32,13 @@
 {
 public:
     void add(EmergencyCleanupHandler const& handler) override;
+    void add(ModuleEmergencyCleanupHandler handler) override;
     void operator()() const override;
 
 private:
     struct ListItem
     {
-        EmergencyCleanupHandler handler;
+        std::shared_ptr<EmergencyCleanupHandler> handler;
         std::unique_ptr<ListItem> next;
     };
 

=== modified file 'tests/include/mir/test/doubles/null_emergency_cleanup.h'
--- tests/include/mir/test/doubles/null_emergency_cleanup.h	2014-05-21 11:59:30 +0000
+++ tests/include/mir/test/doubles/null_emergency_cleanup.h	2015-11-09 11:17:37 +0000
@@ -32,6 +32,7 @@
 {
  public:
     void add(EmergencyCleanupHandler const&) override {}
+    void add(ModuleEmergencyCleanupHandler) override {}
     void operator()() const override {}
 };
 

=== modified file 'tests/unit-tests/graphics/mesa/kms/test_platform.cpp'
--- tests/unit-tests/graphics/mesa/kms/test_platform.cpp	2015-11-02 21:56:56 +0000
+++ tests/unit-tests/graphics/mesa/kms/test_platform.cpp	2015-11-09 11:17:37 +0000
@@ -235,12 +235,16 @@
 
 struct StubEmergencyCleanupRegistry : mir::EmergencyCleanupRegistry
 {
-    void add(mir::EmergencyCleanupHandler const& handler) override
-    {
-        this->handler = handler;
-    }
-
-    mir::EmergencyCleanupHandler handler;
+    void add(mir::EmergencyCleanupHandler const&) override
+    {
+    }
+
+    void add(mir::ModuleEmergencyCleanupHandler handler) override
+    {
+        this->handler = std::move(handler);
+    }
+
+    mir::ModuleEmergencyCleanupHandler handler;
 };
 
 TEST_F(MesaGraphicsPlatform, restores_vt_on_emergency_cleanup)
@@ -257,7 +261,7 @@
 
     EXPECT_CALL(*mock_vt, restore());
 
-    emergency_cleanup_registry.handler();
+    (*emergency_cleanup_registry.handler)();
 
     Mock::VerifyAndClearExpectations(mock_vt.get());
 }
@@ -278,7 +282,7 @@
     EXPECT_CALL(mock_drm, drmDropMaster(mock_drm.fake_drm.fd()))
         .WillOnce(Return(success_code));
 
-    emergency_cleanup_registry.handler();
+    (*emergency_cleanup_registry.handler)();
 
     Mock::VerifyAndClearExpectations(&mock_drm);
 }
@@ -300,7 +304,7 @@
     EXPECT_CALL(mock_drm, drmDropMaster(mock_drm.fake_drm.fd()))
         .WillOnce(Throw(std::runtime_error("drm drop master exception")));
 
-    emergency_cleanup_registry.handler();
+    (*emergency_cleanup_registry.handler)();
 
     Mock::VerifyAndClearExpectations(&mock_drm);
 }