Mir

Merge lp:~alan-griffiths/mir/preserve-ExternalInputDeviceHub into lp:mir

preserve-ExternalInputDeviceHub
Merge into development-branch

Proposed by Alan Griffiths on 2017-04-04

Status:	Merged
Approved by:	Alan Griffiths on 2017-04-06
Approved revision:	4145
Merged at revision:	4142
Proposed branch:	lp:~alan-griffiths/mir/preserve-ExternalInputDeviceHub
Merge into:	lp:mir
Diff against target:	146 lines (+40/-9) 7 files modified src/server/input/config_changer.cpp (+3/-1) src/server/input/config_changer.h (+4/-1) src/server/input/default_configuration.cpp (+3/-1) src/server/input/default_input_device_hub.cpp (+21/-1) tests/acceptance-tests/test_client_input.cpp (+5/-3) tests/integration-tests/input/test_single_seat_setup.cpp (+2/-1) tests/unit-tests/input/test_config_changer.cpp (+2/-1)
To merge this branch:	bzr merge lp:~alan-griffiths/mir/preserve-ExternalInputDeviceHub
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Mir CI Bot	continuous-integration		Approve on 2017-04-06
Alberto Aguirre			Approve on 2017-04-06
Alan Griffiths			Abstain on 2017-04-06
Andreas Pokorny (community)		2017-04-04	Approve on 2017-04-04
Review via email: mp+321845@code.launchpad.net

Commit Message

Recent changes mean that the_input_device_hub() ownership was not retained by the server. Fix it.

Andreas Pokorny (andreas-pokorny) wrote on 2017-04-04:

ok .. the DO_NOT_USE is a bit excessive..

review: Approve

Mir CI Bot (mir-ci-bot) wrote on 2017-04-04:

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

review: Needs Fixing (continuous-integration)

Alan Griffiths (alan-griffiths) wrote on 2017-04-04:

Logged as lp:1679713 - but two architectures on the same MP?

Mir CI Bot (mir-ci-bot) wrote on 2017-04-04:

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

review: Needs Fixing (continuous-integration)

Alan Griffiths (alan-griffiths) wrote on 2017-04-04:

The problem code manifests in TestClientInput::wait_for_input_devices()

It appears that (as the hub now lives longer than the function scope) there is a callback presumably after the scope is left that accesses memory that is no longer valid.

