Merge lp:~albaguirre/mir/fix-1421255 into lp:mir

[Alberto Aguirre]

Fix deadlock caused by lock ordering issues.

The framedropping policy acquires an internal lock while invoking the user callback. BufferQueue calls framedrop policy methods while holding its own lock; the callback implementation given to the policy also acquires the same lock which results in a classic lock ordering deadlock issue.

To avoid such lock order issues, the framedrop policy and alarm factory interfaces expose an additional API to allow users to pass in lock/unlock functions, which can then be used by the callback dispatching context to preserve lock ordering.

Fix deadlock caused by lock ordering issues.

The framedropping policy acquires an internal lock while invoking the user callback. BufferQueue calls framedrop policy methods while holding its own lock; the callback implementation given to the policy also acquires the same lock which results in a classic lock ordering deadlock issue.

To avoid such lock order issues, the framedrop policy and alarm factory interfaces expose an additional API to allow users to pass in lock/unlock functions, which can then be used by the callback dispatching context to preserve lock ordering.

The alternatives include:
- Avoid holding a lock while dispatching callback
--- Lifetime of objects gets tricky when trying to destroy the underlying TimerGSource and releated resources.
- Make async versions of the current alarm APIs (reschedule APIs have an implicit cancel operation)
--- Needs some sort of garbage collector of GSourceHandles (to keep existing implementation)
--- or rework of TimerGSource and md::add_timer_gsource to be able to handle an async cancel call with all the object lifetime issues it opens up.

This approach was chosen because:
- Solves the actual root cause - lock ordering
- It's a bit easier to reason about than the alternatives; Object lifetimes concerns are already addressed by existing code

[Kevin DuBois (kdub)]

211 + [this] { guard_lock = std::move(std::unique_lock<decltype(guard)>{guard}); },
212 + []{});
It seems strange that if I were to call:
lock();
unlock();
lock()
The second lock call would block, because we haven't called the callback, which is what will end up releasing the lock.

594 + CallerAutoLock caller_lock{ctx.lock, ctx.unlock};
Could mir/raii.h help reduce diff? (and don't need CallerAutoLock)

99 + virtual std::unique_ptr<FrameDroppingPolicy> create_policy(std::function<void()> const& drop_frame) const = 0;
119 + virtual std::unique_ptr<FrameDroppingPolicy> create_policy(
120 + std::function<void()> const& drop_frame,
121 + std::function<void()> const& lock,
122 + std::function<void()> const& unlock) const = 0;
I guess I'd rather just keep one function (the latter), and 3 parameters in create_policy seem like they're becoming an interface that meets the requirements of BasicLockable.

review: Needs Information

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2327
http://jenkins.qa.ubuntu.com/job/mir-ci/3011/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1347
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1346
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1303/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1008
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1008/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1303
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1303/artifact/work/output/*zip*/output.zip
    ABORTED: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4335/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/18171

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/3011/rebuild

review: Needs Fixing (continuous-integration)

[Francis Ginther (fginther)]

The mir-mediumtests-runner-mako job failed due to a failed device. I've restarted the mir-ci job.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2327
http://jenkins.qa.ubuntu.com/job/mir-ci/3014/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1351/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1350/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1307/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1011/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1307/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/3014/rebuild

review: Needs Fixing (continuous-integration)

[Daniel van Vugt (vanvugt)]

A simpler and guaranteed more reliable solution would be to move away from a pure callback approach (which we use in many places)...

Instead, if we replaced such logic:
   call callback(args)
with:
   signal event(args)
then there is zero risk of deadlocks.

Receivers could then wait for an event, multiple events, or even join a thread pool that turns events into callbacks if they want to work that way. But the important point is even in the callback case then, the thread making the callback has no risk of causing deadlocks. Because it's dedicated just to making unlocked callbacks and nothing else. But of course, it's more efficient to not need additional threads too.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2328
http://jenkins.qa.ubuntu.com/job/mir-ci/3015/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1352
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1351
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1308
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1012
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1012/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1308
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1308/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4339
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/18184

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/3015/rebuild

review: Approve (continuous-integration)

[Alberto Aguirre (albaguirre)]

> A simpler and guaranteed more reliable solution would be to move away from a
> pure callback approach (which we use in many places)...
>
> Instead, if we replaced such logic:
> call callback(args)
> with:
> signal event(args)
> then there is zero risk of deadlocks.
>

How would that be simpler? I guess simpler in which respect? Development, maintenance, usage?

A high level event-hub concept cannot guarantee zero deadlocks however, that's an implementation detail.

Maybe such an event hub could just post and dispatch events through the main loop to save a thread.

In any case, that's probably a much bigger re-factoring than what is proposed here.

[Alberto Aguirre (albaguirre)]

> 211 + [this] { guard_lock =
> std::move(std::unique_lock<decltype(guard)>{guard}); },
> 212 + []{});
> It seems strange that if I were to call:
> lock();
> unlock();
> lock()
> The second lock call would block, because we haven't called the callback,
> which is what will end up releasing the lock.

OK, I suppose I can change BufferQueue private methods to take a unique_lock reference instead
of value, so I don't have to move the instance and consequently I can use it in the "unlock" lambda.

> 594 + CallerAutoLock caller_lock{ctx.lock, ctx.unlock};
> Could mir/raii.h help reduce diff? (and don't need CallerAutoLock)

I tried, but couldn't figure out why paired_call did not like std::function. I'll try harder I suppose :)

> 99 + virtual std::unique_ptr<FrameDroppingPolicy>
> create_policy(std::function<void()> const& drop_frame) const = 0;
> 119 + virtual std::unique_ptr<FrameDroppingPolicy> create_policy(
> 120 + std::function<void()> const& drop_frame,
> 121 + std::function<void()> const& lock,
> 122 + std::function<void()> const& unlock) const = 0;
>
> I guess I'd rather just keep one function (the latter), and 3 parameters in
> create_policy seem like they're becoming an interface that meets the
> requirements of BasicLockable.

OK. Maybe a LockableCallable?

[Alan Griffiths (alan-griffiths)]

Ran out of time trying to understand. Will look again tomorrow if it doesn't land meanwhile.

review: Abstain

[Alan Griffiths (alan-griffiths)]

This seems elaborate and easy to make mistakes using but I don't have a better suggestion (yet).

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2331
http://jenkins.qa.ubuntu.com/job/mir-ci/3030/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1371
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1370
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1327
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1027
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1027/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1327
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1327/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4356
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/18242

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/3030/rebuild

review: Approve (continuous-integration)

[Alberto Aguirre (albaguirre)]

> This seems elaborate and easy to make mistakes using but I don't have a better
> suggestion (yet).

Yeah but no different that what we currently have (i.e. easy to make mistakes too).

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2332
http://jenkins.qa.ubuntu.com/job/mir-ci/3032/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1373
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1372
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1329
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1029
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1029/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1329
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1329/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4358
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/18246

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/3032/rebuild

review: Approve (continuous-integration)

[Kevin DuBois (kdub)]

seems the issues were addressed, so okay by me. This buffer queue code has gotten hard to understand again, but thats a pre-existing issue.

review: Approve

