Mir

Merge lp:~raof/mir/fix-deadlock-in-glib-alarm into lp:mir

fix-deadlock-in-glib-alarm
Merge into development-branch

Proposed by Chris Halse Rogers on 2015-08-03

Status:	Merged
Approved by:	Alberto Aguirre on 2015-08-11
Approved revision:	2808
Merged at revision:	2832
Proposed branch:	lp:~raof/mir/fix-deadlock-in-glib-alarm
Merge into:	lp:mir
Diff against target:	406 lines (+139/-38) 9 files modified examples/server_example.cpp (+6/-3) examples/server_example_test_client.cpp (+15/-14) examples/server_example_test_client.h (+12/-2) src/include/server/mir/glib_main_loop_sources.h (+4/-2) src/server/glib_main_loop.cpp (+6/-0) src/server/glib_main_loop_sources.cpp (+19/-8) tests/mir_test_framework/stub_input_platform.cpp (+9/-7) tests/mir_test_framework/stub_input_platform.h (+2/-2) tests/unit-tests/test_glib_main_loop.cpp (+66/-0)
To merge this branch:	bzr merge lp:~raof/mir/fix-deadlock-in-glib-alarm
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Alberto Aguirre			Approve on 2015-08-11
Alan Griffiths			Approve on 2015-08-11
PS Jenkins bot	continuous-integration		Approve on 2015-08-11
Kevin DuBois (community)		2015-08-03	Approve on 2015-08-03
Review via email: mp+266682@code.launchpad.net

Commit Message

Fix a deadlock in GLibMainLoop's Alarm implementation.

There's currently a deadlock in the following case:

Thread 1: enters an alarm callback. (And so has taken TimerContext::mutex)
Thread 2: calls alarm->reschedule_in(), takes AlarmImpl::alarm_mutex, blocks when trying to take TimerContext::mutex
Thread 1: In the alarm callback, calls alarm->reschedule_in(), blocks on AlarmImpl::alarm_mutex.

The fundamental problem here is that we use ~GSourceHandle to guarantee that we've left all callbacks. This is needed for the guarantees provided by ~Alarm (and is nice for ::cancel), but is unnecessary for the reschedule_* calls - barring external synchronisation with the MainLoop, calling reschedule_* when there's an alarm already pending is inherently racing against real-time.

So, split the ensure-callbacks-are-cancelled guarantee out into GSourceHandle::ensure_no_further_dispatch(), and let ~GSourceHandle merely clean up all the relevant resources.

Description of the Change

Oh god.

Fix a deadlock in GLibMainLoop's Alarm implementation.

There's currently a deadlock in the following case:

So, split the ensure-callbacks-are-cancelled guarantee out into GSourceHandle::ensure_no_further_dispatch(), and let ~GSourceHandle merely clean up all the relevant resources.

For bonus points allows us to switch from a std::recursive_mutex to a std::mutex in TimerContext.

PS Jenkins bot (ps-jenkins) wrote on 2015-08-03:

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

review: Needs Fixing (continuous-integration)

Kevin DuBois (kdub) wrote on 2015-08-03:

lgtm

review: Approve

lp:~raof/mir/fix-deadlock-in-glib-alarm updated on 2015-08-04

2800. By Chris Halse Rogers on 2015-08-04

Fix alarm-lifecycle FIXME in example server.

Don't have Alarms that outlive the server.

PS Jenkins bot (ps-jenkins) wrote on 2015-08-04:

FAILED: Continuous integration, rev:2800
http://jenkins.qa.ubuntu.com/job/mir-ci/4464/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3389/console
    FAILURE: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-wily-amd64-build/244/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/916/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3337/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/613/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3337/console

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

review: Needs Fixing (continuous-integration)

lp:~raof/mir/fix-deadlock-in-glib-alarm updated on 2015-08-04

2801. By Chris Halse Rogers on 2015-08-04: Merge trunk, fixing GSourceHandle conflict

PS Jenkins bot (ps-jenkins) wrote on 2015-08-04:

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

review: Needs Fixing (continuous-integration)

PS Jenkins bot (ps-jenkins) wrote on 2015-08-04:

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

review: Needs Fixing (continuous-integration)

lp:~raof/mir/fix-deadlock-in-glib-alarm updated on 2015-08-05

2802. By Chris Halse Rogers on 2015-08-05: Fix race in Alarm::cancel test case

PS Jenkins bot (ps-jenkins) wrote on 2015-08-05:

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

