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

[Chris Halse Rogers]

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.

[Chris Halse Rogers (raof)]

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)]

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)]

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)]

- 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)]

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)]

> 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)]

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.

=== modified file 'include/server/mir/scene/session_coordinator.h'
--- include/server/mir/scene/session_coordinator.h	2017-07-28 17:00:43 +0000
+++ include/server/mir/scene/session_coordinator.h	2017-08-18 08:06:31 +0000
@@ -54,8 +54,6 @@
 
     virtual std::shared_ptr<Session> successor_of(std::shared_ptr<Session> const&) const = 0;
 
-    virtual void add_listener(std::shared_ptr<SessionListener> const&) = 0;
-    virtual void remove_listener(std::shared_ptr<SessionListener> const&) = 0;
 protected:
     SessionCoordinator() = default;
     virtual ~SessionCoordinator() = default;

=== modified file 'src/server/scene/default_configuration.cpp'
--- src/server/scene/default_configuration.cpp	2017-07-28 17:00:43 +0000
+++ src/server/scene/default_configuration.cpp	2017-08-18 08:06:31 +0000
@@ -132,7 +132,7 @@
     return mediating_display_changer(
         [this]()
         {
-            return std::make_shared<ms::MediatingDisplayChanger>(
+            return ms::MediatingDisplayChanger::create(
                 the_display(),
                 the_compositor(),
                 the_display_configuration_policy(),
@@ -142,7 +142,6 @@
                 the_display_configuration_observer(),
                 the_main_loop());
         });
-
 }
 
 std::shared_ptr<mf::DisplayChanger>

=== modified file 'src/server/scene/mediating_display_changer.cpp'
--- src/server/scene/mediating_display_changer.cpp	2017-08-09 15:03:31 +0000
+++ src/server/scene/mediating_display_changer.cpp	2017-08-18 08:06:31 +0000
@@ -124,6 +124,103 @@
 };
 }
 