=== modified file 'include/server/mir/time/timer.h'
--- include/server/mir/time/timer.h	2015-01-21 07:34:50 +0000
+++ include/server/mir/time/timer.h	2015-02-20 17:57:50 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2014 Canonical Ltd.
+ * Copyright © 2014-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,
@@ -14,6 +14,7 @@
  * along with this program. If not, see <http://www.gnu.org/licenses/>.
  *
  * Authored by: Christopher James Halse Rogers <[e-mail]>
+ *              Alberto Aguirre <[e-mail]>
  */
 
 
@@ -47,7 +48,7 @@
      * \return A handle to an Alarm that will fire after delay ms.
      */
     virtual std::unique_ptr<Alarm> notify_in(std::chrono::milliseconds delay,
-                                             std::function<void()> callback) = 0;
+                                             std::function<void()> const& callback) = 0;
     /**
      * \brief Create an Alarm that calls the callback at the specified time
      *
@@ -57,7 +58,7 @@
      * \return A handle to an Alarm that will fire after delay ms.
      */
     virtual std::unique_ptr<Alarm> notify_at(Timestamp time_point,
-                                             std::function<void()> callback) = 0;
+                                             std::function<void()> const& callback) = 0;
     /**
      * \brief Create an Alarm that will not fire until scheduled
      *
@@ -65,7 +66,30 @@
      *
      * \return A handle to an Alarm that can later be scheduled
      */
-    virtual std::unique_ptr<Alarm> create_alarm(std::function<void()> callback) = 0;
+    virtual std::unique_ptr<Alarm> create_alarm(std::function<void()> const& callback) = 0;
+
+    /**
+     * \brief Create an Alarm that will not fire until scheduled
+     *
+     * The lock/unlock functions allow the user to preserve lock ordering
+     * in situations where Alarm methods need to be called under external lock
+     * and the callback implementation needs to run code protected by the same
+     * lock. An alarm implementation may have internal locks of its own, which
+     * maybe acquired during callback dispatching; to preserve lock ordering
+     * the given lock function will be invoked during callback dispatch before
+     * any internal locks are acquired.
+     *
+     * \param callback Function to call when the Alarm signals
+     * \param lock     Function called within callback dispatching context
+     *                 before the alarm implementation acquires any internal
+     *                 lock
+     * \param unlock   Function called within callback dispatching context after
+     *                 the alarm implementation releases any internal lock
+     *
+     * \return A handle to an Alarm that can later be scheduled
+     */
+    virtual std::unique_ptr<Alarm> create_alarm(std::function<void()> const& callback,
+        std::function<void()> const& lock, std::function<void()> const& unlock) = 0;
 
     Timer(Timer const&) = delete;
     Timer& operator=(Timer const&) = delete;

=== modified file 'src/include/server/mir/compositor/frame_dropping_policy_factory.h'
--- src/include/server/mir/compositor/frame_dropping_policy_factory.h	2015-01-21 07:34:50 +0000
+++ src/include/server/mir/compositor/frame_dropping_policy_factory.h	2015-02-20 17:57:50 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2014 Canonical Ltd.
+ * Copyright © 2014-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,
@@ -14,6 +14,7 @@
  * along with this program. If not, see <http://www.gnu.org/licenses/>.
  *
  * Authored by: Christopher James Halse Rogers <[e-mail]>
+ *              Alberto Aguirre <[e-mail]>
  */
 
 #ifndef MIR_COMPOSITOR_FRAME_DROPPING_POLICY_FACTORY_H_
@@ -44,11 +45,27 @@
     FrameDroppingPolicyFactory& operator=(FrameDroppingPolicyFactory const&) = delete;
 
     /**
-     * \brief Create a FrameDroppingPolicy that will call \a drop_frame when it decides to drop a frame
-     * \param drop_frame    Function to call when a frame needs to be dropped
-     * \return              The policy object.
+     * \brief Create a FrameDroppingPolicy that will call \a drop_frame when it
+     *       decides to drop a frame
+     *
+     * The lock/unlock functions allow the user to preserve lock ordering
+     * in situations where FrameDroppingPolicy methods need to be called under
+     * external lock and the callback implementation needs to run code protected
+     * by the same lock. A FrameDroppingPolicy implementation may have internal
+     * locks of its own, which maybe acquired during callback dispatching;
+     * to preserve lock ordering the given lock function will be invoked during
+     * callback dispatch before any internal locks are acquired.
+     *
+     * \param drop_frame Function to call when a frame needs to be dropped
+     * \param lock       Function called within the callback dispatcher context
+     *                   before any internal locks are acquired.
+     * \param unlock     Function called within the callback dispatcher context
+     *                   after any internal locks are released.
      */
-    virtual std::unique_ptr<FrameDroppingPolicy> create_policy(std::function<void(void)> drop_frame) const = 0;
+    virtual std::unique_ptr<FrameDroppingPolicy> create_policy(
+        std::function<void()> const& drop_frame,
+        std::function<void()> const& lock,
+        std::function<void()> const& unlock) const = 0;
 };
 
 }

=== modified file 'src/include/server/mir/glib_main_loop.h'
--- src/include/server/mir/glib_main_loop.h	2015-01-21 07:34:50 +0000
+++ src/include/server/mir/glib_main_loop.h	2015-02-20 17:57:50 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2014 Canonical Ltd.
+ * Copyright © 2014-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,
@@ -72,14 +72,19 @@
 
     std::unique_ptr<mir::time::Alarm> notify_in(
         std::chrono::milliseconds delay,
-        std::function<void()> callback) override;
+        std::function<void()> const& callback) override;
 
     std::unique_ptr<mir::time::Alarm> notify_at(
         mir::time::Timestamp t,
-        std::function<void()> callback) override;
-
-    std::unique_ptr<mir::time::Alarm> create_alarm(
-        std::function<void()> callback) override;
+        std::function<void()> const& callback) override;
+
+    std::unique_ptr<mir::time::Alarm> create_alarm(
+        std::function<void()> const& callback) override;
+
+    std::unique_ptr<mir::time::Alarm> create_alarm(
+        std::function<void()> const& callback,
+        std::function<void()> const& lock,
+        std::function<void()> const& unlock) override;
 
     void reprocess_all_sources();
 

=== modified file 'src/include/server/mir/glib_main_loop_sources.h'
--- src/include/server/mir/glib_main_loop_sources.h	2015-02-13 06:12:34 +0000
+++ src/include/server/mir/glib_main_loop_sources.h	2015-02-20 17:57:50 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2014 Canonical Ltd.
+ * Copyright © 2014-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,
@@ -74,6 +74,8 @@
     GMainContext* main_context,
     std::shared_ptr<time::Clock> const& clock,
     std::function<void()> const& handler,
+    std::function<void()> const& lock,
+    std::function<void()> const& unlock,
     time::Timestamp target_time);
 
 class FdSources

=== modified file 'src/server/compositor/buffer_queue.cpp'
--- src/server/compositor/buffer_queue.cpp	2015-02-17 06:38:35 +0000
+++ src/server/compositor/buffer_queue.cpp	2015-02-20 17:57:50 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2014 Canonical Ltd.
+ * Copyright © 2014-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,
@@ -132,11 +132,18 @@
     if (nbuffers == 1)
         free_buffers.push_back(current_compositor_buffer);
 
