Merge lp:~mir-team/mir/fix-1339700-take2-in-0.4 into lp:mir/0.4

[Alberto Aguirre]

Ensure AlarmImpl is not locked during callbacks to arbitrary user code so
that it can't form a deadlock with the caller's own mutexes. (LP: #1339700).

Ensure AlarmImpl is not locked during callbacks to arbitrary user code so
that it can't form a deadlock with the caller's own mutexes. (LP: #1339700)

This is a combination of devel r1766 and https://code.launchpad.net/~mir-team/mir/fix-1339700-take2/+merge/226534

[Daniel van Vugt (vanvugt)]

Resubmit.

This proposal merges a couple of different branches, one of which is not approved and needs fixing upstream still:
https://code.launchpad.net/~mir-team/mir/fix-1339700-take2/+merge/226534

review: Needs Resubmitting

[Daniel van Vugt (vanvugt)]

Also, the beginnings of the semantic changes to Alarm should never appear in a stable maintenance branch.

[Daniel van Vugt (vanvugt)]

Superseded by: https://code.launchpad.net/~vanvugt/mir/fix-1339700-both-0.4/+merge/226629

review: Disapprove

Unmerged revisions

1742. By Alberto Aguirre on 2014-07-11

Update changelog

1741. By Alberto Aguirre on 2014-07-11

Only ~AlarmImpl should wait for callback completion (LP: #1339700)

Keep the semantics of AlarmImpl::cancel similar to boost::::deadline_timer. However,
make the destructor of ~AlarmImpl wait for current executing callbacks to avoid lifetime
issues. In the future, mir::time::Alarm should expose the stop method.

1740. By Alberto Aguirre on 2014-07-11

Ensure AlarmImpl is not locked during callbacks to arbitrary user code so
that it can't form a deadlock with the caller's own mutexes.

1739. By Daniel van Vugt on 2014-07-09

Correct the changelog text for 0.4.0+14.10.20140701.1-0ubuntu1.

It mistakenly listed enhancements under ABI/API changes and didn't mention
most of the bugs that got fixed.

1738. By Daniel van Vugt on 2014-07-04

Merge released changelog from lp:mir ...
mir (0.4.0+14.10.20140701.1-0ubuntu1) utopic; urgency=medium

1737. By Daniel van Vugt on 2014-07-04

Revert my corrections from r1736. Annoyingly Tarmac lost it and landed r1735
to lp:mir instead :(

Also moved the v0.4.0 tag to accurately reflect which revision got released.

1736. By Daniel van Vugt on 2014-07-02

Corrections to the new debian/changelog entry for 0.4.0

1735. By Cemil Azizoglu on 2014-07-01

Merge r1737 and r1738 from devel.

1734. By Cemil Azizoglu on 2014-07-01

Integrate r1734 from devel.

1733. By Cemil Azizoglu on 2014-06-30

Update changelog.

=== modified file 'debian/changelog'
--- debian/changelog	2014-07-04 07:37:55 +0000
+++ debian/changelog	2014-07-11 21:08:53 +0000
@@ -1,3 +1,13 @@
+mir (0.4.1) UNRELEASED; urgency=medium
+
+  * New upstream release 0.4.1 (https://launchpad.net/mir/+milestone/0.4.1)
+    - mirclient ABI unchanged, still at 8. Clients do not need rebuilding.
+    - mirserver ABI unchanged, still at 22. Shells do not need rebuilding.
+    - Bugs fixed:
+     . fix for deadlock when invoking alarm handlers (LP: #1339700) 
+
+ -- Alberto Aguirre <[e-mail]>  Fri, 11 Jul 2014 15:38:20 -0500
+
 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-11 21:08:53 +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_l();
     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,26 @@
 
 AlarmImpl::~AlarmImpl() noexcept
 {
-    AlarmImpl::cancel();
+    AlarmImpl::stop();
+}
+
+void AlarmImpl::stop()
+{
+    std::unique_lock<decltype(data->m)> lock(data->m);
+
+    data->callback_done.wait(lock,[this]{return data->callbacks_running == 0;});
+
+    cancel_l();
 }
 
 bool AlarmImpl::cancel()
 {
     std::lock_guard<decltype(data->m)> lock(data->m);
+    return cancel_l();
+}
+
+bool AlarmImpl::cancel_l()
+{
     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-11 21:08:53 +0000
@@ -86,11 +86,13 @@
 
     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 it 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-11 21:08:53 +0000
@@ -41,22 +41,23 @@
 
 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-11 21:08:53 +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};