Merge lp:~andreas-pokorny/mir/fix-1672955 into lp:mir

[Andreas Pokorny]

Ensure that no action is still queued that may access the mir::input::InputDevice (LP: #1672955)

Shared pointers to mir::input::Device are handed out to other threads and may still be used after the mir::input::InputDevice was removed. To avoid this problem the ActionQueues for manipulating input devices are now per device and will be removed with the InputDevice

The attached bug was caused by actions in the queue that try to apply configuration while the device was already removed. mir::input::Device does not to keep the platform devices alive. Changing that to shared_ptr does not really improve the situation as we would keep the device alive until the queue is processed. Instead this change will remove the queue instead such that outstanding actions are canceled. All that happens inside the input thread..

Proposing that on top of lp:~andreas-pokorny/mir/store-device-config since it would conflict otherwise.

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

PASSED: Continuous integration, rev:4074
https://mir-jenkins.ubuntu.com/job/mir-ci/3180/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/4273
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/4360
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/4350
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/4350
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/4350
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4300
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4300/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/4300
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/4300/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4300
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4300/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/4300
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/4300/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/4300
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/4300/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/4300
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/4300/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

Looks good.

review: Approve

[Alan Griffiths (alan-griffiths)]

LGTM

review: Approve

[Kevin DuBois (kdub)]

lgtm too, would be nice to note bug in code comments somewhere

review: Approve

=== modified file 'src/server/input/default_device.cpp'
--- src/server/input/default_device.cpp	2017-03-16 15:10:51 +0000
+++ src/server/input/default_device.cpp	2017-03-16 15:10:52 +0000
@@ -150,12 +150,13 @@
     {
         std::lock_guard<std::mutex> lock(config_mutex);
         pointer = settings;
+        if (!actions) // device is disabled
+            return;
+        actions->enqueue([settings = std::move(settings), dev=&device]
+                         {
+                             dev->apply_settings(settings);
+                         });
     }
-
-    actions->enqueue([settings = std::move(settings), dev=&device]
-                     {
-                         dev->apply_settings(settings);
-                     });
 }
 
 void mi::DefaultDevice::apply_touchpad_configuration(MirTouchpadConfig const& conf)
@@ -188,12 +189,13 @@
     {
         std::lock_guard<std::mutex> lock(config_mutex);
         touchpad = settings;
+        if (!actions) // device is disabled
+            return;
+        actions->enqueue([settings = std::move(settings), dev=&device]
+                         {
+                             dev->apply_settings(settings);
+                         });
     }
-
-    actions->enqueue([settings = std::move(settings), dev=&device]
-                     {
-                         dev->apply_settings(settings);
-                     });
 }
 
 mir::optional_value<MirKeyboardConfig> mi::DefaultDevice::keyboard_configuration() const
@@ -214,6 +216,8 @@
 void mi::DefaultDevice::set_keyboard_configuration(MirKeyboardConfig const& conf)
 {
     std::lock_guard<std::mutex> lock(config_mutex);
+    if (!actions) // device is disabled
+        return;
     if (keyboard.value().device_keymap() != conf.device_keymap())
         keyboard = conf;
     else
@@ -246,12 +250,17 @@
     settings.output_id = config.output_id();
     settings.mapping_mode = config.mapping_mode();
 
-    touchscreen = settings;
+    {
+        std::lock_guard<std::mutex> lock(config_mutex);
+        touchscreen = settings;
 
-    actions->enqueue([settings = std::move(settings), dev=&device]
-                     {
-                         dev->apply_settings(settings);
-                     });
+        if (!actions) // device is disabled
+            return;
+        actions->enqueue([settings = std::move(settings), dev=&device]
+                         {
+                             dev->apply_settings(settings);
+                         });
+    }
 }
 
 MirInputDevice mi::DefaultDevice::config() const
@@ -276,3 +285,9 @@
 
     return stored_dev;
 }
+
+void mi::DefaultDevice::disable_queue()
+{
+    std::lock_guard<std::mutex> lock(config_mutex);
+    actions.reset();
+}

=== modified file 'src/server/input/default_device.h'
--- src/server/input/default_device.h	2017-03-16 15:10:51 +0000
+++ src/server/input/default_device.h	2017-03-16 15:10:52 +0000
@@ -78,6 +78,7 @@
     void apply_touchscreen_configuration(MirTouchscreenConfig const&) override;
 
     MirInputDevice config() const;
+    void disable_queue();
 private:
     void set_pointer_configuration(MirPointerConfig const&);
     void set_touchpad_configuration(MirTouchpadConfig const&);
@@ -92,7 +93,7 @@
     optional_value<TouchpadSettings> touchpad;
     optional_value<MirKeyboardConfig> keyboard;
     optional_value<TouchscreenSettings> touchscreen;