+    // A lock_guard will be created by the policy dispatcher invoking the given "lock" lambda
+    // before it acquires any internal locks of its own.
     framedrop_policy = policy_provider.create_policy([this]
     {
-       std::unique_lock<decltype(guard)> lock{guard};
-       drop_frame(std::move(lock));
-    });
+        // We ignore any ongoing snapshotting as it could lead to deadlock.
+        // In order to wait the guard_lock needs to be released; a BufferQueue::release
+        // call can sneak in at that time from a different thread which
+        // can invoke framedrop_policy methods
+        drop_frame(guard_lock, ignore_snapshot);
+    },
+    [this] { guard_lock = std::move(std::unique_lock<decltype(guard)>{guard}); },
+    [this] { if (guard_lock.owns_lock()) guard_lock.unlock(); });
 }
 
 void mc::BufferQueue::client_acquire(mc::BufferQueue::Callback complete)
@@ -149,7 +156,7 @@
     {
         auto const buffer = free_buffers.back();
         free_buffers.pop_back();
-        give_buffer_to_client(buffer, std::move(lock));
+        give_buffer_to_client(buffer, lock);
         return;
     }
 
@@ -162,14 +169,14 @@
     {
         auto const& buffer = gralloc->alloc_buffer(the_properties);
         buffers.push_back(buffer);
-        give_buffer_to_client(buffer.get(), std::move(lock));
+        give_buffer_to_client(buffer.get(), lock);
         return;
     }
 
     /* Last resort, drop oldest buffer from the ready queue */
     if (frame_dropping_enabled)
     {
-        drop_frame(std::move(lock));
+        drop_frame(lock, wait_for_snapshot);
         return;
     }
 
@@ -323,7 +330,7 @@
         {
             free_buffers.push_back(pop(ready_to_composite_queue));
         }
-        give_buffer_to_client(buffer, std::move(lock));
+        give_buffer_to_client(buffer, lock, ignore_snapshot);
     }
 }
 
@@ -363,7 +370,15 @@
 
 void mc::BufferQueue::give_buffer_to_client(
     mg::Buffer* buffer,
-    std::unique_lock<std::mutex> lock)
+    std::unique_lock<std::mutex>& lock)
+{
+    give_buffer_to_client(buffer, lock, wait_for_snapshot);
+}
+
+void mc::BufferQueue::give_buffer_to_client(
+    mg::Buffer* buffer,
+    std::unique_lock<std::mutex>& lock,
+    SnapshotWait wait_type)
 {
     /* Clears callback */
     auto give_to_client_cb = std::move(pending_client_notifications.front());
@@ -383,7 +398,7 @@
     }
 
     /* Don't give to the client just yet if there's a pending snapshot */
-    if (!resize_buffer && contains(buffer, pending_snapshots))
+    if (wait_type == wait_for_snapshot && !resize_buffer && contains(buffer, pending_snapshots))
     {
         snapshot_released.wait(lock,
             [&]{ return !contains(buffer, pending_snapshots); });
@@ -420,7 +435,7 @@
     if (!pending_client_notifications.empty())
     {
         framedrop_policy->swap_unblocked();
-        give_buffer_to_client(buffer, std::move(lock));
+        give_buffer_to_client(buffer, lock);
     }
     else if (!frame_dropping_enabled && buffers.size() > size_t(nbuffers))
     {
@@ -446,7 +461,7 @@
         free_buffers.push_back(buffer);
 }
 
-void mc::BufferQueue::drop_frame(std::unique_lock<std::mutex> lock)
+void mc::BufferQueue::drop_frame(std::unique_lock<std::mutex>& lock, SnapshotWait wait_type)
 {
     // Make sure there is a client waiting for the frame before we drop it.
     // If not, then there's nothing to do.
@@ -498,7 +513,7 @@
         buffer_to_give = buffer.get();
     }
         
-    give_buffer_to_client(buffer_to_give, std::move(lock));
+    give_buffer_to_client(buffer_to_give, lock, wait_type);
 }
 
 void mc::BufferQueue::drop_old_buffers()

=== modified file 'src/server/compositor/buffer_queue.h'
--- src/server/compositor/buffer_queue.h	2015-01-21 08:53:28 +0000
+++ src/server/compositor/buffer_queue.h	2015-02-20 17:57:50 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2014 Canonical Ltd.
+ * Copyright © 2014-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,
@@ -66,13 +66,21 @@
     void drop_client_requests() override;
 
 private:
-    void give_buffer_to_client(graphics::Buffer* buffer,
-        std::unique_lock<std::mutex> lock);
+    enum SnapshotWait
+    {
+        wait_for_snapshot,
+        ignore_snapshot
+    };
+    void give_buffer_to_client(graphics::Buffer* buffer,
+        std::unique_lock<std::mutex>& lock);
+    void give_buffer_to_client(graphics::Buffer* buffer,
+        std::unique_lock<std::mutex>& lock, SnapshotWait wait_type);
     void release(graphics::Buffer* buffer,
         std::unique_lock<std::mutex> lock);
-    void drop_frame(std::unique_lock<std::mutex> lock);
+    void drop_frame(std::unique_lock<std::mutex>& lock, SnapshotWait wait_type);
 
     mutable std::mutex guard;
+    std::unique_lock<decltype(guard)> guard_lock;
 
     std::vector<std::shared_ptr<graphics::Buffer>> buffers;
     std::deque<graphics::Buffer*> ready_to_composite_queue;

=== modified file 'src/server/compositor/timeout_frame_dropping_policy_factory.cpp'
--- src/server/compositor/timeout_frame_dropping_policy_factory.cpp	2015-02-17 11:58:56 +0000
+++ src/server/compositor/timeout_frame_dropping_policy_factory.cpp	2015-02-20 17:57:50 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2014 Canonical Ltd.
+ * Copyright © 2014-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,
@@ -14,6 +14,7 @@
  * along with this program. If not, see <http://www.gnu.org/licenses/>.
  *
  * Authored by: Christopher James Halse Rogers <[e-mail]>
+ *              Alberto Aguirre <[e-mail]>
  */
 
 #include "mir/compositor/frame_dropping_policy.h"
@@ -34,7 +35,9 @@
 public:
     TimeoutFrameDroppingPolicy(std::shared_ptr<mir::time::Timer> const& timer,
                                std::chrono::milliseconds timeout,
-                               std::function<void(void)> drop_frame);
+                               std::function<void()> const& drop_frame,
+                               std::function<void()> const& lock,
+                               std::function<void()> const& unlock);
 
     void swap_now_blocking() override;
     void swap_unblocked() override;
@@ -50,7 +53,9 @@
 
 TimeoutFrameDroppingPolicy::TimeoutFrameDroppingPolicy(std::shared_ptr<mir::time::Timer> const& timer,
                                                        std::chrono::milliseconds timeout,
-                                                       std::function<void(void)> drop_frame)
+                                                       std::function<void()> const& drop_frame,
+                                                       std::function<void()> const& lock,
+                                                       std::function<void()> const& unlock)
     : timeout{timeout},
       pending_swaps{0},
       alarm{timer->create_alarm([this, drop_frame]
@@ -59,7 +64,7 @@
             drop_frame();
             if (--pending_swaps > 0)
                 alarm->reschedule_in(this->timeout);
-        })}
+        }, lock, unlock)}
 {
 }
 
