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

[Andreas Pokorny]

Fixes the potential endless repeat on unplugged devices

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 is the small version of the fix. It makes the KeyRepeatDispatcher observe the input devices attached and resets the repeat handling alarms on device removal.

The changes in src/server/input/default_configuration.cpp were necessary since there is already a dependency path from InputDeviceHub to InputDispatcher. I plan to make a different fix on lp:mir that moves the repeat handling directly to the device/platform itself.

[Andreas Pokorny (andreas-pokorny)]

The effect of the branch is sometimes hidden by the fact that some bluetooth drivers keep track for input states and release keys when unplugging the device. The problem is easier to reproduce with USB devices.

[Kevin DuBois (kdub)]

src/server/input/default_configuration.cpp

- [this]()
+ [this]() -> std::shared_ptr<mi::InputDispatcher>

- the_event_filter_chain_dispatcher(), the_main_loop(), the_cookie_authority(),
- enable_repeat, key_repeat_timeout, key_repeat_delay);
+ the_event_filter_chain_dispatcher(), the_main_loop(), the_cookie_authority(),
+ enable_repeat, key_repeat_timeout, key_repeat_delay);

unneeded changes?

+ void set_input_device_hub(std::shared_ptr<InputDeviceHub> const& hub);
two step initialization.

review: Needs Fixing

[Andreas Pokorny (andreas-pokorny)]

> src/server/input/default_configuration.cpp
>
> - [this]()
> + [this]() -> std::shared_ptr<mi::InputDispatcher>
>
> - the_event_filter_chain_dispatcher(), the_main_loop(),
> the_cookie_authority(),
> - enable_repeat, key_repeat_timeout, key_repeat_delay);
> + the_event_filter_chain_dispatcher(), the_main_loop(),
> the_cookie_authority(),
> + enable_repeat, key_repeat_timeout, key_repeat_delay);
>
> unneeded changes?

yes reverting that..

>
> + void set_input_device_hub(std::shared_ptr<InputDeviceHub> const& hub);
> two step initialization.

This is one of the reasons why I make a better solution into lp:mir - it will turn into a bigger change and somewhat turn into something that no longer looks like simple fix.
Two step init is necessary to avoid a dependency cycle between 'the intput dispatchers' and InputDeviceHub.

[Kevin DuBois (kdub)]

I suppose I don't mind as much if the two step initialization will be improved before going to trunk.

It would be good (necessary?) to see a CI pass on the MP before merging as the release. I suppose all that adds up to a +1 from me then.

review: Approve

[Andreas Pokorny (andreas-pokorny)]

> I suppose I don't mind as much if the two step initialization will be improved
> before going to trunk.
>
> It would be good (necessary?) to see a CI pass on the MP before merging as the
> release. I suppose all that adds up to a +1 from me then.

Yeah our ci has yet no way of running against a release branch.. Maybe a manually triggered run could work

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

FAILED: Continuous integration, rev:3334
https://mir-jenkins.ubuntu.com/job/mir-ci/503/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/345/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-0-fetch/375/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/367/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/367/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/354/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/354/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/354/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/354/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/354/console

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

review: Needs Fixing (continuous-integration)

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

FAILED: Continuous integration, rev:3334
https://mir-jenkins.ubuntu.com/job/mir-ci/504/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/346/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-0-fetch/376/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/368/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/368/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/355/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/355/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/355/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/355/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/355/console

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

review: Needs Fixing (continuous-integration)

[Andreas Pokorny (andreas-pokorny)]

ok does not work that way..