+std::shared_ptr<ms::MediatingDisplayChanger> ms::MediatingDisplayChanger::create(
+    std::shared_ptr<mg::Display> const& display,
+    std::shared_ptr<mc::Compositor> const& compositor,
+    std::shared_ptr<mg::DisplayConfigurationPolicy> const& display_configuration_policy,
+    std::shared_ptr<SessionContainer> const& session_container,
+    std::shared_ptr<SessionEventHandlerRegister> const& session_event_handler_register,
+    std::shared_ptr<ServerActionQueue> const& server_action_queue,
+    std::shared_ptr<mg::DisplayConfigurationObserver> const& observer,
+    std::shared_ptr<mt::AlarmFactory> const& alarm_factory)
+{
+    std::shared_ptr<ms::MediatingDisplayChanger> const result{
+        new ms::MediatingDisplayChanger{
+            display,
+            compositor,
+            display_configuration_policy,
+            session_container,
+            session_event_handler_register,
+            server_action_queue,
+            observer,
+            alarm_factory
+        }};
+
+    std::weak_ptr<ms::MediatingDisplayChanger> const weak_changer = result;
+
+    /*
+     * Caution! Reference cycle!
+     *
+     * result owns a reference to session_event_handler_register;
+     * if we capture a strong reference in the handler, then
+     * session_Event_handler_register will have a strong reference to
+     * result.
+     */
+    session_event_handler_register->register_focus_change_handler(
+        [weak_changer](std::shared_ptr<ms::Session> const& session)
+        {
+            auto const weak_session = std::weak_ptr<ms::Session>(session);
+            if (auto const changer = weak_changer.lock())
+            {
+                /*
+                 * Caution! Bonus reference cycle!
+                 *
+                 * As before, changer owns a reference to server_action_queue.
+                 */
+                changer->server_action_queue->enqueue(
+                    changer.get(),
+                    [weak_changer, weak_session]
+                    {
+                        if (auto const changer = weak_changer.lock())
+                        {
+                            if (auto const session = weak_session.lock())
+                            {
+                                changer->focus_change_handler(session);
+                            }
+                        }
+                    });
+            }
+        });
+
+    session_event_handler_register->register_no_focus_handler(
+        [weak_changer]
+        {
+            if (auto const changer = weak_changer.lock())
+            {
+                changer->server_action_queue->enqueue(
+                    changer.get(),
+                    [weak_changer]
+                    {
+                        if (auto const changer = weak_changer.lock())
+                        {
+                            changer->no_focus_handler();
+                        }
+                    });
+            }
+        });
+
+    session_event_handler_register->register_session_stopping_handler(
+        [weak_changer](std::shared_ptr<ms::Session> const& session)
+        {
+            auto const weak_session = std::weak_ptr<ms::Session>(session);
+            if (auto const changer = weak_changer.lock())
+            {
+                changer->server_action_queue->enqueue(
+                    changer.get(),
+                    [weak_changer,weak_session]
+                    {
+                        if (auto const changer = weak_changer.lock())
+                        {
+                            if (auto const session = weak_session.lock())
+                                changer->session_stopping_handler(session);
+                        }
+                    });
+            }
+        });
+
+    return result;
+}
+
 ms::MediatingDisplayChanger::MediatingDisplayChanger(
     std::shared_ptr<mg::Display> const& display,
     std::shared_ptr<mc::Compositor> const& compositor,
@@ -144,40 +241,6 @@
       base_configuration_applied{true},
       alarm_factory{alarm_factory}
 {
-    session_event_handler_register->register_focus_change_handler(
-        [this](std::shared_ptr<ms::Session> const& session)
-        {
-            auto const weak_session = std::weak_ptr<ms::Session>(session);
-            this->server_action_queue->enqueue(
-                this,
-                [this,weak_session]
-                {
-                    if (auto const session = weak_session.lock())
-                        focus_change_handler(session);
-                });
-        });
-
-    session_event_handler_register->register_no_focus_handler(
-        [this]
-        {
-            this->server_action_queue->enqueue(
-                this,
-                [this] { no_focus_handler(); });
-        });
-
-    session_event_handler_register->register_session_stopping_handler(
-        [this](std::shared_ptr<ms::Session> const& session)
-        {
-            auto const weak_session = std::weak_ptr<ms::Session>(session);
-            this->server_action_queue->enqueue(
-                this,
-                [this,weak_session]
-                {
-                    if (auto const session = weak_session.lock())
-                        session_stopping_handler(session);
-                });
-        });
-
     observer->initial_configuration(base_configuration_);
 }
 

=== modified file 'src/server/scene/mediating_display_changer.h'
--- src/server/scene/mediating_display_changer.h	2017-07-28 17:00:43 +0000
+++ src/server/scene/mediating_display_changer.h	2017-08-18 08:06:31 +0000
@@ -55,7 +55,7 @@
                                 public shell::DisplayConfigurationController
 {
 public:
-    MediatingDisplayChanger(
+    static std::shared_ptr<MediatingDisplayChanger> create(
         std::shared_ptr<graphics::Display> const& display,
         std::shared_ptr<compositor::Compositor> const& compositor,
         std::shared_ptr<graphics::DisplayConfigurationPolicy> const& display_configuration_policy,
@@ -92,6 +92,16 @@
     void set_base_configuration(std::shared_ptr<graphics::DisplayConfiguration> const &conf) override;
 
 private:
+    MediatingDisplayChanger(
+        std::shared_ptr<graphics::Display> const& display,
+        std::shared_ptr<compositor::Compositor> const& compositor,
+        std::shared_ptr<graphics::DisplayConfigurationPolicy> const& display_configuration_policy,
+        std::shared_ptr<SessionContainer> const& session_container,
+        std::shared_ptr<SessionEventHandlerRegister> const& session_event_handler_register,
+        std::shared_ptr<ServerActionQueue> const& server_action_queue,
+        std::shared_ptr<graphics::DisplayConfigurationObserver> const& observer,
+        std::shared_ptr<time::AlarmFactory> const& alarm_factory);
+
     void focus_change_handler(std::shared_ptr<Session> const& session);
     void no_focus_handler();
     void session_stopping_handler(std::shared_ptr<Session> const& session);

=== modified file 'src/server/scene/session_manager.cpp'
--- src/server/scene/session_manager.cpp	2017-07-28 17:00:43 +0000
+++ src/server/scene/session_manager.cpp	2017-08-18 08:06:31 +0000
@@ -186,13 +186,3 @@
 {
     return app_container->successor_of(session);
 }
-
-void ms::SessionManager::add_listener(std::shared_ptr<SessionListener> const& listener)
-{
-    observers->register_interest(listener);
-}
-
-void ms::SessionManager::remove_listener(std::shared_ptr<SessionListener> const& listener)
-{
-    observers->unregister_interest(*listener);
-}

=== modified file 'src/server/scene/session_manager.h'
--- src/server/scene/session_manager.h	2017-07-28 17:00:43 +0000
+++ src/server/scene/session_manager.h	2017-08-18 08:06:31 +0000
@@ -77,9 +77,6 @@
     void set_focus_to(std::shared_ptr<Session> const& focus) override;
     void unset_focus() override;
 
-    void add_listener(std::shared_ptr<SessionListener> const& listener) override;
-    void remove_listener(std::shared_ptr<SessionListener> const& listener) override;
-
 protected:
     SessionManager(const SessionManager&) = delete;
     SessionManager& operator=(const SessionManager&) = delete;

=== modified file 'tests/unit-tests/scene/test_mediating_display_changer.cpp'
--- tests/unit-tests/scene/test_mediating_display_changer.cpp	2017-08-09 11:30:59 +0000
+++ tests/unit-tests/scene/test_mediating_display_changer.cpp	2017-08-18 08:06:31 +0000
@@ -140,7 +140,7 @@
     {
         using namespace testing;
 
-        changer = std::make_shared<ms::MediatingDisplayChanger>(
+        changer = ms::MediatingDisplayChanger::create(
                       mt::fake_shared(mock_display),
                       mt::fake_shared(mock_compositor),
                       mt::fake_shared(mock_conf_policy),
@@ -688,7 +688,7 @@
     stub_session_container.insert_session(session1);
     stub_session_container.insert_session(session2);
 
-    ms::MediatingDisplayChanger display_changer(
+    auto display_changer = ms::MediatingDisplayChanger::create(
       mt::fake_shared(mock_display),
       mt::fake_shared(mock_compositor),
       mt::fake_shared(mock_conf_policy),
@@ -706,11 +706,11 @@
     Mock::VerifyAndClearExpectations(&mock_server_action_queue);
 
     EXPECT_CALL(mock_server_action_queue, enqueue(owner, _));
-    display_changer.configure(session1, conf);
+    display_changer->configure(session1, conf);
     Mock::VerifyAndClearExpectations(&mock_server_action_queue);
 
     EXPECT_CALL(mock_server_action_queue, enqueue(owner, _));
-    display_changer.configure_for_hardware_change(conf);
+    display_changer->configure_for_hardware_change(conf);
     Mock::VerifyAndClearExpectations(&mock_server_action_queue);
 
     EXPECT_CALL(mock_server_action_queue, enqueue(owner, _));
@@ -722,11 +722,11 @@
     Mock::VerifyAndClearExpectations(&mock_server_action_queue);
 
     EXPECT_CALL(mock_server_action_queue, pause_processing_for(owner));
-    display_changer.pause_display_config_processing();
+    display_changer->pause_display_config_processing();
     Mock::VerifyAndClearExpectations(&mock_server_action_queue);
 
     EXPECT_CALL(mock_server_action_queue, resume_processing_for(owner));
-    display_changer.resume_display_config_processing();
+    display_changer->resume_display_config_processing();
     Mock::VerifyAndClearExpectations(&mock_server_action_queue);
 }
 
@@ -742,7 +742,7 @@
     stub_session_container.insert_session(active_session);
     stub_session_container.insert_session(inactive_session);
 
-    ms::MediatingDisplayChanger display_changer(
+    auto display_changer = ms::MediatingDisplayChanger::create(
         mt::fake_shared(mock_display),
         mt::fake_shared(mock_compositor),
         mt::fake_shared(mock_conf_policy),
@@ -758,7 +758,7 @@
 
     EXPECT_CALL(mock_server_action_queue, enqueue(_, _)).Times(0);
 
-    display_changer.configure(inactive_session, conf);
+    display_changer->configure(inactive_session, conf);
 }
 
 TEST_F(MediatingDisplayChangerTest, set_base_configuration_doesnt_override_session_configuration)
@@ -871,7 +871,7 @@
         MOCK_METHOD1(base_configuration_updated, void (std::shared_ptr<mg::DisplayConfiguration const> const& base_config));
     } display_configuration_observer;
 
-    changer = std::make_shared<ms::MediatingDisplayChanger>(
+    changer = ms::MediatingDisplayChanger::create(
         mt::fake_shared(mock_display),
         mt::fake_shared(mock_compositor),
         mt::fake_shared(mock_conf_policy),

=== modified file 'tests/unit-tests/scene/test_session_manager.cpp'
--- tests/unit-tests/scene/test_session_manager.cpp	2017-07-28 17:00:43 +0000
+++ tests/unit-tests/scene/test_session_manager.cpp	2017-08-18 08:06:31 +0000
@@ -184,49 +184,6 @@
     session_manager.close_session(session);
 }
 
-TEST_F(SessionManagerSessionListenerSetup, additional_listeners_receive_session_callbacks)
-{
-    using namespace ::testing;
-
-    auto additional_listener = std::make_shared<testing::NiceMock<mtd::MockSessionListener>>();
-    EXPECT_CALL(*additional_listener, starting(_)).Times(1);
-    EXPECT_CALL(*additional_listener, stopping(_)).Times(1);
-
-    session_manager.add_listener(additional_listener);
-    auto session = session_manager.open_session(__LINE__, "XPlane", std::shared_ptr<mf::EventSink>());
-    session_manager.close_session(session);
-}
-
-TEST_F(SessionManagerSessionListenerSetup, additional_listeners_receive_focus_changes)
-{
-    using namespace ::testing;
-
-    auto additional_listener = std::make_shared<testing::NiceMock<mtd::MockSessionListener>>();
-    EXPECT_CALL(*additional_listener, starting(_)).Times(1);
-    EXPECT_CALL(*additional_listener, focused(_)).Times(1);
-    EXPECT_CALL(*additional_listener, unfocused()).Times(1);
-
-    session_manager.add_listener(additional_listener);
-    auto session = session_manager.open_session(__LINE__, "XPlane", std::shared_ptr<mf::EventSink>());
-    session_manager.set_focus_to(session);
-    session_manager.unset_focus();
-}
-
-TEST_F(SessionManagerSessionListenerSetup, additional_listeners_receive_surface_creation)
-{
-    using namespace ::testing;
-    mtd::NullEventSink event_sink;
-    auto additional_listener = std::make_shared<testing::NiceMock<mtd::MockSessionListener>>();
-    EXPECT_CALL(*additional_listener, starting(_)).Times(1);
-    EXPECT_CALL(*additional_listener, surface_created(_,_)).Times(1);
-
-    session_manager.add_listener(additional_listener);
-    auto session = session_manager.open_session(__LINE__, "XPlane", std::shared_ptr<mf::EventSink>());
-    auto bs = session->create_buffer_stream(
-        mg::BufferProperties{{640, 480}, mir_pixel_format_abgr_8888, mg::BufferUsage::hardware});
-    session->create_surface(ms::SurfaceCreationParameters().with_buffer_stream(bs), mt::fake_shared(event_sink));
-}
-
 namespace
 {
 struct SessionManagerSessionEventsSetup : public testing::Test