@@ -81,15 +86,18 @@
 }
 }
 
-mc::TimeoutFrameDroppingPolicyFactory::TimeoutFrameDroppingPolicyFactory(std::shared_ptr<mir::time::Timer> const& timer,
-                                                                         std::chrono::milliseconds timeout)
+mc::TimeoutFrameDroppingPolicyFactory::TimeoutFrameDroppingPolicyFactory(
+    std::shared_ptr<mir::time::Timer> const& timer,
+    std::chrono::milliseconds timeout)
     : timer{timer},
       timeout{timeout}
 {
 }
 
-
-std::unique_ptr<mc::FrameDroppingPolicy> mc::TimeoutFrameDroppingPolicyFactory::create_policy(std::function<void ()> drop_frame) const
+std::unique_ptr<mc::FrameDroppingPolicy>
+mc::TimeoutFrameDroppingPolicyFactory::create_policy(std::function<void()> const& drop_frame,
+    std::function<void()> const& lock,
+    std::function<void()> const& unlock) const
 {
-    return std::make_unique<TimeoutFrameDroppingPolicy>(timer, timeout, drop_frame);
+    return std::make_unique<TimeoutFrameDroppingPolicy>(timer, timeout, drop_frame, lock, unlock);
 }

=== modified file 'src/server/compositor/timeout_frame_dropping_policy_factory.h'
--- src/server/compositor/timeout_frame_dropping_policy_factory.h	2014-05-22 20:48:20 +0000
+++ src/server/compositor/timeout_frame_dropping_policy_factory.h	2015-02-20 17:57:50 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2014 Canonical Ltd.
+ * Copyright © 2014-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,
@@ -14,6 +14,7 @@
  * along with this program. If not, see <http://www.gnu.org/licenses/>.
  *
  * Authored by: Christopher James Halse Rogers <[e-mail]>
+ *              Alberto Aguirre <[e-mail]>
  */
 
 
@@ -44,7 +45,10 @@
     TimeoutFrameDroppingPolicyFactory(std::shared_ptr<mir::time::Timer> const& timer,
                                       std::chrono::milliseconds timeout);
 
-    std::unique_ptr<FrameDroppingPolicy> create_policy(std::function<void(void)> drop_frame) const override;
+    std::unique_ptr<FrameDroppingPolicy> create_policy(
+        std::function<void()> const& drop_frame,
+        std::function<void()> const& lock,
+        std::function<void()> const& unlock) const override;
 private:
     std::shared_ptr<mir::time::Timer> const timer;
     std::chrono::milliseconds timeout;

=== modified file 'src/server/glib_main_loop.cpp'
--- src/server/glib_main_loop.cpp	2015-02-17 11:58:56 +0000
+++ src/server/glib_main_loop.cpp	2015-02-20 17:57:50 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2014 Canonical Ltd.
+ * Copyright © 2014-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,
@@ -14,6 +14,7 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  *
  * Authored by: Alexandros Frantzis <[e-mail]>
+ *              Alberto Aguirre <[e-mail]>
  */
 
 #include "mir/glib_main_loop.h"
@@ -33,10 +34,14 @@
     AlarmImpl(
         GMainContext* main_context,
         std::shared_ptr<mir::time::Clock> const& clock,
-        std::function<void()> const& callback)
+        std::function<void()> const& callback,
+        std::function<void()> const& lock,
+        std::function<void()> const& unlock)
         : main_context{main_context},
           clock{clock},
           callback{callback},
+          ext_lock{lock},
+          ext_unlock{unlock},
           state_{State::cancelled}
     {
     }
@@ -70,6 +75,8 @@
             main_context,
             clock,
             [&] { state_ = State::triggered; callback(); },
+            ext_lock,
+            ext_unlock,
             time_point);
 
         return true;
@@ -80,6 +87,8 @@
     GMainContext* main_context;
     std::shared_ptr<mir::time::Clock> const clock;
     std::function<void()> const callback;
+    std::function<void()> const ext_lock;
+    std::function<void()> const ext_unlock;
     State state_;
     mir::detail::GSourceHandle gsource;
 };
@@ -223,7 +232,7 @@
 
 std::unique_ptr<mir::time::Alarm> mir::GLibMainLoop::notify_in(
     std::chrono::milliseconds delay,
-    std::function<void()> callback)
+    std::function<void()> const& callback)
 {
     auto alarm = create_alarm(callback);
 
@@ -234,7 +243,7 @@
 
 std::unique_ptr<mir::time::Alarm> mir::GLibMainLoop::notify_at(
     mir::time::Timestamp t,
-    std::function<void()> callback)
+    std::function<void()> const& callback)
 {
     auto alarm = create_alarm(callback);
 
@@ -244,7 +253,15 @@
 }
 
 std::unique_ptr<mir::time::Alarm> mir::GLibMainLoop::create_alarm(
-    std::function<void()> callback)
+    std::function<void()> const& callback)
+{
+    return create_alarm(callback, []{}, []{});
+}
+
+std::unique_ptr<mir::time::Alarm> mir::GLibMainLoop::create_alarm(
+    std::function<void()> const& callback,
+    std::function<void()> const& lock,
+    std::function<void()> const& unlock)
 {
     auto const callback_with_exception_handling =
         [this, callback]
@@ -254,7 +271,7 @@
         };
 
     return std::make_unique<AlarmImpl>(
-        main_context, clock, callback_with_exception_handling);
+        main_context, clock, callback_with_exception_handling, lock, unlock);
 }
 
 void mir::GLibMainLoop::reprocess_all_sources()

=== modified file 'src/server/glib_main_loop_sources.cpp'
--- src/server/glib_main_loop_sources.cpp	2015-02-13 06:12:34 +0000
+++ src/server/glib_main_loop_sources.cpp	2015-02-20 17:57:50 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2014 Canonical Ltd.
+ * Copyright © 2014-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,
@@ -14,11 +14,13 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  *
  * Authored by: Alexandros Frantzis <[e-mail]>
+ *              Alberto Aguirre <[e-mail]>
  */
 
 #include "mir/glib_main_loop_sources.h"
 #include "mir/recursive_read_write_mutex.h"
 #include "mir/thread_safe_list.h"
+#include "mir/raii.h"
 
 #include <algorithm>
 #include <atomic>
@@ -233,18 +235,25 @@
     GMainContext* main_context,
     std::shared_ptr<time::Clock> const& clock,
     std::function<void()> const& handler,
