Merge lp:~afrantzis/unity-system-compositor/fix-1491566-deadlock into lp:unity-system-compositor

[Alexandros Frantzis]

Fix deadlock by ensuring our alarm handlers acquire locks in the correct order

Fix deadlock by ensuring our alarm handlers acquire locks in the correct order

This MP adds the before_power_off_alarm and before_dimmer_alarm virtual functions which do nothing in production. They are used only in tests for in order to allow us to deterministically reproduce the deadlock. The runtime cost in production is minimal and the worth we get from protection against regressions of this bug is significant.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:253
http://jenkins.qa.ubuntu.com/job/unity-system-compositor-ci/285/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-system-compositor-wily-amd64-ci/73
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-system-compositor-wily-armhf-ci/73
        deb: http://jenkins.qa.ubuntu.com/job/unity-system-compositor-wily-armhf-ci/73/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-system-compositor-wily-i386-ci/73

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-system-compositor-ci/285/rebuild

review: Approve (continuous-integration)

[Andreas Pokorny (andreas-pokorny)]

Do you really need the before_* methods in MirScreen?
You could simply override the method and call a before function<> inside the test setup.

Why does that fix the issue? The lock ordering is still the same now?

review: Needs Information

[lp:~afrantzis/unity-system-compositor/fix-1491566-deadlock]

254. By Alexandros Frantzis on 2015-09-23

Make MirScreen alarm handlers overridable for test purposes

[Alexandros Frantzis (afrantzis)]

> Do you really need the before_* methods in MirScreen?
> You could simply override the method and call a before function<> inside the test setup.

Ack. Changed!

> Why does that fix the issue? The lock ordering is still the same now?

The LockableCallback::lock() and unlock() functions are called before acquiring the internal alarm lock ([1]). So when the alarm callback occurs we now get [lock MirScreen], [lock Alarm], [dispatch callback] instead of [lock Alarm], [dispatch callback], [lock MirScreen] we had previously.

[1] http://bazaar.launchpad.net/~mir-team/mir/development-branch/view/head:/src/server/glib_main_loop_sources.cpp#L308

[Alexandros Frantzis (afrantzis)]

> The LockableCallback::lock() and unlock() functions are called before

s/and unlock()//

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:254
http://jenkins.qa.ubuntu.com/job/unity-system-compositor-ci/286/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-system-compositor-wily-amd64-ci/74
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-system-compositor-wily-armhf-ci/74
        deb: http://jenkins.qa.ubuntu.com/job/unity-system-compositor-wily-armhf-ci/74/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-system-compositor-wily-i386-ci/74

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-system-compositor-ci/286/rebuild

review: Approve (continuous-integration)

[Andreas Pokorny (andreas-pokorny)]

Happy with the change.. I wonder if it would be simpler code if we had Dispatchables there and execute them single threaded. But thats a bigger rework

review: Approve

=== modified file 'src/mir_screen.cpp'
--- src/mir_screen.cpp	2015-08-24 18:14:36 +0000
+++ src/mir_screen.cpp	2015-09-23 20:13:49 +0000
@@ -18,6 +18,7 @@
 #include "clock.h"
 
 #include <mir/main_loop.h>
+#include <mir/lockable_callback.h>
 #include <mir/time/alarm_factory.h>
 #include <mir/compositor/compositor.h>
 #include <mir/graphics/display.h>
@@ -32,6 +33,50 @@
 namespace mi = mir::input;
 namespace mg = mir::graphics;
 
+class usc::MirScreen::LockableCallback : public mir::LockableCallback
+{
+public:
+    LockableCallback(MirScreen* mir_screen)
+        : mir_screen{mir_screen}
+    {
+    }
+
+    void lock() override
+    {
+        guard_lock = std::unique_lock<std::mutex>{mir_screen->guard};
+    }
+
+    void unlock() override
+    {
+        if (guard_lock.owns_lock())
+            guard_lock.unlock();
+    }
+    
+protected:
+    MirScreen* const mir_screen;
+    std::unique_lock<std::mutex> guard_lock;
+};
+
+class usc::MirScreen::PowerOffLockableCallback : public usc::MirScreen::LockableCallback
+{
+    using usc::MirScreen::LockableCallback::LockableCallback;
+
+    void operator()() override
+    {
+        mir_screen->power_off_alarm_notification();
+    }
+};
+
+class usc::MirScreen::DimmerLockableCallback : public usc::MirScreen::LockableCallback
+{
+    using usc::MirScreen::LockableCallback::LockableCallback;
+
+    void operator()() override
+    {
+        mir_screen->dimmer_alarm_notification();
+    }
+};
+
 usc::MirScreen::MirScreen(
     std::shared_ptr<usc::ScreenHardware> const& screen_hardware,
     std::shared_ptr<mir::compositor::Compositor> const& compositor,
@@ -48,9 +93,9 @@
       alarm_factory{alarm_factory},
       clock{clock},
       power_off_alarm{alarm_factory->create_alarm(
-              std::bind(&usc::MirScreen::power_off_alarm_notification, this))},
+              std::make_shared<PowerOffLockableCallback>(this))},
       dimmer_alarm{alarm_factory->create_alarm(
-              std::bind(&usc::MirScreen::dimmer_alarm_notification, this))},
+              std::make_shared<DimmerLockableCallback>(this))},
       inactivity_timeouts(inactivity_timeouts),
       notification_timeouts(notification_timeouts),
       current_power_mode{MirPowerMode::mir_power_mode_on},
