xdg-shell: emit xdg_surface destroy when role object is destroyed

Fixes https://github.com/swaywm/wlroots/issues/1407

include/types/wlr_xdg_shell.h

@@ -18,6 +18,7 @@ struct wlr_xdg_surface *create_xdg_surface(
 	struct wlr_xdg_client *client, struct wlr_surface *surface,
 	uint32_t id);
 void unmap_xdg_surface(struct wlr_xdg_surface *surface);
+void reset_xdg_surface(struct wlr_xdg_surface *xdg_surface);
 void destroy_xdg_surface(struct wlr_xdg_surface *surface);
 void handle_xdg_surface_commit(struct wlr_surface *wlr_surface);
 void handle_xdg_surface_precommit(struct wlr_surface *wlr_surface);

types/xdg_shell/wlr_xdg_popup.c

@@ -207,9 +207,7 @@ static const struct xdg_popup_interface xdg_popup_implementation = {
 static void xdg_popup_handle_resource_destroy(struct wl_resource *resource) {
 	struct wlr_xdg_surface *surface =
 		wlr_xdg_surface_from_popup_resource(resource);
-	if (surface != NULL) {
 	destroy_xdg_popup(surface);
-	}
 }
 
 const struct wlr_surface_role xdg_popup_surface_role = {
@@ -241,14 +239,13 @@ void create_xdg_popup(struct wlr_xdg_surface *xdg_surface,
 		return;
 	}
 
-	if (xdg_surface->popup == NULL) {
+	assert(xdg_surface->popup == NULL);
 	xdg_surface->popup = calloc(1, sizeof(struct wlr_xdg_popup));
 	if (!xdg_surface->popup) {
 		wl_resource_post_no_memory(xdg_surface->resource);
 		return;
 	}
 	xdg_surface->popup->base = xdg_surface;
-	}
 
 	xdg_surface->popup->resource = wl_resource_create(
 		xdg_surface->client->client, &xdg_popup_interface,
@@ -280,18 +277,11 @@ void create_xdg_popup(struct wlr_xdg_surface *xdg_surface,
 }
 
 void destroy_xdg_popup(struct wlr_xdg_surface *xdg_surface) {
+	if (xdg_surface == NULL) {
+		return;
+	}
 	assert(xdg_surface->role == WLR_XDG_SURFACE_ROLE_POPUP);
-	unmap_xdg_surface(xdg_surface);
+	reset_xdg_surface(xdg_surface);
-
-	// Don't destroy the popup state yet, the compositor might have some
-	// listeners set up. Anyway the client can only re-create another xdg-popup
-	// with this xdg-surface because of role restrictions.
-	wl_resource_set_user_data(xdg_surface->popup->resource, NULL);
-	xdg_surface->toplevel->resource = NULL;
-
-	wl_list_remove(&xdg_surface->popup->link);
-
-	xdg_surface->role = WLR_XDG_SURFACE_ROLE_NONE;
 }
 
 void wlr_xdg_popup_get_anchor_point(struct wlr_xdg_popup *popup,

types/xdg_shell/wlr_xdg_surface.c

@@ -439,39 +439,53 @@ struct wlr_xdg_surface *create_xdg_surface(
 	return xdg_surface;
 }
 
-void destroy_xdg_surface(struct wlr_xdg_surface *surface) {
+void reset_xdg_surface(struct wlr_xdg_surface *xdg_surface) {
-	if (surface->role != WLR_XDG_SURFACE_ROLE_NONE) {
+	if (xdg_surface->role != WLR_XDG_SURFACE_ROLE_NONE) {
-		unmap_xdg_surface(surface);
+		unmap_xdg_surface(xdg_surface);
 	}
 
-	wlr_signal_emit_safe(&surface->events.destroy, surface);
+	if (xdg_surface->added) {
-
+		wlr_signal_emit_safe(&xdg_surface->events.destroy, xdg_surface);
-	struct wlr_xdg_popup *popup_state, *next;
+		xdg_surface->added = false;
-	wl_list_for_each_safe(popup_state, next, &surface->popups, link) {
-		xdg_popup_send_popup_done(popup_state->resource);
-		destroy_xdg_popup(popup_state->base);
 	}
 
-	switch (surface->role) {
+	switch (xdg_surface->role) {
 	case WLR_XDG_SURFACE_ROLE_TOPLEVEL:
-		destroy_xdg_toplevel(surface);
+		wl_resource_set_user_data(xdg_surface->toplevel->resource, NULL);
+		xdg_surface->toplevel->resource = NULL;
+
+		free(xdg_surface->toplevel);
+		xdg_surface->toplevel = NULL;
 		break;
 	case WLR_XDG_SURFACE_ROLE_POPUP:
-		destroy_xdg_popup(surface);
+		wl_resource_set_user_data(xdg_surface->popup->resource, NULL);
+		xdg_surface->toplevel->resource = NULL;
+
+		wl_list_remove(&xdg_surface->popup->link);
+
+		free(xdg_surface->popup);
+		xdg_surface->popup = NULL;
 		break;
 	case WLR_XDG_SURFACE_ROLE_NONE:
 		// This space is intentionally left blank
 		break;
 	}
 
-	if (surface->surface->role == &xdg_toplevel_surface_role) {
+	xdg_surface->role = WLR_XDG_SURFACE_ROLE_NONE;
-		free(surface->toplevel);
+}
-	} else if (surface->surface->role == &xdg_popup_surface_role) {
+
-		free(surface->popup);
+void destroy_xdg_surface(struct wlr_xdg_surface *surface) {
+	reset_xdg_surface(surface);
+
+	struct wlr_xdg_popup *popup_state, *next;
+	wl_list_for_each_safe(popup_state, next, &surface->popups, link) {
+		xdg_popup_send_popup_done(popup_state->resource);
+		destroy_xdg_popup(popup_state->base);
 	}
 
 	wl_resource_set_user_data(surface->resource, NULL);
 	surface->surface->role_data = NULL;
+
 	wl_list_remove(&surface->link);
 	wl_list_remove(&surface->surface_destroy.link);
 	wl_list_remove(&surface->surface_commit.link);
@@ -625,7 +639,8 @@ void wlr_xdg_surface_for_each_popup(struct wlr_xdg_surface *surface,
 	xdg_surface_for_each_popup(surface, 0, 0, iterator, user_data);
 }
 
-void wlr_xdg_surface_get_geometry(struct wlr_xdg_surface *surface, struct wlr_box *box) {
+void wlr_xdg_surface_get_geometry(struct wlr_xdg_surface *surface,
+		struct wlr_box *box) {
 	wlr_surface_get_extends(surface->surface, box);
 	/* The client never set the geometry */
 	if (!surface->geometry.width) {

types/xdg_shell/wlr_xdg_toplevel.c

@@ -444,9 +444,7 @@ static const struct xdg_toplevel_interface xdg_toplevel_implementation = {
 static void xdg_toplevel_handle_resource_destroy(struct wl_resource *resource) {
 	struct wlr_xdg_surface *surface =
 		wlr_xdg_surface_from_toplevel_resource(resource);
-	if (surface != NULL) {
 	destroy_xdg_toplevel(surface);
-	}
 }
 
 const struct wlr_surface_role xdg_toplevel_surface_role = {
@@ -469,12 +467,14 @@ void create_xdg_toplevel(struct wlr_xdg_surface *xdg_surface,
 		return;
 	}
 
-	if (xdg_surface->toplevel == NULL) {
+	assert(xdg_surface->toplevel == NULL);
 	xdg_surface->toplevel = calloc(1, sizeof(struct wlr_xdg_toplevel));
 	if (xdg_surface->toplevel == NULL) {
 		wl_resource_post_no_memory(xdg_surface->resource);
 		return;
 	}
+	xdg_surface->toplevel->base = xdg_surface;
+
 	wl_signal_init(&xdg_surface->toplevel->events.request_maximize);
 	wl_signal_init(&xdg_surface->toplevel->events.request_fullscreen);
 	wl_signal_init(&xdg_surface->toplevel->events.request_minimize);
@@ -485,12 +485,6 @@ void create_xdg_toplevel(struct wlr_xdg_surface *xdg_surface,
 	wl_signal_init(&xdg_surface->toplevel->events.set_title);
 	wl_signal_init(&xdg_surface->toplevel->events.set_app_id);
 
-		xdg_surface->toplevel->base = xdg_surface;
-	}
-
-	xdg_surface->role = WLR_XDG_SURFACE_ROLE_TOPLEVEL;
-
-	assert(xdg_surface->toplevel->resource == NULL);
 	xdg_surface->toplevel->resource = wl_resource_create(
 		xdg_surface->client->client, &xdg_toplevel_interface,
 		wl_resource_get_version(xdg_surface->resource), id);
@@ -502,19 +496,16 @@ void create_xdg_toplevel(struct wlr_xdg_surface *xdg_surface,
 	wl_resource_set_implementation(xdg_surface->toplevel->resource,
 		&xdg_toplevel_implementation, xdg_surface,
 		xdg_toplevel_handle_resource_destroy);
+
+	xdg_surface->role = WLR_XDG_SURFACE_ROLE_TOPLEVEL;
 }
 
 void destroy_xdg_toplevel(struct wlr_xdg_surface *xdg_surface) {
+	if (xdg_surface == NULL) {
+		return;
+	}
 	assert(xdg_surface->role == WLR_XDG_SURFACE_ROLE_TOPLEVEL);
-	unmap_xdg_surface(xdg_surface);
+	reset_xdg_surface(xdg_surface);
-
-	// Don't destroy the toplevel state yet, the compositor might have some
-	// listeners set up. Anyway the client can only re-create another
-	// xdg-toplevel with this xdg-surface because of role restrictions.
-	wl_resource_set_user_data(xdg_surface->toplevel->resource, NULL);
-	xdg_surface->toplevel->resource = NULL;
-
-	xdg_surface->role = WLR_XDG_SURFACE_ROLE_NONE;
 }
 
 uint32_t wlr_xdg_toplevel_set_size(struct wlr_xdg_surface *surface,