Merge lp:~andreas-pokorny/mir/rebase-fix-1550050 into lp:mir

[Andreas Pokorny]

Fixes the potential endless repeat on unplugged devices (LP: #1550050)

The key repeat dispatcher now reacts on the removal of input devices by resetting potentially still active repeat alarms. The integration here is a bit clumsy, since the input device hub needs the input dispatcher to forward device events. While the input dispatcher needs it to register at the input device hub as an observer.

This fix registers for device changes and then stops sending repeat events for removed devices.

A different approach to this one which requires adding a two step initialization will be proposed soon, but requires an ABI change in the input platform interface.

[Mir CI Bot (mir-ci-bot)]

PASSED: Continuous integration, rev:3386
https://mir-jenkins.ubuntu.com/job/mir-ci/560/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/448
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/478
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/470
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/470
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/457
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/457/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/457
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/457/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/457
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/457/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/457
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/457/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/457
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/457/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/560/rebuild

review: Approve (continuous-integration)

[Cemil Azizoglu (cemil-azizoglu)]

LGTM

review: Approve

[Daniel van Vugt (vanvugt)]

The version is incorrect. Should be 0.21.0 in lp:mir

Also the changelog is incorrect for the same reason.

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Separate MPs are up to fix the above issues. You can just remove your version changes here...

review: Needs Fixing

[Andreas Pokorny (andreas-pokorny)]

ok removed the 'unreleased' changelog entry.
My understanding was that we will make another 0.20.3 release using what is currently in lp:mir - since there are no ABI breaks yet.

[Mir CI Bot (mir-ci-bot)]

FAILED: Continuous integration, rev:3387
https://mir-jenkins.ubuntu.com/job/mir-ci/572/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/467/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/497
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/489
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/489
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/476
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/476/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/476/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/476
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/476/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/476
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/476/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/476
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/476/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/572/rebuild

review: Needs Fixing (continuous-integration)

[Daniel van Vugt (vanvugt)]

lp:mir is series 0.21, which is clear in the launchpad config:
https://launchpad.net/mir

So lp:mir should always call itself series 0.21 until such time as that's released and we're working on series 0.22.

If you want to release a 0.20.3 then that must occur in lp:mir/0.20.

review: Abstain

[Andreas Pokorny (andreas-pokorny)]

Agreed. Depending on where we release from we do not need this MP!

review: Needs Information

[Alan Griffiths (alan-griffiths)]

We probably don't need this on trunk. Either we:

1. release 0.20.x from mir/0.20 - which already has this fix;
2. release a better solution (which I understand will break ABI) as 0.21; or,
3. we release 0.21 with this solution and apply the better solution in 0.22

I think we intend to release 0.20.3 and don't need this (yet).

review: Disapprove

[Mir CI Bot (mir-ci-bot)]

FAILED: Continuous integration, rev:3387
https://mir-jenkins.ubuntu.com/job/mir-ci/578/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/475/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/505
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/497
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/497
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/484
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/484/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/484
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/484/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/484
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/484/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/484
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/484/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/484/console

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/578/rebuild

review: Needs Fixing (continuous-integration)

[Alan Griffiths (alan-griffiths)]

> We probably don't need this on trunk. Either we:
>
> 1. release 0.20.x from mir/0.20 - which already has this fix;
> 2. release a better solution (which I understand will break ABI) as 0.21; or,
> 3. we release 0.21 with this solution and apply the better solution in 0.22
>
> I think we intend to release 0.20.3 and don't need this (yet).

...we went for option 3

review: Approve

[Daniel van Vugt (vanvugt)]

The failure is due to more random display configuration API failures. We've had a lot of them lately.

=== modified file 'src/server/input/default_configuration.cpp'
--- src/server/input/default_configuration.cpp	2016-03-09 11:19:15 +0000
+++ src/server/input/default_configuration.cpp	2016-03-15 07:45:18 +0000
@@ -295,12 +295,18 @@
     return default_input_device_hub(
         [this]()
         {
-            return std::make_shared<mi::DefaultInputDeviceHub>(
+            auto input_dispatcher = the_input_dispatcher();
+            auto key_repeater = std::dynamic_pointer_cast<mi::KeyRepeatDispatcher>(input_dispatcher);
+            auto hub = std::make_shared<mi::DefaultInputDeviceHub>(
                 the_global_event_sink(),
                 the_seat(),
                 the_input_reading_multiplexer(),
                 the_main_loop(),
                 the_cookie_authority());
+
+            if (key_repeater)
+                key_repeater->set_input_device_hub(hub);
+            return hub;
         });
 }
 
@@ -309,11 +315,17 @@
     return default_input_device_hub(
         [this]()
         {
-            return std::make_shared<mi::DefaultInputDeviceHub>(
+            auto input_dispatcher = the_input_dispatcher();
+            auto key_repeater = std::dynamic_pointer_cast<mi::KeyRepeatDispatcher>(input_dispatcher);
+            auto hub = std::make_shared<mi::DefaultInputDeviceHub>(
                 the_global_event_sink(),
                 the_seat(),
                 the_input_reading_multiplexer(),
                 the_main_loop(),
                 the_cookie_authority());
+
+            if (key_repeater)
+                key_repeater->set_input_device_hub(hub);
+            return hub;
         });
 }

=== modified file 'src/server/input/default_input_device_hub.cpp'
--- src/server/input/default_input_device_hub.cpp	2016-03-08 20:43:41 +0000
+++ src/server/input/default_input_device_hub.cpp	2016-03-15 07:45:18 +0000
@@ -244,17 +244,23 @@
 void mi::DefaultInputDeviceHub::remove_device_handle(MirInputDeviceId id)
 {
     std::unique_lock<std::mutex> lock(observer_guard);
-    auto handle_it = remove_if(begin(handles),
-                               end(handles),
-                               [&id](auto const& handle){return handle->id() == id;});
+    auto handle_it = remove_if(
+        begin(handles),
+        end(handles),
+        [this,&id](auto const& handle)
+        {
+            if (handle->id() != id)
+                return false;
+            for (auto const& observer : observers)
+            {
+                observer->device_removed(handle);
+                observer->changes_complete();
+            }
+            return true;
+        });
 
     if (handle_it == end(handles))
         return;
-    for (auto const& observer : observers)
-    {
-        observer->device_removed(*handle_it);
-        observer->changes_complete();
-    }
 
     handles.erase(handle_it, end(handles));
     sink->handle_input_device_change(handles);

=== modified file 'src/server/input/key_repeat_dispatcher.cpp'
--- src/server/input/key_repeat_dispatcher.cpp	2016-01-29 08:18:22 +0000
+++ src/server/input/key_repeat_dispatcher.cpp	2016-03-15 07:45:18 +0000
@@ -18,6 +18,8 @@
 
 #include "key_repeat_dispatcher.h"
 
+#include "mir/input/device.h"
+#include "mir/input/input_device_hub.h"
 #include "mir/time/alarm_factory.h"
 #include "mir/time/alarm.h"
 #include "mir/events/event_private.h"
@@ -31,6 +33,34 @@
 
 namespace mi = mir::input;
 
+namespace
+{
+struct DeviceRemovalFilter : mi::InputDeviceObserver
+{
+    DeviceRemovalFilter(std::function<void(MirInputDeviceId)> const& on_removal)
+        : on_removal(on_removal) {}
+
+    void device_added(std::shared_ptr<mi::Device> const&) override
+    {
+    }
+
+    void device_changed(std::shared_ptr<mi::Device> const&) override
+    {
+    }
+
+    void device_removed(std::shared_ptr<mi::Device> const& device) override
+    {
+        on_removal(device->id());
+    }
+
+    void changes_complete() override
+    {
+    }
+    std::function<void(MirInputDeviceId)> on_removal;
+};
+
+}
+
 mi::KeyRepeatDispatcher::KeyRepeatDispatcher(
     std::shared_ptr<mi::InputDispatcher> const& next_dispatcher,
     std::shared_ptr<mir::time::AlarmFactory> const& factory,
@@ -47,6 +77,17 @@
 {
 }
 
+void mi::KeyRepeatDispatcher::set_input_device_hub(std::shared_ptr<InputDeviceHub> const& hub)
+{
+    hub->add_observer(std::make_shared<DeviceRemovalFilter>(
+            [this](MirInputDeviceId id)
+            {
+                std::unique_lock<std::mutex> lock(repeat_state_mutex);
+                repeat_state_by_device.erase(id); // destructor cancels alarms
+            }
+            ));
+}
+
 mi::KeyRepeatDispatcher::KeyboardState& mi::KeyRepeatDispatcher::ensure_state_for_device_locked(std::lock_guard<std::mutex> const&, MirInputDeviceId id)
 {
     repeat_state_by_device.insert(std::make_pair(id, KeyboardState()));

=== modified file 'src/server/input/key_repeat_dispatcher.h'
--- src/server/input/key_repeat_dispatcher.h	2016-01-29 08:18:22 +0000
+++ src/server/input/key_repeat_dispatcher.h	2016-03-15 07:45:18 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2015 Canonical Ltd.
+ * Copyright © 2015-2016 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,
@@ -20,6 +20,7 @@
 #define MIR_INPUT_KEY_REPEAT_DISPATCHER_H_
 
 #include "mir/input/input_dispatcher.h"
+#include "mir/input/input_device_observer.h"
 
 #include <memory>
 #include <chrono>
@@ -39,8 +40,8 @@
 }
 namespace input
 {
-
-class KeyRepeatDispatcher : public mir::input::InputDispatcher
+class InputDeviceHub;
+class KeyRepeatDispatcher : public InputDispatcher
 {
 public:
     KeyRepeatDispatcher(std::shared_ptr<InputDispatcher> const& next_dispatcher,
@@ -54,7 +55,9 @@
     bool dispatch(MirEvent const& event) override;
     void start() override;
     void stop() override;
-    
+
+    void set_input_device_hub(std::shared_ptr<InputDeviceHub> const& hub);
+
 private:
     std::mutex repeat_state_mutex;
 

=== modified file 'tests/unit-tests/input/test_key_repeat_dispatcher.cpp'
--- tests/unit-tests/input/test_key_repeat_dispatcher.cpp	2016-01-29 08:18:22 +0000
+++ tests/unit-tests/input/test_key_repeat_dispatcher.cpp	2016-03-15 07:45:18 +0000
@@ -23,9 +23,15 @@
 #include "mir/time/alarm.h"
 #include "mir/time/alarm_factory.h"
 #include "mir/cookie/authority.h"
+#include "mir/input/input_device_observer.h"
+#include "mir/input/pointer_configuration.h"
+#include "mir/input/touchpad_configuration.h"
+#include "mir/input/device.h"
 
+#include "mir/test/fake_shared.h"
 #include "mir/test/event_matchers.h"
 #include "mir/test/doubles/mock_input_dispatcher.h"
+#include "mir/test/doubles/mock_input_device_hub.h"
 
 #include <gtest/gtest.h>
 #include <gmock/gmock.h>
@@ -35,6 +41,8 @@
 namespace mt = mir::test;
 namespace mtd = mt::doubles;
 
+using namespace ::testing;
+
 namespace
 {
 struct MockAlarm : public mir::time::Alarm
@@ -43,6 +51,12 @@
     MOCK_CONST_METHOD0(state, mir::time::Alarm::State());
     MOCK_METHOD1(reschedule_in, bool(std::chrono::milliseconds));
     MOCK_METHOD1(reschedule_for, bool(mir::time::Timestamp));
+
+    // destructor cancels the alarm
+    ~MockAlarm()
+    {
+        cancel();
+    }
 };
 
 struct MockAlarmFactory : public mir::time::AlarmFactory
@@ -59,25 +73,60 @@
     }
 };
 
+struct StubDevice : public mi::Device
+{
+    MirInputDeviceId device_id;
+    StubDevice(MirInputDeviceId id) : device_id(id) {}
+    MirInputDeviceId id() const { return device_id;}
+    mi::DeviceCapabilities capabilities() const {return mi::DeviceCapability::keyboard;}
+    std::string name() const {return {};}
+    std::string unique_id() const {return {};}
+
+    mir::optional_value<mi::PointerConfiguration> pointer_configuration() const {return {};}
+    void apply_pointer_configuration(mi::PointerConfiguration const&) {;}
+    mir::optional_value<mi::TouchpadConfiguration> touchpad_configuration() const {return {};}
+    void apply_touchpad_configuration(mi::TouchpadConfiguration const&) {}
+};
+
 struct KeyRepeatDispatcher : public testing::Test
 {
     KeyRepeatDispatcher()
         : dispatcher(mock_next_dispatcher, mock_alarm_factory, cookie_authority, true, repeat_time, repeat_delay)
     {
-    }
+        ON_CALL(hub,add_observer(_)).WillByDefault(SaveArg<0>(&observer));
+        dispatcher.set_input_device_hub(mt::fake_shared(hub));
+    }
+    void simulate_device_removal()
+    {
+        StubDevice dev(test_device);
+        observer->device_removed(mt::fake_shared(dev));
+        observer->changes_complete();
+    }
+
+    const MirInputDeviceId test_device = 123;
     std::shared_ptr<mtd::MockInputDispatcher> mock_next_dispatcher = std::make_shared<mtd::MockInputDispatcher>();
     std::shared_ptr<MockAlarmFactory> mock_alarm_factory = std::make_shared<MockAlarmFactory>();
     std::shared_ptr<mir::cookie::Authority> cookie_authority = mir::cookie::Authority::create();
     std::chrono::milliseconds const repeat_time{2};
     std::chrono::milliseconds const repeat_delay{1};
+    std::shared_ptr<mi::InputDeviceObserver> observer;
+    NiceMock<mtd::MockInputDeviceHub> hub;
     mi::KeyRepeatDispatcher dispatcher;
+
+    mir::EventUPtr a_key_down_event()
+    {
+        return mev::make_event(test_device, std::chrono::nanoseconds(0), std::vector<uint8_t>{}, mir_keyboard_action_down, 0, 0, mir_input_event_modifier_alt);
+    }
+
+    mir::EventUPtr a_key_up_event()
+    {
+        return mev::make_event(test_device, std::chrono::nanoseconds(0), std::vector<uint8_t>{}, mir_keyboard_action_up, 0, 0, mir_input_event_modifier_alt);
+    }
 };
 }
 
 TEST_F(KeyRepeatDispatcher, forwards_start_stop)
 {
-    using namespace ::testing;
-
     InSequence seq;
     EXPECT_CALL(*mock_next_dispatcher, start()).Times(1);
     EXPECT_CALL(*mock_next_dispatcher, stop()).Times(1);
@@ -86,22 +135,8 @@
     dispatcher.stop();
 }
 
-namespace
-{
-mir::EventUPtr a_key_down_event()
-{
-    return mev::make_event(0, std::chrono::nanoseconds(0), std::vector<uint8_t>{}, mir_keyboard_action_down, 0, 0, mir_input_event_modifier_alt);
-}
-mir::EventUPtr a_key_up_event()
-{
-    return mev::make_event(0, std::chrono::nanoseconds(0), std::vector<uint8_t>{}, mir_keyboard_action_up, 0, 0, mir_input_event_modifier_alt);
-}
-}
-
 TEST_F(KeyRepeatDispatcher, schedules_alarm_to_repeat_key_down)
 {
-    using namespace ::testing;
-    
     MockAlarm *mock_alarm = new MockAlarm; // deleted by AlarmFactory
     std::function<void()> alarm_function;
 
@@ -122,3 +157,28 @@
     // Trigger the cancel
     dispatcher.dispatch(*a_key_up_event());
 }
+
+TEST_F(KeyRepeatDispatcher, stops_repeat_on_device_removal)
+{
+    MockAlarm *mock_alarm = new MockAlarm;
+    std::function<void()> alarm_function;
+    bool alarm_canceled = false;
+
+    InSequence seq;
+    EXPECT_CALL(*mock_alarm_factory, create_alarm_adapter(_)).Times(1).
+        WillOnce(DoAll(SaveArg<0>(&alarm_function), Return(mock_alarm)));
+    // Once for initial down and again when invoked
+    EXPECT_CALL(*mock_alarm, reschedule_in(repeat_time)).Times(1).WillOnce(Return(true));
+    EXPECT_CALL(*mock_next_dispatcher, dispatch(mt::KeyDownEvent())).Times(1);
+    EXPECT_CALL(*mock_next_dispatcher, dispatch(mt::KeyRepeatEvent())).Times(1);
+    EXPECT_CALL(*mock_alarm, reschedule_in(repeat_delay)).Times(1).WillOnce(Return(true));
+    ON_CALL(*mock_alarm, cancel()).WillByDefault(Invoke([&](){alarm_canceled = true; return true;}));
+
+    dispatcher.dispatch(*a_key_down_event());
+
+    alarm_function();
+    Mock::VerifyAndClearExpectations(mock_alarm); // mock_alarm will be deleted after this
+
+    simulate_device_removal();
+    EXPECT_THAT(alarm_canceled, Eq(true));
+}