wl_resource management fixes (#73)

* view-backend-exportable-private: some cleanups and renaming around frame callbacks.

* view-backend-exportable-private: improve frame callback wl_resource management

Use a destroy listener on frame callback resources in order to be able to
properly clean up in case the underlying client disappears.

Internal management of these resources in ViewBackend now uses wl_list.

* view-backend-exportable-fdo: improve buffer wl_resource management

Use a destroy handle on the buffer wl_resource in order to properly track the
resource's lifetime. In case the resource was destroyed while corresponding
wl_resource was exported to the client, the subsequent release should avoid
dispatching buffer release notification on the already-destroyed resource.

To achieve this, buffer resources are tracked inside the ClientBundleBuffer
implementation in a wl_list, with entries added to that list upon export
and removed from it upon release or destruction.

* view-backend-exportable-fdo-egl: improve buffer wl_resource management in the deprecated implementation

Use a destroy handler on the buffer wl_resource in order to properly track
resource's lifetime. In case the resource was destroyed while corresponding
wl_resource was exported to the client, the subsequent release should avoid
dispatching buffer release notification on the already-destroyed resource.

ClientBundleEGLDeprecated tracks the exported resources (on which EGLImage
objects are based) in a wl_list. Upon resource destruction, the tracked
resource is removed.

When the buffer resource is returned, it is released if it is still tracked
and was not yet destroyed. The corresponding EGLImage is destroyed
unconditionally.

* view-backend-exportable-fdo-egl: clean up management of buffer resources in the wpe_fdo_egl_exported_image implementation

The ClientBundleEGL implementat that operates with wpe_fdo_egl_exported_image
objects already relies on destroy listeners to properly track wl_resource
lifetime, so these changes only polish things.

The basics remain the same: if the wl_resource is destroyed, the
wpe_fdo_egl_exported_image object is destroyed immediately if it's not currently
exported, otherwise it's destroyed when the client returns it.

src/view-backend-exportable-fdo-egl.cpp

@@ -34,41 +34,54 @@
 
 namespace {
 
-struct buffer_data {
-    struct wl_resource *buffer_resource;
-    EGLImageKHR egl_image;
-};
-
 class ClientBundleEGL final : public ClientBundle {
 public:
+    struct BufferResource {
+        struct wl_resource* resource;
+        EGLImageKHR image;
+
+        struct wl_list link;
+        struct wl_listener destroyListener;
+
+        static void destroyNotify(struct wl_listener*, void*);
+    };
+
     ClientBundleEGL(const struct wpe_view_backend_exportable_fdo_egl_client* _client, void* data,
                     ViewBackend* viewBackend, uint32_t initialWidth, uint32_t initialHeight)
         : ClientBundle(data, viewBackend, initialWidth, initialHeight)
         , client(_client)
     {
+        wl_list_init(&bufferResources);
     }
 
     virtual ~ClientBundleEGL()
     {
-        for (auto* buf_data : m_buffers) {
-            assert(buf_data->egl_image);
-            WS::Instance::singleton().destroyImage(buf_data->egl_image);
-
-            delete buf_data;
+        BufferResource* resource;
+        BufferResource* next;
+        wl_list_for_each_safe(resource, next, &bufferResources, link) {
+            WS::Instance::singleton().destroyImage(resource->image);
+            viewBackend->releaseBuffer(resource->resource);
+
+            wl_list_remove(&resource->link);
+            wl_list_remove(&resource->destroyListener.link);
+            delete resource;
         }
-        m_buffers.clear();
+        wl_list_init(&bufferResources);
     }
 
-    void exportBuffer(struct wl_resource *bufferResource) override
+    void exportBuffer(struct wl_resource *buffer) override
     {
-        EGLImageKHR image = WS::Instance::singleton().createImage(bufferResource);
+        EGLImageKHR image = WS::Instance::singleton().createImage(buffer);
         if (!image)
             return;
 
-        auto* buf_data = new struct buffer_data;
-        buf_data->buffer_resource = bufferResource;
-        buf_data->egl_image = image;
-        m_buffers.push_back(buf_data);
+        auto* resource = new BufferResource;
+        resource->resource = buffer;
+        resource->image = image;
+        resource->destroyListener.notify = BufferResource::destroyNotify;
+
+        wl_resource_add_destroy_listener(buffer, &resource->destroyListener);
+        wl_list_insert(&bufferResources, &resource->link);
 
         client->export_egl_image(data, image);
     }
@@ -79,33 +92,53 @@ class ClientBundleEGL final : public ClientBundle {
         if (!image)
             return;
 
-        auto* buf_data = new struct buffer_data;
-        buf_data->buffer_resource = nullptr;
-        buf_data->egl_image = image;
-        m_buffers.push_back(buf_data);
+        auto* resource = new BufferResource;
+        resource->resource = dmabuf_buffer->buffer_resource;
+        resource->image = image;
+        resource->destroyListener.notify = BufferResource::destroyNotify;
+
+        wl_resource_add_destroy_listener(dmabuf_buffer->buffer_resource, &resource->destroyListener);
+        wl_list_insert(&bufferResources, &resource->link);
 
         client->export_egl_image(data, image);
     }
 
-    struct buffer_data* releaseImage(EGLImageKHR image)
+    void releaseImage(EGLImageKHR image)
     {
-        for (auto* buf_data : m_buffers)
-            if (buf_data->egl_image == image) {
-                m_buffers.remove(buf_data);
-                WS::Instance::singleton().destroyImage(buf_data->egl_image);
-
-                return buf_data;
+        BufferResource* matchingResource = nullptr;
+        BufferResource* resource;
+        wl_list_for_each(resource, &bufferResources, link) {
+            if (resource->image == image) {
+                matchingResource = resource;
+                break;
             }
+        }
+
+        WS::Instance::singleton().destroyImage(image);
 
-        return nullptr;
+        if (matchingResource) {
+            viewBackend->releaseBuffer(matchingResource->resource);
+
+            wl_list_remove(&matchingResource->link);
+            wl_list_remove(&matchingResource->destroyListener.link);
+            delete matchingResource;
+        }
     }
 
     const struct wpe_view_backend_exportable_fdo_egl_client* client;
 
-private:
-    std::list<struct buffer_data*> m_buffers;
+    struct wl_list bufferResources;
 };
 
+void ClientBundleEGL::BufferResource::destroyNotify(struct wl_listener* listener, void*)
+{
+    BufferResource* resource;
+    resource = wl_container_of(listener, resource, destroyListener);
+
+    wl_list_remove(&resource->link);
+    delete resource;
+}
+
 } // namespace
 
 extern "C" {
@@ -129,20 +162,7 @@ __attribute__((visibility("default")))
 void
 wpe_view_backend_exportable_fdo_egl_dispatch_release_image(struct wpe_view_backend_exportable_fdo* exportable, EGLImageKHR image)
 {
-    auto* clientBundle = reinterpret_cast<ClientBundleEGL*>(exportable->clientBundle);
-
-    auto* buffer_data = clientBundle->releaseImage(image);
-    if (!buffer_data)
-        return;
-
-    /* the EGL image has already been destroyed by ClientBundleEGL */
-
-    if (buffer_data->buffer_resource) {
-        wpe_view_backend_exportable_fdo_dispatch_release_buffer(exportable,
-                                                                buffer_data->buffer_resource);
-    }
-
-    delete buffer_data;
+    static_cast<ClientBundleEGL*>(exportable->clientBundle)->releaseImage(image);
 }
 
 }

src/view-backend-exportable-fdo.cpp

@@ -31,28 +31,89 @@ namespace {
 
 class ClientBundleBuffer final : public ClientBundle {
 public:
+    struct BufferResource {
+        struct wl_resource* resource;
+
+        struct wl_list link;
+        struct wl_listener destroyListener;
+
+        static void destroyNotify(struct wl_listener*, void*);
+    };
+
     ClientBundleBuffer(const struct wpe_view_backend_exportable_fdo_client* _client, void* data, ViewBackend* viewBackend,
                  uint32_t initialWidth, uint32_t initialHeight)
         : ClientBundle(data, viewBackend, initialWidth, initialHeight)
         , client(_client)
     {
+        wl_list_init(&bufferResources);
     }
 
-    virtual ~ClientBundleBuffer() = default;
+    virtual ~ClientBundleBuffer()
+    {
+        BufferResource* resource;
+        BufferResource* next;
+        wl_list_for_each_safe(resource, next, &bufferResources, link) {
+            viewBackend->releaseBuffer(resource->resource);
+
+            wl_list_remove(&resource->link);
+            wl_list_remove(&resource->destroyListener.link);
+            delete resource;
+        }
+        wl_list_init(&bufferResources);
+    }
 
-    void exportBuffer(struct wl_resource *bufferResource) override
+    void exportBuffer(struct wl_resource *buffer) override
     {
-        client->export_buffer_resource(data, bufferResource);
+        auto* resource = new BufferResource;
+        resource->resource = buffer;
+        resource->destroyListener.notify = BufferResource::destroyNotify;
+
+        wl_resource_add_destroy_listener(buffer, &resource->destroyListener);
+        wl_list_insert(&bufferResources, &resource->link);
+
+        client->export_buffer_resource(data, buffer);
     }
 
     void exportBuffer(const struct linux_dmabuf_buffer *dmabuf_buffer) override
     {
         assert(!"This interface doesn't support Linux DMA buffers");
     }
 
+    void releaseBuffer(struct wl_resource* buffer)
+    {
+        BufferResource* matchingResource = nullptr;
+        BufferResource* resource;
+        wl_list_for_each(resource, &bufferResources, link) {
+            if (resource->resource == buffer) {
+                matchingResource = resource;
+                break;
+            }
+        }
+
+        if (!matchingResource)
+            return;
+
+        viewBackend->releaseBuffer(buffer);
+
+        wl_list_remove(&matchingResource->link);
+        wl_list_remove(&matchingResource->destroyListener.link);
+        delete matchingResource;
+    }
+
     const struct wpe_view_backend_exportable_fdo_client* client;
+
+    struct wl_list bufferResources;
 };
 
+void ClientBundleBuffer::BufferResource::destroyNotify(struct wl_listener* listener, void*)
+{
+    BufferResource* resource;
+    resource = wl_container_of(listener, resource, destroyListener);
+
+    wl_list_remove(&resource->link);
+    delete resource;
+}
+
 } // namespace
 
 extern "C" {
@@ -91,14 +152,14 @@ __attribute__((visibility("default")))
 void
 wpe_view_backend_exportable_fdo_dispatch_frame_complete(struct wpe_view_backend_exportable_fdo* exportable)
 {
-    exportable->clientBundle->viewBackend->dispatchFrameCallback();
+    exportable->clientBundle->viewBackend->dispatchFrameCallbacks();
 }
 
 __attribute__((visibility("default")))
 void
-wpe_view_backend_exportable_fdo_dispatch_release_buffer(struct wpe_view_backend_exportable_fdo* exportable, struct wl_resource* buffer_resource)
+wpe_view_backend_exportable_fdo_dispatch_release_buffer(struct wpe_view_backend_exportable_fdo* exportable, struct wl_resource* buffer)
 {
-    exportable->clientBundle->viewBackend->releaseBuffer(buffer_resource);
+    static_cast<ClientBundleBuffer*>(exportable->clientBundle)->releaseBuffer(buffer);
 }
 
 }

src/view-backend-exportable-private.cpp

@@ -32,12 +32,13 @@ ViewBackend::ViewBackend(ClientBundle* clientBundle, struct wpe_view_backend* ba
     , m_backend(backend)
 {
     m_clientBundle->viewBackend = this;
+
+    wl_list_init(&m_frameCallbacks);
 }
 
 ViewBackend::~ViewBackend()
 {
-    for (auto* resource : m_callbackResources)
-        wl_resource_destroy(resource);
+    clearFrameCallbacks();
 
     WS::Instance::singleton().unregisterViewBackend(m_id);
 
@@ -86,9 +87,13 @@ int ViewBackend::clientFd()
     return dup(m_clientFd);
 }
 
-void ViewBackend::frameCallback(struct wl_resource* callbackResource)
+void ViewBackend::frameCallback(struct wl_resource* callback)
 {
-    m_callbackResources.push_back(callbackResource);
+    auto* resource = new FrameCallbackResource;
+    resource->resource = callback;
+    resource->destroyListener.notify = FrameCallbackResource::destroyNotify;
+    wl_resource_add_destroy_listener(callback, &resource->destroyListener);
+    wl_list_insert(&m_frameCallbacks, &resource->link);
 }
 
 void ViewBackend::exportBufferResource(struct wl_resource* bufferResource)
@@ -101,13 +106,14 @@ void ViewBackend::exportLinuxDmabuf(const struct linux_dmabuf_buffer *dmabuf_buf
     m_clientBundle->exportBuffer(dmabuf_buffer);
 }
 
-void ViewBackend::dispatchFrameCallback()
+void ViewBackend::dispatchFrameCallbacks()
 {
-    for (auto* resource : m_callbackResources) {
-        wl_callback_send_done(resource, 0);
-        wl_resource_destroy(resource);
+    FrameCallbackResource* resource;
+    wl_list_for_each(resource, &m_frameCallbacks, link) {
+        wl_callback_send_done(resource->resource, 0);
     }
-    m_callbackResources.clear();
+    clearFrameCallbacks();
+
     wl_client_flush(m_client);
     wpe_view_backend_dispatch_frame_displayed(m_backend);
 }
@@ -137,3 +143,25 @@ gboolean ViewBackend::s_socketCallback(GSocket* socket, GIOCondition condition,
 
     return TRUE;
 }
+
+void ViewBackend::clearFrameCallbacks()
+{
+    FrameCallbackResource* resource;
+    FrameCallbackResource* next;
+    wl_list_for_each_safe(resource, next, &m_frameCallbacks, link) {
+        wl_list_remove(&resource->link);
+        wl_list_remove(&resource->destroyListener.link);
+        wl_resource_destroy(resource->resource);
+        delete resource;
+    }
+    wl_list_init(&m_frameCallbacks);
+}
+
+void ViewBackend::FrameCallbackResource::destroyNotify(struct wl_listener* listener, void*)
+{
+    FrameCallbackResource* resource;
+    resource = wl_container_of(listener, resource, destroyListener);
+
+    wl_list_remove(&resource->link);
+    delete resource;
+}

src/view-backend-exportable-private.h

@@ -27,7 +27,6 @@
 
 #include "ws.h"
 #include <gio/gio.h>
-#include <vector>
 #include <wpe-fdo/view-backend-exportable.h>
 
 class ViewBackend;
@@ -63,10 +62,21 @@ class ViewBackend : public WS::ExportableClient {
     void frameCallback(struct wl_resource* callbackResource) override;
     void exportBufferResource(struct wl_resource* bufferResource) override;
     void exportLinuxDmabuf(const struct linux_dmabuf_buffer *dmabuf_buffer) override;
-    void dispatchFrameCallback();
+    void dispatchFrameCallbacks();
     void releaseBuffer(struct wl_resource* buffer_resource);
 
 private:
+    struct FrameCallbackResource {
+        struct wl_resource* resource;
+
+        struct wl_list link;
+        struct wl_listener destroyListener;
+
+        static void destroyNotify(struct wl_listener*, void*);
+    };
+
+    void clearFrameCallbacks();
+
     static gboolean s_socketCallback(GSocket*, GIOCondition, gpointer);
 
     uint32_t m_id { 0 };
@@ -75,7 +85,7 @@ class ViewBackend : public WS::ExportableClient {
     ClientBundle* m_clientBundle;
     struct wpe_view_backend* m_backend;
 
-    std::vector<struct wl_resource*> m_callbackResources;
+    struct wl_list m_frameCallbacks;
 
     GSocket* m_socket;
     GSource* m_source;