+    std::function<void()> const& lock,
+    std::function<void()> const& unlock,
     time::Timestamp target_time)
 {
     struct TimerContext
     {
         TimerContext(std::shared_ptr<time::Clock> const& clock,
                      std::function<void()> const& handler,
+                     std::function<void()> const& lock,
+                     std::function<void()> const& unlock,
                      time::Timestamp target_time)
-            : clock{clock}, handler{handler}, target_time{target_time}, enabled{true}
+            : clock{clock}, handler{handler}, lock{lock}, unlock{unlock},
+              target_time{target_time}, enabled{true}
         {
         }
         std::shared_ptr<time::Clock> clock;
         std::function<void()> handler;
+        std::function<void()> lock;
+        std::function<void()> unlock;
         time::Timestamp target_time;
         bool enabled;
         mir::RecursiveReadWriteMutex mutex;
@@ -284,6 +293,9 @@
         {
             auto& ctx = reinterpret_cast<TimerGSource*>(source)->ctx;
 
+            // Caller may pass std::function objects to preserve locking
+            // order during callback dispatching, so aquire them first.
+            auto caller_lock = mir::raii::paired_calls(std::ref(ctx.lock), std::ref(ctx.unlock));
             RecursiveReadLock lock{ctx.mutex};
             if (ctx.enabled)
                 ctx.handler();
@@ -321,7 +333,7 @@
     auto const timer_gsource = reinterpret_cast<TimerGSource*>(static_cast<GSource*>(gsource));
 
     timer_gsource->ctx_constructed = false;
-    new (&timer_gsource->ctx) TimerContext{clock, handler, target_time};
+    new (&timer_gsource->ctx) TimerContext{clock, handler, lock, unlock, target_time};
     timer_gsource->ctx_constructed = true;
 
     g_source_attach(gsource, main_context);

=== modified file 'tests/include/mir_test_doubles/mock_frame_dropping_policy_factory.h'
--- tests/include/mir_test_doubles/mock_frame_dropping_policy_factory.h	2014-05-22 20:48:20 +0000
+++ tests/include/mir_test_doubles/mock_frame_dropping_policy_factory.h	2015-02-20 17:57:50 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2014 Canonical Ltd.
+ * Copyright © 2014-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,
@@ -14,6 +14,7 @@
  * along with this program. If not, see <http://www.gnu.org/licenses/>.
  *
  * Authored by: Christopher James Halse Rogers <[e-mail]>
+ *              Alberto Aguirre <[e-mail]>
  */
 
 
@@ -23,6 +24,8 @@
 #include "mir/compositor/frame_dropping_policy_factory.h"
 #include "mir/compositor/frame_dropping_policy.h"
 
+#include "mir_test/gmock_fixes.h"
+
 #include <unordered_set>
 #include <functional>
 
@@ -42,7 +45,9 @@
 class MockFrameDroppingPolicy : public mc::FrameDroppingPolicy
 {
 public:
-    MockFrameDroppingPolicy(std::function<void(void)> callback,
+    MockFrameDroppingPolicy(std::function<void()> const& callback,
+                            std::function<void()> const& lock,
+                            std::function<void()> const& unlock,
                             MockFrameDroppingPolicyFactory const* parent);
     ~MockFrameDroppingPolicy();
 
@@ -55,14 +60,19 @@
     friend class MockFrameDroppingPolicyFactory;
     void parent_destroyed();
 
-    std::function<void(void)> callback;
+    std::function<void()> callback;
+    std::function<void()> lock;
+    std::function<void()> unlock;
     MockFrameDroppingPolicyFactory const* parent;
 };
 
 class MockFrameDroppingPolicyFactory : public mc::FrameDroppingPolicyFactory
 {
 public:
-    std::unique_ptr<mc::FrameDroppingPolicy> create_policy(std::function<void(void)> drop_frame) const override;
+    std::unique_ptr<mc::FrameDroppingPolicy> create_policy(
+        std::function<void()> const& drop_frame,
+        std::function<void()> const& lock,
+        std::function<void()> const& unlock) const override;
 
     ~MockFrameDroppingPolicyFactory();
 
@@ -75,6 +85,15 @@
     mutable std::unordered_set<MockFrameDroppingPolicy*> policies;
 };
 
+class FrameDroppingPolicyFactoryMock : public mc::FrameDroppingPolicyFactory
+{
+public:
+    MOCK_CONST_METHOD3(create_policy, std::unique_ptr<mc::FrameDroppingPolicy>(
+                     std::function<void()> const&,
+                     std::function<void()> const&,
+                     std::function<void()> const&));
+};
+
 }
 }
 }

=== modified file 'tests/include/mir_test_doubles/mock_main_loop.h'
--- tests/include/mir_test_doubles/mock_main_loop.h	2014-06-24 14:12:25 +0000
+++ tests/include/mir_test_doubles/mock_main_loop.h	2015-02-20 17:57:50 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2014 Canonical Ltd.
+ * Copyright © 2014-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
@@ -52,11 +52,19 @@
     MOCK_METHOD1(pause_processing_for,void (void const*));
     MOCK_METHOD1(resume_processing_for,void (void const*));
 
-    MOCK_METHOD2(notify_in, std::unique_ptr<time::Alarm>(std::chrono::milliseconds delay,
-                                           std::function<void(void)> callback));
-    MOCK_METHOD2(notify_at, std::unique_ptr<time::Alarm>(time::Timestamp time_point,
-                                           std::function<void(void)> callback));
-    MOCK_METHOD1(create_alarm, std::unique_ptr<time::Alarm>(std::function<void ()> callback));
+    MOCK_METHOD2(notify_in,
+                 std::unique_ptr<time::Alarm>(std::chrono::milliseconds delay,
+                                              std::function<void()> const& callback));
+    MOCK_METHOD2(notify_at,
+                 std::unique_ptr<time::Alarm>(time::Timestamp time_point,
+                                              std::function<void()> const& callback));
+    MOCK_METHOD1(create_alarm, std::unique_ptr<time::Alarm>(std::function<void()> const& callback));
+    MOCK_METHOD3(create_alarm,
+                 std::unique_ptr<time::Alarm>(
+                     std::function<void()> const& callback,
+                     std::function<void()> const& lock,
+                     std::function<void()> const& unlock));
+
 };
 
 }

=== modified file 'tests/include/mir_test_doubles/mock_timer.h'
--- tests/include/mir_test_doubles/mock_timer.h	2014-05-22 20:48:20 +0000
+++ tests/include/mir_test_doubles/mock_timer.h	2015-02-20 17:57:50 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2014 Canonical Ltd.
+ * Copyright © 2014-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,
@@ -14,6 +14,7 @@
  * along with this program. If not, see <http://www.gnu.org/licenses/>.
  *
  * Authored by: Christopher James Halse Rogers <[e-mail]>
+ *              Alberto Aguirre <[e-mail]>
  */
 
 #ifndef MIR_TEST_DOUBLES_MOCK_TIMER_H_
@@ -36,11 +37,12 @@
     FakeTimer(std::shared_ptr<FakeClock> const& clock);
 
     std::unique_ptr<time::Alarm> notify_in(std::chrono::milliseconds delay,
-                                           std::function<void(void)> callback) override;
+                                           std::function<void()> const& callback) override;
     std::unique_ptr<time::Alarm> notify_at(time::Timestamp time_point,
-                                           std::function<void(void)> callback) override;
-    std::unique_ptr<time::Alarm> create_alarm(std::function<void ()> callback) override;
-
+                                           std::function<void()> const& callback) override;
+    std::unique_ptr<time::Alarm> create_alarm(std::function<void()> const& callback) override;
+    std::unique_ptr<time::Alarm> create_alarm(std::function<void()> const& callback,
+        std::function<void()> const& lock, std::function<void()> const& unlock) override;
 private:
     std::shared_ptr<FakeClock> const clock;
 };