review: Needs Fixing (continuous-integration)

lp:~raof/mir/fix-deadlock-in-glib-alarm updated on 2015-08-05

2803. By Chris Halse Rogers on 2015-08-05

Guard access to stub platform pointers.

One more data race down. How many to go?

2804. By Chris Halse Rogers on 2015-08-05

Simplify GLibMainLoopAlarmTest::cancel_blocks_until_definitely_cancelled

At the cost of being slightly less accurate. It should also eliminate false-positives.

PS Jenkins bot (ps-jenkins) wrote on 2015-08-05:

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

review: Needs Fixing (continuous-integration)

Chris Halse Rogers (raof) wrote on 2015-08-06:

Ok. Failure is:
+ phablet-test-run -x -s 00693fd555c9186a -v mir_demo_server --test-client /usr/bin/mir_demo_client_egltriangle
running mir_demo_server --test-client /usr/bin/mir_demo_client_egltriangle
…
Surface 0 DPI
Signal 15 received. Good night.
[1438765839.346519] mirserver: Stopping
/bin/bash: line 1: 11723 Segmentation fault (core dumped) mir_demo_server --test-client /usr/bin/mir_demo_client_egltriangle

And is the same for all the mir_demo_server --test-client... runs.

Unfortunately, I can't reproduce this failure crash, either on my development system or on a cross-built arale.

Chris Halse Rogers (raof) wrote on 2015-08-06:

Maybe it's cosmic rays?

PS Jenkins bot (ps-jenkins) wrote on 2015-08-06:

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

review: Needs Fixing (continuous-integration)

Alberto Aguirre (albaguirre) wrote on 2015-08-06:

I can replicate the error with a cross-compile build using -Duse_debflags=ON and

"bin/mir_demo_server --test-client /home/phablet/mir/bin/mir_demo_client_egltriangle"

Probably related to the static alarm objects used in server_example_test_client.cpp

Alberto Aguirre (albaguirre) wrote on 2015-08-06:

Actually even without -Duse_debflags=ON the server still crashes at the end.

Alberto Aguirre (albaguirre) wrote on 2015-08-06:

And actually just exiting bin/mir_demo_server results in a crash too.

Chris Halse Rogers (raof) wrote on 2015-08-07:

Ok, I'm pretty sure this only happens on mako(!). I've tried, with a debug build and use_debflags=ON, on desktop and arale (rc-proposed, wily), and on none of those platforms does mir_demo_server crash on shutdown.

Time to see if my mako has awoken from its dead-battery fugue.

Alberto Aguirre (albaguirre) wrote on 2015-08-07:

This is happening on arale too (image #80 from ubuntu-touch/devel-proposed/meizu.en)

Stack trace:
http://pastebin.ubuntu.com/12022909/

Seems like memory corruption.

Alberto Aguirre (albaguirre) wrote on 2015-08-07:

Ahh this seems to be the same TLS issue I encountered in lp:1280086.

The relevant bit from screencast.cpp:

"/* On devices with android based openGL drivers, the vendor dispatcher table
* may be optimized if USE_FAST_TLS_KEY is set, which hardcodes a TLS slot where
* the vendor opengl function pointers live. Since glibc is not aware of this
* use, collisions may happen.
"

lp:~raof/mir/fix-deadlock-in-glib-alarm updated on 2015-08-10

2805. By Chris Halse Rogers on 2015-08-10: Remove static std::mutexes. Looks like this runs into TLS memory corruption issues on hybris
2806. By Chris Halse Rogers on 2015-08-10: Resolve StubInputPlatform data race with an atomic.

PS Jenkins bot (ps-jenkins) wrote on 2015-08-10:

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

review: Needs Fixing (continuous-integration)

lp:~raof/mir/fix-deadlock-in-glib-alarm updated on 2015-08-10

2807. By Chris Halse Rogers on 2015-08-10

Unfix alarm-lifecycle FIXME in example server.

Trying to actually tie the alarms' lifecycles to the server apparently hits
TLS-glibc-hybris-borkage resulting in memory corruption and segfault on exit,
and I'm disinclined to spend *more* time debugging why.

Just leak the alarms, since we never needed to run their destructors in the
first place.

PS Jenkins bot (ps-jenkins) wrote on 2015-08-10:

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

review: Approve (continuous-integration)

Chris Halse Rogers (raof) wrote on 2015-08-10:

WOOOOOOO! FINALLY!

I wonder what the triggering condition for std::future clashing with the Android TLS slot is? We use futures elsewhere in the code without issue...

Chris Halse Rogers (raof) wrote on 2015-08-10:

Oh! Of course it's the TLS mode. The binary can (and almost certainly will) use the local-exec TLS model. The Mir libraries can at most use initial-exec model, and will probably(?) be using the local-dynamic model, and will be getting a non-conflicting slot.

Alan Griffiths (alan-griffiths) wrote on 2015-08-10:

Admittedly the existing code using "static" to extend the lifetime of these alarms is wrong, but I can't help feeling leaking them is also inelegant. (I've not thought of a better solution yet.)

