Merge pull request #1373 from emersion/fix-teardown-segfaults

xdg-shell: don't destroy xdg role state on role destroy

3 files changed, 71 insertions, 41 deletions

diff --git a/types/xdg_shell/wlr_xdg_popup.c b/types/xdg_shell/wlr_xdg_popup.c
index b3409261..d23d7dac 100644
--- a/types/xdg_shell/wlr_xdg_popup.c
+++ b/types/xdg_shell/wlr_xdg_popup.c
@@ -234,12 +234,22 @@ void create_xdg_popup(struct wlr_xdg_surface *xdg_surface,
 		return;
 	}
 
-	xdg_surface->popup = calloc(1, sizeof(struct wlr_xdg_popup));
-	if (!xdg_surface->popup) {
-		wl_resource_post_no_memory(xdg_surface->resource);
+	if (xdg_surface->role != WLR_XDG_SURFACE_ROLE_NONE) {
+		wl_resource_post_error(xdg_surface->resource,
+			XDG_SURFACE_ERROR_ALREADY_CONSTRUCTED,
+			"xdg-surface has already been constructed");
 		return;
 	}
 
+	if (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,
 		wl_resource_get_version(xdg_surface->resource), id);
@@ -253,7 +263,6 @@ void create_xdg_popup(struct wlr_xdg_surface *xdg_surface,
 		xdg_popup_handle_resource_destroy);
 
 	xdg_surface->role = WLR_XDG_SURFACE_ROLE_POPUP;
-	xdg_surface->popup->base = xdg_surface;
 
 	// positioner properties
 	memcpy(&xdg_surface->popup->positioner, &positioner->attrs,
@@ -265,19 +274,24 @@ void create_xdg_popup(struct wlr_xdg_surface *xdg_surface,
 		xdg_surface->popup->parent = parent->surface;
 		wl_list_insert(&parent->popups, &xdg_surface->popup->link);
 		wlr_signal_emit_safe(&parent->events.new_popup, xdg_surface->popup);
+	} else {
+		wl_list_init(&xdg_surface->popup->link);
 	}
 }
 
-void destroy_xdg_popup(struct wlr_xdg_surface *surface) {
-	assert(surface->role == WLR_XDG_SURFACE_ROLE_POPUP);
-	unmap_xdg_surface(surface);
+void destroy_xdg_popup(struct wlr_xdg_surface *xdg_surface) {
+	assert(xdg_surface->role == WLR_XDG_SURFACE_ROLE_POPUP);
+	unmap_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_resource_set_user_data(surface->popup->resource, NULL);
-	wl_list_remove(&surface->popup->link);
-	free(surface->popup);
-	surface->popup = NULL;
+	wl_list_remove(&xdg_surface->popup->link);
 
-	surface->role = WLR_XDG_SURFACE_ROLE_NONE;
+	xdg_surface->role = WLR_XDG_SURFACE_ROLE_NONE;
 }
 
 void wlr_xdg_popup_get_anchor_point(struct wlr_xdg_popup *popup,
diff --git a/types/xdg_shell/wlr_xdg_surface.c b/types/xdg_shell/wlr_xdg_surface.c
index e32b7f18..0b1d599f 100644
--- a/types/xdg_shell/wlr_xdg_surface.c
+++ b/types/xdg_shell/wlr_xdg_surface.c
@@ -83,18 +83,6 @@ void unmap_xdg_surface(struct wlr_xdg_surface *surface) {
 	memset(&surface->next_geometry, 0, sizeof(struct wlr_box));
 }
 
-void destroy_xdg_toplevel(struct wlr_xdg_surface *surface) {
-	assert(surface->role == WLR_XDG_SURFACE_ROLE_TOPLEVEL);
-	unmap_xdg_surface(surface);
-
-	wl_resource_set_user_data(surface->toplevel->resource, NULL);
-	free(surface->toplevel);
-	surface->toplevel = NULL;
-
-	surface->role = WLR_XDG_SURFACE_ROLE_NONE;
-}
-
-
 
 static void xdg_surface_handle_ack_configure(struct wl_client *client,
 		struct wl_resource *resource, uint32_t serial) {
@@ -476,6 +464,12 @@ void destroy_xdg_surface(struct wlr_xdg_surface *surface) {
 		break;
 	}
 
+	if (surface->surface->role == &xdg_toplevel_surface_role) {
+		free(surface->toplevel);
+	} else if (surface->surface->role == &xdg_popup_surface_role) {
+		free(surface->popup);
+	}
+
 	wl_resource_set_user_data(surface->resource, NULL);
 	surface->surface->role_data = NULL;
 	wl_list_remove(&surface->link);
diff --git a/types/xdg_shell/wlr_xdg_toplevel.c b/types/xdg_shell/wlr_xdg_toplevel.c
index 77398c7e..d3220994 100644
--- a/types/xdg_shell/wlr_xdg_toplevel.c
+++ b/types/xdg_shell/wlr_xdg_toplevel.c
@@ -462,37 +462,59 @@ void create_xdg_toplevel(struct wlr_xdg_surface *xdg_surface,
 		return;
 	}
 
-	xdg_surface->toplevel = calloc(1, sizeof(struct wlr_xdg_toplevel));
-	if (xdg_surface->toplevel == NULL) {
-		wl_resource_post_no_memory(xdg_surface->resource);
+	if (xdg_surface->role != WLR_XDG_SURFACE_ROLE_NONE) {
+		wl_resource_post_error(xdg_surface->resource,
+			XDG_SURFACE_ERROR_ALREADY_CONSTRUCTED,
+			"xdg-surface has already been constructed");
 		return;
 	}
-	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);
-	wl_signal_init(&xdg_surface->toplevel->events.request_move);
-	wl_signal_init(&xdg_surface->toplevel->events.request_resize);
-	wl_signal_init(&xdg_surface->toplevel->events.request_show_window_menu);
-	wl_signal_init(&xdg_surface->toplevel->events.set_parent);
-	wl_signal_init(&xdg_surface->toplevel->events.set_title);
-	wl_signal_init(&xdg_surface->toplevel->events.set_app_id);
+
+	if (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;
+		}
+		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);
+		wl_signal_init(&xdg_surface->toplevel->events.request_move);
+		wl_signal_init(&xdg_surface->toplevel->events.request_resize);
+		wl_signal_init(&xdg_surface->toplevel->events.request_show_window_menu);
+		wl_signal_init(&xdg_surface->toplevel->events.set_parent);
+		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;
-	xdg_surface->toplevel->base = xdg_surface;
 
-	struct wl_resource *toplevel_resource = wl_resource_create(
+	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);
-	if (toplevel_resource == NULL) {
+	if (xdg_surface->toplevel->resource == NULL) {
 		free(xdg_surface->toplevel);
 		wl_resource_post_no_memory(xdg_surface->resource);
 		return;
 	}
-	wl_resource_set_implementation(toplevel_resource,
+	wl_resource_set_implementation(xdg_surface->toplevel->resource,
 		&xdg_toplevel_implementation, xdg_surface,
 		xdg_toplevel_handle_resource_destroy);
+}
+
+void destroy_xdg_toplevel(struct wlr_xdg_surface *xdg_surface) {
+	assert(xdg_surface->role == WLR_XDG_SURFACE_ROLE_TOPLEVEL);
+	unmap_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->toplevel->resource = toplevel_resource;
+	xdg_surface->role = WLR_XDG_SURFACE_ROLE_NONE;
 }
 
 uint32_t wlr_xdg_toplevel_set_size(struct wlr_xdg_surface *surface,