-    std::shared_ptr<dispatch::ActionQueue> const actions;
+    std::shared_ptr<dispatch::ActionQueue> actions;
     std::shared_ptr<KeyMapper> const key_mapper;
     std::function<void(Device*)> device_changed_callback;
     std::mutex mutable config_mutex;

=== modified file 'src/server/input/default_input_device_hub.cpp'
--- src/server/input/default_input_device_hub.cpp	2017-03-16 15:10:51 +0000
+++ src/server/input/default_input_device_hub.cpp	2017-03-16 15:10:52 +0000
@@ -187,16 +187,17 @@
 
     if (it == end(devices))
     {
-        auto handle = restore_or_create_device(*device);
+        auto queue = std::make_shared<dispatch::ActionQueue>();
+        auto handle = restore_or_create_device(*device, queue);
         // send input device info to observer loop..
         devices.push_back(std::make_unique<RegisteredDevice>(
-            device, handle->id(), input_dispatchable, cookie_authority, handle));
+            device, handle->id(), queue, cookie_authority, handle));
 
         auto const& dev = devices.back();
         add_device_handle(handle);
 
         seat->add_device(*handle);
-        dev->start(seat);
+        dev->start(seat, input_dispatchable);
     }
     else
     {
@@ -222,7 +223,7 @@
                 if (seat)
                 {
                     seat->remove_device(*item->handle);
-                    item->stop();
+                    item->stop(input_dispatchable);
                 }
                 remove_device_handle(item->id());
 
@@ -242,14 +243,14 @@
 mi::DefaultInputDeviceHub::RegisteredDevice::RegisteredDevice(
     std::shared_ptr<InputDevice> const& dev,
     MirInputDeviceId device_id,
-    std::shared_ptr<dispatch::MultiplexingDispatchable> const& multiplexer,
+    std::shared_ptr<dispatch::ActionQueue> const& queue,
     std::shared_ptr<mir::cookie::Authority> const& cookie_authority,
     std::shared_ptr<mi::DefaultDevice> const& handle)
     : handle(handle),
       device_id(device_id),
       cookie_authority(cookie_authority),
       device(dev),
-      multiplexer(multiplexer)
+      queue(queue)
 {
 }
 
@@ -282,17 +283,23 @@
     return dev == device;
 }
 
-void mi::DefaultInputDeviceHub::RegisteredDevice::start(std::shared_ptr<Seat> const& seat)
+void mi::DefaultInputDeviceHub::RegisteredDevice::start(std::shared_ptr<Seat> const& seat, std::shared_ptr<dispatch::MultiplexingDispatchable> const& multiplexer)
 {
+    multiplexer->add_watch(queue);
+
     this->seat = seat;
     builder = std::make_unique<DefaultEventBuilder>(device_id, cookie_authority, seat);
     device->start(this, builder.get());
 }
 
-void mi::DefaultInputDeviceHub::RegisteredDevice::stop()
+void mi::DefaultInputDeviceHub::RegisteredDevice::stop(std::shared_ptr<dispatch::MultiplexingDispatchable> const& multiplexer)
 {
+    multiplexer->remove_watch(queue);
+    handle->disable_queue();
+
     device->stop();
     seat = nullptr;
+    queue.reset();
     builder.reset();
 }
 
@@ -516,22 +523,22 @@
     return optional_config;
 }
 