I'd also be inclined to make leaked_kill_action & leaked_exit_action function scope static to make it clear they are not used elsewhere.

Chris Halse Rogers (raof) wrote on 2015-08-10:

Well, I *could* resurrect the pass-to-main implementation that ran into TLS problems. We could either try forcing a different TLS model or do what screencast does and just hope adding a couple of unused TLS entries will move stdlib's TLS entries out of the conflicting slot...

-----Original Message-----
From: "Alan Griffiths" <email address hidden>
Sent: ‎10/‎08/‎2015 18:56
To: "Chris Halse Rogers" <email address hidden>
Subject: Re: [Merge] lp:~raof/mir/fix-deadlock-in-glib-alarm into lp:mir

Admittedly the existing code using "static" to extend the lifetime of these alarms is wrong, but I can't help feeling leaking them is also inelegant. (I've not thought of a better solution yet.)

I'd also be inclined to make leaked_kill_action & leaked_exit_action function scope static to make it clear they are not used elsewhere.
--
https://code.launchpad.net/~raof/mir/fix-deadlock-in-glib-alarm/+merge/266682
You are the owner of lp:~raof/mir/fix-deadlock-in-glib-alarm.

Alan Griffiths (alan-griffiths) wrote on 2015-08-10:

On the basis that it is an improvement.

review: Approve

Alberto Aguirre (albaguirre) wrote on 2015-08-10:

You can actually leave them as static, I fixed what the old FIXME allured to (https://code.launchpad.net/~albaguirre/mir/fix-1480420/+merge/266614) but I forgot to remove the comment there.
Or also why not just use a condition variable instead of a std::future? TLS won't be an issue then.

review: Needs Information

lp:~raof/mir/fix-deadlock-in-glib-alarm updated on 2015-08-11

2808. By Chris Halse Rogers on 2015-08-11

Fix alarm-lifecycle FIXME in a simpler manner.

Looking at it fresh, we don't actually need any of the std::future machinery.
We can just pass things around by reference. So do so.

PS Jenkins bot (ps-jenkins) wrote on 2015-08-11:

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

review: Approve (continuous-integration)

Alan Griffiths (alan-griffiths) wrote on 2015-08-11:

review: Approve

Alberto Aguirre (albaguirre) wrote on 2015-08-11:

LGTM.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Chris Halse Rogers

Emanuele Antonio Faraone

Gerry Boland

Mir development team

 === modified file 'examples/server_example.cpp'
 --- examples/server_example.cpp	2015-06-17 05:20:42 +0000
 +++ examples/server_example.cpp	2015-08-11 01:44:53 +0000
@@ -94,8 +94,8 @@
      add_launcher_option_to(server);
      add_timeout_option_to(server);
--    std::atomic<bool> test_failed{false};
--    me::add_test_client_option_to(server, test_failed);
++    me::ClientContext context;
++    me::add_test_client_option_to(server, context);
      // Create some input filters (we need to keep them or they deactivate)
      auto const quit_filter = me::make_quit_filter_for(server);
@@ -108,7 +108,10 @@
      server.run();
      // Propagate any test failure
--    if (test_failed) return EXIT_FAILURE;
++    if (context.test_failed)
++    {
++        return EXIT_FAILURE;
++    }
      return server.exited_normally() ? EXIT_SUCCESS : EXIT_FAILURE;
+ }
 === modified file 'examples/server_example_test_client.cpp'
 --- examples/server_example_test_client.cpp	2015-07-20 03:16:27 +0000
 +++ examples/server_example_test_client.cpp	2015-08-11 01:44:53 +0000