=== added file 'include/test/mir/test/doubles/mock_input_device_hub.h'
--- include/test/mir/test/doubles/mock_input_device_hub.h	1970-01-01 00:00:00 +0000
+++ include/test/mir/test/doubles/mock_input_device_hub.h	2016-03-03 16:17:57 +0000
@@ -0,0 +1,43 @@
+/*
+ * Copyright © 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,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Andreas Pokorny <[e-mail]>
+ */
+
+#ifndef MIR_TEST_DOUBLES_MOCK_INPUT_DEVICE_HUB_H_
+#define MIR_TEST_DOUBLES_MOCK_INPUT_DEVICE_HUB_H_
+
+#include "mir/input/input_device_hub.h"
+
+namespace mir
+{
+namespace test
+{
+namespace doubles
+{
+
+struct MockInputDeviceHub : input::InputDeviceHub
+{
+    MOCK_METHOD1(add_observer, void(std::shared_ptr<input::InputDeviceObserver> const&));
+    MOCK_METHOD1(remove_observer, void(std::weak_ptr<input::InputDeviceObserver> const&));
+    MOCK_METHOD1(for_each_input_device, void(std::function<void(std::shared_ptr<input::Device>const&)> const&));
+
+};
+
+}
+}
+}
+
+#endif

=== modified file 'src/server/input/default_configuration.cpp'
--- src/server/input/default_configuration.cpp	2016-02-12 10:51:29 +0000
+++ src/server/input/default_configuration.cpp	2016-03-03 16:17:57 +0000
@@ -270,33 +270,46 @@
 
 std::shared_ptr<mi::InputDeviceRegistry> mir::DefaultServerConfiguration::the_input_device_registry()
 {
-    return default_input_device_hub([this]()
-                                    {
-                                        return std::make_shared<mi::DefaultInputDeviceHub>(
-                                            std::make_shared<mi::BasicSeat>(
-                                                the_input_dispatcher(),
-                                                the_touch_visualizer(),
-                                                the_cursor_listener(),
-                                                the_input_region()),
-                                            the_input_reading_multiplexer(),
-                                            the_main_loop(),
-                                            the_cookie_authority());
-                                    });
+    return default_input_device_hub(
+        [this]()
+        {
+            auto input_dispatcher = the_input_dispatcher();
+            auto key_repeater = std::dynamic_pointer_cast<mi::KeyRepeatDispatcher>(input_dispatcher);
+            auto hub = std::make_shared<mi::DefaultInputDeviceHub>(
+                std::make_shared<mi::BasicSeat>(
+                    input_dispatcher,
+                    the_touch_visualizer(),
+                    the_cursor_listener(),
+                    the_input_region()),
+                the_input_reading_multiplexer(),
+                the_main_loop(),
+                the_cookie_authority());
+
+            if (key_repeater)
+                key_repeater->set_input_device_hub(hub);
+            return hub;
+        });
 }
 
 std::shared_ptr<mi::InputDeviceHub> mir::DefaultServerConfiguration::the_input_device_hub()
 {
-    return default_input_device_hub([this]()
-                                    {
-                                        return std::make_shared<mi::DefaultInputDeviceHub>(
-std::make_shared<mi::BasicSeat>(
-                                                the_input_dispatcher(),
-                                                the_touch_visualizer(),
-                                                the_cursor_listener(),
-                                                the_input_region()),
+    return default_input_device_hub(
+        [this]()
+        {
+            auto input_dispatcher = the_input_dispatcher();
+            auto key_repeater = std::dynamic_pointer_cast<mi::KeyRepeatDispatcher>(input_dispatcher);
+            auto hub = std::make_shared<mi::DefaultInputDeviceHub>(
+                std::make_shared<mi::BasicSeat>(
+                    input_dispatcher,
+                    the_touch_visualizer(),
+                    the_cursor_listener(),
+                    the_input_region()),
+                the_input_reading_multiplexer(),
+                the_main_loop(),
+                the_cookie_authority());
 
-                                            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-01-29 15:55:17 +0000
+++ src/server/input/default_input_device_hub.cpp	2016-03-03 16:17:57 +0000
@@ -228,17 +228,23 @@
 
 void mi::DefaultInputDeviceHub::remove_device_handle(MirInputDeviceId id)
 {
-    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));
 }

=== 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-03 16:17:57 +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-03 16:17:57 +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-03 16:17:57 +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));
+}