-std::shared_ptr<mi::DefaultDevice>
-mi::DefaultInputDeviceHub::restore_or_create_device(mi::InputDevice& device)
+std::shared_ptr<mi::DefaultDevice> mi::DefaultInputDeviceHub::restore_or_create_device(
+    mi::InputDevice& device, std::shared_ptr<mir::dispatch::ActionQueue> const& queue)
 {
     auto device_config = get_stored_device_config(device.get_device_info().unique_id);
 
     if (device_config.is_set())
         return std::make_shared<DefaultDevice>(
             device_config.value(),
-            device_queue,
+            queue,
             device,
             key_mapper,
             [this](Device *d){device_changed(d);});
     else
         return std::make_shared<DefaultDevice>(
             create_new_device_id(),
-            device_queue,
+            queue,
             device,
             key_mapper,
             [this](Device *d){device_changed(d);});

=== modified file 'src/server/input/default_input_device_hub.h'
--- src/server/input/default_input_device_hub.h	2017-03-16 15:10:51 +0000
+++ src/server/input/default_input_device_hub.h	2017-03-16 15:10:52 +0000
@@ -105,7 +105,8 @@
     void emit_changed_devices();
     MirInputDeviceId create_new_device_id();
     void store_device_config(DefaultDevice const& dev);
-    std::shared_ptr<DefaultDevice> restore_or_create_device(InputDevice& dev);
+    std::shared_ptr<DefaultDevice> restore_or_create_device(InputDevice& dev,
+                                                            std::shared_ptr<dispatch::ActionQueue> const& queue);
     mir::optional_value<MirInputDevice> get_stored_device_config(std::string const& id);
 
     std::shared_ptr<Seat> const seat;
@@ -121,15 +122,15 @@
     public:
         RegisteredDevice(std::shared_ptr<InputDevice> const& dev,
                          MirInputDeviceId dev_id,
-                         std::shared_ptr<dispatch::MultiplexingDispatchable> const& multiplexer,
+                         std::shared_ptr<dispatch::ActionQueue> const& multiplexer,
                          std::shared_ptr<cookie::Authority> const& cookie_authority,
                          std::shared_ptr<DefaultDevice> const& handle);
         void handle_input(MirEvent& event) override;
         geometry::Rectangle bounding_rectangle() const override;
         input::OutputInfo output_info(uint32_t output_id) const override;
         bool device_matches(std::shared_ptr<InputDevice> const& dev) const;
-        void start(std::shared_ptr<Seat> const& seat);
-        void stop();
+        void start(std::shared_ptr<Seat> const& seat, std::shared_ptr<dispatch::MultiplexingDispatchable> const& dispatchable);
+        void stop(std::shared_ptr<dispatch::MultiplexingDispatchable> const& dispatchable);
         MirInputDeviceId id();
         std::shared_ptr<Seat> seat;
         const std::shared_ptr<DefaultDevice> handle;
@@ -141,7 +142,7 @@
         std::unique_ptr<DefaultEventBuilder> builder;
         std::shared_ptr<cookie::Authority> cookie_authority;
         std::shared_ptr<InputDevice> const device;
-        std::shared_ptr<dispatch::MultiplexingDispatchable> const multiplexer;
+        std::shared_ptr<dispatch::ActionQueue> queue;
     };
 
     std::vector<std::shared_ptr<Device>> handles;

=== modified file 'tests/unit-tests/input/test_default_device.cpp'
--- tests/unit-tests/input/test_default_device.cpp	2017-03-16 15:10:51 +0000
+++ tests/unit-tests/input/test_default_device.cpp	2017-03-16 15:10:52 +0000
@@ -308,3 +308,26 @@
 
     EXPECT_EQ(ts_config, dev.touchscreen_configuration().value());
 }
+
+TEST_F(DefaultDevice, disable_queue_ends_config_processing)
+{
+    mi::DefaultDevice dev(MirInputDeviceId{17}, queue, touchpad, mt::fake_shared(key_mapper));
+
+    dev.disable_queue();
+    MirPointerConfig pointer_conf;
+    pointer_conf.cursor_acceleration_bias(1.0);
+
+    EXPECT_CALL(touchpad, apply_settings(Matcher<mi::TouchpadSettings const&>(_))).Times(0);
+    dev.apply_pointer_configuration(pointer_conf);
+}
+
+TEST_F(DefaultDevice, disable_queue_removes_reference_to_queue)
+{
+    std::weak_ptr<md::ActionQueue> ref = queue;
+    mi::DefaultDevice dev(MirInputDeviceId{17}, queue, touchpad, mt::fake_shared(key_mapper));
+    queue.reset();
+
+    EXPECT_THAT(ref.lock(), Ne(nullptr));
+    dev.disable_queue();
+    EXPECT_THAT(ref.lock(), Eq(nullptr));
+}

=== modified file 'tests/unit-tests/input/test_default_input_device_hub.cpp'
--- tests/unit-tests/input/test_default_input_device_hub.cpp	2017-03-16 15:10:51 +0000
+++ tests/unit-tests/input/test_default_input_device_hub.cpp	2017-03-16 15:10:52 +0000
@@ -340,3 +340,22 @@
 
     EXPECT_THAT(dev_ptr->pointer_configuration().value(), Eq(ptr_config));
 }
+
+TEST_F(InputDeviceHubTest, no_device_config_action_after_device_removal)
+{
+    std::shared_ptr<mi::Device> dev_ptr;
+    MirPointerConfig ptr_config;
+    ptr_config.cursor_acceleration_bias(0.5);
+
+    ON_CALL(mock_observer, device_added(WithName("mouse"))).WillByDefault(SaveArg<0>(&dev_ptr));
+
+    hub.add_device(mt::fake_shared(mouse));
+    hub.add_observer(mt::fake_shared(mock_observer));
+    expect_and_execute_multiplexer();
+
+    EXPECT_CALL(mouse, apply_settings(Matcher<mi::PointerSettings const&>(_))).Times(0);
+
+    dev_ptr->apply_pointer_configuration(ptr_config);
+    hub.remove_device(mt::fake_shared(mouse));
+    expect_and_execute_multiplexer();
+}