xwayland: associate/dissociate/map/unmap cleanups

- connect/disconnect map handlers in set_surface()
- call set_surface() at time of associate/dissociate

This separates the concepts of "associate" and "map" more clearly.

It's no longer necessary to listen for wlr_surface "destroy" event,
because dissociate is always received first.

Also, view->content_tree is now destroyed and set to NULL at unmap.
Previously, we relied on wlr_scene to destroy it automatically when
the surface was destroyed, but kept a potentially dangling pointer in
view->content_tree until next map. Similar change for unmanaged.

v2: comment updates

diff --git a/include/view.h b/include/view.h
index a555a44fea57acde0135f5e505d0a9ad4fe3ad44..491a0c9ce881ba8d453e4e05108b4f28a08a02ed 100644 (file)
--- a/include/view.h
+++ b/include/view.h
@@ -237,7 +237,6 @@ struct view {
        struct mappable mappable;
 
        struct wl_listener destroy;
-       struct wl_listener surface_destroy;
        struct wl_listener commit;
        struct wl_listener request_move;
        struct wl_listener request_resize;

diff --git a/src/xwayland-unmanaged.c b/src/xwayland-unmanaged.c
index b003e4ec58b09a4683a604a8aa4baa90cfdd6ff6..fe22398a31db48ea42b55265aedf105dda04baa0 100644 (file)
--- a/src/xwayland-unmanaged.c
+++ b/src/xwayland-unmanaged.c
@@ -68,7 +68,6 @@ handle_map(struct wl_listener *listener, void *data)
                seat_focus_surface(&unmanaged->server->seat, xsurface->surface);
        }
 
-       /* node will be destroyed automatically once surface is destroyed */
        unmanaged->node = &wlr_scene_surface_create(
                        unmanaged->server->unmanaged_tree,
                        xsurface->surface)->buffer->node;
@@ -128,13 +127,15 @@ handle_unmap(struct wl_listener *listener, void *data)
 
        wl_list_remove(&unmanaged->link);
        wl_list_remove(&unmanaged->set_geometry.link);
-       wlr_scene_node_set_enabled(unmanaged->node, false);
 
        /*
-        * Mark the node as gone so a racing configure event
-        * won't try to reposition the node while unmapped.
+        * Destroy the scene node. It would get destroyed later when
+        * the wlr_surface is destroyed, but if the unmanaged surface
+        * gets converted to a managed surface, that may be a while.
         */
+       wlr_scene_node_destroy(unmanaged->node);
        unmanaged->node = NULL;
+
        cursor_update_focus(unmanaged->server);
 
        if (seat->seat->keyboard_state.focused_surface == xsurface->surface) {

diff --git a/src/xwayland.c b/src/xwayland.c
index 4be48c693bda82a8c8f3b1e8d2f128eb2204d7db..5600e8d29827617a755fc44ae232f0430fea95ad 100644 (file)
--- a/src/xwayland.c
+++ b/src/xwayland.c
@@ -320,9 +320,8 @@ handle_associate(struct wl_listener *listener, void *data)
        assert(xwayland_view->xwayland_surface &&
                xwayland_view->xwayland_surface->surface);
 
-       mappable_connect(&xwayland_view->base.mappable,
-               xwayland_view->xwayland_surface->surface,
-               handle_map, handle_unmap);
+       set_surface(&xwayland_view->base,
+               xwayland_view->xwayland_surface->surface);
 }
 
 static void
@@ -331,18 +330,7 @@ handle_dissociate(struct wl_listener *listener, void *data)
        struct xwayland_view *xwayland_view =
                wl_container_of(listener, xwayland_view, dissociate);
 
-       /* https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/4524 */
-       assert(xwayland_view->base.mappable.connected);
-       mappable_disconnect(&xwayland_view->base.mappable);
-}
-
-static void
-handle_surface_destroy(struct wl_listener *listener, void *data)
-{
-       struct view *view = wl_container_of(listener, view, surface_destroy);
-       assert(data && data == view->surface);
-
-       set_surface(view, NULL);
+       set_surface(&xwayland_view->base, NULL);
 }
 
 static void
@@ -781,16 +769,15 @@ set_surface(struct view *view, struct wlr_surface *surface)
 {
        if (view->surface) {
                /* Disconnect wlr_surface event listeners */
+               mappable_disconnect(&view->mappable);
                wl_list_remove(&view->commit.link);
-               wl_list_remove(&view->surface_destroy.link);
        }
        view->surface = surface;
        if (surface) {
                /* Connect wlr_surface event listeners */
-               view->commit.notify = handle_commit;
-               wl_signal_add(&surface->events.commit, &view->commit);
-               view->surface_destroy.notify = handle_surface_destroy;
-               wl_signal_add(&surface->events.destroy, &view->surface_destroy);
+               mappable_connect(&view->mappable, surface,
+                       handle_map, handle_unmap);
+               CONNECT_SIGNAL(surface, view, commit);
        }
 }
 
@@ -803,6 +790,7 @@ handle_map(struct wl_listener *listener, void *data)
                xwayland_view->xwayland_surface;
        assert(xwayland_surface);
        assert(xwayland_surface->surface);
+       assert(xwayland_surface->surface == view->surface);
 
        if (view->mapped) {
                return;
@@ -818,18 +806,14 @@ handle_map(struct wl_listener *listener, void *data)
 
        view->mapped = true;
 
-       if (view->surface != xwayland_surface->surface) {
-               set_surface(view, xwayland_surface->surface);
-
-               /* Will be free'd automatically once the surface is being destroyed */
-               struct wlr_scene_tree *tree = wlr_scene_subsurface_tree_create(
+       if (!view->content_tree) {
+               view->content_tree = wlr_scene_subsurface_tree_create(
                        view->scene_tree, view->surface);
-               if (!tree) {
+               if (!view->content_tree) {
                        /* TODO: might need further clean up */
                        wl_resource_post_no_memory(view->surface->resource);
                        return;
                }
-               view->content_tree = tree;
        }
 
        if (!view->been_mapped) {
@@ -870,6 +854,17 @@ handle_unmap(struct wl_listener *listener, void *data)
        }
        view->mapped = false;
        view_impl_unmap(view);
+
+       /*
+        * Destroy the content_tree at unmap. Alternatively, we could
+        * let wlr_scene manage its lifetime automatically, but this
+        * approach is symmetrical with handle_map() and avoids any
+        * concern of a dangling pointer in view->content_tree.
+        */
+       if (view->content_tree) {
+               wlr_scene_node_destroy(&view->content_tree->node);
+               view->content_tree = NULL;
+       }
 }
 
 static void