Merge lp:unity-system-compositor/0.1 into lp:unity-system-compositor/ubuntu

[Alexandros Frantzis]

Release 0.1.3

Release 0.1.3

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:256
http://jenkins.qa.ubuntu.com/job/unity-system-compositor-ubuntu-ci/27/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-system-compositor-ubuntu-wily-amd64-ci/25
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-system-compositor-ubuntu-wily-armhf-ci/25

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

review: Approve (continuous-integration)

=== modified file 'debian/changelog'
--- debian/changelog	2015-09-22 16:44:54 +0000
+++ debian/changelog	2015-09-25 14:03:36 +0000
@@ -1,6 +1,19 @@
-unity-system-compositor (0.1.2+15.10.20150922-0ubuntu1) wily; urgency=medium
-
-  [ Cemil Azizoglu ]
+<<<<<<< TREE
+unity-system-compositor (0.1.2+15.10.20150922-0ubuntu1) wily; urgency=medium
+
+  [ Cemil Azizoglu ]
+=======
+unity-system-compositor (0.1.3+15.10.20150925-0ubuntu1) UNRELEASED; urgency=medium
+
+  * Fix "unresponsive shell including power button doesn't blank/unblank
+    display" (LP: #1491566)
+
+ -- Alexandros Frantzis <[e-mail]>  Fri, 25 Sep 2015 16:53:27 +0300
+
+unity-system-compositor (0.1.2+15.10.20150922-0ubuntu1) wily; urgency=medium
+
+  [ Cemil Azizoglu ]
+>>>>>>> MERGE-SOURCE
   * Rebuild for Mir 0.16
 
   [ CI Train Bot ]

=== modified file 'src/mir_screen.cpp'
--- src/mir_screen.cpp	2015-08-24 18:14:36 +0000
+++ src/mir_screen.cpp	2015-09-25 14:03:36 +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-25 14:03:36 +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-25 14:03:36 +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-25 14:03:36 +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-25 14:03:36 +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)
 {
     {