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

[Alberto Aguirre]

Fix an additional deadlock that could occur after the original deadlock
is fixed (LP: #1339700)

Only ~AlarmImpl should wait for callback completion.

Warning: This introduces a semantic change whereby Alarm::cancel() is now
non-blocking and doesn't guarantee that callbacks are complete. In future
(when ABI changes permit), stop() will be exposed as the public method for
blocking until callbacks are done.

Until stop is exposed, we ensure that the alarm (and objects with embedded alarms) are the first ones to be destroyed so dead objects are not accessed by installed callbacks.

---
Alberto Aguirre (albaguirre) wrote on 2014-07-11: #
Thread A is calling BufferQueue::release, owns BufferQueue::guard, and it's about to call TimeoutFrameDroppingPolicy::swap_unblocked() gets pre-empted

Thread B is executing the alarm handler in TimeoutFrameDroppingPolicy, which invokes the callback installed by BufferQueue to drop a frame, attempts to own BufferQueue::guard so it blocks.

Thread A resumes, calls TimeoutFrameDroppingPolicy::swap_unblocked() and calls alarm->cancel(), it deadlocks, since data->callbacks_running will never be decremented (waiting for the alarm handler invocation to return).

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1768
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2140/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/925
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/931
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/924
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/661
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/661/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/659
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/659/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/3005
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/3005/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/2064
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/9736

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/2140/rebuild

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

I like the idea of allowing cancel to become asynchronous in future. But I think we need to be careful to avoid clouding the issue of fixing the important bugs first...

(1) Bug 1339700 is fixed already, according to all the details in bug 1339700 that problem is already solved. So this proposal doesn't need to call itself fix-1339700-anything but that's not a blocker.

(2) Changing cancel() to non-blocking and introducing stop() as the blocking method is a semantic API change. Sounds like a good idea to me, but it's not required to declare bug 1339700 fixed, and also changes like that can't be back-ported to a stable branch like 0.4.

(3) Is it necessary to propose changes to Alarm and the buffering code simultaneously?

(4) Please don't use the lambda syntax with wait. Am I the only one who can't read it?
31 + data->callback_done.wait(lock,[this]{return data->callbacks_running == 0;});
vs
40 - while (data->callbacks_running > 0)
41 - data->callback_done.wait(lock);
The latter is more familiar to users of condition variables from before C++ gained them. It's also more efficient (or doesn't require optimization anyway) and easier to read as logic.

(5) This needs a more descriptive name:
47 +bool AlarmImpl::cancel_l()

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Oh, I see you've described a new potential deadlock you're trying to fix. That should probably have been mentioned in this proposal...
https://code.launchpad.net/~vanvugt/mir/fix-1339700-alarm/+merge/226252/comments/546666

This is one of those situations where the deadlock shown in the bug report is solved, but we may have to fix other code to stop additional (previously masked) deadlocks from happening.

[Alberto Aguirre (albaguirre)]

1) no it's not fixed
2) it's REQUIRED to fix 1339700
3) why not?
4) I prefer the lambda
5) ummm ok.

[Daniel van Vugt (vanvugt)]

Yes, sorry. You can go to bed. I'll work on getting this approved and landed for your Monday.

[Daniel van Vugt (vanvugt)]

Seems reasonable. Providing we don't have any lingering code in Mir that still depends on Alarm::cancel() blocking, it should be OK.

review: Approve

[Chris Halse Rogers (raof)]

Looks OK to me.

I think you *might* be the only one who finds lambda syntax with cv.wait() unreadable :)

review: Approve

[Daniel van Vugt (vanvugt)]

"The latter is more familiar to users of condition variables from before C++ gained them. It's also more efficient (or doesn't require optimization anyway) and easier to read as logic."

You know the lambda version just hides the while loop in the function, right? So it's harder to fully understand, and slightly less efficient in passing extra parameters and calling back into a lambda.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1772
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2143/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/929
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/935
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/927
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/664
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/664/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/662
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/662/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/3030
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/3030/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/2066
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/9762

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/2143/rebuild

review: Approve (continuous-integration)

=== modified file 'src/server/asio_main_loop.cpp'
--- src/server/asio_main_loop.cpp	2014-07-11 04:03:02 +0000
+++ src/server/asio_main_loop.cpp	2014-07-14 03:52:53 +0000
@@ -344,11 +344,13 @@
     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}
         {
         }
 
@@ -422,16 +424,27 @@
 
 AlarmImpl::~AlarmImpl() noexcept
 {
-    AlarmImpl::cancel();
+    AlarmImpl::stop();
 }
 
-bool AlarmImpl::cancel()
+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-07-09 09:52:33 +0000
+++ src/server/compositor/buffer_queue.h	2014-07-14 03:52:53 +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 03:52:53 +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}
+        })}
 {
 }