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

@@ -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);

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) {
+	view_set_surface(view, NULL);
-		mappable_disconnect(&view->mappable);
-	}
 
 	wl_list_remove(&view->request_move.link);
 	wl_list_remove(&view->request_resize.link);

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_set_surface(view, xdg_surface->surface);
-	view->surface = xdg_surface->surface;
-	view->surface->data = tree;
 
-	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);

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

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 */
+	view_set_surface(&xwayland_view->base, NULL);
-	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);
 }
 
 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) {
+	if (!view->content_tree) {
-		set_surface(view, xwayland_surface->surface);
+		view->content_tree = wlr_scene_subsurface_tree_create(
-
-		/* Will be free'd automatically once the surface is being destroyed */
-		struct wlr_scene_tree *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,