Merge lp:~raof/mir/wayland-clients-close-their-sessions-on-quit into lp:mir

[Chris Halse Rogers]

Wayland: Close the Mir session on client quit.

Fixes phantom sessions hanging around after client quit.

Fixes: https://bugs.launchpad.net/mir/+bug/1719586

[Alan Griffiths (alan-griffiths)]

Looks sensible and seems to work.

review: Approve

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

FAILED: Continuous integration, rev:4278
https://mir-jenkins.ubuntu.com/job/mir-ci/3692/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/5063/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/5299
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=artful/5287
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/5287
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/5287
    ABORTED: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=artful/5107/console
    ABORTED: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/5107/console
    ABORTED: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/5107/console

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

review: Needs Fixing (continuous-integration)

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

FAILED: Autolanding.
More details in the following jenkins job:
https://mir-jenkins.ubuntu.com/job/mir-autolanding/1443/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/5064/console
    None: https://mir-jenkins.ubuntu.com/job/generic-land-mp/1578/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/5300
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=artful/5288
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/5288
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/5288
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=artful/5108
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=artful/5108/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/5108
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/5108/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=artful/5108
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=artful/5108/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/5108/console
    ABORTED: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/5108/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=artful/5108
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=artful/5108/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/5108
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=zesty/5108/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/5108
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/5108/artifact/output/*zip*/output.zip

review: Needs Fixing (continuous-integration)

=== modified file 'src/server/frontend/wayland/wayland_connector.cpp'
--- src/server/frontend/wayland/wayland_connector.cpp	2017-09-26 15:39:09 +0000
+++ src/server/frontend/wayland/wayland_connector.cpp	2017-09-26 15:39:09 +0000
@@ -158,8 +158,29 @@
 
 struct ClientPrivate
 {
+    ClientPrivate(std::shared_ptr<mf::Session> const& session, mf::Shell& shell)
+        : session{session},
+          shell{&shell}
+    {
+    }
+
+    ~ClientPrivate()
+    {
+        shell->close_session(session);
+        /*
+         * This ensures that further calls to
+         * wl_client_get_destroy_listener(client, &cleanup_private)
+         * - and hence session_for_client(client) - return nullptr.
+         */
+        wl_list_remove(&destroy_listener.link);
+    }
+
     wl_listener destroy_listener;
-    std::shared_ptr<mf::Session> session;
+    std::shared_ptr<mf::Session> const session;
+    /*
+     * This shell is owned by the ClientSessionConstructor, which outlives all clients.
+     */
+    mf::Shell* const shell;
 };
 
 static_assert(
@@ -182,9 +203,10 @@
 {
     auto listener = wl_client_get_destroy_listener(client, &cleanup_private);
 
-    assert(listener && "Client session requested for malformed client");
+    if (listener)
+        return private_from_listener(listener)->session;
 
-    return private_from_listener(listener)->session;
+    return nullptr;
 }
 
 struct ClientSessionConstructor
@@ -220,9 +242,8 @@
         "",
         std::make_shared<WaylandEventSink>([](auto){}));
 
-    auto client_context = new ClientPrivate;
+    auto client_context = new ClientPrivate{session, *construction_context->shell};
     client_context->destroy_listener.notify = &cleanup_private;
-    client_context->session = session;
     wl_client_add_destroy_listener(client, &client_context->destroy_listener);
 }
 
@@ -295,6 +316,19 @@
 
     return buffer;
 }
+
+template<typename Callable>
+auto run_unless(std::shared_ptr<bool> const& condition, Callable&& callable)
+{
+    return
+        [callable = std::move(callable), condition]()
+        {
+            if (*condition)
+                return;
+
+            callable();
+        };
+}
 }
 
 class WlShmBuffer :
@@ -511,7 +545,8 @@
           allocator{allocator},
           executor{executor},
           pending_buffer{nullptr},