=== modified file 'tests/include/mir_test_doubles/stub_frame_dropping_policy_factory.h'
--- tests/include/mir_test_doubles/stub_frame_dropping_policy_factory.h	2014-05-22 20:48:20 +0000
+++ tests/include/mir_test_doubles/stub_frame_dropping_policy_factory.h	2015-02-20 17:57:50 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2014 Canonical Ltd.
+ * Copyright © 2014-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,
@@ -14,6 +14,7 @@
  * along with this program. If not, see <http://www.gnu.org/licenses/>.
  *
  * Authored by: Christopher James Halse Rogers <[e-mail]>
+ *              Alberto Aguirre <[e-mail]>
  */
 
 
@@ -46,9 +47,12 @@
 class StubFrameDroppingPolicyFactory : public mc::FrameDroppingPolicyFactory
 {
 public:
-    std::unique_ptr<mc::FrameDroppingPolicy> create_policy(std::function<void(void)>) const override
+    std::unique_ptr<mc::FrameDroppingPolicy> create_policy(
+        std::function<void()> const&,
+        std::function<void()> const&,
+        std::function<void()> const& ) const override
     {
-        return std::unique_ptr<mc::FrameDroppingPolicy>{new StubFrameDroppingPolicy};
+       return std::unique_ptr<mc::FrameDroppingPolicy>{new StubFrameDroppingPolicy};
     }
 };
 

=== modified file 'tests/include/mir_test_doubles/stub_timer.h'
--- tests/include/mir_test_doubles/stub_timer.h	2015-01-21 07:34:50 +0000
+++ tests/include/mir_test_doubles/stub_timer.h	2015-02-20 17:57:50 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2014 Canonical Ltd.
+ * Copyright © 2014-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,
@@ -51,12 +51,12 @@
 
 class StubTimer : public mir::time::Timer
 {
-    std::unique_ptr<mir::time::Alarm> notify_in(std::chrono::milliseconds, std::function<void(void)>) override
+    std::unique_ptr<mir::time::Alarm> notify_in(std::chrono::milliseconds, std::function<void()> const&) override
     {
         return std::unique_ptr<mir::time::Alarm>{new StubAlarm};
     }
 
-    std::unique_ptr<mir::time::Alarm> notify_at(mir::time::Timestamp, std::function<void(void)>) override
+    std::unique_ptr<mir::time::Alarm> notify_at(mir::time::Timestamp, std::function<void()> const&) override
     {
         return std::unique_ptr<mir::time::Alarm>{new StubAlarm};
     }

=== modified file 'tests/include/mir_test_doubles/triggered_main_loop.h'
--- tests/include/mir_test_doubles/triggered_main_loop.h	2015-01-27 11:47:52 +0000
+++ tests/include/mir_test_doubles/triggered_main_loop.h	2015-02-20 17:57:50 +0000
@@ -39,7 +39,7 @@
 
     void register_fd_handler(std::initializer_list<int> fds, void const* owner, fd_callback const& handler) override;
     void unregister_fd_handler(void const* owner) override;
-    std::unique_ptr<mir::time::Alarm> notify_in(std::chrono::milliseconds delay, callback call) override;
+    std::unique_ptr<mir::time::Alarm> notify_in(std::chrono::milliseconds delay, callback const& call) override;
     void enqueue(void const* owner, ServerAction const& action) override;
 
     void trigger_pending_fds();

=== modified file 'tests/mir_test_doubles/mock_frame_dropping_policy_factory.cpp'
--- tests/mir_test_doubles/mock_frame_dropping_policy_factory.cpp	2014-05-22 20:48:20 +0000
+++ tests/mir_test_doubles/mock_frame_dropping_policy_factory.cpp	2015-02-20 17:57:50 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2014 Canonical Ltd.
+ * Copyright © 2014-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,
@@ -14,6 +14,7 @@
  * along with this program. If not, see <http://www.gnu.org/licenses/>.
  *
  * Authored by: Christopher James Halse Rogers <[e-mail]>
+ *              Alberto Aguirre <[e-mail]>
  */
 
 #include "mir_test_doubles/mock_frame_dropping_policy_factory.h"
@@ -21,9 +22,13 @@
 namespace mc = mir::compositor;
 namespace mtd = mir::test::doubles;
 
-mtd::MockFrameDroppingPolicy::MockFrameDroppingPolicy(std::function<void(void)> callback,
+mtd::MockFrameDroppingPolicy::MockFrameDroppingPolicy(std::function<void()> const& callback,
+                                                      std::function<void()> const& lock,
+                                                      std::function<void()> const& unlock,
                                                       MockFrameDroppingPolicyFactory const* parent)
         : callback{callback},
+          lock{lock},
+          unlock{unlock},
           parent{parent}
 {
 }
@@ -36,7 +41,9 @@
 
 void mtd::MockFrameDroppingPolicy::trigger()
 {
+    lock();
     callback();
+    unlock();
 }
 
 void mtd::MockFrameDroppingPolicy::parent_destroyed()
@@ -45,9 +52,10 @@
 }
 
 std::unique_ptr<mc::FrameDroppingPolicy>
-mtd::MockFrameDroppingPolicyFactory::create_policy(std::function<void(void)> drop_frame) const
+mtd::MockFrameDroppingPolicyFactory::create_policy(std::function<void()> const& drop_frame,
+    std::function<void()> const& lock, std::function<void()> const& unlock) const
 {
-    auto policy = new ::testing::NiceMock<MockFrameDroppingPolicy>{drop_frame, this};
+    auto policy = new ::testing::NiceMock<MockFrameDroppingPolicy>{drop_frame, lock, unlock, this};
     policies.insert(policy);
     return std::unique_ptr<mc::FrameDroppingPolicy>{policy};
 }

=== modified file 'tests/mir_test_doubles/mock_timer.cpp'
--- tests/mir_test_doubles/mock_timer.cpp	2015-01-21 07:34:50 +0000
+++ tests/mir_test_doubles/mock_timer.cpp	2015-02-20 17:57:50 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2014 Canonical Ltd.
+ * Copyright © 2014-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,
@@ -14,6 +14,7 @@
  * along with this program. If not, see <http://www.gnu.org/licenses/>.
  *
  * Authored by: Christopher James Halse Rogers <[e-mail]>
+ *              Alberto Aguirre <[e-mail]>
  */
 
 #include "mir_test_doubles/mock_timer.h"
@@ -27,7 +28,10 @@
 class FakeAlarm : public mir::time::Alarm
 {
 public:
-    FakeAlarm(std::function<void(void)> callback, std::shared_ptr<mt::FakeClock> const& clock);
+    FakeAlarm(std::function<void()> const& callback,
+              std::function<void()> const& lock,
+              std::function<void()> const& unlock,
+              std::shared_ptr<mt::FakeClock> const& clock);
     ~FakeAlarm() override;
 
 
@@ -39,9 +43,13 @@
 private:    
     struct InternalState
     {
-        explicit InternalState(std::function<void(void)> callback);
+        explicit InternalState(std::function<void()> const& callback,
+                               std::function<void()> const& lock,
+                               std::function<void()> const& unlock);
         State state;
-        std::function<void(void)> const callback;
+        std::function<void()> const callback;
+        std::function<void()> const lock;
+        std::function<void()> const unlock;
         mt::FakeClock::time_point threshold;
     };
 
@@ -51,13 +59,20 @@
     std::shared_ptr<mt::FakeClock> const clock;
 };
 
-FakeAlarm::InternalState::InternalState(std::function<void(void)> callback)
-    : state{mir::time::Alarm::pending}, callback{callback}
+FakeAlarm::InternalState::InternalState(
+    std::function<void()> const& callback,
+    std::function<void()> const& lock,
+    std::function<void()> const& unlock)
+    : state{mir::time::Alarm::pending}, callback{callback}, lock{lock}, unlock{unlock}
 {
 }
 
-FakeAlarm::FakeAlarm(std::function<void(void)> callback, std::shared_ptr<mt::FakeClock> const& clock)
-    : internal_state{std::make_shared<InternalState>(callback)}, clock{clock}
+FakeAlarm::FakeAlarm(
+    std::function<void()> const& callback,
+    std::function<void()> const& lock,
+    std::function<void()> const& unlock,
+    std::shared_ptr<mt::FakeClock> const& clock)
+    : internal_state{std::make_shared<InternalState>(callback, lock, unlock)}, clock{clock}
 {
 }
 
@@ -88,7 +103,9 @@
         if (now > state.threshold)
         {
             state.state = triggered;
+            state.lock();
             state.callback();
+            state.unlock();
             return false;
         }
         return true;
@@ -128,22 +145,28 @@
 }
 
 std::unique_ptr<mir::time::Alarm> mtd::FakeTimer::notify_in(std::chrono::milliseconds delay,
-                                                            std::function<void(void)> callback)
+                                                            std::function<void()> const& callback)
 {
-    auto alarm = std::unique_ptr<mir::time::Alarm>{new FakeAlarm{callback, clock}};
+    auto alarm = std::unique_ptr<mir::time::Alarm>{new FakeAlarm{callback, []{}, []{}, clock}};
     alarm->reschedule_in(delay);
     return std::move(alarm);
 }
 
 std::unique_ptr<mir::time::Alarm> mtd::FakeTimer::notify_at(time::Timestamp time_point,
-                                                            std::function<void(void)> callback)
+                                                            std::function<void()> const& callback)
 {
-    auto alarm = std::unique_ptr<mir::time::Alarm>{new FakeAlarm{callback, clock}};
+    auto alarm = std::unique_ptr<mir::time::Alarm>{new FakeAlarm{callback, []{}, []{}, clock}};
     alarm->reschedule_for(time_point);
     return std::move(alarm);
 }
 