@@ -313,14 +358,12 @@
 
 void usc::MirScreen::power_off_alarm_notification()
 {
-    std::lock_guard<std::mutex> lock{guard};
     configure_display_l(MirPowerMode::mir_power_mode_off, PowerStateChangeReason::inactivity);
     next_power_off = {};
 }
 
 void usc::MirScreen::dimmer_alarm_notification()
 {
-    std::lock_guard<std::mutex> lock{guard};
     screen_hardware->set_dim_backlight();
     next_dimming = {};
 }

=== modified file 'src/mir_screen.h'
--- src/mir_screen.h	2015-07-17 12:43:46 +0000
+++ src/mir_screen.h	2015-09-23 20:13:49 +0000
@@ -73,7 +73,16 @@
     void register_power_state_change_handler(
             PowerStateChangeHandler const& power_state_change_handler) override;
 
+protected:
+    // These are protected virtual because we need to override them in tests
+    virtual void power_off_alarm_notification();
+    virtual void dimmer_alarm_notification();
+
 private:
+    class LockableCallback;
+    class PowerOffLockableCallback;
+    class DimmerLockableCallback;
+
     void set_screen_power_mode_l(MirPowerMode mode, PowerStateChangeReason reason);
     void configure_display_l(MirPowerMode mode, PowerStateChangeReason reason);
 
@@ -84,8 +93,6 @@
     Timeouts timeouts_for(PowerStateChangeReason reason);
     bool is_screen_change_allowed(MirPowerMode mode, PowerStateChangeReason reason);
 
-    void power_off_alarm_notification();
-    void dimmer_alarm_notification();
     void long_press_alarm_notification();
 
     std::shared_ptr<usc::ScreenHardware> const screen_hardware;

=== modified file 'tests/integration-tests/CMakeLists.txt'
--- tests/integration-tests/CMakeLists.txt	2015-08-26 08:00:27 +0000
+++ tests/integration-tests/CMakeLists.txt	2015-09-23 20:13:49 +0000
@@ -38,6 +38,7 @@
   test_unity_input_service.cpp
   test_external_spinner.cpp
   test_powerd_mediator.cpp
