From 268e8373ae20ad9c38e983ed444b8ed6746ff30d Mon Sep 17 00:00:00 2001 From: Vlad Zahorodnii Date: Fri, 13 Mar 2026 17:35:27 +0200 Subject: [PATCH] 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 --- protocol/wayland.xml | 40 +++++- src/wayland-server-core.h | 12 ++ src/wayland-server.c | 273 ++++++++++++++++++++++++++++++++------ tests/display-test.c | 223 +++++++++++++++++++++++++++++++ tests/test-compositor.c | 22 ++- tests/test-compositor.h | 1 + 6 files changed, 521 insertions(+), 50 deletions(-) diff --git a/protocol/wayland.xml b/protocol/wayland.xml index 513b8fd8..11f0de36 100644 --- a/protocol/wayland.xml +++ b/protocol/wayland.xml @@ -3335,12 +3335,20 @@ - + This global fixes problems with other core-protocol interfaces that cannot be fixed in these interfaces themselves. + + + These errors can be emitted in response to wl_fixes requests. + + + + @@ -3360,6 +3368,36 @@ + + + + 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. + + + + diff --git a/src/wayland-server-core.h b/src/wayland-server-core.h index c8a64772..cc08de58 100644 --- a/src/wayland-server-core.h +++ b/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 diff --git a/src/wayland-server.c b/src/wayland-server.c index 31cc9824..932ac662 100644 --- a/src/wayland-server.c +++ b/src/wayland-server.c @@ -115,17 +115,31 @@ 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; }; +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 { struct wl_object object; wl_resource_destroy_func_t destroy; @@ -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_announce(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(®istry->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->removed && 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, ®istry->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(®istry->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(®istry->offer_list); + wl_resource_set_implementation(registry_resource, ®istry_interface, - display, unbind_resource); + registry, unbind_registry_resource); wl_list_insert(&display->registry_resource_list, ®istry_resource->link); wl_list_for_each(global, &display->global_list, link) if (wl_global_is_visible(client, global) && !global->removed) - wl_resource_post_event(registry_resource, - WL_REGISTRY_GLOBAL, - global->name, - global->interface->name, - global->version); + wl_global_announce(global, registry_resource); } static const struct wl_display_interface display_interface = { @@ -1399,52 +1487,24 @@ wl_global_create(struct wl_display *display, global->version = version; global->data = data; global->bind = bind; + global->withdrawn = NULL; global->removed = 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_REGISTRY_GLOBAL, - global->name, - global->interface->name, - global->version); + wl_global_announce(global, resource); return global; } -/** Remove the global - * - * \param global The Wayland global. - * - * 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 - * 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. - * - * \since 1.17.90 - */ -WL_EXPORT void -wl_global_remove(struct wl_global *global) +static void +wl_global_send_removed(struct wl_global *global) { struct wl_display *display = global->display; 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) if (wl_global_is_visible(resource->client, global)) wl_resource_post_event(resource, WL_REGISTRY_GLOBAL_REMOVE, @@ -1453,12 +1513,100 @@ wl_global_remove(struct wl_global *global) global->removed = true; } +/** Remove the global + * + * \param global The Wayland global. + * + * Broadcast a global remove event to all clients without destroying the + * global. This function can only be called once per global. + * + * This can be used by compositors to mitigate clients being disconnected + * because a global has been added and removed too quickly. + * + * 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) +{ + if (global->removed) + wl_abort("wl_global_remove: called twice on the same " + "global '%s#%"PRIu32"'", global->interface->name, + global->name); + + wl_global_send_removed(global); + + 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) { + struct wl_global_offer *offer; + if (!global->removed) - wl_global_remove(global); + wl_global_send_removed(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 +2857,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->removed) { + 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 diff --git a/tests/display-test.c b/tests/display-test.c index fe78b521..e6efa41e 100644 --- a/tests/display-test.c +++ b/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, + ®istry_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, + ®istry_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) { diff --git a/tests/test-compositor.c b/tests/test-compositor.c index f412fe6e..c45d3f17 100644 --- a/tests/test-compositor.c +++ b/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); diff --git a/tests/test-compositor.h b/tests/test-compositor.h index 662a81c3..4d94f3fb 100644 --- a/tests/test-compositor.h +++ b/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;