Merge lp:~vanvugt/mir/fix-1339700-both-0.4 into lp:mir/0.4

[Daniel van Vugt]

Fix two potential deadlocks that could lead to the display freezing.
(LP: #1339700)

This is the combination of both fixes that have been approved upstream
for LP: #1339700.

[Cemil Azizoglu (cemil-azizoglu)]

LGTM

review: Approve

[Alexandros Frantzis (afrantzis)]

Looks good.

review: Approve

=== modified file 'debian/changelog'
--- debian/changelog	2014-07-04 07:37:55 +0000
+++ debian/changelog	2014-07-14 06:16:57 +0000
@@ -1,3 +1,10 @@
+mir (0.4.1-0ubuntu1) UNRELEASED; urgency=medium
+
+  * Bug fix release 0.4.1 (https://launchpad.net/mir/+milestone/0.4.1) fixes:
+    - [regression] Device locks randomly on welcome screen (LP: #1339700)
+
+ -- Daniel van Vugt <[e-mail]>  Mon, 14 Jul 2014 14:10:21 +0800
+
 mir (0.4.0+14.10.20140701.1-0ubuntu1) utopic; urgency=medium
 
   * New upstream release 0.4.0 (https://launchpad.net/mir/+milestone/0.4.0)

=== modified file 'src/server/asio_main_loop.cpp'
--- src/server/asio_main_loop.cpp	2014-06-27 07:07:35 +0000
+++ src/server/asio_main_loop.cpp	2014-07-14 06:16:57 +0000
@@ -344,15 +344,19 @@
     bool reschedule_in(std::chrono::milliseconds delay) override;
     bool reschedule_for(mir::time::Timestamp time_point) override;
 private:
+    void stop();
+    bool cancel_unlocked();
     void update_timer();
     struct InternalState
     {
         explicit InternalState(std::function<void(void)> callback)
-            : callback{callback}
+            : callback{callback}, state{pending}
         {
         }
 
-        std::recursive_mutex m;
+        std::mutex m;
+        int callbacks_running = 0;
+        std::condition_variable callback_done;
         std::function<void(void)> const callback;
         State state;
     };
@@ -381,7 +385,12 @@
                 if (data->state == mir::time::Alarm::pending)
                 {
                     data->state = mir::time::Alarm::triggered;
+                    ++data->callbacks_running;
+                    lock.unlock();
                     data->callback();
+                    lock.lock();
+                    --data->callbacks_running;
+                    data->callback_done.notify_all();
                 }
             }
         }
@@ -415,12 +424,27 @@
 
 AlarmImpl::~AlarmImpl() noexcept
 {
-    AlarmImpl::cancel();
+    AlarmImpl::stop();
+}
+
+void AlarmImpl::stop()
+{
+    std::unique_lock<decltype(data->m)> lock(data->m);
+
+    while (data->callbacks_running > 0)
+        data->callback_done.wait(lock);
+
+    cancel_unlocked();
 }
 
 bool AlarmImpl::cancel()
 {
     std::lock_guard<decltype(data->m)> lock(data->m);
+    return cancel_unlocked();
+}
+
+bool AlarmImpl::cancel_unlocked()
+{
     if (data->state == triggered)
         return false;
 

=== modified file 'src/server/compositor/buffer_queue.h'
--- src/server/compositor/buffer_queue.h	2014-06-12 15:35:08 +0000
+++ src/server/compositor/buffer_queue.h	2014-07-14 06:16:57 +0000
@@ -86,11 +86,14 @@
 
     int nbuffers;
     bool frame_dropping_enabled;
-    std::unique_ptr<FrameDroppingPolicy> framedrop_policy;
     graphics::BufferProperties the_properties;
 
     std::condition_variable snapshot_released;
     std::shared_ptr<graphics::GraphicBufferAllocator> gralloc;
+
+    // Ensure framedrop_policy gets destroyed first so the callback installed
+    // does not access dead objects.
+    std::unique_ptr<FrameDroppingPolicy> framedrop_policy;
 };
 
 }

=== modified file 'src/server/compositor/timeout_frame_dropping_policy_factory.cpp'
--- src/server/compositor/timeout_frame_dropping_policy_factory.cpp	2014-06-25 16:30:46 +0000
+++ src/server/compositor/timeout_frame_dropping_policy_factory.cpp	2014-07-14 06:16:57 +0000
@@ -41,22 +41,25 @@
 
 private:
     std::chrono::milliseconds const timeout;
+    std::atomic<unsigned int> pending_swaps;
+
+    // Ensure alarm gets destroyed first so its handler does not access dead
+    // objects.
     std::unique_ptr<mir::time::Alarm> const alarm;
-    std::atomic<unsigned int> pending_swaps;
 };
 
 TimeoutFrameDroppingPolicy::TimeoutFrameDroppingPolicy(std::shared_ptr<mir::time::Timer> const& timer,
                                                        std::chrono::milliseconds timeout,
                                                        std::function<void(void)> drop_frame)
     : timeout{timeout},
+      pending_swaps{0},
       alarm{timer->create_alarm([this, drop_frame]
         {
             assert(pending_swaps.load() > 0);
             drop_frame();
             if (--pending_swaps > 0)
                 alarm->reschedule_in(this->timeout);
-        })},
-      pending_swaps{0}
+        })}
 {
 }
 

=== modified file 'tests/unit-tests/test_asio_main_loop.cpp'
--- tests/unit-tests/test_asio_main_loop.cpp	2014-06-27 09:07:33 +0000
+++ tests/unit-tests/test_asio_main_loop.cpp	2014-07-14 06:16:57 +0000
@@ -631,6 +631,52 @@
     EXPECT_EQ(1, call_count);
 }
 
+TEST_F(AsioMainLoopAlarmTest, alarm_callback_cannot_deadlock)
+{   // Regression test for deadlock bug LP: #1339700
+    std::mutex m;
+    std::atomic_bool failed(false);
+    int i = 0;
+    int const loops = 5;
+
+    auto alarm = ml.notify_in(std::chrono::milliseconds{0}, [&]()
+    {
+        // From this angle, ensure we can lock m (alarm should be unlocked)
+        int tries = 0;
+        while (!m.try_lock() && tries < 100) // 100 x 100 = try for 10 seconds
+        {
+            ++tries;
+            std::this_thread::sleep_for(std::chrono::milliseconds{100});
+        }
+        failed = (tries >= 100);
+        ASSERT_FALSE(failed);
+        ++i;
+        m.unlock();
+    });
+
+    std::thread t([&]()
+        {
+            m.lock();
+            while (i < loops && !failed)
+            {
+                // From this angle, ensure we can lock alarm while holding m
+                (void)alarm->state();
+                m.unlock();
+                std::this_thread::yield();
+                m.lock();
+            }
+            m.unlock();
+        });
+
+    UnblockMainLoop unblocker(ml);
+    for (int j = 0; j < loops; ++j)
+    {
+        clock->advance_by(std::chrono::milliseconds{101}, ml);
+        alarm->reschedule_in(std::chrono::milliseconds{100});
+    }
+
+    t.join();
+}
+
 TEST_F(AsioMainLoopAlarmTest, alarm_fires_at_correct_time_point)
 {
     mir::time::Timestamp real_soon = clock->sample() + std::chrono::milliseconds{120};