+  test_deadlock_lp1491566.cpp
 )
 
 target_link_libraries(

=== added file 'tests/integration-tests/test_deadlock_lp1491566.cpp'
--- tests/integration-tests/test_deadlock_lp1491566.cpp	1970-01-01 00:00:00 +0000
+++ tests/integration-tests/test_deadlock_lp1491566.cpp	2015-09-23 20:13:49 +0000
@@ -0,0 +1,283 @@
+/*
+ * Copyright © 2015 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Alexandros Frantzis <[e-mail]>
+ */
+
+#include "src/server.h"
+#include "src/mir_screen.h"
+#include "src/screen_hardware.h"
+#include "src/power_state_change_reason.h"
+#include "spin_wait.h"
+
+#include <mir/compositor/compositor.h>
+#include <mir/main_loop.h>
+#include <mir/graphics/display.h>
+#include <mir/graphics/display_configuration.h>
+#include <mir/graphics/gl_context.h>
+#include <mir/input/touch_visualizer.h>
+
+#include <gtest/gtest.h>
+#include <gmock/gmock.h>
+
+#include <thread>
+#include <future>
+
+namespace mg = mir::graphics;
+
+namespace
+{
+
+struct NullCompositor : mir::compositor::Compositor
+{
+    void start() override {}
+    void stop() override {}
+};
+
+struct StubDisplayConfiguration : mg::DisplayConfiguration
+{
+    StubDisplayConfiguration()
+    {
+        conf_output.power_mode = MirPowerMode::mir_power_mode_on;
+    }
+
+    void for_each_card(std::function<void(mg::DisplayConfigurationCard const&)> f) const override
+    {
+    }
+
+    void for_each_output(std::function<void(mg::DisplayConfigurationOutput const&)> f) const override
+    {
+        f(conf_output);
+    }
+
+    void for_each_output(std::function<void(mg::UserDisplayConfigurationOutput&)> f)
+    {
+        mg::UserDisplayConfigurationOutput user{conf_output};
+        f(user);
+    }
+
+    mg::DisplayConfigurationOutput conf_output;
+};
+
+struct StubDisplay : mg::Display
+{
+    void for_each_display_sync_group(std::function<void(mg::DisplaySyncGroup&)> const& f) override
+    {
+    }
+
+    std::unique_ptr<mg::DisplayConfiguration> configuration() const override
+    { 
+        return std::unique_ptr<mg::DisplayConfiguration>{new StubDisplayConfiguration{}};
+    }
+
+    void configure(mg::DisplayConfiguration const&) override {}
+
+    void register_configuration_change_handler(
+        mg::EventHandlerRegister& ,
+        mg::DisplayConfigurationChangeHandler const& ) override
+    {
+    }
+
+    void register_pause_resume_handlers(
+        mg::EventHandlerRegister&,
+        mg::DisplayPauseHandler const&,
+        mg::DisplayResumeHandler const&) override
+    {
+    }
+
+    void pause() override {}
+    void resume() override {}
+
+    std::shared_ptr<mg::Cursor> create_hardware_cursor(
+        std::shared_ptr<mg::CursorImage> const&) override
+    {
+        return{};
+    }
+
+    std::unique_ptr<mg::GLContext> create_gl_context() override
+    { 
+        return std::unique_ptr<mg::GLContext>{};
+    }
+};
+
+struct StubScreenHardware : usc::ScreenHardware
+{
+    void set_dim_backlight() override {}
+    void set_normal_backlight() override {}
+    void turn_off_backlight() override {}
+
+    void change_backlight_values(int dim_brightness, int normal_brightness) override {}
+
+    void allow_suspend() override {}
+    void disable_suspend() override {}
+
+    void enable_auto_brightness(bool) override {}
+    bool auto_brightness_supported() override { return true; }
+    void set_brightness(int) override {}
+    int min_brightness() override { return 100; }
+    int max_brightness() override { return 0; }
+
+    void enable_proximity(bool) override { }
+};
+
+struct NullTouchVisualizer : mir::input::TouchVisualizer
+{
+    void enable() override {}
+    void disable() override {}
+    void visualize_touches(std::vector<Spot> const& touches) override {}
+};
+
+class TestMirScreen : public usc::MirScreen
+{
+public:
+    using usc::MirScreen::MirScreen;
+
+    void dimmer_alarm_notification() override
+    {
+        before_dimmer_alarm_func();
+        usc::MirScreen::dimmer_alarm_notification();
+    }
+
+    void power_off_alarm_notification() override
+    {
+        before_power_off_alarm_func();
+        usc::MirScreen::power_off_alarm_notification();
+    }
+
+    std::function<void()> before_dimmer_alarm_func = []{};
+    std::function<void()> before_power_off_alarm_func = []{};
+};
+
+struct DeadlockLP1491566 : public testing::Test
+{
+    DeadlockLP1491566()
+    {
+        main_loop_thread = std::thread{[this] { main_loop->run(); }};
+    }
+
+    ~DeadlockLP1491566()
+    {
+        main_loop->stop();
+        main_loop_thread.join();
+    }
+
+    std::future<void> async_set_screen_power_mode()
+    {
+        return std::async(
+            std::launch::async,
+            [this]
+            {
+                mir_screen.set_screen_power_mode(
+                    MirPowerMode::mir_power_mode_on,
+                    PowerStateChangeReason::power_key);
+            });
+    }
+
+    void schedule_inactivity_handlers()
+    {
+        // We set the screen power mode to on, which will
+        // schedule inactivity handlers
+        mir_screen.set_screen_power_mode(
+            MirPowerMode::mir_power_mode_on,
+            PowerStateChangeReason::power_key);
+    }
+
+    void wait_for_async_operation(std::future<void>& future)
+    {
+        if (!usc::test::spin_wait_for_condition_or_timeout(
+                [&future] { return future.valid(); },
+                std::chrono::seconds{3}))
+        {
+            throw std::runtime_error{"Future is not valid!"};
+        }
+
+        if (future.wait_for(std::chrono::seconds{3}) != std::future_status::ready)
+        {
+            std::cerr << "Deadlock detected. Aborting." << std::endl;
+            abort();
+        }
+    }
+
+    char *argv[1] = {nullptr};
+    usc::Server server{0, argv};
+    std::shared_ptr<mir::MainLoop> const main_loop{server.the_main_loop()};
+    std::chrono::milliseconds const power_off_timeout{200};
+    std::chrono::milliseconds const dimmer_timeout{100};
+
+    TestMirScreen mir_screen{
+        std::make_shared<StubScreenHardware>(),
+        std::make_shared<NullCompositor>(),
+        std::make_shared<StubDisplay>(),
+        std::make_shared<NullTouchVisualizer>(),
+        main_loop,
+        server.the_clock(),
+        {power_off_timeout, dimmer_timeout},
+        {power_off_timeout, dimmer_timeout}};
+
+    std::thread main_loop_thread;
+};
+
+}
+
+// The following tests reproduce the situation we are seeing in bug 1491566,
+// where a set_screen_power_mode request tries to run concurrently with either
+// the dimmer or power-off timeout handlers. The deadlock is caused by two
+// threads trying to acquire the internal alarm lock and the MirScreen lock in
+// opposite orders.
+//
+// To reproduce the race in these tests, we need to run the
+// set_screen_power_mode request just before the timeout handlers are invoked,
+// while the thread invoking the timeout handlers holds the internal alarm
+// lock, but before it has acquired the MirScreen lock. The thread calling
+// set_screen_power_mode will then first acquire the MirScreen lock and then
+// try to acquire the internal alarm clock and block. The timeout handler
+// thread will eventually resume and try to get the MirScreen lock, leading to
+// a deadlock.
+
+TEST_F(DeadlockLP1491566, between_dimmer_handler_and_screen_power_mode_request_is_averted)
+{
+    std::future<void> future;
+
+    mir_screen.before_dimmer_alarm_func = 
+        [this, &future]
+        {
+            future = async_set_screen_power_mode();
+            // Wait a bit for async operation to get processed
+            std::this_thread::sleep_for(
+                std::chrono::milliseconds{500});
+        };
+
+    schedule_inactivity_handlers();
+
+    wait_for_async_operation(future);
+}
+
+TEST_F(DeadlockLP1491566, between_power_off_handler_and_screen_power_mode_request_is_averted)
+{
+    std::future<void> future;
+
+    mir_screen.before_power_off_alarm_func = 
+        [this, &future]
+        {
+            future = async_set_screen_power_mode();
+            // Wait a bit for async operation to get processed
+            std::this_thread::sleep_for(
+                std::chrono::milliseconds{500});
+        };
+
+    schedule_inactivity_handlers();
+
+    wait_for_async_operation(future);
+}

=== modified file 'tests/unit-tests/advanceable_timer.cpp'
--- tests/unit-tests/advanceable_timer.cpp	2015-07-15 13:48:30 +0000
+++ tests/unit-tests/advanceable_timer.cpp	2015-09-23 20:13:49 +0000
@@ -17,6 +17,7 @@
  */
 
 #include "advanceable_timer.h"
+#include <mir/lockable_callback.h>
 #include <algorithm>
 
 namespace
@@ -59,7 +60,7 @@
 {
     AdvanceableAlarm(
         mir::time::Timestamp now,
-        std::function<void(void)> const& callback)
+        std::shared_ptr<mir::LockableCallback> const& callback)
         : now{now}, callback{callback}
     {
     }
@@ -101,7 +102,9 @@
         {
             state_ = mir::time::Alarm::triggered;
             lock.unlock();
-            callback();
+            callback->lock();
+            (*callback)();
+            callback->unlock();
         }
     }
 
