Merge branch 'ack-global-remove' into 'main'

Add wl_global_ack_remove()

See merge request wayland/wayland!533

protocol/wayland.xml

@@ -3335,12 +3335,20 @@
     </request>
   </interface>
 
-  <interface name="wl_fixes" version="1">
+  <interface name="wl_fixes" version="2">
     <description summary="wayland protocol fixes">
       This global fixes problems with other core-protocol interfaces that
       cannot be fixed in these interfaces themselves.
     </description>
 
+    <enum name="error">
+      <description summary="wl_fixes error values">
+        These errors can be emitted in response to wl_fixes requests.
+      </description>
+      <entry name="invalid_ack_remove" value="0"
+             summary="unknown global or the global is not removed"/>
+    </enum>
+
     <request name="destroy" type="destructor">
       <description summary="destroys this object"/>
     </request>
@@ -3360,6 +3368,36 @@
       <arg name="registry" type="object" interface="wl_registry"
            summary="the registry to destroy"/>
     </request>
+
+    <request name="ack_global_remove" since="2">
+      <description summary="acknowledge global removal">
+        Acknowledge the removal of the specified global.
+
+        If no global with the specified name exists or the global is not removed,
+        the wl_fixes.invalid_ack_remove protocol error will be posted.
+
+        Due to the Wayland protocol being asynchronous, the wl_global objects
+        cannot be destroyed immediately. For example, if a wl_global is removed
+        and a client attempts to bind that global around same time, it can
+        result in a protocol error due to an unknown global name in the bind
+        request.
+
+        In order to avoid crashing clients, the compositor should remove the
+        wl_global once it is guaranteed that no more bind requests will come.
+
+        The wl_fixes.ack_global_remove() request is used to signal to the
+        compositor that the client will not bind the given global anymore. After
+        all clients acknowledge the removal of the global, the compositor can
+        safely destroy it.
+
+        The client must call the wl_fixes.ack_global_remove() request in
+        response to a wl_registry.global_remove() event even if it did not bind
+        the corresponding global.
+      </description>
+      <arg name="registry" type="object" interface="wl_registry"
+           summary="the registry object"/>
+      <arg name="name" type="uint" summary="unique name of the global"/>
+    </request>
   </interface>
 
 </protocol>

src/wayland-server-core.h

@@ -222,10 +222,13 @@ wl_display_set_default_max_buffer_size(struct wl_display *display,
 				       size_t max_buffer_size);
 
 struct wl_client;
+struct wl_global;
 
 typedef void (*wl_global_bind_func_t)(struct wl_client *client, void *data,
 				      uint32_t version, uint32_t id);
 
+typedef void (*wl_global_withdrawn_func_t)(struct wl_global *global);
+
 uint32_t
 wl_display_get_serial(const struct wl_display *display);
 
