Add wl_fixes.ack_global_remove()

The wl_global_remove() function was introduce to help mitigate clients
getting unintentionally disconnected if a global is added and removed in
a short burst.

The intended usage was:

 - the compositor calls wl_global_remove()
 - after a certain period of time, the compositor calls
   wl_global_destroy()

Unfortunately, it did not fully fix the issue due to the way monotonic
clock works on Linux. Specifically, it can tick even during sleep.

This change adds a slightly better way to handle global removal. With
the proposed changes, the clients need to signal to the compositor that
they won't bind the global anymore.

After all clients have acknowledged a wl_registry.global_remove, the
compositor can finally destroy the global.

Signed-off-by: Vlad Zahorodnii <vlad.zahorodnii@kde.org>

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)
-		return;
+	if (strcmp(intf, "test") == 0) {
+		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);
-	assert(c->tc && "Failed binding to registry");
-
-	wl_proxy_add_listener((struct wl_proxy *) c->tc,
-			      (void *) &tc_listener, c);
+		wl_proxy_add_listener((struct wl_proxy *) c->tc,
+				      (void *) &tc_listener, c);
+	} else if (strcmp(intf, wl_fixes_interface.name) == 0) {
+		c->wl_fixes = wl_registry_bind(registry, id, &wl_fixes_interface, 2);
+	}
 }
 
 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;