Not sure how this happens (unless there's an exception) as remove_observer() is invoked before the function exits.

That should not happen, so the problem seems to lie in mi::ExternalInputDeviceHub (but I don't see where yet).

review: Needs Fixing

Mir CI Bot (mir-ci-bot) wrote on 2017-04-04:

FAILED: Continuous integration, rev:4141
https://mir-jenkins.ubuntu.com/job/mir-ci/3303/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/4465/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/4580
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/4570
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/4570
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/4570
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/4570

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

review: Needs Fixing (continuous-integration)

Mir CI Bot (mir-ci-bot) wrote on 2017-04-05:

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

review: Needs Fixing (continuous-integration)

Andreas Pokorny (andreas-pokorny) wrote on 2017-04-05:

> The problem code manifests in TestClientInput::wait_for_input_devices()
>
> It appears that (as the hub now lives longer than the function scope) there is
> a callback presumably after the scope is left that accesses memory that is no
> longer valid.
>
> Not sure how this happens (unless there's an exception) as remove_observer()
> is invoked before the function exits.
>
> That should not happen, so the problem seems to lie in
> mi::ExternalInputDeviceHub (but I don't see where yet).

I suspect a race between mir::GLibMainLoop::pause_processing_for and mir::GLibMainLoop::should_process_actions_for.. when it happens in this order

Mir/Input: trigger notifications due to device removal in DefaultInputDeviceHub
Mir/Input: ExternalInputDeviceHub::changes_complete which enqueues an action into the MainLoop
MirMainLoop: GlibMainLoop::should_process_actions_for
main-thread: pause_processing_for and destructor of ExternalInputDeviceHub::Internal
MirMainLoop: action()

Mir CI Bot (mir-ci-bot) wrote on 2017-04-05:

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

review: Needs Fixing (continuous-integration)

Alan Griffiths (alan-griffiths) wrote on 2017-04-06:

OK, finally looked at the right line of code.

ExternalInputDeviceHub::add_observer() enqueues "data->observers.add(observer)", whereas ExternalInputDeviceHub::remove_observer() executes "data->observers.remove(observer)" on the calling thread.

So there's no ordering that guarantees that "add(observer)" happens-before "remove(observer)". And if that order is inverted... BOOM

Alan Griffiths (alan-griffiths) on 2017-04-06:

review: Abstain

Mir CI Bot (mir-ci-bot) wrote on 2017-04-06:

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

review: Approve (continuous-integration)

Mir CI Bot (mir-ci-bot) wrote on 2017-04-06:

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

review: Approve (continuous-integration)

Alberto Aguirre (albaguirre) wrote on 2017-04-06:

Makes sense..

Sleep is still there.

lp:~alan-griffiths/mir/preserve-ExternalInputDeviceHub updated on 2017-04-06

4145. By Alan Griffiths on 2017-04-06: Remove test code

Alan Griffiths (alan-griffiths) wrote on 2017-04-06:

> Makes sense..
>
> Sleep is still there.

Thanks.

Alberto Aguirre (albaguirre) wrote on 2017-04-06:

LGTM

review: Approve

Mir CI Bot (mir-ci-bot) wrote on 2017-04-06:

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

review: Approve (continuous-integration)

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:

Alan Griffiths

Emanuele Antonio Faraone

Gerry Boland

Mir development team

1	=== modified file 'src/server/input/config_changer.cpp'
2	--- src/server/input/config_changer.cpp 2017-03-20 11:26:22 +0000
3	+++ src/server/input/config_changer.cpp 2017-04-06 13:43:37 +0000
4	@@ -144,11 +144,13 @@
5	std::shared_ptr<InputManager> const& manager,
6	std::shared_ptr<InputDeviceHub> const& devices,
7	std::shared_ptr<scene::SessionContainer> const& session_container,
8	- std::shared_ptr<scene::SessionEventHandlerRegister> const& session_event_handler_register)
9	+ std::shared_ptr<scene::SessionEventHandlerRegister> const& session_event_handler_register,
10	+ std::shared_ptr<InputDeviceHub> const& devices_wrapper)
11	: input_manager{manager},
12	devices{devices},
13	session_container{session_container},
14	session_event_handler_register{session_event_handler_register},
15	+ devices_wrapper_DO_NOT_USE{devices_wrapper},
16	device_observer(std::make_shared<DeviceChangeTracker>(*this)),
17	base_configuration_applied(true)
18	{
19
20	=== modified file 'src/server/input/config_changer.h'
21	--- src/server/input/config_changer.h 2017-01-13 12:33:24 +0000
22	+++ src/server/input/config_changer.h 2017-04-06 13:43:37 +0000
23	@@ -47,7 +47,8 @@
24	ConfigChanger(std::shared_ptr<InputManager> const& input_manager,
25	std::shared_ptr<InputDeviceHub> const& devices,
26	std::shared_ptr<scene::SessionContainer> const& session_container,
27	- std::shared_ptr<scene::SessionEventHandlerRegister> const& session_event_handler_register);
28	+ std::shared_ptr<scene::SessionEventHandlerRegister> const& session_event_handler_register,
29	+ std::shared_ptr<InputDeviceHub> const& devices_wrapper);
30	~ConfigChanger();
31	MirInputConfig base_configuration() override;
32	void configure(std::shared_ptr<frontend::Session> const&, MirInputConfig &&) override;
33	@@ -66,6 +67,8 @@
34	std::shared_ptr<InputDeviceHub> const devices;
35	std::shared_ptr<scene::SessionContainer> const session_container;
36	std::shared_ptr<scene::SessionEventHandlerRegister> const session_event_handler_register;
37	+ // needs to be owned (but not used) in Mir, where better?
38	+ std::shared_ptr<InputDeviceHub> const devices_wrapper_DO_NOT_USE;
39	std::shared_ptr<InputDeviceObserver> const device_observer;
40	bool base_configuration_applied;
41
42
43	=== modified file 'src/server/input/default_configuration.cpp'
44	--- src/server/input/default_configuration.cpp 2017-03-24 19:37:47 +0000
45	+++ src/server/input/default_configuration.cpp 2017-04-06 13:43:37 +0000
46	@@ -367,7 +367,9 @@
47	return input_configuration_changer(
48	[this]()
49	{
50	- return std::make_shared<mi::ConfigChanger>(the_input_manager(), the_default_input_device_hub(), the_session_container(), the_session_event_handler_register());
51	+ return std::make_shared<mi::ConfigChanger>(
52	+ the_input_manager(), the_default_input_device_hub(), the_session_container(),
53	+ the_session_event_handler_register(), the_input_device_hub());
54	}
55	);
56	}
57
58	=== modified file 'src/server/input/default_input_device_hub.cpp'
59	--- src/server/input/default_input_device_hub.cpp 2017-04-04 07:04:57 +0000
60	+++ src/server/input/default_input_device_hub.cpp 2017-04-06 13:43:37 +0000
61	@@ -87,7 +87,27 @@
62	{
63	auto observer = obs.lock();
64	if (observer)
65	- data->observers.remove(observer);
66	+ {
67	+ std::mutex mutex;
68	+ std::condition_variable cv;
69	+ std::unique_lock<decltype(mutex)> lock{mutex};
70	+ bool removed{false};
71	+
72	+ data->observer_queue->enqueue(
73	+ data.get(),
74	+ [&,this]
75	+ {
76	+ // Unless remove() is on the same thread as add() external synchronization would be needed
77	+ data->observers.remove(observer);
78	+
79	+ std::lock_guard<decltype(mutex)> lock{mutex};
80	+ removed = true;
81	+ cv.notify_one();
82	+ });
83	+
84	+ // Before returning wait for the remove - otherwise notifications can still happen
85	+ cv.wait(lock, [&] { return removed; });
86	+ }
87	}
88
89	void mi::ExternalInputDeviceHub::for_each_input_device(std::function<void(Device const& device)> const& callback)
90
91	=== modified file 'tests/acceptance-tests/test_client_input.cpp'
92	--- tests/acceptance-tests/test_client_input.cpp 2017-03-27 22:18:17 +0000
93	+++ tests/acceptance-tests/test_client_input.cpp 2017-04-06 13:43:37 +0000
94	@@ -26,6 +26,7 @@
95	#include "mir/input/mir_input_config.h"
96	#include "mir/input/input_device.h"
97	#include "mir/input/touchscreen_settings.h"
98	+#include <mir/raii.h>
99
100	#include "mir_test_framework/headless_in_process_server.h"
101	#include "mir_test_framework/fake_input_device.h"
102	@@ -344,12 +345,13 @@
103	});
104
105	auto hub = server.the_input_device_hub();
106	- hub->add_observer(counter);
107	+
108	+ auto const register_counter = mir::raii::paired_calls(
109	+ [&]{ hub->add_observer(counter); },
110	+ [&]{ hub->remove_observer(counter); });
111
112	devices_available.wait_for(5s);
113	ASSERT_THAT(counter->count_devices, Eq(expected_number_of_input_devices));
114	-
115	- hub->remove_observer(counter);
116	}
117
118	MirInputDevice const* get_device_with_capabilities(MirInputConfig const* config, MirInputDeviceCapabilities caps)
119
120	=== modified file 'tests/integration-tests/input/test_single_seat_setup.cpp'
121	--- tests/integration-tests/input/test_single_seat_setup.cpp 2017-03-14 16:17:51 +0000
122	+++ tests/integration-tests/input/test_single_seat_setup.cpp 2017-04-06 13:43:37 +0000
123	@@ -140,7 +140,8 @@
124	mt::fake_shared(mock_input_manager),
125	mt::fake_shared(hub),
126	mt::fake_shared(stub_session_container),
127	- mt::fake_shared(session_event_sink)
128	+ mt::fake_shared(session_event_sink),
129	+ mt::fake_shared(hub)
130	};
131
132	mi::DeviceCapabilities const keyboard_caps = mi::DeviceCapability::keyboard \| mi::DeviceCapability::alpha_numeric;
133
134	=== modified file 'tests/unit-tests/input/test_config_changer.cpp'
135	--- tests/unit-tests/input/test_config_changer.cpp 2017-03-14 16:17:51 +0000
136	+++ tests/unit-tests/input/test_config_changer.cpp 2017-04-06 13:43:37 +0000
137	@@ -112,7 +112,8 @@
138	ms::BroadcastingSessionEventSink session_event_sink;
139
140	mi::ConfigChanger changer{mt::fake_shared(mock_input_manager), mt::fake_shared(hub),
141	- mt::fake_shared(stub_session_container), mt::fake_shared(session_event_sink)};
142	+ mt::fake_shared(stub_session_container), mt::fake_shared(session_event_sink),
143	+ mt::fake_shared(hub)};
144
145	auto get_full_device_conf(MirInputDeviceId id,
146	mi::DeviceCapabilities caps,

Mir

Merge lp:~alan-griffiths/mir/preserve-ExternalInputDeviceHub into lp:mir

Commit Message

Description of the Change

Preview Diff

Subscribers