-std::unique_ptr<mir::time::Alarm> mir::test::doubles::FakeTimer::create_alarm(std::function<void(void)> callback)
-{
-    return std::unique_ptr<mir::time::Alarm>{new FakeAlarm{callback, clock}};
+std::unique_ptr<mir::time::Alarm> mir::test::doubles::FakeTimer::create_alarm(std::function<void()> const& callback)
+{
+    return std::unique_ptr<mir::time::Alarm>{new FakeAlarm{callback, []{}, []{}, clock}};
+}
+
+std::unique_ptr<mir::time::Alarm> mir::test::doubles::FakeTimer::create_alarm(std::function<void()> const& callback,
+    std::function<void()> const& lock, std::function<void()> const& unlock)
+{
+    return std::unique_ptr<mir::time::Alarm>{new FakeAlarm{callback, lock, unlock, clock}};
 }

=== modified file 'tests/mir_test_doubles/triggered_main_loop.cpp'
--- tests/mir_test_doubles/triggered_main_loop.cpp	2015-01-27 11:47:52 +0000
+++ tests/mir_test_doubles/triggered_main_loop.cpp	2015-02-20 17:57:50 +0000
@@ -85,8 +85,9 @@
         action();
 }
 
-std::unique_ptr<mir::time::Alarm> mtd::TriggeredMainLoop::notify_in(std::chrono::milliseconds delay,
-                                            callback call)
+std::unique_ptr<mir::time::Alarm>
+mtd::TriggeredMainLoop::notify_in(std::chrono::milliseconds delay,
+                                  callback const& call)
 {
     base::notify_in(delay, call);
     timeout_callbacks.push_back(call);

=== modified file 'tests/unit-tests/compositor/test_buffer_queue.cpp'
--- tests/unit-tests/compositor/test_buffer_queue.cpp	2015-02-17 06:38:35 +0000
+++ tests/unit-tests/compositor/test_buffer_queue.cpp	2015-02-20 17:57:50 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2013-2014 Canonical Ltd.
+ * Copyright © 2013-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
@@ -1740,3 +1740,12 @@
     auto comp2 = q.compositor_acquire(new_compositor_id);
     ASSERT_THAT(comp2->id(), Eq(handle2->id()));
 }
+
+TEST_F(BufferQueueTest, creates_policy_with_lock_unlock_functions)
+{
+    int const nbuffers = 3;
+
+    mtd::FrameDroppingPolicyFactoryMock mock_policy_factory;
+    EXPECT_CALL(mock_policy_factory, create_policy(_, _, _));
+    mc::BufferQueue q(nbuffers, allocator, basic_properties, mock_policy_factory);
+}

=== modified file 'tests/unit-tests/compositor/test_timeout_frame_dropping_policy.cpp'
--- tests/unit-tests/compositor/test_timeout_frame_dropping_policy.cpp	2014-05-22 20:48:20 +0000
+++ tests/unit-tests/compositor/test_timeout_frame_dropping_policy.cpp	2015-02-20 17:57:50 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2014 Canonical Ltd.
+ * Copyright © 2014-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,
@@ -14,16 +14,18 @@
  * along with this program. If not, see <http://www.gnu.org/licenses/>.
  *
  * Authored by: Christopher James Halse Rogers <[e-mail]>
+ *              Alberto Aguirre <[e-mail]>
  */
 
 #include "src/server/compositor/timeout_frame_dropping_policy_factory.h"
 #include "mir/compositor/frame_dropping_policy.h"
 
 #include "mir_test_doubles/mock_timer.h"
-
+#include "mir_test/auto_unblock_thread.h"
 #include "mir_test/gmock_fixes.h"
 
 #include <stdexcept>
+#include <mutex>
 
 #include <gtest/gtest.h>
 #include <gmock/gmock.h>
@@ -40,7 +42,7 @@
     bool frame_dropped{false};
 
     mc::TimeoutFrameDroppingPolicyFactory factory{timer, timeout};
-    auto policy = factory.create_policy([&frame_dropped]{ frame_dropped = true; });
+    auto policy = factory.create_policy([&frame_dropped]{ frame_dropped = true; }, []{}, []{});
 
     clock->advance_time(timeout + std::chrono::milliseconds{1});
     EXPECT_FALSE(frame_dropped);
@@ -54,7 +56,7 @@
     bool frame_dropped{false};
 
     mc::TimeoutFrameDroppingPolicyFactory factory{timer, timeout};
