From 21f425f11e9f26d8fd18e226bea362dd43e97a5c Mon Sep 17 00:00:00 2001 From: John Lindgren Date: Mon, 17 Nov 2025 21:14:06 -0500 Subject: [PATCH] view: manage view->surface lifetime via common view_set_surface() - add new commit() function to view->impl table - add view_set_surface() which encapsulates: - setting view->surface - connecting/disconnecting map, unmap, and commit handlers For xwayland, view_set_surface() is called from associate/dissociate() handlers. This means that view->surface is now set slightly earlier, in associate() rather than map(). It's no longer necessary to have two separate cleanup paths depending on whether the wlr_surface or wlr_xwayland_surface is destroyed first, because wlroots calls dissociate() beforehand in either case. 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: add assert v3: synthesize dissociate in set_override_redirect v4: rework scene node lifetimes v5: reduce asserts --- include/view.h | 4 +-- src/view.c | 30 +++++++++++++++--- src/xdg.c | 14 +++------ src/xwayland-unmanaged.c | 9 ++++-- src/xwayland.c | 66 +++++++++++++--------------------------- 5 files changed, 60 insertions(+), 63 deletions(-) diff --git a/include/view.h b/include/view.h index 46a8e29f..a36306dc 100644 --- a/include/view.h +++ b/include/view.h @@ -111,6 +111,7 @@ struct view_impl { void (*set_fullscreen)(struct view *view, bool fullscreen); void (*notify_tiled)(struct view *view); void (*unmap)(struct view *view); + void (*commit)(struct view *view); void (*maximize)(struct view *view, enum view_axis maximized); void (*minimize)(struct view *view, bool minimize); struct view *(*get_parent)(struct view *self); @@ -239,7 +240,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; @@ -584,7 +584,7 @@ void view_adjust_size(struct view *view, int *w, int *h); void view_evacuate_region(struct view *view); void view_on_output_destroy(struct view *view); -void view_connect_map(struct view *view, struct wlr_surface *surface); +void view_set_surface(struct view *view, struct wlr_surface *surface); void view_update_visibility(struct view *view); void view_init(struct view *view); diff --git a/src/view.c b/src/view.c index 8a0d3caf..600268cc 100644 --- a/src/view.c +++ b/src/view.c @@ -12,6 +12,7 @@ #include "buffer.h" #include "common/box.h" #include "common/list.h" +#include "common/macros.h" #include "common/match.h" #include "common/mem.h" #include "config/rcxml.h" @@ -2459,11 +2460,32 @@ handle_unmap(struct wl_listener *listener, void *data) view->impl->unmap(view); } +static void +handle_commit(struct wl_listener *listener, void *data) +{ + struct view *view = wl_container_of(listener, view, commit); + assert(data && data == view->surface); + view->impl->commit(view); +} + void -view_connect_map(struct view *view, struct wlr_surface *surface) +view_set_surface(struct view *view, struct wlr_surface *surface) { assert(view); - mappable_connect(&view->mappable, surface, handle_map, handle_unmap); + assert(!view->mapped); + + if (view->surface) { + mappable_disconnect(&view->mappable); + wl_list_remove(&view->commit.link); + } + + view->surface = surface; + + if (surface) { + mappable_connect(&view->mappable, surface, + handle_map, handle_unmap); + CONNECT_SIGNAL(surface, view, commit); + } } /* Used in both (un)map and (un)minimize */ @@ -2593,9 +2615,7 @@ view_destroy(struct view *view) wl_signal_emit_mutable(&view->events.destroy, NULL); snap_constraints_invalidate(view); - if (view->mappable.connected) { - mappable_disconnect(&view->mappable); - } + view_set_surface(view, NULL); wl_list_remove(&view->request_move.link); wl_list_remove(&view->request_resize.link); diff --git a/src/xdg.c b/src/xdg.c index 48179a6d..e7bfcbb1 100644 --- a/src/xdg.c +++ b/src/xdg.c @@ -133,12 +133,10 @@ do_late_positioning(struct view *view) static void set_pending_configure_serial(struct view *view, uint32_t serial); static void -handle_commit(struct wl_listener *listener, void *data) +xdg_toplevel_view_commit(struct view *view) { - struct view *view = wl_container_of(listener, view, commit); struct wlr_xdg_surface *xdg_surface = xdg_surface_from_view(view); struct wlr_xdg_toplevel *toplevel = xdg_toplevel_from_view(view); - assert(view->surface); if (xdg_surface->initial_commit) { uint32_t serial = @@ -378,7 +376,6 @@ handle_destroy(struct wl_listener *listener, void *data) wl_list_remove(&xdg_toplevel_view->set_app_id.link); wl_list_remove(&xdg_toplevel_view->request_show_window_menu.link); wl_list_remove(&xdg_toplevel_view->new_popup.link); - wl_list_remove(&view->commit.link); if (view->pending_configure_timeout) { wl_event_source_remove(view->pending_configure_timeout); @@ -837,6 +834,7 @@ static const struct view_impl xdg_toplevel_view_impl = { .set_fullscreen = xdg_toplevel_view_set_fullscreen, .notify_tiled = xdg_toplevel_view_notify_tiled, .unmap = xdg_toplevel_view_unmap, + .commit = xdg_toplevel_view_commit, .maximize = xdg_toplevel_view_maximize, .minimize = xdg_toplevel_view_minimize, .get_parent = xdg_toplevel_view_get_parent, @@ -1002,11 +1000,10 @@ handle_new_xdg_toplevel(struct wl_listener *listener, void *data) */ kde_server_decoration_set_view(view, xdg_surface->surface); - /* In support of xdg popups and IME popup */ - view->surface = xdg_surface->surface; - view->surface->data = tree; + view_set_surface(view, xdg_surface->surface); - view_connect_map(view, xdg_surface->surface); + /* In support of xdg popups and IME popup */ + view->surface->data = tree; struct wlr_xdg_toplevel *toplevel = xdg_surface->toplevel; CONNECT_SIGNAL(toplevel, view, destroy); @@ -1016,7 +1013,6 @@ handle_new_xdg_toplevel(struct wl_listener *listener, void *data) CONNECT_SIGNAL(toplevel, view, request_maximize); CONNECT_SIGNAL(toplevel, view, request_fullscreen); CONNECT_SIGNAL(toplevel, view, set_title); - CONNECT_SIGNAL(view->surface, view, commit); /* Events specific to XDG toplevel views */ CONNECT_SIGNAL(toplevel, xdg_toplevel_view, set_app_id); diff --git a/src/xwayland-unmanaged.c b/src/xwayland-unmanaged.c index b003e4ec..b02a4bd2 100644 --- 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,7 +127,13 @@ 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); + + /* + * 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); /* * Mark the node as gone so a racing configure event diff --git a/src/xwayland.c b/src/xwayland.c index ed974a33..5e04c388 100644 --- a/src/xwayland.c +++ b/src/xwayland.c @@ -38,7 +38,6 @@ static_assert(ARRAY_SIZE(atom_names) == ATOM_COUNT, "atom names out of sync"); static xcb_atom_t atoms[ATOM_COUNT] = {0}; -static void set_surface(struct view *view, struct wlr_surface *surface); static void xwayland_view_unmap(struct view *view); static struct xwayland_view * @@ -256,11 +255,8 @@ want_deco(struct wlr_xwayland_surface *xwayland_surface) } static void -handle_commit(struct wl_listener *listener, void *data) +xwayland_view_commit(struct view *view) { - struct view *view = wl_container_of(listener, view, commit); - assert(data && data == view->surface); - /* Must receive commit signal before accessing surface->current* */ struct wlr_surface_state *state = &view->surface->current; struct wlr_box *current = &view->current; @@ -319,7 +315,7 @@ handle_associate(struct wl_listener *listener, void *data) assert(xwayland_view->xwayland_surface && xwayland_view->xwayland_surface->surface); - view_connect_map(&xwayland_view->base, + view_set_surface(&xwayland_view->base, xwayland_view->xwayland_surface->surface); } @@ -329,18 +325,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); + view_set_surface(&xwayland_view->base, NULL); } static void @@ -350,8 +335,6 @@ handle_destroy(struct wl_listener *listener, void *data) struct xwayland_view *xwayland_view = xwayland_view_from_view(view); assert(xwayland_view->xwayland_surface->data == view); - set_surface(view, NULL); - /* * Break view <-> xsurface association. Note that the xsurface * may not actually be destroyed at this point; it may become an @@ -559,6 +542,9 @@ handle_set_override_redirect(struct wl_listener *listener, void *data) if (mapped) { xwayland_view_unmap(view); } + if (view->surface) { + handle_dissociate(&xwayland_view->dissociate, NULL); + } handle_destroy(&view->destroy, xsurface); /* view is invalid after this point */ xwayland_unmanaged_create(server, xsurface, mapped); @@ -774,24 +760,6 @@ set_initial_position(struct view *view, view_adjust_for_layout_change(view); } -static void -set_surface(struct view *view, struct wlr_surface *surface) -{ - if (view->surface) { - /* Disconnect wlr_surface event listeners */ - 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); - } -} - static void xwayland_view_map(struct view *view) { @@ -800,6 +768,7 @@ xwayland_view_map(struct view *view) xwayland_view->xwayland_surface; assert(xwayland_surface); assert(xwayland_surface->surface); + assert(xwayland_surface->surface == view->surface); if (view->mapped) { return; @@ -815,18 +784,14 @@ xwayland_view_map(struct view *view) 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; } /* @@ -879,6 +844,16 @@ xwayland_view_unmap(struct view *view) } view->mapped = false; view_impl_unmap(view); + + /* + * Destroy the content_tree at unmap. It would get destroyed + * with the wlr_surface later, but that might be a while, + * especially if the surface gets converted to unmanaged. + */ + if (view->content_tree) { + wlr_scene_node_destroy(&view->content_tree->node); + view->content_tree = NULL; + } } static void @@ -986,6 +961,7 @@ static const struct view_impl xwayland_view_impl = { .set_activated = xwayland_view_set_activated, .set_fullscreen = xwayland_view_set_fullscreen, .unmap = xwayland_view_unmap, + .commit = xwayland_view_commit, .maximize = xwayland_view_maximize, .minimize = xwayland_view_minimize, .get_parent = xwayland_view_get_parent,