Mir

Merge lp:~raof/mir/display-changer-callback-lifetime into lp:mir

Proposed by Chris Halse Rogers on 2017-08-18
Status: Rejected
Rejected by: Alan Griffiths on 2017-08-21
Proposed branch: lp:~raof/mir/display-changer-callback-lifetime
Merge into: lp:mir
Diff against target: 375 lines (+118/-104)
8 files modified
include/server/mir/scene/session_coordinator.h (+0/-2)
src/server/scene/default_configuration.cpp (+1/-2)
src/server/scene/mediating_display_changer.cpp (+97/-34)
src/server/scene/mediating_display_changer.h (+11/-1)
src/server/scene/session_manager.cpp (+0/-10)
src/server/scene/session_manager.h (+0/-3)
tests/unit-tests/scene/test_mediating_display_changer.cpp (+9/-9)
tests/unit-tests/scene/test_session_manager.cpp (+0/-43)
To merge this branch: bzr merge lp:~raof/mir/display-changer-callback-lifetime
Reviewer Review Type Date Requested Status
Mir CI Bot continuous-integration Approve on 2017-08-18
Alan Griffiths Needs Information on 2017-08-18
Chris Halse Rogers Needs Fixing on 2017-08-18
Review via email: mp+329241@code.launchpad.net

Commit message

ms::MediatingDisplayChanger: Ensure we live at least as long as our signal handlers.

MediatingDisplayChanger registers some handlers on the session_event_handler_register; these need to ensure that the display changer is actually live before invoking methods on it.

To post a comment you must log in.
Chris Halse Rogers (raof) wrote :

For some reason which is absolutely opaque to me this appears to cause the testsuite to crash with an invalid read from 0x0 when *exiting* from ConnectedClientWithASurface::SetUp(), when run under Valgrind and under load.

review: Needs Fixing
Alan Griffiths (alan-griffiths) wrote :

Oh, nasty! Unlike some other listener patterns we have the SessionEventHandlerRegister has no way to unregister listeners.

Is this approach the best approach to resolving the problem this causes?

review: Needs Information
Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:4232
https://mir-jenkins.ubuntu.com/job/mir-ci/3560/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/4879
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/5094
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=artful/5083
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/5083
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/5083
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=artful/4916
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=artful/4916/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4916
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4916/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=artful/4916
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=artful/4916/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/4916
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/4916/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4916
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4916/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=artful/4916
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=artful/4916/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=zesty/4916
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=zesty/4916/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/4916
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/4916/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)
Alan Griffiths (alan-griffiths) wrote :

- void add_listener(std::shared_ptr<SessionListener> const& listener) override;
- void remove_listener(std::shared_ptr<SessionListener> const& listener) override;
-

I guess this is an unrelated deletion of dead code?

Chris Halse Rogers (raof) wrote :

It's (hopefully) the most expedient. I also have a branch with much of the changes required to switch to a more classical observe with MultiplexingObserver done.

This was less effort. It, at least, would be, if it didn't mysteriously die under load.

Alan Griffiths (alan-griffiths) wrote :

> It's (hopefully) the most expedient. I also have a branch with much of the
> changes required to switch to a more classical observe with
> MultiplexingObserver done.
>
> This was less effort. It, at least, would be, if it didn't mysteriously die
> under load.

Well, using BasicObservers<> seems to both simplify the code and address the end-of-life issues:

    lp:~alan-griffiths/mir/rework-BroadcastingSessionEventSink/+merge/329255

Alan Griffiths (alan-griffiths) wrote :

We landed an alternative

Unmerged revisions

4232. By Chris Halse Rogers on 2017-08-18

ms::MediatingDisplayChanger: Ensure we live at least as long as our signal handlers.

MediatingDisplayChanger registers some handlers on the session_event_handler_register;
these need to ensure that the display changer is actually live before invoking methods on
it.

4231. By Chris Halse Rogers on 2017-08-18

ms::SessionCoordinator: Remove {add,remove}_listener methods.

These methods are used only by the tests which check their behaviour.

