Mir

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

Proposed by Andreas Pokorny
Status: Merged
Approved by: Daniel van Vugt
Approved revision: no longer in the source branch.
Merged at revision: 4114
Proposed branch: lp:~andreas-pokorny/mir/fix-1672955
Merge into: lp:mir
Prerequisite: lp:~andreas-pokorny/mir/store-device-config
Diff against target: 308 lines (+99/-33)
6 files modified
src/server/input/default_device.cpp (+30/-15)
src/server/input/default_device.h (+2/-1)
src/server/input/default_input_device_hub.cpp (+19/-12)
src/server/input/default_input_device_hub.h (+6/-5)
tests/unit-tests/input/test_default_device.cpp (+23/-0)
tests/unit-tests/input/test_default_input_device_hub.cpp (+19/-0)
To merge this branch: bzr merge lp:~andreas-pokorny/mir/fix-1672955
Reviewer Review Type Date Requested Status
Kevin DuBois (community) Approve
Alan Griffiths Approve
Alexandros Frantzis (community) Approve
Mir CI Bot continuous-integration Approve
Review via email: mp+320066@code.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

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)
Revision history for this message
Alexandros Frantzis (afrantzis) wrote :

Looks good.

review: Approve
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

LGTM

review: Approve
Revision history for this message
Kevin DuBois (kdub) wrote :

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

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
150 {150 {
151 std::lock_guard<std::mutex> lock(config_mutex);151 std::lock_guard<std::mutex> lock(config_mutex);
152 pointer = settings;152 pointer = settings;
153 if (!actions) // device is disabled
154 return;
155 actions->enqueue([settings = std::move(settings), dev=&device]
156 {
157 dev->apply_settings(settings);
158 });
153 }159 }
154
155 actions->enqueue([settings = std::move(settings), dev=&device]
156 {
157 dev->apply_settings(settings);
158 });
159}160}
160161
161void mi::DefaultDevice::apply_touchpad_configuration(MirTouchpadConfig const& conf)162void mi::DefaultDevice::apply_touchpad_configuration(MirTouchpadConfig const& conf)
@@ -188,12 +189,13 @@
188 {189 {
189 std::lock_guard<std::mutex> lock(config_mutex);190 std::lock_guard<std::mutex> lock(config_mutex);
190 touchpad = settings;191 touchpad = settings;
192 if (!actions) // device is disabled
193 return;
194 actions->enqueue([settings = std::move(settings), dev=&device]
195 {
196 dev->apply_settings(settings);
197 });
191 }198 }
192
193 actions->enqueue([settings = std::move(settings), dev=&device]
194 {
195 dev->apply_settings(settings);
196 });
197}199}
198200
199mir::optional_value<MirKeyboardConfig> mi::DefaultDevice::keyboard_configuration() const201mir::optional_value<MirKeyboardConfig> mi::DefaultDevice::keyboard_configuration() const
@@ -214,6 +216,8 @@
214void mi::DefaultDevice::set_keyboard_configuration(MirKeyboardConfig const& conf)216void mi::DefaultDevice::set_keyboard_configuration(MirKeyboardConfig const& conf)
215{217{
216 std::lock_guard<std::mutex> lock(config_mutex);218 std::lock_guard<std::mutex> lock(config_mutex);
219 if (!actions) // device is disabled
220 return;
217 if (keyboard.value().device_keymap() != conf.device_keymap())221 if (keyboard.value().device_keymap() != conf.device_keymap())
218 keyboard = conf;222 keyboard = conf;
219 else223 else
@@ -246,12 +250,17 @@
246 settings.output_id = config.output_id();250 settings.output_id = config.output_id();
247 settings.mapping_mode = config.mapping_mode();251 settings.mapping_mode = config.mapping_mode();
248252
249 touchscreen = settings;253 {
254 std::lock_guard<std::mutex> lock(config_mutex);
255 touchscreen = settings;
250256
251 actions->enqueue([settings = std::move(settings), dev=&device]257 if (!actions) // device is disabled
252 {258 return;
253 dev->apply_settings(settings);259 actions->enqueue([settings = std::move(settings), dev=&device]
254 });260 {
261 dev->apply_settings(settings);
262 });
263 }
255}264}
256265
257MirInputDevice mi::DefaultDevice::config() const266MirInputDevice mi::DefaultDevice::config() const
@@ -276,3 +285,9 @@
276285
277 return stored_dev;286 return stored_dev;
278}287}
288
289void mi::DefaultDevice::disable_queue()
290{
291 std::lock_guard<std::mutex> lock(config_mutex);
292 actions.reset();
293}
279294
=== 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 @@
78 void apply_touchscreen_configuration(MirTouchscreenConfig const&) override;78 void apply_touchscreen_configuration(MirTouchscreenConfig const&) override;
7979
80 MirInputDevice config() const;80 MirInputDevice config() const;
81 void disable_queue();
81private:82private:
82 void set_pointer_configuration(MirPointerConfig const&);83 void set_pointer_configuration(MirPointerConfig const&);
83 void set_touchpad_configuration(MirTouchpadConfig const&);84 void set_touchpad_configuration(MirTouchpadConfig const&);
@@ -92,7 +93,7 @@
92 optional_value<TouchpadSettings> touchpad;93 optional_value<TouchpadSettings> touchpad;
93 optional_value<MirKeyboardConfig> keyboard;94 optional_value<MirKeyboardConfig> keyboard;
94 optional_value<TouchscreenSettings> touchscreen;95 optional_value<TouchscreenSettings> touchscreen;
95 std::shared_ptr<dispatch::ActionQueue> const actions;96 std::shared_ptr<dispatch::ActionQueue> actions;
96 std::shared_ptr<KeyMapper> const key_mapper;97 std::shared_ptr<KeyMapper> const key_mapper;
97 std::function<void(Device*)> device_changed_callback;98 std::function<void(Device*)> device_changed_callback;
98 std::mutex mutable config_mutex;99 std::mutex mutable config_mutex;
99100
=== 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 @@
187187
188 if (it == end(devices))188 if (it == end(devices))
189 {189 {
190 auto handle = restore_or_create_device(*device);190 auto queue = std::make_shared<dispatch::ActionQueue>();
191 auto handle = restore_or_create_device(*device, queue);
191 // send input device info to observer loop..192 // send input device info to observer loop..
192 devices.push_back(std::make_unique<RegisteredDevice>(193 devices.push_back(std::make_unique<RegisteredDevice>(
193 device, handle->id(), input_dispatchable, cookie_authority, handle));194 device, handle->id(), queue, cookie_authority, handle));
194195
195 auto const& dev = devices.back();196 auto const& dev = devices.back();
196 add_device_handle(handle);197 add_device_handle(handle);
197198
198 seat->add_device(*handle);199 seat->add_device(*handle);
199 dev->start(seat);200 dev->start(seat, input_dispatchable);
200 }201 }
201 else202 else
202 {203 {
@@ -222,7 +223,7 @@
222 if (seat)223 if (seat)
223 {224 {
224 seat->remove_device(*item->handle);225 seat->remove_device(*item->handle);
225 item->stop();226 item->stop(input_dispatchable);
226 }227 }
227 remove_device_handle(item->id());228 remove_device_handle(item->id());
228229
@@ -242,14 +243,14 @@
242mi::DefaultInputDeviceHub::RegisteredDevice::RegisteredDevice(243mi::DefaultInputDeviceHub::RegisteredDevice::RegisteredDevice(
243 std::shared_ptr<InputDevice> const& dev,244 std::shared_ptr<InputDevice> const& dev,
244 MirInputDeviceId device_id,245 MirInputDeviceId device_id,
245 std::shared_ptr<dispatch::MultiplexingDispatchable> const& multiplexer,246 std::shared_ptr<dispatch::ActionQueue> const& queue,
246 std::shared_ptr<mir::cookie::Authority> const& cookie_authority,247 std::shared_ptr<mir::cookie::Authority> const& cookie_authority,
247 std::shared_ptr<mi::DefaultDevice> const& handle)248 std::shared_ptr<mi::DefaultDevice> const& handle)
248 : handle(handle),249 : handle(handle),
249 device_id(device_id),250 device_id(device_id),
250 cookie_authority(cookie_authority),251 cookie_authority(cookie_authority),
251 device(dev),252 device(dev),
252 multiplexer(multiplexer)253 queue(queue)
253{254{
254}255}
255256
@@ -282,17 +283,23 @@
282 return dev == device;283 return dev == device;
283}284}
284285
285void mi::DefaultInputDeviceHub::RegisteredDevice::start(std::shared_ptr<Seat> const& seat)286void mi::DefaultInputDeviceHub::RegisteredDevice::start(std::shared_ptr<Seat> const& seat, std::shared_ptr<dispatch::MultiplexingDispatchable> const& multiplexer)
286{287{
288 multiplexer->add_watch(queue);
289
287 this->seat = seat;290 this->seat = seat;
288 builder = std::make_unique<DefaultEventBuilder>(device_id, cookie_authority, seat);291 builder = std::make_unique<DefaultEventBuilder>(device_id, cookie_authority, seat);
289 device->start(this, builder.get());292 device->start(this, builder.get());
290}293}
291294
292void mi::DefaultInputDeviceHub::RegisteredDevice::stop()295void mi::DefaultInputDeviceHub::RegisteredDevice::stop(std::shared_ptr<dispatch::MultiplexingDispatchable> const& multiplexer)
293{296{
297 multiplexer->remove_watch(queue);
298 handle->disable_queue();
299
294 device->stop();300 device->stop();
295 seat = nullptr;301 seat = nullptr;
302 queue.reset();
296 builder.reset();303 builder.reset();
297}304}
298305
@@ -516,22 +523,22 @@
516 return optional_config;523 return optional_config;
517}524}
518525
519std::shared_ptr<mi::DefaultDevice>526std::shared_ptr<mi::DefaultDevice> mi::DefaultInputDeviceHub::restore_or_create_device(
520mi::DefaultInputDeviceHub::restore_or_create_device(mi::InputDevice& device)527 mi::InputDevice& device, std::shared_ptr<mir::dispatch::ActionQueue> const& queue)
521{528{
522 auto device_config = get_stored_device_config(device.get_device_info().unique_id);529 auto device_config = get_stored_device_config(device.get_device_info().unique_id);
523530
524 if (device_config.is_set())531 if (device_config.is_set())
525 return std::make_shared<DefaultDevice>(532 return std::make_shared<DefaultDevice>(
526 device_config.value(),533 device_config.value(),
527 device_queue,534 queue,
528 device,535 device,
529 key_mapper,536 key_mapper,
530 [this](Device *d){device_changed(d);});537 [this](Device *d){device_changed(d);});
531 else538 else
532 return std::make_shared<DefaultDevice>(539 return std::make_shared<DefaultDevice>(
533 create_new_device_id(),540 create_new_device_id(),
534 device_queue,541 queue,
535 device,542 device,
536 key_mapper,543 key_mapper,
537 [this](Device *d){device_changed(d);});544 [this](Device *d){device_changed(d);});
538545
=== 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 @@
105 void emit_changed_devices();105 void emit_changed_devices();
106 MirInputDeviceId create_new_device_id();106 MirInputDeviceId create_new_device_id();
107 void store_device_config(DefaultDevice const& dev);107 void store_device_config(DefaultDevice const& dev);
108 std::shared_ptr<DefaultDevice> restore_or_create_device(InputDevice& dev);108 std::shared_ptr<DefaultDevice> restore_or_create_device(InputDevice& dev,
109 std::shared_ptr<dispatch::ActionQueue> const& queue);
109 mir::optional_value<MirInputDevice> get_stored_device_config(std::string const& id);110 mir::optional_value<MirInputDevice> get_stored_device_config(std::string const& id);
110111
111 std::shared_ptr<Seat> const seat;112 std::shared_ptr<Seat> const seat;
@@ -121,15 +122,15 @@
121 public:122 public:
122 RegisteredDevice(std::shared_ptr<InputDevice> const& dev,123 RegisteredDevice(std::shared_ptr<InputDevice> const& dev,
123 MirInputDeviceId dev_id,124 MirInputDeviceId dev_id,
124 std::shared_ptr<dispatch::MultiplexingDispatchable> const& multiplexer,125 std::shared_ptr<dispatch::ActionQueue> const& multiplexer,
125 std::shared_ptr<cookie::Authority> const& cookie_authority,126 std::shared_ptr<cookie::Authority> const& cookie_authority,
126 std::shared_ptr<DefaultDevice> const& handle);127 std::shared_ptr<DefaultDevice> const& handle);
127 void handle_input(MirEvent& event) override;128 void handle_input(MirEvent& event) override;
128 geometry::Rectangle bounding_rectangle() const override;129 geometry::Rectangle bounding_rectangle() const override;
129 input::OutputInfo output_info(uint32_t output_id) const override;130 input::OutputInfo output_info(uint32_t output_id) const override;
130 bool device_matches(std::shared_ptr<InputDevice> const& dev) const;131 bool device_matches(std::shared_ptr<InputDevice> const& dev) const;
131 void start(std::shared_ptr<Seat> const& seat);132 void start(std::shared_ptr<Seat> const& seat, std::shared_ptr<dispatch::MultiplexingDispatchable> const& dispatchable);
132 void stop();133 void stop(std::shared_ptr<dispatch::MultiplexingDispatchable> const& dispatchable);
133 MirInputDeviceId id();134 MirInputDeviceId id();
134 std::shared_ptr<Seat> seat;135 std::shared_ptr<Seat> seat;
135 const std::shared_ptr<DefaultDevice> handle;136 const std::shared_ptr<DefaultDevice> handle;
@@ -141,7 +142,7 @@
141 std::unique_ptr<DefaultEventBuilder> builder;142 std::unique_ptr<DefaultEventBuilder> builder;
142 std::shared_ptr<cookie::Authority> cookie_authority;143 std::shared_ptr<cookie::Authority> cookie_authority;
143 std::shared_ptr<InputDevice> const device;144 std::shared_ptr<InputDevice> const device;
144 std::shared_ptr<dispatch::MultiplexingDispatchable> const multiplexer;145 std::shared_ptr<dispatch::ActionQueue> queue;
145 };146 };
146147
147 std::vector<std::shared_ptr<Device>> handles;148 std::vector<std::shared_ptr<Device>> handles;
148149
=== 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 @@
308308
309 EXPECT_EQ(ts_config, dev.touchscreen_configuration().value());309 EXPECT_EQ(ts_config, dev.touchscreen_configuration().value());
310}310}
311
312TEST_F(DefaultDevice, disable_queue_ends_config_processing)
313{
314 mi::DefaultDevice dev(MirInputDeviceId{17}, queue, touchpad, mt::fake_shared(key_mapper));
315
316 dev.disable_queue();
317 MirPointerConfig pointer_conf;
318 pointer_conf.cursor_acceleration_bias(1.0);
319
320 EXPECT_CALL(touchpad, apply_settings(Matcher<mi::TouchpadSettings const&>(_))).Times(0);
321 dev.apply_pointer_configuration(pointer_conf);
322}
323
324TEST_F(DefaultDevice, disable_queue_removes_reference_to_queue)
325{
326 std::weak_ptr<md::ActionQueue> ref = queue;
327 mi::DefaultDevice dev(MirInputDeviceId{17}, queue, touchpad, mt::fake_shared(key_mapper));
328 queue.reset();
329
330 EXPECT_THAT(ref.lock(), Ne(nullptr));
331 dev.disable_queue();
332 EXPECT_THAT(ref.lock(), Eq(nullptr));
333}
311334
=== 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 @@
340340
341 EXPECT_THAT(dev_ptr->pointer_configuration().value(), Eq(ptr_config));341 EXPECT_THAT(dev_ptr->pointer_configuration().value(), Eq(ptr_config));
342}342}
343
344TEST_F(InputDeviceHubTest, no_device_config_action_after_device_removal)
345{
346 std::shared_ptr<mi::Device> dev_ptr;
347 MirPointerConfig ptr_config;
348 ptr_config.cursor_acceleration_bias(0.5);
349
350 ON_CALL(mock_observer, device_added(WithName("mouse"))).WillByDefault(SaveArg<0>(&dev_ptr));
351
352 hub.add_device(mt::fake_shared(mouse));
353 hub.add_observer(mt::fake_shared(mock_observer));
354 expect_and_execute_multiplexer();
355
356 EXPECT_CALL(mouse, apply_settings(Matcher<mi::PointerSettings const&>(_))).Times(0);
357
358 dev_ptr->apply_pointer_configuration(ptr_config);
359 hub.remove_device(mt::fake_shared(mouse));
360 expect_and_execute_multiplexer();
361}

Subscribers

People subscribed via source and target branches