-          pending_frames{std::make_shared<std::vector<wl_resource*>>()}
+          pending_frames{std::make_shared<std::vector<wl_resource*>>()},
+          destroyed{std::make_shared<bool>(false)}
     {
         auto session = session_for_client(client);
         mg::BufferProperties const props{
@@ -527,6 +562,13 @@
         stream->allow_framedropping(true);
     }
 
+    ~WlSurface()
+    {
+        *destroyed = true;
+        if (auto session = session_for_client(client))
+            session->destroy_buffer_stream(stream_id);
+    }
+
     void set_resize_handler(std::function<void(geom::Size)> const& handler)
     {
         resize_handler = handler;
@@ -548,6 +590,7 @@
 
     wl_resource* pending_buffer;
     std::shared_ptr<std::vector<wl_resource*>> const pending_frames;
+    std::shared_ptr<bool> const destroyed;
 
     void destroy();
     void attach(std::experimental::optional<wl_resource*> const& buffer, int32_t x, int32_t y);
@@ -620,9 +663,10 @@
         std::shared_ptr<mg::Buffer> mir_buffer;
         auto shm_buffer = wl_shm_buffer_get(pending_buffer);
         auto send_frame_notifications =
-            [executor = executor, frames = pending_frames]()
+            [executor = executor, frames = pending_frames, destroyed = destroyed]()
             {
-                executor->spawn(
+                executor->spawn(run_unless(
+                    destroyed,
                     [frames]()
                     {
                         /*
@@ -639,7 +683,7 @@
                             wl_resource_destroy(frame);
                         }
                         frames->clear();
-                    });
+                    }));
             };
 
         if (shm_buffer)
@@ -745,19 +789,6 @@
 class WlPointer;
 class WlTouch;
 
-template<typename Callable>
-auto run_unless(std::shared_ptr<bool> const& condition, Callable&& callable)
-{
-    return
-        [callable = std::move(callable), condition]()
-        {
-            if (*condition)
-                return;
-
-            callable();
-        };
-}
-
 class WlKeyboard : public wayland::Keyboard
 {
 public:
@@ -1555,15 +1586,15 @@
                 hide_spec.state = mir_window_state_hidden;
                 shell->modify_surface(session, id, hide_spec);
             });
-
-        auto shim = new DestructionShim{session, shell, surface_id};
-        shim->destruction_listener.notify = &cleanup_surface;
-        wl_resource_add_destroy_listener(
-            resource,
-            &shim->destruction_listener);
-    }
-
-    ~WlShellSurface() override = default;
+    }
+
+    ~WlShellSurface() override
+    {
+        if (auto session = session_for_client(client))
+        {
+            shell->destroy_surface(session, surface_id);
+        }
+    }
 protected:
     void pong(uint32_t /*serial*/) override
     {
@@ -1603,7 +1634,6 @@
 
         auto const session = session_for_client(client);
         shell->modify_surface(session, surface_id, mods);
-
     }
 
     void set_popup(
@@ -1636,38 +1666,6 @@
     {
     }
 private:
-    struct DestructionShim
-    {
-        DestructionShim(
-            std::shared_ptr<mf::Session> const& session,
-            std::shared_ptr<mf::Shell> const& shell,
-            mf::SurfaceId id)
-            : session{session},
-              shell{shell},
-              surface_id{id}
-        {
-        }
-
-        std::shared_ptr<mf::Session> const session;
-        std::shared_ptr<mf::Shell> const shell;
-        mf::SurfaceId const surface_id;
-        wl_listener destruction_listener;
-    };
-
-    static_assert(
-        std::is_standard_layout<DestructionShim>::value,
-        "DestructionShim must be Standard Layout for wl_container_of to be defined behaviour");
-
-    static void cleanup_surface(wl_listener* listener, void*)
-    {
-        DestructionShim* shim;
-        shim = wl_container_of(listener, shim, destruction_listener);
-
-        shim->shell->destroy_surface(shim->session, shim->surface_id);
-
-        delete shim;
-    }
-
     std::shared_ptr<mf::Shell> const shell;
     mf::SurfaceId surface_id;
 };