Sigh.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'include/server/mir/scene/session_coordinator.h'
2--- include/server/mir/scene/session_coordinator.h 2017-07-28 17:00:43 +0000
3+++ include/server/mir/scene/session_coordinator.h 2017-08-18 08:06:31 +0000
4@@ -54,8 +54,6 @@
5
6 virtual std::shared_ptr<Session> successor_of(std::shared_ptr<Session> const&) const = 0;
7
8- virtual void add_listener(std::shared_ptr<SessionListener> const&) = 0;
9- virtual void remove_listener(std::shared_ptr<SessionListener> const&) = 0;
10 protected:
11 SessionCoordinator() = default;
12 virtual ~SessionCoordinator() = default;
13
14=== modified file 'src/server/scene/default_configuration.cpp'
15--- src/server/scene/default_configuration.cpp 2017-07-28 17:00:43 +0000
16+++ src/server/scene/default_configuration.cpp 2017-08-18 08:06:31 +0000
17@@ -132,7 +132,7 @@
18 return mediating_display_changer(
19 [this]()
20 {
21- return std::make_shared<ms::MediatingDisplayChanger>(
22+ return ms::MediatingDisplayChanger::create(
23 the_display(),
24 the_compositor(),
25 the_display_configuration_policy(),
26@@ -142,7 +142,6 @@
27 the_display_configuration_observer(),
28 the_main_loop());
29 });
30-
31 }
32
33 std::shared_ptr<mf::DisplayChanger>
34
35=== modified file 'src/server/scene/mediating_display_changer.cpp'
36--- src/server/scene/mediating_display_changer.cpp 2017-08-09 15:03:31 +0000
37+++ src/server/scene/mediating_display_changer.cpp 2017-08-18 08:06:31 +0000
38@@ -124,6 +124,103 @@
39 };
40 }
41
42+std::shared_ptr<ms::MediatingDisplayChanger> ms::MediatingDisplayChanger::create(
43+ std::shared_ptr<mg::Display> const& display,
44+ std::shared_ptr<mc::Compositor> const& compositor,
45+ std::shared_ptr<mg::DisplayConfigurationPolicy> const& display_configuration_policy,
46+ std::shared_ptr<SessionContainer> const& session_container,
47+ std::shared_ptr<SessionEventHandlerRegister> const& session_event_handler_register,
48+ std::shared_ptr<ServerActionQueue> const& server_action_queue,
49+ std::shared_ptr<mg::DisplayConfigurationObserver> const& observer,
50+ std::shared_ptr<mt::AlarmFactory> const& alarm_factory)
51+{
52+ std::shared_ptr<ms::MediatingDisplayChanger> const result{
53+ new ms::MediatingDisplayChanger{
54+ display,
55+ compositor,
56+ display_configuration_policy,
57+ session_container,
58+ session_event_handler_register,
59+ server_action_queue,
60+ observer,
61+ alarm_factory
62+ }};
63+
64+ std::weak_ptr<ms::MediatingDisplayChanger> const weak_changer = result;
65+
66+ /*
67+ * Caution! Reference cycle!
68+ *
69+ * result owns a reference to session_event_handler_register;
70+ * if we capture a strong reference in the handler, then
71+ * session_Event_handler_register will have a strong reference to
72+ * result.
73+ */
74+ session_event_handler_register->register_focus_change_handler(
75+ [weak_changer](std::shared_ptr<ms::Session> const& session)
76+ {
77+ auto const weak_session = std::weak_ptr<ms::Session>(session);
78+ if (auto const changer = weak_changer.lock())
79+ {
80+ /*
81+ * Caution! Bonus reference cycle!
82+ *
83+ * As before, changer owns a reference to server_action_queue.
84+ */
85+ changer->server_action_queue->enqueue(
86+ changer.get(),
87+ [weak_changer, weak_session]
88+ {
89+ if (auto const changer = weak_changer.lock())
90+ {
91+ if (auto const session = weak_session.lock())
92+ {
93+ changer->focus_change_handler(session);
94+ }
95+ }
96+ });
97+ }
98+ });
99+
100+ session_event_handler_register->register_no_focus_handler(
101+ [weak_changer]
102+ {
103+ if (auto const changer = weak_changer.lock())
104+ {
105+ changer->server_action_queue->enqueue(
106+ changer.get(),
107+ [weak_changer]
108+ {
109+ if (auto const changer = weak_changer.lock())
110+ {
111+ changer->no_focus_handler();
112+ }
113+ });
114+ }
115+ });
116+
117+ session_event_handler_register->register_session_stopping_handler(
118+ [weak_changer](std::shared_ptr<ms::Session> const& session)
119+ {
120+ auto const weak_session = std::weak_ptr<ms::Session>(session);
121+ if (auto const changer = weak_changer.lock())
122+ {
123+ changer->server_action_queue->enqueue(
124+ changer.get(),
125+ [weak_changer,weak_session]
126+ {
127+ if (auto const changer = weak_changer.lock())
128+ {
129+ if (auto const session = weak_session.lock())
130+ changer->session_stopping_handler(session);
131+ }
132+ });
133+ }
134+ });
135+
136+ return result;
137+}
138+
139 ms::MediatingDisplayChanger::MediatingDisplayChanger(
140 std::shared_ptr<mg::Display> const& display,
141 std::shared_ptr<mc::Compositor> const& compositor,
142@@ -144,40 +241,6 @@
143 base_configuration_applied{true},
144 alarm_factory{alarm_factory}
145 {
146- session_event_handler_register->register_focus_change_handler(
147- [this](std::shared_ptr<ms::Session> const& session)
148- {
149- auto const weak_session = std::weak_ptr<ms::Session>(session);
150- this->server_action_queue->enqueue(
151- this,
152- [this,weak_session]
153- {
154- if (auto const session = weak_session.lock())
155- focus_change_handler(session);
156- });
157- });
158-
159- session_event_handler_register->register_no_focus_handler(
160- [this]
161- {
162- this->server_action_queue->enqueue(
163- this,
164- [this] { no_focus_handler(); });
165- });
166-
167- session_event_handler_register->register_session_stopping_handler(
168- [this](std::shared_ptr<ms::Session> const& session)
169- {
170- auto const weak_session = std::weak_ptr<ms::Session>(session);
171- this->server_action_queue->enqueue(
172- this,
173- [this,weak_session]
174- {
175- if (auto const session = weak_session.lock())
176- session_stopping_handler(session);
177- });
178- });
179-
180 observer->initial_configuration(base_configuration_);
181 }
182
183
184=== modified file 'src/server/scene/mediating_display_changer.h'
185--- src/server/scene/mediating_display_changer.h 2017-07-28 17:00:43 +0000
186+++ src/server/scene/mediating_display_changer.h 2017-08-18 08:06:31 +0000
187@@ -55,7 +55,7 @@
188 public shell::DisplayConfigurationController
189 {
190 public:
191- MediatingDisplayChanger(
192+ static std::shared_ptr<MediatingDisplayChanger> create(
193 std::shared_ptr<graphics::Display> const& display,
194 std::shared_ptr<compositor::Compositor> const& compositor,
195 std::shared_ptr<graphics::DisplayConfigurationPolicy> const& display_configuration_policy,
196@@ -92,6 +92,16 @@
197 void set_base_configuration(std::shared_ptr<graphics::DisplayConfiguration> const &conf) override;
198
199 private:
200+ MediatingDisplayChanger(
201+ std::shared_ptr<graphics::Display> const& display,
202+ std::shared_ptr<compositor::Compositor> const& compositor,
203+ std::shared_ptr<graphics::DisplayConfigurationPolicy> const& display_configuration_policy,
204+ std::shared_ptr<SessionContainer> const& session_container,
205+ std::shared_ptr<SessionEventHandlerRegister> const& session_event_handler_register,
206+ std::shared_ptr<ServerActionQueue> const& server_action_queue,
207+ std::shared_ptr<graphics::DisplayConfigurationObserver> const& observer,
208+ std::shared_ptr<time::AlarmFactory> const& alarm_factory);
209+
210 void focus_change_handler(std::shared_ptr<Session> const& session);
211 void no_focus_handler();
212 void session_stopping_handler(std::shared_ptr<Session> const& session);
213
214=== modified file 'src/server/scene/session_manager.cpp'
215--- src/server/scene/session_manager.cpp 2017-07-28 17:00:43 +0000
216+++ src/server/scene/session_manager.cpp 2017-08-18 08:06:31 +0000
217@@ -186,13 +186,3 @@
218 {
219 return app_container->successor_of(session);
220 }
221-
222-void ms::SessionManager::add_listener(std::shared_ptr<SessionListener> const& listener)
223-{
224- observers->register_interest(listener);
225-}
226-
227-void ms::SessionManager::remove_listener(std::shared_ptr<SessionListener> const& listener)
228-{
229- observers->unregister_interest(*listener);
230-}
231
232=== modified file 'src/server/scene/session_manager.h'
233--- src/server/scene/session_manager.h 2017-07-28 17:00:43 +0000
234+++ src/server/scene/session_manager.h 2017-08-18 08:06:31 +0000
235@@ -77,9 +77,6 @@
236 void set_focus_to(std::shared_ptr<Session> const& focus) override;
237 void unset_focus() override;
238
239- void add_listener(std::shared_ptr<SessionListener> const& listener) override;
240- void remove_listener(std::shared_ptr<SessionListener> const& listener) override;
241-
242 protected:
243 SessionManager(const SessionManager&) = delete;
244 SessionManager& operator=(const SessionManager&) = delete;
245
246=== modified file 'tests/unit-tests/scene/test_mediating_display_changer.cpp'
247--- tests/unit-tests/scene/test_mediating_display_changer.cpp 2017-08-09 11:30:59 +0000
248+++ tests/unit-tests/scene/test_mediating_display_changer.cpp 2017-08-18 08:06:31 +0000
249@@ -140,7 +140,7 @@
250 {
251 using namespace testing;
252
253- changer = std::make_shared<ms::MediatingDisplayChanger>(
254+ changer = ms::MediatingDisplayChanger::create(
255 mt::fake_shared(mock_display),
256 mt::fake_shared(mock_compositor),
257 mt::fake_shared(mock_conf_policy),
258@@ -688,7 +688,7 @@
259 stub_session_container.insert_session(session1);
260 stub_session_container.insert_session(session2);
261
262- ms::MediatingDisplayChanger display_changer(
263+ auto display_changer = ms::MediatingDisplayChanger::create(
264 mt::fake_shared(mock_display),
265 mt::fake_shared(mock_compositor),
266 mt::fake_shared(mock_conf_policy),
267@@ -706,11 +706,11 @@
268 Mock::VerifyAndClearExpectations(&mock_server_action_queue);
269
270 EXPECT_CALL(mock_server_action_queue, enqueue(owner, _));
271- display_changer.configure(session1, conf);
272+ display_changer->configure(session1, conf);
273 Mock::VerifyAndClearExpectations(&mock_server_action_queue);
274
275 EXPECT_CALL(mock_server_action_queue, enqueue(owner, _));
276- display_changer.configure_for_hardware_change(conf);
277+ display_changer->configure_for_hardware_change(conf);
278 Mock::VerifyAndClearExpectations(&mock_server_action_queue);
279
280 EXPECT_CALL(mock_server_action_queue, enqueue(owner, _));
281@@ -722,11 +722,11 @@
282 Mock::VerifyAndClearExpectations(&mock_server_action_queue);
283
284 EXPECT_CALL(mock_server_action_queue, pause_processing_for(owner));
285- display_changer.pause_display_config_processing();
286+ display_changer->pause_display_config_processing();
287 Mock::VerifyAndClearExpectations(&mock_server_action_queue);
288
289 EXPECT_CALL(mock_server_action_queue, resume_processing_for(owner));
290- display_changer.resume_display_config_processing();
291+ display_changer->resume_display_config_processing();
292 Mock::VerifyAndClearExpectations(&mock_server_action_queue);
293 }
294
295@@ -742,7 +742,7 @@
296 stub_session_container.insert_session(active_session);
297 stub_session_container.insert_session(inactive_session);
298
299- ms::MediatingDisplayChanger display_changer(
300+ auto display_changer = ms::MediatingDisplayChanger::create(
301 mt::fake_shared(mock_display),
302 mt::fake_shared(mock_compositor),
303 mt::fake_shared(mock_conf_policy),
304@@ -758,7 +758,7 @@
305
306 EXPECT_CALL(mock_server_action_queue, enqueue(_, _)).Times(0);
307
308- display_changer.configure(inactive_session, conf);
309+ display_changer->configure(inactive_session, conf);
310 }
311
312 TEST_F(MediatingDisplayChangerTest, set_base_configuration_doesnt_override_session_configuration)
313@@ -871,7 +871,7 @@
314 MOCK_METHOD1(base_configuration_updated, void (std::shared_ptr<mg::DisplayConfiguration const> const& base_config));
315 } display_configuration_observer;
316
317- changer = std::make_shared<ms::MediatingDisplayChanger>(
318+ changer = ms::MediatingDisplayChanger::create(
319 mt::fake_shared(mock_display),
320 mt::fake_shared(mock_compositor),
321 mt::fake_shared(mock_conf_policy),
322
323=== modified file 'tests/unit-tests/scene/test_session_manager.cpp'
324--- tests/unit-tests/scene/test_session_manager.cpp 2017-07-28 17:00:43 +0000
325+++ tests/unit-tests/scene/test_session_manager.cpp 2017-08-18 08:06:31 +0000
326@@ -184,49 +184,6 @@
327 session_manager.close_session(session);
328 }
329
330-TEST_F(SessionManagerSessionListenerSetup, additional_listeners_receive_session_callbacks)
331-{
332- using namespace ::testing;
333-
334- auto additional_listener = std::make_shared<testing::NiceMock<mtd::MockSessionListener>>();
335- EXPECT_CALL(*additional_listener, starting(_)).Times(1);
336- EXPECT_CALL(*additional_listener, stopping(_)).Times(1);
337-
338- session_manager.add_listener(additional_listener);
339- auto session = session_manager.open_session(__LINE__, "XPlane", std::shared_ptr<mf::EventSink>());
340- session_manager.close_session(session);
341-}
342-
343-TEST_F(SessionManagerSessionListenerSetup, additional_listeners_receive_focus_changes)
344-{
345- using namespace ::testing;
346-
347- auto additional_listener = std::make_shared<testing::NiceMock<mtd::MockSessionListener>>();
348- EXPECT_CALL(*additional_listener, starting(_)).Times(1);
349- EXPECT_CALL(*additional_listener, focused(_)).Times(1);
350- EXPECT_CALL(*additional_listener, unfocused()).Times(1);
351-
352- session_manager.add_listener(additional_listener);
353- auto session = session_manager.open_session(__LINE__, "XPlane", std::shared_ptr<mf::EventSink>());
354- session_manager.set_focus_to(session);
355- session_manager.unset_focus();
356-}
357-
358-TEST_F(SessionManagerSessionListenerSetup, additional_listeners_receive_surface_creation)
359-{
360- using namespace ::testing;
361- mtd::NullEventSink event_sink;
362- auto additional_listener = std::make_shared<testing::NiceMock<mtd::MockSessionListener>>();
363- EXPECT_CALL(*additional_listener, starting(_)).Times(1);
364- EXPECT_CALL(*additional_listener, surface_created(_,_)).Times(1);
365-
366- session_manager.add_listener(additional_listener);
367- auto session = session_manager.open_session(__LINE__, "XPlane", std::shared_ptr<mf::EventSink>());
368- auto bs = session->create_buffer_stream(
369- mg::BufferProperties{{640, 480}, mir_pixel_format_abgr_8888, mg::BufferUsage::hardware});
370- session->create_surface(ms::SurfaceCreationParameters().with_buffer_stream(bs), mt::fake_shared(event_sink));
371-}
372-
373 namespace
374 {
375 struct SessionManagerSessionEventsSetup : public testing::Test

Subscribers

People subscribed via source and target branches