@@ -81,7 +81,7 @@
+ }
+ }
--void me::add_test_client_option_to(mir::Server& server, std::atomic<bool>& test_failed)
++void me::add_test_client_option_to(mir::Server& server, me::ClientContext& context)
+ {
      static const char* const test_client_opt = "test-client";
      static const char* const test_client_descr = "client executable";
@@ -92,11 +92,14 @@
      server.add_configuration_option(test_client_opt, test_client_descr, mir::OptionType::string);
      server.add_configuration_option(test_timeout_opt, test_timeout_descr, 10);
--    server.add_init_callback([&]
++    server.add_init_callback([&server, &context]
+     {
          const auto options = server.get_options();
++
          if (options->is_set(test_client_opt))
+         {
++            context.test_failed = true;
++
              auto const pid = fork();
              if (pid == 0)
@@ -107,32 +110,30 @@
+             }
              else if (pid > 0)
+             {
--                //FIXME: These alarm objects outlive the server - their destructor is called after the server is destroyed.
--                //The alarm destructor implementation reference glib objects that are assumed to exist which leads to crashes.
--                //For now, as a workaround, canceling the alarm will release such internal resources
--                static std::unique_ptr<mir::time::Alarm> const kill_action = server.the_main_loop()->create_alarm(
++                context.client_kill_action = server.the_main_loop()->create_alarm(
                      [pid]
+                     {
--                        kill_action->cancel();
                          kill(pid, SIGTERM);
                      });
--                static std::unique_ptr<mir::time::Alarm> const exit_action = server.the_main_loop()->create_alarm(
--                    [pid, &server, &test_failed]
++                context.server_stop_action = server.the_main_loop()->create_alarm(
++                    [pid, &server, &context]()
+                     {
--                        if (!exit_success(pid))
--                            test_failed = true;
--                        exit_action->cancel();
++                        context.test_failed = !exit_success(pid);
                          server.stop();
                      });
--                kill_action->reschedule_in(std::chrono::seconds(options->get<int>(test_timeout_opt)));
--                exit_action->reschedule_in(std::chrono::seconds(options->get<int>(test_timeout_opt)+1));
++                context.client_kill_action->reschedule_in(std::chrono::seconds(options->get<int>(test_timeout_opt)));
++                context.server_stop_action->reschedule_in(std::chrono::seconds(options->get<int>(test_timeout_opt)+1));
+             }
              else
+             {
                  BOOST_THROW_EXCEPTION(std::runtime_error("Client failed to launch"));
+             }
+         }
++        else
++        {
++            context.test_failed = false;
++        }
      });
+ }
 === modified file 'examples/server_example_test_client.h'
 --- examples/server_example_test_client.h	2014-12-17 17:01:13 +0000
 +++ examples/server_example_test_client.h	2015-08-11 01:44:53 +0000
@@ -19,7 +19,10 @@
  #ifndef MIR_EXAMPLE_TEST_CLIENT_H_
  #define MIR_EXAMPLE_TEST_CLIENT_H_
--#include <atomic>
++#include <memory>
++#include <future>
++
++#include "mir/main_loop.h"
  namespace mir
+ {
@@ -27,7 +30,14 @@
  namespace examples
+ {
--void add_test_client_option_to(mir::Server& server, std::atomic<bool>& test_failed);
++struct ClientContext
++{
++    std::unique_ptr<mir::time::Alarm> client_kill_action;
++    std::unique_ptr<mir::time::Alarm> server_stop_action;
++    std::atomic<bool> test_failed;
++};
++
++void add_test_client_option_to(mir::Server& server, ClientContext& context);
+ }
+ }
 === modified file 'src/include/server/mir/glib_main_loop_sources.h'
 --- src/include/server/mir/glib_main_loop_sources.h	2015-06-17 05:20:42 +0000
 +++ src/include/server/mir/glib_main_loop_sources.h	2015-08-11 01:44:53 +0000
@@ -50,16 +50,18 @@
+ {
  public:
      GSourceHandle();
--    GSourceHandle(GSource* gsource, std::function<void(GSource*)> const& pre_destruction_hook);
++    GSourceHandle(GSource* gsource, std::function<void(GSource*)> const& terminate_dispatch);
      GSourceHandle(GSourceHandle&& other);
      GSourceHandle& operator=(GSourceHandle other);
      ~GSourceHandle();
++    void ensure_no_further_dispatch();
++
      operator GSource*() const;
  private:
      GSource* gsource;
--    std::function<void(GSource*)> pre_destruction_hook;
++    std::function<void(GSource*)> terminate_dispatch;
  };
  void add_idle_gsource(
 === modified file 'src/server/glib_main_loop.cpp'
 --- src/server/glib_main_loop.cpp	2015-06-17 05:20:42 +0000
 +++ src/server/glib_main_loop.cpp	2015-08-11 01:44:53 +0000
@@ -47,10 +47,16 @@
+     {
+     }
++    ~AlarmImpl() override
++    {
++        gsource.ensure_no_further_dispatch();
++    }
++
      bool cancel() override
+     {
          std::lock_guard<std::mutex> lock{alarm_mutex};
++        gsource.ensure_no_further_dispatch();
          gsource = mir::detail::GSourceHandle{};
          state_ = State::cancelled;
          return true;
 === modified file 'src/server/glib_main_loop_sources.cpp'
 --- src/server/glib_main_loop_sources.cpp	2015-07-31 18:57:59 +0000
 +++ src/server/glib_main_loop_sources.cpp	2015-08-11 01:44:53 +0000
@@ -80,24 +80,24 @@
  md::GSourceHandle::GSourceHandle(
      GSource* gsource,
--    std::function<void(GSource*)> const& pre_destruction_hook)
++    std::function<void(GSource*)> const& terminate_dispatch)
      : gsource(gsource),
--      pre_destruction_hook(pre_destruction_hook)
++      terminate_dispatch(terminate_dispatch)
+ {
+ }
  md::GSourceHandle::GSourceHandle(GSourceHandle&& other)
      : gsource(std::move(other.gsource)),
--      pre_destruction_hook(std::move(other.pre_destruction_hook))
++      terminate_dispatch(std::move(other.terminate_dispatch))
+ {
      other.gsource = nullptr;
--    other.pre_destruction_hook = [](GSource*){};
++    other.terminate_dispatch = [](GSource*){};
+ }
  md::GSourceHandle& md::GSourceHandle::operator=(GSourceHandle other)
+ {
      std::swap(other.gsource, gsource);
--    std::swap(other.pre_destruction_hook, pre_destruction_hook);
++    std::swap(other.terminate_dispatch, terminate_dispatch);
      return *this;
+ }
@@ -105,8 +105,6 @@
+ {
      if (gsource)
+     {
--        pre_destruction_hook(gsource);
--
  #ifdef GLIB_HAS_FIXED_LP_1401488
          g_source_destroy(gsource);
          g_source_unref(gsource);
@@ -138,6 +136,14 @@
+     }
+ }
++void md::GSourceHandle::ensure_no_further_dispatch()
++{
++    if (gsource)
++    {
++        terminate_dispatch(gsource);
++    }
++}
++
  md::GSourceHandle::operator GSource*() const
+ {
      return gsource;
@@ -260,8 +266,8 @@
          std::shared_ptr<LockableCallback> handler;
          std::function<void()> exception_handler;
          time::Timestamp target_time;
++        std::mutex mutex;
          bool enabled;
--        std::recursive_mutex mutex;
      };
      struct TimerGSource
@@ -393,6 +399,11 @@
  struct md::FdSources::FdSource
+ {
++    ~FdSource()
++    {
++        gsource.ensure_no_further_dispatch();
++    }
++
      GSourceHandle gsource;
      void const* const owner;
  };
 === modified file 'tests/mir_test_framework/stub_input_platform.cpp'
 --- tests/mir_test_framework/stub_input_platform.cpp	2015-07-30 08:33:38 +0000
 +++ tests/mir_test_framework/stub_input_platform.cpp	2015-08-11 01:44:53 +0000
@@ -68,14 +68,15 @@
  void mtf::StubInputPlatform::add(std::shared_ptr<mir::input::InputDevice> const& dev)
+ {
--    if (!stub_input_platform)
++    auto input_platform = stub_input_platform.load();
++    if (!input_platform)
+     {
          device_store.push_back(dev);
          return;
+     }
--    stub_input_platform->platform_queue->enqueue(
--        [registry=stub_input_platform->registry,dev]
++    input_platform->platform_queue->enqueue(
++        [registry=input_platform->registry,dev]
+         {
              registry->add_device(dev);
          });
@@ -83,15 +84,16 @@
  void mtf::StubInputPlatform::remove(std::shared_ptr<mir::input::InputDevice> const& dev)
+ {
--    if (!stub_input_platform)
++    auto input_platform = stub_input_platform.load();
++    if (!input_platform)
          BOOST_THROW_EXCEPTION(std::runtime_error("No stub input platform available"));
--    stub_input_platform->platform_queue->enqueue(
--        [registry=stub_input_platform->registry,dev]
++    input_platform->platform_queue->enqueue(
++        [registry=input_platform->registry,dev]
+         {
              registry->remove_device(dev);
          });
+ }
--mtf::StubInputPlatform* mtf::StubInputPlatform::stub_input_platform = nullptr;
++std::atomic<mtf::StubInputPlatform*> mtf::StubInputPlatform::stub_input_platform{nullptr};
  std::vector<std::weak_ptr<mir::input::InputDevice>> mtf::StubInputPlatform::device_store;
 === modified file 'tests/mir_test_framework/stub_input_platform.h'
 --- tests/mir_test_framework/stub_input_platform.h	2015-04-09 08:57:24 +0000
 +++ tests/mir_test_framework/stub_input_platform.h	2015-08-11 01:44:53 +0000
@@ -19,7 +19,7 @@
  #define MIR_TEST_FRAMEWORK_STUB_INPUT_PLATFORM_H_
  #include "mir/input/platform.h"
--#include <mutex>
++#include <atomic>
  #include <memory>
  #include <vector>
@@ -53,7 +53,7 @@
  private:
      std::shared_ptr<mir::dispatch::ActionQueue> const platform_queue;
      std::shared_ptr<mir::input::InputDeviceRegistry> const registry;
--    static StubInputPlatform* stub_input_platform;
++    static std::atomic<StubInputPlatform*> stub_input_platform;
      static std::vector<std::weak_ptr<mir::input::InputDevice>> device_store;
  };
 === modified file 'tests/unit-tests/test_glib_main_loop.cpp'
 --- tests/unit-tests/test_glib_main_loop.cpp	2015-08-06 16:19:28 +0000
 +++ tests/unit-tests/test_glib_main_loop.cpp	2015-08-11 01:44:53 +0000
@@ -24,6 +24,7 @@
  #include "mir/test/pipe.h"
  #include "mir/test/fake_shared.h"
  #include "mir/test/auto_unblock_thread.h"
++#include "mir/test/barrier.h"
  #include "mir/test/doubles/advanceable_clock.h"
  #include "mir/test/doubles/mock_lockable_callback.h"
  #include "mir_test_framework/process.h"
@@ -1095,6 +1096,71 @@
      EXPECT_THAT(num_triggers, Eq(expected_triggers));
+ }
++TEST_F(GLibMainLoopAlarmTest, rescheduling_alarm_from_within_alarm_callback_doesnt_deadlock_with_external_reschedule)
++{
++    using namespace testing;
++    using namespace std::literals::chrono_literals;
++
++    mt::Signal in_alarm;
++    mt::Signal alarm_rescheduled;
++
++    std::shared_ptr<mir::time::Alarm> alarm = ml.create_alarm(
++        [&]
++        {
++            // Ensure that the external thread reschedules us while we're
++            // in the callback.
++            in_alarm.raise();
++            ASSERT_TRUE(alarm_rescheduled.wait_for(5s));
++
++            alarm->reschedule_in(0ms);
++
++            ml.stop();
++        });
++
++    alarm->reschedule_in(0ms);
++
++    mt::AutoJoinThread rescheduler{
++        [alarm, &in_alarm, &alarm_rescheduled]()
++        {
++            ASSERT_TRUE(in_alarm.wait_for(5s));
++            alarm->reschedule_in(0ms);
++            alarm_rescheduled.raise();
++        }};
++
++    ml.run();
++}
++
++TEST_F(GLibMainLoopAlarmTest, cancel_blocks_until_definitely_cancelled)
++{
++    using namespace testing;
++    using namespace std::literals::chrono_literals;
++
++    auto waiting_in_lock = std::make_shared<mt::Barrier>(2);
++    auto has_been_called = std::make_shared<mt::Signal>();
++
++    std::shared_ptr<mir::time::Alarm> alarm = ml.create_alarm(
++        [waiting_in_lock, has_been_called]()
++        {
++            waiting_in_lock->ready();
++            std::this_thread::sleep_for(500ms);
++            has_been_called->raise();
++        });
++
++    alarm->reschedule_in(0ms);
++
++    mt::AutoJoinThread canceller{
++        [waiting_in_lock, has_been_called, alarm, this]()
++        {
++            waiting_in_lock->ready();
++            alarm->cancel();
++            EXPECT_TRUE(has_been_called->raised());
++            ml.stop();
++        }
++    };
++
++    ml.run();
++}
++
  // More targeted regression test for LP: #1381925
  TEST_F(GLibMainLoopTest, stress_emits_alarm_notification_with_zero_timeout)
+ {