@@ -253,6 +256,10 @@ wl_global_create(struct wl_display *display,
 void
 wl_global_remove(struct wl_global *global);
 
+void
+wl_global_set_withdrawn_listener(struct wl_global *global,
+				 wl_global_withdrawn_func_t func);
+
 void
 wl_global_destroy(struct wl_global *global);
 
@@ -725,6 +732,11 @@ wl_display_add_protocol_logger(struct wl_display *display,
 void
 wl_protocol_logger_destroy(struct wl_protocol_logger *logger);
 
+void
+wl_fixes_handle_ack_global_remove(struct wl_resource *fixes_resource,
+				  struct wl_resource *registry_resource,
+				  uint32_t global_name);
+
 #ifdef  __cplusplus
 }
 #endif

src/wayland-server.c

@@ -115,15 +115,29 @@ struct wl_display {
 	size_t max_buffer_size;
 };
 
+struct wl_registry {
+	struct wl_display *display;
+	struct wl_list offer_list;
+};
+
 struct wl_global {
 	struct wl_display *display;
 	const struct wl_interface *interface;
+	struct wl_list offer_list;
 	uint32_t name;
 	uint32_t version;
 	void *data;
 	wl_global_bind_func_t bind;
+	wl_global_withdrawn_func_t withdrawn;
 	struct wl_list link;
-	bool removed;
+	bool unpublished;
+};
+
+struct wl_global_offer {
+	struct wl_list global_link; /**< in struct wl_global::offer_list */
+	struct wl_list registry_link; /**< in struct wl_registry::offer_list */
+	struct wl_resource *registry_resource;
+	struct wl_global *global;
 };
 
 struct wl_resource {
@@ -1048,7 +1062,8 @@ registry_bind(struct wl_client *client,
 	      const char *interface, uint32_t version, uint32_t id)
 {
 	struct wl_global *global;
-	struct wl_display *display = resource->data;
+	struct wl_registry *registry = resource->data;
+	struct wl_display *display = registry->display;
 
 	wl_list_for_each(global, &display->global_list, link)
 		if (global->name == name)
@@ -1100,9 +1115,76 @@ display_sync(struct wl_client *client,
 }
 
 static void
-unbind_resource(struct wl_resource *resource)
+wl_global_publish(struct wl_global *global,
+		  struct wl_resource *registry_resource)
 {
+	struct wl_global_offer *offer;
+	struct wl_registry *registry = registry_resource->data;
+
+	offer = zalloc(sizeof *offer);
+	if (offer == NULL) {
+		wl_resource_post_no_memory(registry_resource);
+		return;
+	}
+
+	offer->registry_resource = registry_resource;
+	offer->global = global;
+	wl_list_insert(&global->offer_list, &offer->global_link);
+	wl_list_insert(&registry->offer_list, &offer->registry_link);
+
+	wl_resource_post_event(registry_resource,
+			       WL_REGISTRY_GLOBAL,
+			       global->name,
+			       global->interface->name,
+			       global->version);
+}
+
+static void
+wl_global_offer_destroy(struct wl_global_offer *offer)
+{
+	wl_list_remove(&offer->global_link);
+	wl_list_remove(&offer->registry_link);
+	free(offer);
+}
+
+static void
+wl_global_offer_done(struct wl_global_offer *offer)
+{
+	struct wl_global *global = offer->global;
+
+	wl_global_offer_destroy(offer);
+
+	if (global->unpublished && global->withdrawn && wl_list_empty(&global->offer_list))
+		global->withdrawn(global);
+}
+
+static struct wl_global_offer *
+wl_registry_find_offer(struct wl_registry *registry,
+		       uint32_t global_name)
+{
+	struct wl_global_offer *offer;
+
+	wl_list_for_each(offer, &registry->offer_list, registry_link) {
+		if (offer->global->name == global_name)
+			return offer;
+	}
+
+	return NULL;
+}
+
+static void
+unbind_registry_resource(struct wl_resource *resource)
+{
+	struct wl_registry *registry = resource->data;
+	struct wl_global_offer *offer;
+
+	while (!wl_list_empty(&registry->offer_list)) {
+		offer = wl_container_of(registry->offer_list.next, offer, registry_link);
+		wl_global_offer_done(offer);
+	}
+
 	wl_list_remove(&resource->link);
+	free(registry);
 }
 
 static void
@@ -1112,6 +1194,7 @@ display_get_registry(struct wl_client *client,
 	struct wl_display *display = resource->data;
 	struct wl_resource *registry_resource;
 	struct wl_global *global;
+	struct wl_registry *registry;
 
 	registry_resource =
 		wl_resource_create(client, &wl_registry_interface, 1, id);
@@ -1120,20 +1203,25 @@ display_get_registry(struct wl_client *client,
 		return;
 	}
 
+	registry = zalloc(sizeof *registry);
+	if (registry == NULL) {
+		wl_client_post_no_memory(client);
+		return;
+	}
+
+	registry->display = display;
+	wl_list_init(&registry->offer_list);
+
 	wl_resource_set_implementation(registry_resource,
 				       &registry_interface,
-				       display, unbind_resource);
+				       registry, unbind_registry_resource);
 
 	wl_list_insert(&display->registry_resource_list,
 		       &registry_resource->link);
 
 	wl_list_for_each(global, &display->global_list, link)
-		if (wl_global_is_visible(client, global) && !global->removed)
+		if (wl_global_is_visible(client, global) && !global->unpublished)
-			wl_resource_post_event(registry_resource,
+			wl_global_publish(global, registry_resource);
-					       WL_REGISTRY_GLOBAL,
-					       global->name,
-					       global->interface->name,
-					       global->version);
 }
 
 static const struct wl_display_interface display_interface = {
@@ -1399,20 +1487,30 @@ wl_global_create(struct wl_display *display,
 	global->version = version;
 	global->data = data;
 	global->bind = bind;
-	global->removed = false;
+	global->withdrawn = NULL;
+	global->unpublished = false;
 	wl_list_insert(display->global_list.prev, &global->link);
+	wl_list_init(&global->offer_list);
 
 	wl_list_for_each(resource, &display->registry_resource_list, link)
 		if (wl_global_is_visible(resource->client, global))
-			wl_resource_post_event(resource,
+			wl_global_publish(global, resource);
-					       WL_REGISTRY_GLOBAL,
-					       global->name,
-					       global->interface->name,
-					       global->version);
 
 	return global;
 }
 
+static void
+wl_global_unpublish(struct wl_global *global)
+{
+	struct wl_global_offer *offer;
+
+	wl_list_for_each(offer, &global->offer_list, global_link)
+		wl_resource_post_event(offer->registry_resource,
+				       WL_REGISTRY_GLOBAL_REMOVE, global->name);
+
+	global->unpublished = true;
+}
+
 /** Remove the global
  *
  * \param global The Wayland global.
@@ -1420,45 +1518,93 @@ wl_global_create(struct wl_display *display,
  * Broadcast a global remove event to all clients without destroying the
  * global. This function can only be called once per global.
  *
- * wl_global_destroy() removes the global and immediately destroys it. On
- * the other end, this function only removes the global, allowing clients
- * that have not yet received the global remove event to continue to bind to
- * it.
- *
  * This can be used by compositors to mitigate clients being disconnected
- * because a global has been added and removed too quickly. Compositors can call
+ * because a global has been added and removed too quickly.
- * wl_global_remove(), then wait an implementation-defined amount of time, then
- * call wl_global_destroy(). Note that the destruction of a global is still
- * racy, since clients have no way to acknowledge that they received the remove
- * event.
  *
+ * Due to the Wayland protocol being asynchronous, the wl_global objects
+ * cannot be destroyed immediately. For example, if a wl_global is destroyed
+ * and a client attempts to bind that global around same time, it can
+ * result in a protocol error due to an unknown global name in the bind request.
+ *
+ * In order to avoid crashing clients, the compositor should destroy the
+ * wl_global once it is guaranteed that no more bind requests will come.
+ *
+ * The recommended way to destroy globals is as follows:
+ *
+ * - the compositor registers a callback using the wl_global_set_withdrawn_listener()
+ *   function, which will be called when it is safe to call wl_global_destroy();
+ * - the compositor calls wl_global_remove(). This will broadcast
+ *   wl_registry.global_remove events to all clients;
+ * - upon receiving a wl_registry.global_remove event, a client must
+ *   reply to it by calling the wl_fixes.ack_global_remove() request. The
+ *   client must call the wl_fixes.ack_global_remove() request even if it
+ *   did not bind the global;
+ * - the compositor will call the wl_fixes_handle_ack_global_remove() function when
+ *   it receives the wl_fixes.ack_global_remove() request from the client;
+ * - after all clients have acknowledged the global removal, the "withdrawn"
+ *   callback will be called to notify the compositor that the
+ *   wl_global_destroy() function can be called now.
+ *
+ * \sa wl_global_set_withdrawn_listener
+ * \sa wl_fixes_handle_ack_global_remove
  * \since 1.17.90
  */
 WL_EXPORT void
 wl_global_remove(struct wl_global *global)
 {
-	struct wl_display *display = global->display;
+	if (global->unpublished)
-	struct wl_resource *resource;
-
-	if (global->removed)
 		wl_abort("wl_global_remove: called twice on the same "
 			 "global '%s#%"PRIu32"'", global->interface->name,
 			 global->name);
 
-	wl_list_for_each(resource, &display->registry_resource_list, link)
+	wl_global_unpublish(global);
-		if (wl_global_is_visible(resource->client, global))
-			wl_resource_post_event(resource, WL_REGISTRY_GLOBAL_REMOVE,
-					       global->name);
 
-	global->removed = true;
+	if (global->withdrawn && wl_list_empty(&global->offer_list))
+		global->withdrawn(global);
+}
+
+/** Set the withdrawn callback.
+ *
+ * \param global The Wayland global.
+ * \param func The withdrawn callback.
+ *
+ * The withdrawn callback notifies the compositor that the global can be
+ * safely destroyed by calling wl_global_destroy(). The callback will be called
+ * in the following cases:
+ *
+ * - immediately by wl_global_remove(), if there are no registeries where the
+ *   global was announced;
+ * - or some time later after wl_global_remove(), due to either receiving the
+ *   last wl_fixes_handle_ack_global_remove() or getting the last wl_registry
+ *   where the global was announced destroyed.
+ *
+ * The withdrawn callback will never be called without prior wl_global_remove().
+ *
+ * \sa wl_fixes_handle_ack_global_remove
+ * \since 1.26
+ */
+WL_EXPORT void
+wl_global_set_withdrawn_listener(struct wl_global *global,
+				 wl_global_withdrawn_func_t func)
+{
+	global->withdrawn = func;
 }
 
 WL_EXPORT void
 wl_global_destroy(struct wl_global *global)
 {
-	if (!global->removed)
+	struct wl_global_offer *offer;
-		wl_global_remove(global);
+
+	if (!global->unpublished)
+		wl_global_unpublish(global);
+
 	wl_list_remove(&global->link);
+
+	while (!wl_list_empty(&global->offer_list)) {
+		offer = wl_container_of(global->offer_list.next, offer, global_link);
+		wl_global_offer_destroy(offer);
+	}
+
 	free(global);
 }
 
@@ -2709,6 +2855,47 @@ wl_display_remove_global(struct wl_display *display, struct wl_global *global)
 	wl_global_destroy(global);
 }
 
+/** Acknowledge global removal by a client
+ *
+ * \param fixes_resource The wl_fixes resource.
+ * \param registry_resource The wl_registry resource.
+ * \param global_name The unique id of the global.
+ *
+ * The compositor should call this function from the wl_fixes.ack_global_remove
+ * request implementation.
+ *
+ * libwayland-server automatically takes care of the implied ack-remove due
+ * to client disconnection or explicit wl_resource_destroy() of the wl_registry
+ * resource.
+ *
+ * \sa wl_global_remove
+ * \sa wl_global_set_withdrawn_listener
+ * \since 1.26
+ */
+WL_EXPORT void
+wl_fixes_handle_ack_global_remove(struct wl_resource *fixes_resource,
+				  struct wl_resource *registry_resource,
+				  uint32_t global_name)
+{
+	struct wl_global_offer *offer;
+	struct wl_registry *registry = registry_resource->data;
+
+	offer = wl_registry_find_offer(registry, global_name);
+	if (!offer) {
+		wl_resource_post_error(fixes_resource, WL_FIXES_ERROR_INVALID_ACK_REMOVE,
+				       "the given registry did not announce global %u", global_name);
+		return;
+	}
+
+	if (!offer->global->unpublished) {
+		wl_resource_post_error(fixes_resource, WL_FIXES_ERROR_INVALID_ACK_REMOVE,
+				       "global %u is not removed", global_name);
+		return;
+	}
+
+	wl_global_offer_done(offer);
+}
+
 /** \endcond */
 
 /* Functions at the end of this file are deprecated.  Instead of adding new

tests/display-test.c

@@ -51,6 +51,63 @@
 #include "tests-server-protocol.h"
 #include "tests-client-protocol.h"
 
+static void
+registry_handle_global_noop(void *data,
+			    struct wl_registry *registry,
+			    uint32_t id, const char *interface,
+			    uint32_t ver)
+{
+}
+
+static void
+registry_handle_global_remove_noop(void *data,
+				   struct wl_registry *registry,
+				   uint32_t id)
+{
+}
+
+static void
+fixes_destroy(struct wl_client *client,
+	      struct wl_resource *resource)
+{
+	wl_resource_destroy(resource);
+}
+
+static void
+fixes_destroy_registry(struct wl_client *client,
+		       struct wl_resource *resource,
+		       struct wl_resource *registry)
+{
+	wl_resource_destroy(registry);
+}
+
+static void
+fixes_ack_global_remove(struct wl_client *client,
+			struct wl_resource *resource,
+			struct wl_resource *registry,
+			uint32_t global_id)
+{
+	wl_fixes_handle_ack_global_remove(resource, registry, global_id);
+}
+
+static const struct wl_fixes_interface fixes_implementation = {
+	.destroy = fixes_destroy,
+	.destroy_registry = fixes_destroy_registry,
+	.ack_global_remove = fixes_ack_global_remove,
+};
+
+static void
+bind_fixes(struct wl_client *client,
+	   void *data,
+	   uint32_t version,
+	   uint32_t id)
+{
+	struct wl_resource *resource;
+
+	resource = wl_resource_create(client, &wl_fixes_interface, version, id);
+	wl_resource_set_implementation(resource, &fixes_implementation, NULL, NULL);
+}
+
 struct display_destroy_listener {
 	struct wl_listener listener;
 	int done;
@@ -1695,6 +1752,172 @@ TEST(global_remove)
 	display_destroy(d);
 }
 
+static void
+registry_ack_global_remove_handle_global_remove(void *data,
+						struct wl_registry *registry,
+						uint32_t id)
+{
+	struct client *client = data;
+
+	wl_fixes_ack_global_remove(client->wl_fixes, registry, id);
+}
+
+static struct wl_registry_listener registry_ack_global_remove_listener = {
+	.global = registry_handle_global_noop,
+	.global_remove = registry_ack_global_remove_handle_global_remove,
+};
+
+static void
+ack_global_remove_client(void *data)
+{
+	struct client *c = client_connect();
+	struct wl_registry *registry;
+	struct wl_registry_listener *registry_listener = data;
+	int ret;
+
+	registry = wl_display_get_registry(c->wl_display);
+	wl_registry_add_listener(registry,
+				 registry_listener,
+				 c);
+
+	ret = wl_display_roundtrip(c->wl_display);
+	assert(ret >= 0);
+
+	/* yield the control back to the compositor so it can remove
+	 * the global */
+	assert(stop_display(c, 1) >= 0);
+
+	/* check if there are any global_remove events */
+	ret = wl_display_roundtrip(c->wl_display);
+	assert(ret >= 0);
+
+	/* yield the control back to the compositor so it can check
+	 * whether the global is withdrawn */
+	assert(stop_display(c, 1) >= 0);
+
+	wl_registry_destroy(registry);
+
+	client_disconnect(c);
+}
+
+static void
+mark_global_withdrawn(struct wl_global *global)
+{
+	bool *withdrawn = wl_global_get_user_data(global);
+
+	*withdrawn = true;
+}
+
+TEST(ack_global_remove)
+{
+	struct display *d;
+	struct wl_global *seat;
+	struct wl_global *fixes;
+	bool withdrawn = false;
+
+	d = display_create();
+
+	fixes = wl_global_create(d->wl_display, &wl_fixes_interface,
+				 2, NULL, bind_fixes);
+
+	seat = wl_global_create(d->wl_display, &wl_seat_interface,
+				1, &withdrawn, NULL);
+
+	wl_global_set_withdrawn_listener(seat, mark_global_withdrawn);
+
+	client_create(d, ack_global_remove_client,
+		      &registry_ack_global_remove_listener);
+
+	display_run(d);
+	assert(!withdrawn);
+
+	wl_global_remove(seat);
+
+	/* the global will be marked as withdrawn after the client
+	 * acknowledges the wl_registry.global_remove event */
+	display_resume(d);
+	assert(withdrawn);
+
+	/* the global will be still marked as withdrawn even after
+	 * the client disconnects */
+	display_resume(d);
+	assert(withdrawn);
+
+	wl_global_destroy(seat);
+	wl_global_destroy(fixes);
+
+	display_destroy(d);
+}
+
+static struct wl_registry_listener registry_no_ack_global_remove_listener = {
+	.global = registry_handle_global_noop,
+	.global_remove = registry_handle_global_remove_noop,
+};
+
+TEST(no_ack_global_remove)
+{
+	struct display *d;
+	struct wl_global *seat;
+	struct wl_global *fixes;
+	bool withdrawn = false;
+
+	d = display_create();
+
+	fixes = wl_global_create(d->wl_display, &wl_fixes_interface,
+				 2, NULL, bind_fixes);
+
+	seat = wl_global_create(d->wl_display, &wl_seat_interface,
+				1, &withdrawn, NULL);
+
+	wl_global_set_withdrawn_listener(seat, mark_global_withdrawn);
+
+	client_create(d, ack_global_remove_client,
+		      &registry_no_ack_global_remove_listener);
+
+	display_run(d);
+	assert(!withdrawn);
+
+	wl_global_remove(seat);
+
+	/* if the client does not ack the wl_registry.global_remove event,
+	 * the global will not be marked as withdrawn */
+	display_resume(d);
+	assert(!withdrawn);
+
+	/* when the client disconnects, the wl_registry.global_remove will
+	 * be implicitly acknowledged and the global can be destroyed then */
+	display_resume(d);
+	assert(withdrawn);
+
+	wl_global_destroy(seat);
+	wl_global_destroy(fixes);
+
+	display_destroy(d);
+}
+
+TEST(global_remove_without_clients)
+{
+	struct display *d;
+	struct wl_global *global;
+	bool withdrawn = false;
+
+	d = display_create();
+
+	global = wl_global_create(d->wl_display, &wl_seat_interface,
+				  1, &withdrawn, NULL);
+
+	wl_global_set_withdrawn_listener(global, mark_global_withdrawn);
+
+	/* if there are no clients at all, the global can be destroyed
+	 * immediately */
+	wl_global_remove(global);
+	assert(withdrawn);
+
+	wl_global_destroy(global);
+
+	display_destroy(d);
+}
+
 static void
 dispatch_single_read_events(struct wl_display *d)
 {

tests/test-compositor.c

@@ -491,14 +491,15 @@ registry_handle_globals(void *data, struct wl_registry *registry,
 {
 	struct client *c = data;
 
-	if (strcmp(intf, "test") != 0)
+	if (strcmp(intf, "test") == 0) {
-		return;
+		c->tc = wl_registry_bind(registry, id, &test_compositor_interface, ver);
+		assert(c->tc && "Failed binding to registry");
 
-	c->tc = wl_registry_bind(registry, id, &test_compositor_interface, ver);
+		wl_proxy_add_listener((struct wl_proxy *) c->tc,
-	assert(c->tc && "Failed binding to registry");
+				      (void *) &tc_listener, c);
-
+	} else if (strcmp(intf, wl_fixes_interface.name) == 0) {
-	wl_proxy_add_listener((struct wl_proxy *) c->tc,
+		c->wl_fixes = wl_registry_bind(registry, id, &wl_fixes_interface, 2);
-			      (void *) &tc_listener, c);
+	}
 }
 
 static const struct wl_registry_listener registry_listener =
@@ -524,6 +525,9 @@ struct client *client_connect(void)
 	wl_display_roundtrip(c->wl_display);
 	assert(c->tc);
 
+	if (c->wl_fixes) {
+		wl_fixes_destroy_registry(c->wl_fixes, reg);
+	}
 	wl_registry_destroy(reg);
 
 	return c;
@@ -556,6 +560,10 @@ client_disconnect(struct client *c)
 	/* check for errors */
 	check_error(c->wl_display);
 
+	if (c->wl_fixes) {
+		wl_fixes_destroy(c->wl_fixes);
+	}
+
 	wl_proxy_destroy((struct wl_proxy *) c->tc);
 	wl_display_disconnect(c->wl_display);
 	free(c);

tests/test-compositor.h

@@ -66,6 +66,7 @@ struct display {
  * filled. */
 struct client {
 	struct wl_display *wl_display;
+	struct wl_fixes *wl_fixes;
 	struct test_compositor *tc;
 
 	atomic_bool display_stopped;