-    auto policy = factory.create_policy([&frame_dropped]{ frame_dropped = true; });
+    auto policy = factory.create_policy([&frame_dropped]{ frame_dropped = true; }, []{}, []{});
     policy->swap_now_blocking();
 
     clock->advance_time(timeout - std::chrono::milliseconds{1});
@@ -71,7 +73,7 @@
 
     bool frame_dropped{false};
     mc::TimeoutFrameDroppingPolicyFactory factory{timer, timeout};
-    auto policy = factory.create_policy([&frame_dropped]{ frame_dropped = true; });
+    auto policy = factory.create_policy([&frame_dropped]{ frame_dropped = true; }, []{}, []{});
 
     policy->swap_now_blocking();
     policy->swap_unblocked();
@@ -89,7 +91,7 @@
 
     bool frame_dropped{false};
     mc::TimeoutFrameDroppingPolicyFactory factory{timer, timeout};
-    auto policy = factory.create_policy([&frame_dropped]{ frame_dropped = true; });
+    auto policy = factory.create_policy([&frame_dropped]{ frame_dropped = true; }, []{}, []{});
 
     policy->swap_now_blocking();
     policy->swap_now_blocking();
@@ -119,7 +121,7 @@
 
     bool frame_dropped{false};
     mc::TimeoutFrameDroppingPolicyFactory factory{timer, timeout};
-    auto policy = factory.create_policy([&frame_dropped]{ frame_dropped = true; });
+    auto policy = factory.create_policy([&frame_dropped]{ frame_dropped = true; }, []{}, []{});
 
     policy->swap_now_blocking();
     policy->swap_now_blocking();
@@ -142,7 +144,7 @@
 
     bool frame_dropped{false};
     mc::TimeoutFrameDroppingPolicyFactory factory{timer, timeout};
-    auto policy = factory.create_policy([&frame_dropped]{ frame_dropped = true; });
+    auto policy = factory.create_policy([&frame_dropped]{ frame_dropped = true; }, []{}, []{});
 
     policy->swap_now_blocking();
     clock->advance_time(timeout - std::chrono::milliseconds{1});
@@ -160,7 +162,7 @@
 
     bool frame_dropped{false};
     mc::TimeoutFrameDroppingPolicyFactory factory{timer, timeout};
-    auto policy = factory.create_policy([&frame_dropped]{ frame_dropped = true; });
+    auto policy = factory.create_policy([&frame_dropped]{ frame_dropped = true; }, []{}, []{});
 
     policy->swap_now_blocking();
     policy->swap_now_blocking();
@@ -184,3 +186,28 @@
     clock->advance_time(timeout + std::chrono::milliseconds{1});
     EXPECT_FALSE(frame_dropped);
 }
+
+TEST(TimeoutFrameDroppingPolicy, policy_calls_lock_unlock_functions)
+{
+    using namespace testing;
+
+    auto clock = std::make_shared<mt::FakeClock>();
+    auto timer = std::make_shared<mtd::FakeTimer>(clock);
+    std::chrono::milliseconds const timeout{1000};
+
+    mc::TimeoutFrameDroppingPolicyFactory factory{timer, timeout};
+
+    std::mutex m;
+    std::unique_lock<std::mutex> handler_guard{m, std::defer_lock};
+
+    auto policy = factory.create_policy([&]
+    {
+        EXPECT_THAT(handler_guard.owns_lock(), Eq(true));
+    },
+    [&] { handler_guard.lock(); },
+    [&] { handler_guard.unlock(); });
+
+
+    policy->swap_now_blocking();
+    clock->advance_time(timeout + std::chrono::milliseconds{1});
+}

=== modified file 'tests/unit-tests/test_glib_main_loop.cpp'
--- tests/unit-tests/test_glib_main_loop.cpp	2015-02-03 15:07:44 +0000
+++ tests/unit-tests/test_glib_main_loop.cpp	2015-02-20 17:57:50 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2014 Canonical Ltd.
+ * Copyright © 2014-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
@@ -14,6 +14,7 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  *
  * Authored by: Alexandros Frantzis <[e-mail]>
+ *              Alberto Aguirre <[e-mail]>
  */
 
 #include "mir/glib_main_loop.h"
@@ -960,43 +961,45 @@
 }
 
 TEST_F(GLibMainLoopAlarmTest, alarm_callback_cannot_deadlock)
-{   // Regression test for deadlock bug LP: #1339700
-    std::timed_mutex m;
-    std::atomic_bool failed(false);
+{
+    using namespace testing;
+
+    std::mutex m;
+    std::unique_lock<std::mutex> handler_guard{m, std::defer_lock};
+    std::atomic_bool exit{false};
     int i = 0;
     int const loops = 5;
 
-    auto alarm = ml.notify_in(std::chrono::milliseconds{0}, [&]()
+    auto alarm = ml.create_alarm([&]()
     {
-        // From this angle, ensure we can lock m (alarm should be unlocked)
-        failed = !m.try_lock_for(std::chrono::seconds{5});
-        ASSERT_FALSE(failed);
+        //In this handler context, the alarm dispatcher should have
+        //acquired the handler guard
+        EXPECT_THAT(handler_guard.owns_lock(), Eq(true));
         ++i;
-        m.unlock();
+    },
+    [&] { handler_guard.lock(); },
+    [&] { handler_guard.unlock(); });
+
+    mt::AutoUnblockThread t([&]{ exit = true; }, [&]()
+    {
+        std::unique_lock<std::mutex> guard{m};
+        while (i < loops && !exit)
+        {
+            // It's safe to call cancel now because lock ordering is enforced
+            // by passing the lock/unlock lambdas to alarm above
+            alarm->cancel();
+            guard.unlock();
+            std::this_thread::yield();
+            guard.lock();
+        }
     });
 
-    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)
     {
+        alarm->reschedule_in(std::chrono::milliseconds{10});
         clock->advance_by(std::chrono::milliseconds{11}, ml);
-        alarm->reschedule_in(std::chrono::milliseconds{10});
     }
-
-    t.join();
 }
 
 TEST_F(GLibMainLoopAlarmTest, alarm_fires_at_correct_time_point)

=== modified file 'tests/unit-tests/test_raii.cpp'
--- tests/unit-tests/test_raii.cpp	2013-10-21 14:21:00 +0000
+++ tests/unit-tests/test_raii.cpp	2015-02-20 17:57:50 +0000
@@ -179,3 +179,23 @@
 
     EXPECT_EQ(this, raii.get());
 }
+
+TEST_F(RaiiTest, paired_call_takes_std_function_refs)
+{
+    int creator_call_count = 0;
+    int deleter_call_count = 0;
+    int const expected_calls = 2;
+
+    std::function<void()> creator = [&creator_call_count] { creator_call_count++; };
+    std::function<void()> deleter = [&deleter_call_count] { deleter_call_count++; };
+
+    for (int i = 0; i < expected_calls; i++)
+    {
+        auto const raii = mir::raii::paired_calls(
+            std::ref(creator),
+            std::ref(deleter));
+    }
+
+    EXPECT_THAT(creator_call_count, Eq(expected_calls));
+    EXPECT_THAT(deleter_call_count, Eq(expected_calls));
+}