@@ -113,7 +116,7 @@
 
 private:
     mir::time::Timestamp now;
-    std::function<void()> const callback;
+    std::shared_ptr<mir::LockableCallback> const callback;
     mir::time::Timestamp next_trigger_;
     mutable std::mutex mutex;
     mir::time::Alarm::State state_ = mir::time::Alarm::triggered;
@@ -123,6 +126,26 @@
 std::unique_ptr<mir::time::Alarm> AdvanceableTimer::create_alarm(
     std::function<void()> const& callback)
 {
+    struct SimpleLockableCallback : public mir::LockableCallback
+    {
+        SimpleLockableCallback(std::function<void()> const& callback)
+            : callback{callback}
+        {
+        }
+
+        void operator()() override { callback(); }
+        void lock() override {}
+        void unlock() override {}
+
+        std::function<void()> const callback;
+    };
+
+    return create_alarm(std::make_shared<SimpleLockableCallback>(callback));
+}
+
+std::unique_ptr<mir::time::Alarm> AdvanceableTimer::create_alarm(
+    std::shared_ptr<mir::LockableCallback> const& callback)
+{
     auto const adv_alarm =
         std::make_shared<detail::AdvanceableAlarm>(now(), callback);
 
@@ -131,12 +154,6 @@
     return std::unique_ptr<AlarmWrapper>(new AlarmWrapper{adv_alarm});
 }
 
-std::unique_ptr<mir::time::Alarm> AdvanceableTimer::create_alarm(
-    std::shared_ptr<mir::LockableCallback> const&)
-{
-    throw std::runtime_error("Not implemented");
-}
-
 void AdvanceableTimer::advance_by(std::chrono::milliseconds advance)
 {
     {