From 520ef1ae5b42dbbc02964c6167a42348ba54f9b5 Mon Sep 17 00:00:00 2001 From: Nikolay Borodin Date: Thu, 5 Sep 2024 22:37:45 +0200 Subject: [PATCH 01/24] core: fixed segmentation fault in handle_pending_updates --- src/wm/win.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/wm/win.c b/src/wm/win.c index 82b942cbf5..2c508a127c 100644 --- a/src/wm/win.c +++ b/src/wm/win.c @@ -235,6 +235,13 @@ static inline void win_release_mask(backend_t *base, struct win *w) { } } +static inline void win_release_saved_win_image(backend_t *base, struct win *w) { + if (w->saved_win_image) { + base->ops.release_image(base, w->saved_win_image); + w->saved_win_image = NULL; + } +} + void win_release_images(struct backend_base *backend, struct win *w) { // We don't want to decide what we should do if the image we want to // release is stale (do we clear the stale flags or not?) But if we are @@ -244,6 +251,7 @@ void win_release_images(struct backend_base *backend, struct win *w) { win_release_pixmap(backend, w); win_release_shadow(backend, w); win_release_mask(backend, w); + win_release_saved_win_image(backend, w); } /// Returns true if the `prop` property is stale, as well as clears the stale From ed619f3f17233071743b2ce5ecd0c69d4c81156b Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Tue, 3 Sep 2024 10:07:04 +0100 Subject: [PATCH 02/24] x: wrap some libxcb functions To make it possible to stub them out. These are the only requests wm.c makes to the X server, we will be able to test the wm code on its own if we can provide mocked version of these functions. Signed-off-by: Yuxuan Shui --- src/wm/wm.c | 19 +++++++------------ src/x.c | 26 ++++++++++++++++++++++++++ src/x.h | 15 +++++++++++++++ 3 files changed, 48 insertions(+), 12 deletions(-) diff --git a/src/wm/wm.c b/src/wm/wm.c index bbabb6545c..0ef520c90d 100644 --- a/src/wm/wm.c +++ b/src/wm/wm.c @@ -496,10 +496,10 @@ wm_handle_query_tree_reply(struct x_connection *c, struct x_async_request_base * } wm_tree_attach(&wm->tree, child_node, node); } + x_flush(c); // Actually send the requests out: free(req); - xcb_flush(c->c); // Actually send the requests if (wm_is_consistent(wm)) { wm_reap_orphans(wm); } @@ -542,9 +542,9 @@ static void wm_handle_get_wm_state_reply(struct x_connection * /*c*/, static void wm_import_start_no_flush(struct wm *wm, struct x_connection *c, struct atom *atoms, xcb_window_t wid, struct wm_tree_node *parent) { log_debug("Starting import process for window %#010x", wid); - x_set_error_action_ignore( - c, xcb_change_window_attributes(c->c, wid, XCB_CW_EVENT_MASK, - (const uint32_t[]){WM_IMPORT_EV_MASK})); + x_change_window_attributes(c, wid, XCB_CW_EVENT_MASK, + (const uint32_t[]){WM_IMPORT_EV_MASK}, + PENDING_REPLY_ACTION_IGNORE); // Try to see if any orphaned window has the same window ID, if so, it must // have been destroyed without us knowing, so we should reuse the node. @@ -587,29 +587,24 @@ static void wm_import_start_no_flush(struct wm *wm, struct x_connection *c, stru } { - auto cookie = xcb_query_tree(c->c, wid); auto req = ccalloc(1, struct wm_query_tree_request); req->base.callback = wm_handle_query_tree_reply; - req->base.sequence = cookie.sequence; req->node = new; req->wm = wm; req->atoms = atoms; req->pending_index = dynarr_len(wm->pending_query_trees); dynarr_push(wm->pending_query_trees, req); - x_await_request(c, &req->base); + x_async_query_tree(c, wid, &req->base); } // (It's OK to resend the get property request even if one is already in-flight, // unlike query tree.) { - auto cookie = - xcb_get_property(c->c, 0, wid, atoms->aWM_STATE, XCB_ATOM_ANY, 0, 2); auto req = ccalloc(1, struct wm_get_property_request); req->base.callback = wm_handle_get_wm_state_reply; - req->base.sequence = cookie.sequence; req->wm = wm; req->wid = wid; - x_await_request(c, &req->base); + x_async_get_property(c, wid, atoms->aWM_STATE, XCB_ATOM_ANY, 0, 2, &req->base); } } @@ -622,7 +617,7 @@ void wm_import_start(struct wm *wm, struct x_connection *c, struct atom *atoms, return; } wm_import_start_no_flush(wm, c, atoms, wid, parent_node); - xcb_flush(c->c); // Actually send the requests + x_flush(c); // Actually send the requests } bool wm_is_consistent(const struct wm *wm) { diff --git a/src/x.c b/src/x.c index 3961c73789..f928be5dff 100644 --- a/src/x.c +++ b/src/x.c @@ -673,6 +673,28 @@ uint32_t x_create_region(struct x_connection *c, const region_t *reg) { return ret; } +void x_change_window_attributes_with_loc(struct x_connection *c, xcb_window_t wid, + uint32_t mask, const uint32_t *values, + enum x_error_action error_action, + const char *func, const char *file, int line) { + x_set_error_action(c, xcb_change_window_attributes(c->c, wid, mask, values).sequence, + error_action, func, file, line); +} + +void x_async_query_tree(struct x_connection *c, xcb_window_t wid, + struct x_async_request_base *req) { + req->sequence = xcb_query_tree(c->c, wid).sequence; + x_await_request(c, req); +} + +void x_async_get_property(struct x_connection *c, xcb_window_t wid, xcb_atom_t atom, + xcb_atom_t type, uint32_t long_offset, uint32_t long_length, + struct x_async_request_base *req) { + req->sequence = + xcb_get_property(c->c, 0, wid, atom, type, long_offset, long_length).sequence; + x_await_request(c, req); +} + void x_destroy_region(struct x_connection *c, xcb_xfixes_region_t r) { if (r != XCB_NONE) { x_set_error_action_debug_abort(c, xcb_xfixes_destroy_region(c->c, r)); @@ -741,6 +763,10 @@ const char *x_strerror(xcb_generic_error_t *e) { e->error_code); } +void x_flush(struct x_connection *c) { + xcb_flush(c->c); +} + /** * Create a pixmap and check that creation succeeded. */ diff --git a/src/x.h b/src/x.h index 1f8b0bead7..f3cb8b2f88 100644 --- a/src/x.h +++ b/src/x.h @@ -333,6 +333,19 @@ bool x_set_region(struct x_connection *c, xcb_xfixes_region_t dst, const region_ /// Create a X region from a pixman region uint32_t x_create_region(struct x_connection *c, const region_t *reg); +void x_change_window_attributes_with_loc(struct x_connection *c, xcb_window_t wid, + uint32_t mask, const uint32_t *values, + enum x_error_action error_action, + const char *func, const char *file, int line); +#define x_change_window_attributes(c, wid, mask, values, action) \ + x_change_window_attributes_with_loc(c, wid, mask, values, action, __func__, \ + __FILE__, __LINE__) +void x_async_query_tree(struct x_connection *c, xcb_window_t wid, + struct x_async_request_base *req); +void x_async_get_property(struct x_connection *c, xcb_window_t wid, xcb_atom_t atom, + xcb_atom_t type, uint32_t long_offset, uint32_t long_length, + struct x_async_request_base *req); + /// Destroy a X region void x_destroy_region(struct x_connection *c, uint32_t region); @@ -364,6 +377,8 @@ void x_print_error_impl(unsigned long serial, uint8_t major, uint16_t minor, */ const char *x_strerror(xcb_generic_error_t *e); +void x_flush(struct x_connection *c); + xcb_pixmap_t x_create_pixmap(struct x_connection *, uint8_t depth, int width, int height); /** From a7ac0f9b54d57d488e05695954e939536447962d Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Tue, 3 Sep 2024 10:09:42 +0100 Subject: [PATCH 03/24] wm: fix confusion caused by window ID reuse If a new window reuses the ID of a previously destroyed window, we would get confused and will not send a query tree request for this new window, thus misses all of its children. This is because we saved destroyed window as orphans, and we assume all orphans are either imported or in the process of being imported, which is not true for destroyed windows. With this commit we no longer save destroyed windows. We did that to gracefully handle a window being destroyed with a query tree request still in-flight. Now we handle it the more obvious way - we mark any in-flight query trees for omission when a window is destroyed. (Now, event if we don't save explicitly destroyed windows anymore, some orphans might get destroyed and have their ID reused without us knowing. This is because we don't get DestroyNotify for orphans. To catch this case, we pessimistically re-query orphans that don't have children.) Even if the window ID is reused after by a newly created window and the query tree actually succeeds with information from the new window, we can't use that result anyway, because at the point when the query tree is answered, we wouldn't have set up the event mask for the new window yet. Signed-off-by: Yuxuan Shui --- src/wm/wm.c | 178 ++++++++++++++++++++++++------------------- src/wm/wm_internal.h | 5 ++ 2 files changed, 106 insertions(+), 77 deletions(-) diff --git a/src/wm/wm.c b/src/wm/wm.c index 0ef520c90d..87c8a7b7ba 100644 --- a/src/wm/wm.c +++ b/src/wm/wm.c @@ -20,7 +20,6 @@ struct wm_query_tree_request { struct wm_tree_node *node; struct wm *wm; struct atom *atoms; - size_t pending_index; }; struct wm_get_property_request { @@ -44,11 +43,11 @@ struct wm { /// created its list of children is always up to date. struct wm_tree_node orphan_root; - /// Tree nodes that have pending async query tree requests. We also have async get - /// property requests, but they are not tracked because they don't affect the tree - /// structure. We guarantee that when there are pending query tree requests, no - /// tree nodes will be freed. This is a dynarr. - struct wm_query_tree_request **pending_query_trees; + /// Number of pending async query tree requests. We also have async get + /// property requests, but they are not tracked because they don't + /// affect the tree structure. We guarantee that when there are pending + /// query tree requests, no tree nodes will be freed. + unsigned n_pending_query_trees; /// Whether cached window leaders should be recalculated. Following tree changes /// will trigger a leader refresh: @@ -255,15 +254,6 @@ void wm_refresh_leaders(struct wm *wm) { } } -static long wm_find_pending_query_tree(struct wm *wm, struct wm_tree_node *node) { - dynarr_foreach(wm->pending_query_trees, i) { - if ((*i)->node == node) { - return i - wm->pending_query_trees; - } - } - return -1; -} - /// Move window `w` so it's right above `below`, if `below` is 0, `w` is moved /// to the bottom of the stack void wm_stack_move_to_above(struct wm *wm, struct wm_ref *cursor, struct wm_ref *below) { @@ -288,7 +278,7 @@ struct wm *wm_new(void) { auto wm = ccalloc(1, struct wm); wm_tree_init(&wm->tree); list_init_head(&wm->orphan_root.children); - wm->pending_query_trees = dynarr_new(struct wm_query_tree_request *, 0); + wm->n_pending_query_trees = 0; return wm; } @@ -314,7 +304,6 @@ void wm_free(struct wm *wm) { wm_tree_clear(&wm->tree); assert(wm_is_consistent(wm)); assert(list_is_empty(&wm->orphan_root.children)); - dynarr_free_pod(wm->pending_query_trees); free(wm); } @@ -333,8 +322,12 @@ static void wm_reap_orphans(struct wm *wm) { list_remove(&node->siblings); if (!list_is_empty(&node->children)) { log_error("Orphaned window %#010x still has children", node->id.x); + list_foreach(struct wm_tree_node, i, &node->children, siblings) { + log_error(" Child: %#010x", i->id.x); + } list_splice(&node->children, &wm->orphan_root.children); } + log_debug("Reaped orphaned window %#010x", node->id.x); HASH_DEL(wm->tree.nodes, node); free(node); } @@ -361,7 +354,13 @@ void wm_destroy(struct wm *wm, xcb_window_t wid) { log_debug("Destroying window %#010x", wid); if (!list_is_empty(&node->children)) { - log_error("Window %#010x is destroyed but it still has children", wid); + log_error("Window %#010x is destroyed but it still has children. " + "Orphaning them.", + wid); + list_foreach(struct wm_tree_node, i, &node->children, siblings) { + log_error(" Child: %#010x", i->id.x); + } + list_splice(&node->children, &wm->orphan_root.children); } if (node == wm->focused_win) { @@ -373,13 +372,15 @@ void wm_destroy(struct wm *wm, xcb_window_t wid) { if (zombie != NULL) { wm_move_win(node, zombie); } - // There could be an in-flight query tree request for this window, we orphan it. - // It will be reaped when all query tree requests are completed. (Or right now if - // the tree is already consistent.) - wm_tree_attach(&wm->tree, node, &wm->orphan_root); - if (wm_is_consistent(wm)) { - wm_reap_orphans(wm); + + if (node->req != NULL) { + // Effectively "cancel" the query tree request by setting `node->req` to + // NULL. This will cause its reply to be ignored. + node->req->node = NULL; + wm->n_pending_query_trees--; } + HASH_DEL(wm->tree.nodes, node); + free(node); } void wm_reap_zombie(struct wm_ref *zombie) { @@ -394,7 +395,7 @@ void wm_reparent(struct wm *wm, xcb_window_t wid, xcb_window_t parent) { // this window later as query tree requests being completed. if (window == NULL) { if (wm_is_consistent(wm)) { - log_error("Window %#010x reparented, but it's not in " + log_error("Window %#010x reparented, but it's not in " "our tree.", wid); } @@ -423,11 +424,14 @@ void wm_reparent(struct wm *wm, xcb_window_t wid, xcb_window_t parent) { // applies to `wm_import_start`. // // Alternatively if the new parent isn't in our tree yet, we orphan the window - // too. - if (new_parent == NULL || wm_find_pending_query_tree(wm, new_parent) != -1) { + // too. Or if we have an orphaned window indicating the new parent was reusing + // a destroyed window's ID, then we know we will re-query the new parent later + // when we encounter it in a query tree reply, so we orphan the window in this + // case as well. + if (new_parent == NULL || new_parent->req != NULL) { log_debug("Window %#010x is attached to window %#010x which is " "currently been queried, orphaning.", - window->id.x, new_parent->id.x); + window->id.x, parent); wm_tree_attach(&wm->tree, window, &wm->orphan_root); } else { wm_tree_attach(&wm->tree, window, new_parent); @@ -441,36 +445,55 @@ void wm_set_has_wm_state(struct wm *wm, struct wm_ref *cursor, bool has_wm_state static const xcb_event_mask_t WM_IMPORT_EV_MASK = XCB_EVENT_MASK_SUBSTRUCTURE_NOTIFY | XCB_EVENT_MASK_PROPERTY_CHANGE; -static void wm_import_start_no_flush(struct wm *wm, struct x_connection *c, struct atom *atoms, - xcb_window_t wid, struct wm_tree_node *parent); +/// Start the import process of `wid`. If `new` is not NULL, it means the window is +/// reusing the same window ID as a previously destroyed window, and that destroyed window +/// is in our orphan tree. In this case, we revive the orphaned window instead of creating +/// a new one. +static void +wm_import_start_no_flush(struct wm *wm, struct x_connection *c, struct atom *atoms, + xcb_window_t wid, struct wm_tree_node *parent, bool is_create); static void wm_handle_query_tree_reply(struct x_connection *c, struct x_async_request_base *base, xcb_raw_generic_event_t *reply_or_error) { auto req = (struct wm_query_tree_request *)base; auto wm = req->wm; - { - auto last_req = dynarr_last(wm->pending_query_trees); - dynarr_remove_swap(req->wm->pending_query_trees, req->pending_index); - last_req->pending_index = req->pending_index; + + auto node = req->node; + + if (node == NULL) { + // If the node was previously destroyed, then it means a newly created + // window has reused its window ID. We should ignore the query tree reply, + // because we haven't setup event mask for this window yet, we won't be + // able to stay up-to-date with this window, and would have to re-query in + // `wm_import_start_no_flush` anyway. And we don't have a node to attach + // the children to anyway. + if (reply_or_error->response_type != 0) { + log_debug("Ignoring query tree reply for unknown window, it has " + "been destroyed."); + } + goto out; } + wm->n_pending_query_trees--; + if (reply_or_error == NULL) { goto out; } - auto node = req->node; - if (reply_or_error->response_type == 0) { // This is an error, most likely the window is gone when we tried // to query it. xcb_generic_error_t *err = (xcb_generic_error_t *)reply_or_error; log_debug("Query tree request for window %#010x failed with " "error %s", - node->id.x, x_strerror(err)); + node == NULL ? 0 : node->id.x, x_strerror(err)); goto out; } + BUG_ON(node->req != req); + node->req = NULL; + xcb_query_tree_reply_t *reply = (xcb_query_tree_reply_t *)reply_or_error; log_debug("Finished querying tree for window %#010x", node->id.x); @@ -479,22 +502,7 @@ wm_handle_query_tree_reply(struct x_connection *c, struct x_async_request_base * xcb_query_tree_children_length(reply)); for (int i = 0; i < xcb_query_tree_children_length(reply); i++) { auto child = children[i]; - auto child_node = wm_tree_find(&wm->tree, child); - if (child_node == NULL) { - wm_import_start_no_flush(wm, c, req->atoms, child, node); - continue; - } - - // If child node exists, it must be a previously orphaned node. - assert(child_node->parent == &wm->orphan_root); - auto zombie = wm_tree_detach(&wm->tree, child_node); - if (zombie != NULL) { - // This only happens if `child_node` is not orphaned, which means - // things are already going wrong. (the assert above would fail - // too). - wm_tree_reap_zombie(zombie); - } - wm_tree_attach(&wm->tree, child_node, node); + wm_import_start_no_flush(wm, c, req->atoms, child, node, false); } x_flush(c); // Actually send the requests @@ -539,16 +547,13 @@ static void wm_handle_get_wm_state_reply(struct x_connection * /*c*/, free(req); } -static void wm_import_start_no_flush(struct wm *wm, struct x_connection *c, struct atom *atoms, - xcb_window_t wid, struct wm_tree_node *parent) { - log_debug("Starting import process for window %#010x", wid); - x_change_window_attributes(c, wid, XCB_CW_EVENT_MASK, - (const uint32_t[]){WM_IMPORT_EV_MASK}, - PENDING_REPLY_ACTION_IGNORE); - - // Try to see if any orphaned window has the same window ID, if so, it must - // have been destroyed without us knowing, so we should reuse the node. +static void +wm_import_start_no_flush(struct wm *wm, struct x_connection *c, struct atom *atoms, + xcb_window_t wid, struct wm_tree_node *parent, bool is_create) { + // Try to see there is a window with an unknown parent with the same ID. If so, we + // now know who its parent is, and we just need to attach it. auto new = wm_tree_find(&wm->tree, wid); + assert(new == NULL || new->parent == &wm->orphan_root); if (new == NULL) { new = wm_tree_new_window(&wm->tree, wid); wm_tree_add_window(&wm->tree, new); @@ -565,35 +570,53 @@ static void wm_import_start_no_flush(struct wm *wm, struct x_connection *c, stru wid, new->parent->id.x, parent->id.x); assert(false); } - + log_debug("Re-attaching orphaned window %#010x to window %#010x", + new->id.x, parent->id.x); auto zombie = wm_tree_detach(&wm->tree, new); if (zombie != NULL) { // This only happens if `new` is not orphaned, which means things // are already going wrong. wm_tree_reap_zombie(zombie); } - // Need to bump the generation number, as `new` is actually an entirely - // new window, just reusing the same window ID. - new->id.gen = wm->tree.gen++; } wm_tree_attach(&wm->tree, new, parent); - // In theory, though very very unlikely, a window could be reparented (so we won't - // receive its DestroyNotify), destroyed, and then a new window was created with - // the same window ID, all before the previous query tree request is completed. In - // this case, we shouldn't resend another query tree request. (And we also know in - // this case the previous get property request isn't completed either.) - if (wm_find_pending_query_tree(wm, new) != -1) { + + if (!list_is_empty(&new->children)) { + // If new is an orphan that has children, it means it must have been a + // window that we have imported. Because it either retained some of its + // children, and a window that has children cannot be destroyed; or it has + // acquired new children via create/reparent events, which means we have + // set up event mask for it. In this case, we don't import it again. + // + // If it doesn't have children, it is possible that this `new` we see here + // is a completely different window just reusing the same window ID as the + // orphan. This is because orphans have parents not yet imported, we won't + // receive destroy events for them, so we won't know when they are + // destroyed. In this case, we treat it as a new window (even though it + // might not be). + BUG_ON(is_create); return; } + log_debug("Starting import process for window %#010x", wid); + x_change_window_attributes(c, wid, XCB_CW_EVENT_MASK, + (const uint32_t[]){WM_IMPORT_EV_MASK}, + PENDING_REPLY_ACTION_IGNORE); + + if (new->req != NULL) { + // If there is already a query tree request in-flight, cancel it. + new->req->node = NULL; + wm->n_pending_query_trees--; + } + { auto req = ccalloc(1, struct wm_query_tree_request); req->base.callback = wm_handle_query_tree_reply; req->node = new; req->wm = wm; req->atoms = atoms; - req->pending_index = dynarr_len(wm->pending_query_trees); - dynarr_push(wm->pending_query_trees, req); + new->req = req; + wm->n_pending_query_trees++; x_async_query_tree(c, wid, &req->base); } @@ -611,17 +634,18 @@ static void wm_import_start_no_flush(struct wm *wm, struct x_connection *c, stru void wm_import_start(struct wm *wm, struct x_connection *c, struct atom *atoms, xcb_window_t wid, struct wm_ref *parent) { struct wm_tree_node *parent_node = parent != NULL ? to_tree_node_mut(parent) : NULL; - if (parent_node != NULL && wm_find_pending_query_tree(wm, parent_node) != -1) { + if (parent_node != NULL && parent_node->req != NULL) { // Parent node is currently being queried, we can't attach the new window // to it as that will change its children list. return; } - wm_import_start_no_flush(wm, c, atoms, wid, parent_node); + + wm_import_start_no_flush(wm, c, atoms, wid, parent_node, true); x_flush(c); // Actually send the requests } bool wm_is_consistent(const struct wm *wm) { - return dynarr_is_empty(wm->pending_query_trees); + return wm->n_pending_query_trees == 0; } bool wm_has_tree_changes(const struct wm *wm) { diff --git a/src/wm/wm_internal.h b/src/wm/wm_internal.h index a255edeb08..a1050f8c55 100644 --- a/src/wm/wm_internal.h +++ b/src/wm/wm_internal.h @@ -29,6 +29,8 @@ struct wm_tree { struct list_node free_changes; }; +struct wm_query_tree_request; + struct wm_tree_node { UT_hash_handle hh; @@ -58,6 +60,9 @@ struct wm_tree_node { struct wm_tree_node *leader_final; xcb_window_t leader; + /// The current in-flight query tree request for this window. + struct wm_query_tree_request *req; + bool has_wm_state : 1; /// Whether this window exists only on our side. A zombie window is a toplevel /// that has been destroyed or reparented (i.e. no long a toplevel) on the X From eb2ea1564c169efb11ca3eb783247e1a2b3ea871 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Tue, 3 Sep 2024 11:02:53 +0100 Subject: [PATCH 04/24] wm: fix reparented window not being imported Before, if we got a reparent event for a previously unknown window, we will ignore that event because we expect this window will later be found in a query tree reply. This is not always true. If a previously unknown window is reparented to a window that is already fully imported, because we won't send query tree for a fully imported window, this new will just never be imported. We will now explicitly initiate the import process in this case. This also applies to new window reusing the ID of a previously destroyed window in our orphan tree. Signed-off-by: Yuxuan Shui --- src/event.c | 2 +- src/wm/wm.c | 128 ++++++++++++++++++++++++++++++++++------------------ src/wm/wm.h | 3 +- 3 files changed, 86 insertions(+), 47 deletions(-) diff --git a/src/event.c b/src/event.c index c2dfdb7075..0ff6917df6 100644 --- a/src/event.c +++ b/src/event.c @@ -479,7 +479,7 @@ static inline void ev_reparent_notify(session_t *ps, xcb_reparent_notify_event_t log_debug("Window %#010x has new parent: %#010x, override_redirect: %d, " "send_event: %#010x", ev->window, ev->parent, ev->override_redirect, ev->event); - wm_reparent(ps->wm, ev->window, ev->parent); + wm_reparent(ps->wm, &ps->c, ps->atoms, ev->window, ev->parent); } static inline void ev_circulate_notify(session_t *ps, xcb_circulate_notify_event_t *ev) { diff --git a/src/wm/wm.c b/src/wm/wm.c index 87c8a7b7ba..14d0179ea4 100644 --- a/src/wm/wm.c +++ b/src/wm/wm.c @@ -387,32 +387,44 @@ void wm_reap_zombie(struct wm_ref *zombie) { wm_tree_reap_zombie(to_tree_node_mut(zombie)); } -void wm_reparent(struct wm *wm, xcb_window_t wid, xcb_window_t parent) { +struct wm_wid_or_node { + union { + xcb_window_t wid; + struct wm_tree_node *node; + }; + bool is_wid; +}; + +/// Start the import process of `wid`. If `new` is not NULL, it means the window is +/// reusing the same window ID as a previously destroyed window, and that destroyed window +/// is in our orphan tree. In this case, we revive the orphaned window instead of creating +/// a new one. +static void +wm_new_or_attach_window(struct wm *wm, struct x_connection *c, struct atom *atoms, + struct wm_wid_or_node window, struct wm_tree_node *parent); + +void wm_reparent(struct wm *wm, struct x_connection *c, struct atom *atoms, + xcb_window_t wid, xcb_window_t parent) { auto window = wm_tree_find(&wm->tree, wid); auto new_parent = wm_tree_find(&wm->tree, parent); + bool new_parent_imported = new_parent != NULL && new_parent->req == NULL; - // We orphan the window here if parent is not found. We will reconnect - // this window later as query tree requests being completed. - if (window == NULL) { - if (wm_is_consistent(wm)) { - log_error("Window %#010x reparented, but it's not in " - "our tree.", - wid); - } - return; - } - - if (window->parent == new_parent) { + if (window && window->parent == new_parent) { // Reparent to the same parent moves the window to the top of the // stack + BUG_ON(!new_parent_imported); wm_tree_move_to_end(&wm->tree, window, false); return; } - auto zombie = wm_tree_detach(&wm->tree, window); - assert(zombie != NULL || window->win == NULL); - if (zombie != NULL) { - wm_move_win(window, zombie); + if (window != NULL) { + log_debug("Detaching window %#010x from window %#010x", wid, + window->parent->id.x); + auto zombie = wm_tree_detach(&wm->tree, window); + assert(zombie != NULL || window->win == NULL); + if (zombie != NULL) { + wm_move_win(window, zombie); + } } // Attaching `window` to `new_parent` will change the children list of @@ -428,14 +440,37 @@ void wm_reparent(struct wm *wm, xcb_window_t wid, xcb_window_t parent) { // a destroyed window's ID, then we know we will re-query the new parent later // when we encounter it in a query tree reply, so we orphan the window in this // case as well. - if (new_parent == NULL || new_parent->req != NULL) { - log_debug("Window %#010x is attached to window %#010x which is " - "currently been queried, orphaning.", - window->id.x, parent); - wm_tree_attach(&wm->tree, window, &wm->orphan_root); + if (!new_parent_imported) { + if (window != NULL) { + log_debug("Window %#010x is attached to window %#010x which is " + "currently been queried, orphaning.", + window->id.x, parent); + wm_tree_attach(&wm->tree, window, &wm->orphan_root); + } + return; + } + + struct wm_wid_or_node wid_or_window = { + .is_wid = window == NULL, + }; + if (window == NULL) { + if (wm_is_consistent(wm)) { + log_error("Window %#010x reparented, but it's not in " + "our tree.", + wid); + } + wid_or_window.wid = wid; } else { - wm_tree_attach(&wm->tree, window, new_parent); + wid_or_window.node = window; } + + /// If a previously unseen window (window reusing a destroyed window's ID is + /// considered unseen as well) is reparented to a window that has been fully + /// imported, we must treat it as a newly created window. Because it will not be + /// included in a query tree reply, so we must initiate its import process + /// explicitly. `wm_new_or_attach_window` will handle this appropriately. + log_debug("Reparented window %#010x to window %#010x", window->id.x, parent); + wm_new_or_attach_window(wm, c, atoms, wid_or_window, new_parent); } void wm_set_has_wm_state(struct wm *wm, struct wm_ref *cursor, bool has_wm_state) { @@ -445,14 +480,6 @@ void wm_set_has_wm_state(struct wm *wm, struct wm_ref *cursor, bool has_wm_state static const xcb_event_mask_t WM_IMPORT_EV_MASK = XCB_EVENT_MASK_SUBSTRUCTURE_NOTIFY | XCB_EVENT_MASK_PROPERTY_CHANGE; -/// Start the import process of `wid`. If `new` is not NULL, it means the window is -/// reusing the same window ID as a previously destroyed window, and that destroyed window -/// is in our orphan tree. In this case, we revive the orphaned window instead of creating -/// a new one. -static void -wm_import_start_no_flush(struct wm *wm, struct x_connection *c, struct atom *atoms, - xcb_window_t wid, struct wm_tree_node *parent, bool is_create); - static void wm_handle_query_tree_reply(struct x_connection *c, struct x_async_request_base *base, xcb_raw_generic_event_t *reply_or_error) { @@ -502,7 +529,9 @@ wm_handle_query_tree_reply(struct x_connection *c, struct x_async_request_base * xcb_query_tree_children_length(reply)); for (int i = 0; i < xcb_query_tree_children_length(reply); i++) { auto child = children[i]; - wm_import_start_no_flush(wm, c, req->atoms, child, node, false); + wm_new_or_attach_window( + wm, c, req->atoms, + (struct wm_wid_or_node){.is_wid = true, .wid = child}, node); } x_flush(c); // Actually send the requests @@ -547,27 +576,35 @@ static void wm_handle_get_wm_state_reply(struct x_connection * /*c*/, free(req); } +/// Create a window for `wid`, or if an orphan with the same ID exists, re-attach it to +/// the tree at `parent`. Send query tree and get property requests for the window if it +/// is possible that it is a new window. An orphan could actually be a completely +/// different window when it comes back, because we don't receive destroy events for +/// orphans. +/// +/// Note this function does not flush the X connection. static void -wm_import_start_no_flush(struct wm *wm, struct x_connection *c, struct atom *atoms, - xcb_window_t wid, struct wm_tree_node *parent, bool is_create) { +wm_new_or_attach_window(struct wm *wm, struct x_connection *c, struct atom *atoms, + struct wm_wid_or_node window, struct wm_tree_node *parent) { // Try to see there is a window with an unknown parent with the same ID. If so, we // now know who its parent is, and we just need to attach it. - auto new = wm_tree_find(&wm->tree, wid); + auto new = window.is_wid ? wm_tree_find(&wm->tree, window.wid) : window.node; assert(new == NULL || new->parent == &wm->orphan_root); if (new == NULL) { - new = wm_tree_new_window(&wm->tree, wid); + BUG_ON(!window.is_wid); + new = wm_tree_new_window(&wm->tree, window.wid); wm_tree_add_window(&wm->tree, new); } else { if (new->parent == parent) { // What's going on??? - log_error("Importing window %#010x a second time", wid); + log_error("Importing window %#010x a second time", new->id.x); assert(false); return; } if (new->parent != &wm->orphan_root) { log_error("Window %#010x appeared in the children list of both " "%#010x (previous) and %#010x (current).", - wid, new->parent->id.x, parent->id.x); + new->id.x, new->parent->id.x, parent->id.x); assert(false); } log_debug("Re-attaching orphaned window %#010x to window %#010x", @@ -594,12 +631,11 @@ wm_import_start_no_flush(struct wm *wm, struct x_connection *c, struct atom *ato // receive destroy events for them, so we won't know when they are // destroyed. In this case, we treat it as a new window (even though it // might not be). - BUG_ON(is_create); return; } - log_debug("Starting import process for window %#010x", wid); - x_change_window_attributes(c, wid, XCB_CW_EVENT_MASK, + log_debug("Starting import process for window %#010x", new->id.x); + x_change_window_attributes(c, new->id.x, XCB_CW_EVENT_MASK, (const uint32_t[]){WM_IMPORT_EV_MASK}, PENDING_REPLY_ACTION_IGNORE); @@ -617,7 +653,7 @@ wm_import_start_no_flush(struct wm *wm, struct x_connection *c, struct atom *ato req->atoms = atoms; new->req = req; wm->n_pending_query_trees++; - x_async_query_tree(c, wid, &req->base); + x_async_query_tree(c, new->id.x, &req->base); } // (It's OK to resend the get property request even if one is already in-flight, @@ -626,8 +662,9 @@ wm_import_start_no_flush(struct wm *wm, struct x_connection *c, struct atom *ato auto req = ccalloc(1, struct wm_get_property_request); req->base.callback = wm_handle_get_wm_state_reply; req->wm = wm; - req->wid = wid; - x_async_get_property(c, wid, atoms->aWM_STATE, XCB_ATOM_ANY, 0, 2, &req->base); + req->wid = new->id.x; + x_async_get_property(c, new->id.x, atoms->aWM_STATE, XCB_ATOM_ANY, 0, 2, + &req->base); } } @@ -640,7 +677,8 @@ void wm_import_start(struct wm *wm, struct x_connection *c, struct atom *atoms, return; } - wm_import_start_no_flush(wm, c, atoms, wid, parent_node, true); + wm_new_or_attach_window( + wm, c, atoms, (struct wm_wid_or_node){.is_wid = true, .wid = wid}, parent_node); x_flush(c); // Actually send the requests } diff --git a/src/wm/wm.h b/src/wm/wm.h index 893905972d..5e8ea4b806 100644 --- a/src/wm/wm.h +++ b/src/wm/wm.h @@ -163,7 +163,8 @@ void wm_refresh_leaders(struct wm *wm); void wm_destroy(struct wm *wm, xcb_window_t wid); /// Remove a zombie window from the window tree. void wm_reap_zombie(struct wm_ref *zombie); -void wm_reparent(struct wm *wm, xcb_window_t wid, xcb_window_t parent); +void wm_reparent(struct wm *wm, struct x_connection *c, struct atom *atoms, + xcb_window_t wid, xcb_window_t parent); void wm_set_has_wm_state(struct wm *wm, struct wm_ref *cursor, bool has_wm_state); /// Start the import process for `wid`. From 9e3b79077fac4ae3dba93b3fc74b43da95e01350 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Tue, 3 Sep 2024 11:22:53 +0100 Subject: [PATCH 05/24] wm: remove a BUG_ON based on an incorrect assumption `node` can legitimately be NULL inside wm_handle_get_wm_state_reply. Signed-off-by: Yuxuan Shui --- src/wm/wm.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/wm/wm.c b/src/wm/wm.c index 14d0179ea4..fdb3b687d6 100644 --- a/src/wm/wm.c +++ b/src/wm/wm.c @@ -568,9 +568,14 @@ static void wm_handle_get_wm_state_reply(struct x_connection * /*c*/, } auto node = wm_tree_find(&req->wm->tree, req->wid); - BUG_ON_NULL(node); // window must exist at this point, but it might be - // freed then recreated while we were waiting for the - // reply. + if (node == NULL) { + // An in-flight query tree request will keep node alive, but if the query + // tree request is completed, this node will be allowed to be destroyed. + // So if we received a DestroyNotify for `node` in between the reply to + // query tree and the reply to get property, `node` will be NULL here. + free(req); + return; + } auto reply = (xcb_get_property_reply_t *)reply_or_error; wm_tree_set_wm_state(&req->wm->tree, node, reply->type != XCB_NONE); free(req); From e8ebb526b0e9ece916ecddfbd366318c8e0cae5e Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Wed, 4 Sep 2024 04:34:43 +0100 Subject: [PATCH 06/24] wm: avoid ABA problem when importing a window Consider this scenario: 1. Window created with ID A, which generates a CreateNotify for A. 2. Window with ID A is destroyed. 3. Another window is created, reusing ID A. 4. We receive the CreateNotify for A. At step 4, we will be querying a different window than what we think we were querying. It might have a different parent from the one in the CreateNotify, and we should also ignore the destroy event in (2), as it wasn't for the window we are querying. To fix that, when we initiate the import process for a window, we no longer attach it to its declared parent, instead we orphan it. We also ignore any destroy event for it while the query tree is in-flight. We let the query tree reply event decide: 1. what's the window's parent, 2. whether the window still exists. Signed-off-by: Yuxuan Shui --- src/wm/wm.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 47 insertions(+), 7 deletions(-) diff --git a/src/wm/wm.c b/src/wm/wm.c index fdb3b687d6..a52ff17f55 100644 --- a/src/wm/wm.c +++ b/src/wm/wm.c @@ -351,6 +351,17 @@ void wm_destroy(struct wm *wm, xcb_window_t wid) { return; } + if (node->req) { + // This window is currently being queried, we will keep it or destroy it + // based on the query tree reply. If query tree actually succeeds, that + // means we were querying a different window, that was created after this + // destroy event was generated. + log_debug("Ignoring destroy event for window %#010x, it's currently " + "being queried.", + wid); + return; + } + log_debug("Destroying window %#010x", wid); if (!list_is_empty(&node->children)) { @@ -504,6 +515,10 @@ wm_handle_query_tree_reply(struct x_connection *c, struct x_async_request_base * wm->n_pending_query_trees--; + BUG_ON(node->req != req); + BUG_ON(node->parent != &wm->orphan_root); + node->req = NULL; + if (reply_or_error == NULL) { goto out; } @@ -513,17 +528,34 @@ wm_handle_query_tree_reply(struct x_connection *c, struct x_async_request_base * // to query it. xcb_generic_error_t *err = (xcb_generic_error_t *)reply_or_error; log_debug("Query tree request for window %#010x failed with " - "error %s", + "error %s, this window doesn't exist anymore, destroying.", node == NULL ? 0 : node->id.x, x_strerror(err)); + BUG_ON(wm_tree_detach(&wm->tree, node) != NULL); + HASH_DEL(wm->tree.nodes, node); + free(node); goto out; } - BUG_ON(node->req != req); - node->req = NULL; - xcb_query_tree_reply_t *reply = (xcb_query_tree_reply_t *)reply_or_error; log_debug("Finished querying tree for window %#010x", node->id.x); + auto parent = wm_tree_find(&wm->tree, reply->parent); + if (reply->parent != XCB_NONE && parent == NULL) { + log_debug("Query tree reply for window %#010x has parent %#010x, " + "which is not in our tree.", + node->id.x, reply->parent); + } else if (parent != NULL && parent->req != NULL) { + log_debug("Query tree reply for window %#010x has parent %#010x is " + "currently being queried.", + node->id.x, reply->parent); + } else { + log_debug("Query tree reply for window %#010x has parent %#010x, " + "attaching.", + node->id.x, reply->parent); + BUG_ON(wm_tree_detach(&wm->tree, node) != NULL); + wm_tree_attach(&wm->tree, node, parent); + } + auto children = xcb_query_tree_children(reply); log_debug("Window %#010x has %d children", node->id.x, xcb_query_tree_children_length(reply)); @@ -594,12 +626,12 @@ wm_new_or_attach_window(struct wm *wm, struct x_connection *c, struct atom *atom // Try to see there is a window with an unknown parent with the same ID. If so, we // now know who its parent is, and we just need to attach it. auto new = window.is_wid ? wm_tree_find(&wm->tree, window.wid) : window.node; - assert(new == NULL || new->parent == &wm->orphan_root); + assert(new == NULL || new->parent == NULL || new->parent == &wm->orphan_root); if (new == NULL) { BUG_ON(!window.is_wid); new = wm_tree_new_window(&wm->tree, window.wid); wm_tree_add_window(&wm->tree, new); - } else { + } else if (new->parent != NULL) { if (new->parent == parent) { // What's going on??? log_error("Importing window %#010x a second time", new->id.x); @@ -621,7 +653,6 @@ wm_new_or_attach_window(struct wm *wm, struct x_connection *c, struct atom *atom wm_tree_reap_zombie(zombie); } } - wm_tree_attach(&wm->tree, new, parent); if (!list_is_empty(&new->children)) { // If new is an orphan that has children, it means it must have been a @@ -636,9 +667,18 @@ wm_new_or_attach_window(struct wm *wm, struct x_connection *c, struct atom *atom // receive destroy events for them, so we won't know when they are // destroyed. In this case, we treat it as a new window (even though it // might not be). + wm_tree_attach(&wm->tree, new, parent); return; } + // If the window might be new, we don't attach it directly to the parent, because + // the window might have already be destroyed and another window created with the + // same ID since we got the event about its creation. We will attach it to the + // appropriate parent when we get the query tree reply. + if (new->parent != &wm->orphan_root) { + wm_tree_attach(&wm->tree, new, &wm->orphan_root); + } + log_debug("Starting import process for window %#010x", new->id.x); x_change_window_attributes(c, new->id.x, XCB_CW_EVENT_MASK, (const uint32_t[]){WM_IMPORT_EV_MASK}, From 0aa620255d4e0c5e73c08295a2654c9eb7dce1dd Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Wed, 4 Sep 2024 06:00:40 +0100 Subject: [PATCH 07/24] wm: set StructureNotifyMask on window Turns out inferring a window's life cycle based on DestroyNotify sent by its parent is incredibly hard. Especially if the window is at any point reparented to a window we haven't imported yet, because we won't be receiving any DestroyNotify for it. And we also can't reliably know if such reparenting has happened. So we now set the StructureNotifyMask, which will give us DestroyNotify sent from the window itself. With this the tracking window life cycle from the point when the event mask is set becomes easy, and we always know if a window is destroyed or not. Setting StructureNotifyMask also means we will get copies of Circulate/Configure/Map/Unmap/ReparentNotify from the window itself, we filter this so existing code doesn't have to change. Only for DestroyNotify we favor the one sent from the window itself. Also, setting event masks on window MUST be done synchronously. We use to just send out a ChangeWindowAttributes request and ignore its result. But it is actually possible, though extremely unlikely, for the ChangeWindowAttributes to fail but the QueryTree later to succeed, if the window is destroyed and another created (during the time between we send these two requests) with the same ID. In that case we would thought we have set up the event mask, but actually didn't. Which would be detrimental. Signed-off-by: Yuxuan Shui --- src/event.c | 25 +++++- src/picom.c | 2 +- src/wm/win.c | 5 +- src/wm/wm.c | 226 +++++++++++++++++---------------------------------- src/wm/wm.h | 2 +- src/x.c | 10 +-- src/x.h | 9 +- 7 files changed, 109 insertions(+), 170 deletions(-) diff --git a/src/event.c b/src/event.c index 0ff6917df6..cca9341e73 100644 --- a/src/event.c +++ b/src/event.c @@ -346,7 +346,7 @@ static inline void ev_create_notify(session_t *ps, xcb_create_notify_event_t *ev ev->window, ev->parent); assert(false); } - wm_import_start(ps->wm, &ps->c, ps->atoms, ev->window, parent); + wm_import_start(ps->wm, &ps->c, ps->atoms, ev->window); } /// Handle configure event of a regular window @@ -399,6 +399,10 @@ static inline void ev_configure_notify(session_t *ps, xcb_configure_notify_event return; } + if (ev->window == ev->event) { + return; + } + if (ev->window == ps->c.screen_info->root) { configure_root(ps); } else { @@ -408,10 +412,18 @@ static inline void ev_configure_notify(session_t *ps, xcb_configure_notify_event static inline void ev_destroy_notify(session_t *ps, xcb_destroy_notify_event_t *ev) { log_debug("{ event: %#010x, id: %#010x }", ev->event, ev->window); + if (ev->event != ev->window) { + return; + } + wm_destroy(ps->wm, ev->window); } static inline void ev_map_notify(session_t *ps, xcb_map_notify_event_t *ev) { + if (ev->window == ev->event) { + return; + } + // Unmap overlay window if it got mapped but we are currently not // in redirected state. if (ps->overlay && ev->window == ps->overlay) { @@ -461,6 +473,10 @@ static inline void ev_unmap_notify(session_t *ps, xcb_unmap_notify_event_t *ev) return; } + if (ev->event == ev->window) { + return; + } + auto cursor = wm_find(ps->wm, ev->window); if (cursor == NULL) { if (wm_is_consistent(ps->wm)) { @@ -479,10 +495,17 @@ static inline void ev_reparent_notify(session_t *ps, xcb_reparent_notify_event_t log_debug("Window %#010x has new parent: %#010x, override_redirect: %d, " "send_event: %#010x", ev->window, ev->parent, ev->override_redirect, ev->event); + if (ev->event == ev->window) { + return; + } wm_reparent(ps->wm, &ps->c, ps->atoms, ev->window, ev->parent); } static inline void ev_circulate_notify(session_t *ps, xcb_circulate_notify_event_t *ev) { + if (ev->event == ev->window) { + return; + } + auto cursor = wm_find(ps->wm, ev->window); if (cursor == NULL) { diff --git a/src/picom.c b/src/picom.c index f18a8de171..1198973919 100644 --- a/src/picom.c +++ b/src/picom.c @@ -2467,7 +2467,7 @@ static session_t *session_init(int argc, char **argv, Display *dpy, } ps->wm = wm_new(); - wm_import_start(ps->wm, &ps->c, ps->atoms, ps->c.screen_info->root, NULL); + wm_import_start(ps->wm, &ps->c, ps->atoms, ps->c.screen_info->root); ps->command_builder = command_builder_new(); ps->expose_rects = dynarr_new(rect_t, 0); diff --git a/src/wm/win.c b/src/wm/win.c index 2c508a127c..dd65e63065 100644 --- a/src/wm/win.c +++ b/src/wm/win.c @@ -1364,8 +1364,9 @@ struct win *win_maybe_allocate(session_t *ps, struct wm_ref *cursor, } // Set window event mask - uint32_t frame_event_mask = - XCB_EVENT_MASK_PROPERTY_CHANGE | XCB_EVENT_MASK_SUBSTRUCTURE_NOTIFY; + uint32_t frame_event_mask = XCB_EVENT_MASK_PROPERTY_CHANGE | + XCB_EVENT_MASK_SUBSTRUCTURE_NOTIFY | + XCB_EVENT_MASK_STRUCTURE_NOTIFY; if (!ps->o.use_ewmh_active_win) { frame_event_mask |= XCB_EVENT_MASK_FOCUS_CHANGE; } diff --git a/src/wm/wm.c b/src/wm/wm.c index a52ff17f55..c5f886010b 100644 --- a/src/wm/wm.c +++ b/src/wm/wm.c @@ -344,23 +344,7 @@ static void wm_move_win(struct wm_tree_node *from, struct wm_tree_node *to) { void wm_destroy(struct wm *wm, xcb_window_t wid) { struct wm_tree_node *node = wm_tree_find(&wm->tree, wid); - if (!node) { - if (wm_is_consistent(wm)) { - log_error("Window %#010x destroyed, but it's not in our tree.", wid); - } - return; - } - - if (node->req) { - // This window is currently being queried, we will keep it or destroy it - // based on the query tree reply. If query tree actually succeeds, that - // means we were querying a different window, that was created after this - // destroy event was generated. - log_debug("Ignoring destroy event for window %#010x, it's currently " - "being queried.", - wid); - return; - } + BUG_ON_NULL(node); log_debug("Destroying window %#010x", wid); @@ -392,6 +376,9 @@ void wm_destroy(struct wm *wm, xcb_window_t wid) { } HASH_DEL(wm->tree.nodes, node); free(node); + if (wm_is_consistent(wm)) { + wm_reap_orphans(wm); + } } void wm_reap_zombie(struct wm_ref *zombie) { @@ -410,17 +397,28 @@ struct wm_wid_or_node { /// reusing the same window ID as a previously destroyed window, and that destroyed window /// is in our orphan tree. In this case, we revive the orphaned window instead of creating /// a new one. -static void -wm_new_or_attach_window(struct wm *wm, struct x_connection *c, struct atom *atoms, - struct wm_wid_or_node window, struct wm_tree_node *parent); +static void wm_import_start_no_flush(struct wm *wm, struct x_connection *c, + struct atom *atoms, xcb_window_t wid); -void wm_reparent(struct wm *wm, struct x_connection *c, struct atom *atoms, - xcb_window_t wid, xcb_window_t parent) { +static void wm_reparent_no_flush(struct wm *wm, struct x_connection *c, struct atom *atoms, + xcb_window_t wid, xcb_window_t parent) { auto window = wm_tree_find(&wm->tree, wid); auto new_parent = wm_tree_find(&wm->tree, parent); bool new_parent_imported = new_parent != NULL && new_parent->req == NULL; - if (window && window->parent == new_parent) { + /// If a previously unseen window is reparented to a window that has been fully + /// imported, we must treat it as a newly created window. Because it will not be + /// included in a query tree reply, so we must initiate its import process + /// explicitly. + if (window == NULL) { + if (wm_is_consistent(wm)) { + log_error("Window %#010x reparented, but it's not in our tree.", wid); + assert(false); + } + return wm_import_start_no_flush(wm, c, atoms, wid); + } + + if (window->parent == new_parent) { // Reparent to the same parent moves the window to the top of the // stack BUG_ON(!new_parent_imported); @@ -428,14 +426,11 @@ void wm_reparent(struct wm *wm, struct x_connection *c, struct atom *atoms, return; } - if (window != NULL) { - log_debug("Detaching window %#010x from window %#010x", wid, - window->parent->id.x); - auto zombie = wm_tree_detach(&wm->tree, window); - assert(zombie != NULL || window->win == NULL); - if (zombie != NULL) { - wm_move_win(window, zombie); - } + log_debug("Detaching window %#010x from window %#010x", wid, window->parent->id.x); + auto zombie = wm_tree_detach(&wm->tree, window); + assert(zombie != NULL || window->win == NULL); + if (zombie != NULL) { + wm_move_win(window, zombie); } // Attaching `window` to `new_parent` will change the children list of @@ -452,51 +447,37 @@ void wm_reparent(struct wm *wm, struct x_connection *c, struct atom *atoms, // when we encounter it in a query tree reply, so we orphan the window in this // case as well. if (!new_parent_imported) { - if (window != NULL) { - log_debug("Window %#010x is attached to window %#010x which is " - "currently been queried, orphaning.", - window->id.x, parent); - wm_tree_attach(&wm->tree, window, &wm->orphan_root); - } + log_debug("Window %#010x is attached to window %#010x which is " + "currently been queried, orphaning.", + window->id.x, parent); + wm_tree_attach(&wm->tree, window, &wm->orphan_root); return; } - struct wm_wid_or_node wid_or_window = { - .is_wid = window == NULL, - }; - if (window == NULL) { - if (wm_is_consistent(wm)) { - log_error("Window %#010x reparented, but it's not in " - "our tree.", - wid); - } - wid_or_window.wid = wid; - } else { - wid_or_window.node = window; - } - - /// If a previously unseen window (window reusing a destroyed window's ID is - /// considered unseen as well) is reparented to a window that has been fully - /// imported, we must treat it as a newly created window. Because it will not be - /// included in a query tree reply, so we must initiate its import process - /// explicitly. `wm_new_or_attach_window` will handle this appropriately. log_debug("Reparented window %#010x to window %#010x", window->id.x, parent); - wm_new_or_attach_window(wm, c, atoms, wid_or_window, new_parent); + wm_tree_attach(&wm->tree, window, new_parent); +} + +void wm_reparent(struct wm *wm, struct x_connection *c, struct atom *atoms, + xcb_window_t wid, xcb_window_t parent) { + wm_reparent_no_flush(wm, c, atoms, wid, parent); + x_flush(c); } void wm_set_has_wm_state(struct wm *wm, struct wm_ref *cursor, bool has_wm_state) { wm_tree_set_wm_state(&wm->tree, to_tree_node_mut(cursor), has_wm_state); } -static const xcb_event_mask_t WM_IMPORT_EV_MASK = - XCB_EVENT_MASK_SUBSTRUCTURE_NOTIFY | XCB_EVENT_MASK_PROPERTY_CHANGE; +static const xcb_event_mask_t WM_IMPORT_EV_MASK = XCB_EVENT_MASK_SUBSTRUCTURE_NOTIFY | + XCB_EVENT_MASK_STRUCTURE_NOTIFY | + XCB_EVENT_MASK_PROPERTY_CHANGE; static void wm_handle_query_tree_reply(struct x_connection *c, struct x_async_request_base *base, xcb_raw_generic_event_t *reply_or_error) { auto req = (struct wm_query_tree_request *)base; + auto atoms = req->atoms; auto wm = req->wm; - auto node = req->node; if (node == NULL) { @@ -510,14 +491,14 @@ wm_handle_query_tree_reply(struct x_connection *c, struct x_async_request_base * log_debug("Ignoring query tree reply for unknown window, it has " "been destroyed."); } - goto out; + free(req); + return; } - wm->n_pending_query_trees--; - BUG_ON(node->req != req); BUG_ON(node->parent != &wm->orphan_root); node->req = NULL; + free(req); if (reply_or_error == NULL) { goto out; @@ -527,13 +508,10 @@ wm_handle_query_tree_reply(struct x_connection *c, struct x_async_request_base * // This is an error, most likely the window is gone when we tried // to query it. xcb_generic_error_t *err = (xcb_generic_error_t *)reply_or_error; - log_debug("Query tree request for window %#010x failed with " - "error %s, this window doesn't exist anymore, destroying.", + log_error("Query tree request for window %#010x failed with " + "error %s, this query should have been cancelled.", node == NULL ? 0 : node->id.x, x_strerror(err)); - BUG_ON(wm_tree_detach(&wm->tree, node) != NULL); - HASH_DEL(wm->tree.nodes, node); - free(node); - goto out; + BUG_ON(false); } xcb_query_tree_reply_t *reply = (xcb_query_tree_reply_t *)reply_or_error; @@ -561,14 +539,14 @@ wm_handle_query_tree_reply(struct x_connection *c, struct x_async_request_base * xcb_query_tree_children_length(reply)); for (int i = 0; i < xcb_query_tree_children_length(reply); i++) { auto child = children[i]; - wm_new_or_attach_window( - wm, c, req->atoms, - (struct wm_wid_or_node){.is_wid = true, .wid = child}, node); + // wm_reparent handles both the case where child is new, and the case + // where the child is an known orphan. + wm_reparent_no_flush(wm, c, atoms, child, node->id.x); } x_flush(c); // Actually send the requests out: - free(req); + wm->n_pending_query_trees--; if (wm_is_consistent(wm)) { wm_reap_orphans(wm); } @@ -613,82 +591,34 @@ static void wm_handle_get_wm_state_reply(struct x_connection * /*c*/, free(req); } -/// Create a window for `wid`, or if an orphan with the same ID exists, re-attach it to -/// the tree at `parent`. Send query tree and get property requests for the window if it -/// is possible that it is a new window. An orphan could actually be a completely -/// different window when it comes back, because we don't receive destroy events for -/// orphans. +/// Create a window for `wid`. Send query tree and get property requests for the window if +/// it is possible that it is a new window. /// /// Note this function does not flush the X connection. -static void -wm_new_or_attach_window(struct wm *wm, struct x_connection *c, struct atom *atoms, - struct wm_wid_or_node window, struct wm_tree_node *parent) { - // Try to see there is a window with an unknown parent with the same ID. If so, we - // now know who its parent is, and we just need to attach it. - auto new = window.is_wid ? wm_tree_find(&wm->tree, window.wid) : window.node; - assert(new == NULL || new->parent == NULL || new->parent == &wm->orphan_root); - if (new == NULL) { - BUG_ON(!window.is_wid); - new = wm_tree_new_window(&wm->tree, window.wid); - wm_tree_add_window(&wm->tree, new); - } else if (new->parent != NULL) { - if (new->parent == parent) { - // What's going on??? - log_error("Importing window %#010x a second time", new->id.x); - assert(false); - return; - } - if (new->parent != &wm->orphan_root) { - log_error("Window %#010x appeared in the children list of both " - "%#010x (previous) and %#010x (current).", - new->id.x, new->parent->id.x, parent->id.x); - assert(false); - } - log_debug("Re-attaching orphaned window %#010x to window %#010x", - new->id.x, parent->id.x); - auto zombie = wm_tree_detach(&wm->tree, new); - if (zombie != NULL) { - // This only happens if `new` is not orphaned, which means things - // are already going wrong. - wm_tree_reap_zombie(zombie); - } - } - - if (!list_is_empty(&new->children)) { - // If new is an orphan that has children, it means it must have been a - // window that we have imported. Because it either retained some of its - // children, and a window that has children cannot be destroyed; or it has - // acquired new children via create/reparent events, which means we have - // set up event mask for it. In this case, we don't import it again. - // - // If it doesn't have children, it is possible that this `new` we see here - // is a completely different window just reusing the same window ID as the - // orphan. This is because orphans have parents not yet imported, we won't - // receive destroy events for them, so we won't know when they are - // destroyed. In this case, we treat it as a new window (even though it - // might not be). - wm_tree_attach(&wm->tree, new, parent); +static void wm_import_start_no_flush(struct wm *wm, struct x_connection *c, + struct atom *atoms, xcb_window_t wid) { + BUG_ON(wm_tree_find(&wm->tree, wid) != NULL); + + auto e = x_change_window_attributes(c, wid, XCB_CW_EVENT_MASK, + (const uint32_t[]){WM_IMPORT_EV_MASK}); + if (e) { + log_debug("Failed to set event mask for window %#010x: %s, ignoring this " + "window.", + wid, x_strerror(e)); + free(e); return; } - // If the window might be new, we don't attach it directly to the parent, because - // the window might have already be destroyed and another window created with the - // same ID since we got the event about its creation. We will attach it to the - // appropriate parent when we get the query tree reply. - if (new->parent != &wm->orphan_root) { - wm_tree_attach(&wm->tree, new, &wm->orphan_root); - } + auto new = wm_tree_new_window(&wm->tree, wid); + wm_tree_add_window(&wm->tree, new); log_debug("Starting import process for window %#010x", new->id.x); - x_change_window_attributes(c, new->id.x, XCB_CW_EVENT_MASK, - (const uint32_t[]){WM_IMPORT_EV_MASK}, - PENDING_REPLY_ACTION_IGNORE); - if (new->req != NULL) { - // If there is already a query tree request in-flight, cancel it. - new->req->node = NULL; - wm->n_pending_query_trees--; - } + // We don't attach the window directly to the parent here, because the + // window might have already be destroyed and another window created + // with the same ID since we got the event about its creation. We will + // attach it to the appropriate parent when we get the query tree reply. + wm_tree_attach(&wm->tree, new, &wm->orphan_root); { auto req = ccalloc(1, struct wm_query_tree_request); @@ -714,17 +644,9 @@ wm_new_or_attach_window(struct wm *wm, struct x_connection *c, struct atom *atom } void wm_import_start(struct wm *wm, struct x_connection *c, struct atom *atoms, - xcb_window_t wid, struct wm_ref *parent) { - struct wm_tree_node *parent_node = parent != NULL ? to_tree_node_mut(parent) : NULL; - if (parent_node != NULL && parent_node->req != NULL) { - // Parent node is currently being queried, we can't attach the new window - // to it as that will change its children list. - return; - } - - wm_new_or_attach_window( - wm, c, atoms, (struct wm_wid_or_node){.is_wid = true, .wid = wid}, parent_node); - x_flush(c); // Actually send the requests + xcb_window_t wid) { + wm_import_start_no_flush(wm, c, atoms, wid); + x_flush(c); } bool wm_is_consistent(const struct wm *wm) { diff --git a/src/wm/wm.h b/src/wm/wm.h index 5e8ea4b806..fb93a2d48a 100644 --- a/src/wm/wm.h +++ b/src/wm/wm.h @@ -205,7 +205,7 @@ void wm_set_has_wm_state(struct wm *wm, struct wm_ref *cursor, bool has_wm_state /// /// (Now you have a glimpse of how much X11 sucks.) void wm_import_start(struct wm *wm, struct x_connection *c, struct atom *atoms, - xcb_window_t wid, struct wm_ref *parent); + xcb_window_t wid); /// Check if there are tree change events bool wm_has_tree_changes(const struct wm *wm); diff --git a/src/x.c b/src/x.c index f928be5dff..c13eb41475 100644 --- a/src/x.c +++ b/src/x.c @@ -673,12 +673,10 @@ uint32_t x_create_region(struct x_connection *c, const region_t *reg) { return ret; } -void x_change_window_attributes_with_loc(struct x_connection *c, xcb_window_t wid, - uint32_t mask, const uint32_t *values, - enum x_error_action error_action, - const char *func, const char *file, int line) { - x_set_error_action(c, xcb_change_window_attributes(c->c, wid, mask, values).sequence, - error_action, func, file, line); +xcb_generic_error_t *x_change_window_attributes(struct x_connection *c, xcb_window_t wid, + uint32_t mask, const uint32_t *values) { + return xcb_request_check( + c->c, xcb_change_window_attributes_checked(c->c, wid, mask, values)); } void x_async_query_tree(struct x_connection *c, xcb_window_t wid, diff --git a/src/x.h b/src/x.h index f3cb8b2f88..8e90558f57 100644 --- a/src/x.h +++ b/src/x.h @@ -333,13 +333,8 @@ bool x_set_region(struct x_connection *c, xcb_xfixes_region_t dst, const region_ /// Create a X region from a pixman region uint32_t x_create_region(struct x_connection *c, const region_t *reg); -void x_change_window_attributes_with_loc(struct x_connection *c, xcb_window_t wid, - uint32_t mask, const uint32_t *values, - enum x_error_action error_action, - const char *func, const char *file, int line); -#define x_change_window_attributes(c, wid, mask, values, action) \ - x_change_window_attributes_with_loc(c, wid, mask, values, action, __func__, \ - __FILE__, __LINE__) +xcb_generic_error_t *x_change_window_attributes(struct x_connection *c, xcb_window_t wid, + uint32_t mask, const uint32_t *values); void x_async_query_tree(struct x_connection *c, xcb_window_t wid, struct x_async_request_base *req); void x_async_get_property(struct x_connection *c, xcb_window_t wid, xcb_atom_t atom, From f36c5e32061848fcbecaba64c70d7d4f9001d933 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Wed, 4 Sep 2024 08:01:03 +0100 Subject: [PATCH 08/24] wm: children don't get to choose who their parents are Well, we tried to use query tree replies to attach windows to correct parents. But that's not good enough. Because the parent can also actively acquire children via reparent events. The result is windows fighting each other for control of their parent pointer. And this is also bad because in we don't know the stacking order of a window from its query tree reply. The conclusion is, a window must be fully responsible for updating its own children list. Because of ABA problem caused by window ID reuse, it's possible for a window to be attached to a wrong parent temporarily. This also means receiving a DestroyNotify from a window X about one of its children with ID Y, doesn't mean the window with ID Y is actually destroyed, if Y is currently attached to the wrong parent. From that DestroyNotify, the only thing we can deduce is that X lost a child with ID Y. Which means events must be handled in two steps: If we get a DestroyNotify from window X about its child Y, we disconnect Y from X. If we get a DestroyNotify from window Y for itself, only then can we really destroy Y. Signed-off-by: Yuxuan Shui --- src/event.c | 16 ++++--- src/picom.c | 2 +- src/wm/wm.c | 124 +++++++++++++++++++++++++++++++++------------------- src/wm/wm.h | 3 +- 4 files changed, 93 insertions(+), 52 deletions(-) diff --git a/src/event.c b/src/event.c index cca9341e73..217a9cd3fc 100644 --- a/src/event.c +++ b/src/event.c @@ -346,7 +346,7 @@ static inline void ev_create_notify(session_t *ps, xcb_create_notify_event_t *ev ev->window, ev->parent); assert(false); } - wm_import_start(ps->wm, &ps->c, ps->atoms, ev->window); + wm_import_start(ps->wm, &ps->c, ps->atoms, ev->window, parent); } /// Handle configure event of a regular window @@ -412,11 +412,13 @@ static inline void ev_configure_notify(session_t *ps, xcb_configure_notify_event static inline void ev_destroy_notify(session_t *ps, xcb_destroy_notify_event_t *ev) { log_debug("{ event: %#010x, id: %#010x }", ev->event, ev->window); + // If we hit an ABA problem, it is possible to get a DestroyNotify event from a + // parent for its child, but not from the child for itself. if (ev->event != ev->window) { - return; + wm_disconnect(ps->wm, ev->window, ev->event); + } else { + wm_destroy(ps->wm, ev->window); } - - wm_destroy(ps->wm, ev->window); } static inline void ev_map_notify(session_t *ps, xcb_map_notify_event_t *ev) { @@ -498,7 +500,11 @@ static inline void ev_reparent_notify(session_t *ps, xcb_reparent_notify_event_t if (ev->event == ev->window) { return; } - wm_reparent(ps->wm, &ps->c, ps->atoms, ev->window, ev->parent); + if (ev->parent != ev->event) { + wm_disconnect(ps->wm, ev->window, ev->event); + } else { + wm_reparent(ps->wm, &ps->c, ps->atoms, ev->window, ev->parent); + } } static inline void ev_circulate_notify(session_t *ps, xcb_circulate_notify_event_t *ev) { diff --git a/src/picom.c b/src/picom.c index 1198973919..f18a8de171 100644 --- a/src/picom.c +++ b/src/picom.c @@ -2467,7 +2467,7 @@ static session_t *session_init(int argc, char **argv, Display *dpy, } ps->wm = wm_new(); - wm_import_start(ps->wm, &ps->c, ps->atoms, ps->c.screen_info->root); + wm_import_start(ps->wm, &ps->c, ps->atoms, ps->c.screen_info->root, NULL); ps->command_builder = command_builder_new(); ps->expose_rects = dynarr_new(rect_t, 0); diff --git a/src/wm/wm.c b/src/wm/wm.c index c5f886010b..05dfa43e86 100644 --- a/src/wm/wm.c +++ b/src/wm/wm.c @@ -342,6 +342,43 @@ static void wm_move_win(struct wm_tree_node *from, struct wm_tree_node *to) { from->win = NULL; } +static void wm_detach_inner(struct wm *wm, struct wm_tree_node *child) { + auto zombie = wm_tree_detach(&wm->tree, child); + assert(zombie != NULL || child->win == NULL); + if (zombie != NULL) { + wm_move_win(child, zombie); + } +} + +/// Disconnect `child` from `parent`. This is called when a window is removed from its +/// parent's children list. This can be a result of the window being destroyed, or +/// reparented to another window. This `child` is attached to the orphan root. +void wm_disconnect(struct wm *wm, xcb_window_t child, xcb_window_t parent) { + auto parent_node = wm_tree_find(&wm->tree, parent); + BUG_ON_NULL(parent_node); + + auto child_node = wm_tree_find(&wm->tree, child); + if (child_node == NULL) { + // X sends DestroyNotify from child first, then from parent. So if the + // child was imported, we will get here. This is the normal case. + // + // This also happens if the child is created then immediately destroyed, + // before we get the CreateNotify event for it. In + // `wm_import_start_no_flush` we will ignore this window because setting + // the event mask will fail. Then once we get the DestroyNotify for it + // from the parent, we get here. + log_debug("Child window %#010x of %#010x is not in our tree, ignoring", + child, parent); + return; + } + + BUG_ON(child_node->parent != parent_node); + + log_debug("Disconnecting window %#010x from window %#010x", child, parent); + wm_detach_inner(wm, child_node); + wm_tree_attach(&wm->tree, child_node, &wm->orphan_root); +} + void wm_destroy(struct wm *wm, xcb_window_t wid) { struct wm_tree_node *node = wm_tree_find(&wm->tree, wid); BUG_ON_NULL(node); @@ -362,11 +399,7 @@ void wm_destroy(struct wm *wm, xcb_window_t wid) { wm->focused_win = NULL; } - auto zombie = wm_tree_detach(&wm->tree, node); - assert(zombie != NULL || node->win == NULL); - if (zombie != NULL) { - wm_move_win(node, zombie); - } + wm_detach_inner(wm, node); if (node->req != NULL) { // Effectively "cancel" the query tree request by setting `node->req` to @@ -397,8 +430,8 @@ struct wm_wid_or_node { /// reusing the same window ID as a previously destroyed window, and that destroyed window /// is in our orphan tree. In this case, we revive the orphaned window instead of creating /// a new one. -static void wm_import_start_no_flush(struct wm *wm, struct x_connection *c, - struct atom *atoms, xcb_window_t wid); +static void wm_import_start_no_flush(struct wm *wm, struct x_connection *c, struct atom *atoms, + xcb_window_t wid, struct wm_tree_node *parent); static void wm_reparent_no_flush(struct wm *wm, struct x_connection *c, struct atom *atoms, xcb_window_t wid, xcb_window_t parent) { @@ -415,7 +448,10 @@ static void wm_reparent_no_flush(struct wm *wm, struct x_connection *c, struct a log_error("Window %#010x reparented, but it's not in our tree.", wid); assert(false); } - return wm_import_start_no_flush(wm, c, atoms, wid); + if (new_parent_imported) { + wm_import_start_no_flush(wm, c, atoms, wid, new_parent); + } + return; } if (window->parent == new_parent) { @@ -426,12 +462,7 @@ static void wm_reparent_no_flush(struct wm *wm, struct x_connection *c, struct a return; } - log_debug("Detaching window %#010x from window %#010x", wid, window->parent->id.x); - auto zombie = wm_tree_detach(&wm->tree, window); - assert(zombie != NULL || window->win == NULL); - if (zombie != NULL) { - wm_move_win(window, zombie); - } + BUG_ON(window->parent != &wm->orphan_root); // Attaching `window` to `new_parent` will change the children list of // `new_parent`, if there is a pending query tree request for `new_parent`, doing @@ -450,11 +481,11 @@ static void wm_reparent_no_flush(struct wm *wm, struct x_connection *c, struct a log_debug("Window %#010x is attached to window %#010x which is " "currently been queried, orphaning.", window->id.x, parent); - wm_tree_attach(&wm->tree, window, &wm->orphan_root); return; } log_debug("Reparented window %#010x to window %#010x", window->id.x, parent); + BUG_ON(wm_tree_detach(&wm->tree, window) != NULL); wm_tree_attach(&wm->tree, window, new_parent); } @@ -487,7 +518,7 @@ wm_handle_query_tree_reply(struct x_connection *c, struct x_async_request_base * // able to stay up-to-date with this window, and would have to re-query in // `wm_import_start_no_flush` anyway. And we don't have a node to attach // the children to anyway. - if (reply_or_error->response_type != 0) { + if (reply_or_error != NULL && reply_or_error->response_type != 0) { log_debug("Ignoring query tree reply for unknown window, it has " "been destroyed."); } @@ -496,11 +527,13 @@ wm_handle_query_tree_reply(struct x_connection *c, struct x_async_request_base * } BUG_ON(node->req != req); - BUG_ON(node->parent != &wm->orphan_root); node->req = NULL; free(req); if (reply_or_error == NULL) { + // The program is quitting... If this happens when wm is in an + // inconsistent state, there could be errors. + // TODO(yshui): fix this goto out; } @@ -517,23 +550,6 @@ wm_handle_query_tree_reply(struct x_connection *c, struct x_async_request_base * xcb_query_tree_reply_t *reply = (xcb_query_tree_reply_t *)reply_or_error; log_debug("Finished querying tree for window %#010x", node->id.x); - auto parent = wm_tree_find(&wm->tree, reply->parent); - if (reply->parent != XCB_NONE && parent == NULL) { - log_debug("Query tree reply for window %#010x has parent %#010x, " - "which is not in our tree.", - node->id.x, reply->parent); - } else if (parent != NULL && parent->req != NULL) { - log_debug("Query tree reply for window %#010x has parent %#010x is " - "currently being queried.", - node->id.x, reply->parent); - } else { - log_debug("Query tree reply for window %#010x has parent %#010x, " - "attaching.", - node->id.x, reply->parent); - BUG_ON(wm_tree_detach(&wm->tree, node) != NULL); - wm_tree_attach(&wm->tree, node, parent); - } - auto children = xcb_query_tree_children(reply); log_debug("Window %#010x has %d children", node->id.x, xcb_query_tree_children_length(reply)); @@ -595,9 +611,25 @@ static void wm_handle_get_wm_state_reply(struct x_connection * /*c*/, /// it is possible that it is a new window. /// /// Note this function does not flush the X connection. -static void wm_import_start_no_flush(struct wm *wm, struct x_connection *c, - struct atom *atoms, xcb_window_t wid) { - BUG_ON(wm_tree_find(&wm->tree, wid) != NULL); +static void wm_import_start_no_flush(struct wm *wm, struct x_connection *c, struct atom *atoms, + xcb_window_t wid, struct wm_tree_node *parent) { + auto new = wm_tree_find(&wm->tree, wid); + if (new != NULL) { + // This happens if a window A is created, destroyed, and then another + // window B reuses the same window ID. We will receive a CreateNotify for + // A, but will import B. Then we will be here again when we receive the + // CreateNotify for B, which we have already imported. + log_debug("Window %#010x is already in our tree, reparenting.", wid); + if (new->parent != &wm->orphan_root) { + log_error("Window %#010x appeared twice in our tree. Once as a " + "child of %#010x, once as a child of %#010x", + wid, new->parent->id.x, parent->id.x); + BUG_ON(true); + } + BUG_ON(wm_tree_detach(&wm->tree, new) != NULL); + wm_tree_attach(&wm->tree, new, parent); + return; + } auto e = x_change_window_attributes(c, wid, XCB_CW_EVENT_MASK, (const uint32_t[]){WM_IMPORT_EV_MASK}); @@ -609,16 +641,12 @@ static void wm_import_start_no_flush(struct wm *wm, struct x_connection *c, return; } - auto new = wm_tree_new_window(&wm->tree, wid); + new = wm_tree_new_window(&wm->tree, wid); wm_tree_add_window(&wm->tree, new); log_debug("Starting import process for window %#010x", new->id.x); - // We don't attach the window directly to the parent here, because the - // window might have already be destroyed and another window created - // with the same ID since we got the event about its creation. We will - // attach it to the appropriate parent when we get the query tree reply. - wm_tree_attach(&wm->tree, new, &wm->orphan_root); + wm_tree_attach(&wm->tree, new, parent); { auto req = ccalloc(1, struct wm_query_tree_request); @@ -644,8 +672,14 @@ static void wm_import_start_no_flush(struct wm *wm, struct x_connection *c, } void wm_import_start(struct wm *wm, struct x_connection *c, struct atom *atoms, - xcb_window_t wid) { - wm_import_start_no_flush(wm, c, atoms, wid); + xcb_window_t wid, struct wm_ref *parent) { + struct wm_tree_node *parent_node = parent != NULL ? to_tree_node_mut(parent) : NULL; + if (parent_node != NULL && parent_node->req != NULL) { + // Parent node is currently being queried, we can't attach the new window + // to it as that will change its children list. + return; + } + wm_import_start_no_flush(wm, c, atoms, wid, parent_node); x_flush(c); } diff --git a/src/wm/wm.h b/src/wm/wm.h index fb93a2d48a..d5b1646a3d 100644 --- a/src/wm/wm.h +++ b/src/wm/wm.h @@ -165,6 +165,7 @@ void wm_destroy(struct wm *wm, xcb_window_t wid); void wm_reap_zombie(struct wm_ref *zombie); void wm_reparent(struct wm *wm, struct x_connection *c, struct atom *atoms, xcb_window_t wid, xcb_window_t parent); +void wm_disconnect(struct wm *wm, xcb_window_t child, xcb_window_t parent); void wm_set_has_wm_state(struct wm *wm, struct wm_ref *cursor, bool has_wm_state); /// Start the import process for `wid`. @@ -205,7 +206,7 @@ void wm_set_has_wm_state(struct wm *wm, struct wm_ref *cursor, bool has_wm_state /// /// (Now you have a glimpse of how much X11 sucks.) void wm_import_start(struct wm *wm, struct x_connection *c, struct atom *atoms, - xcb_window_t wid); + xcb_window_t wid, struct wm_ref *parent); /// Check if there are tree change events bool wm_has_tree_changes(const struct wm *wm); From 7738c5bde0e41e8fbe6b921744a7ae7700affbbe Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Wed, 4 Sep 2024 08:14:19 +0100 Subject: [PATCH 09/24] wm: remove an incorrect assertion We could be getting reparent events for windows we already found out to be non-existent by trying to set its event mask. Signed-off-by: Yuxuan Shui --- src/wm/wm.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/wm/wm.c b/src/wm/wm.c index 05dfa43e86..913cce0f2c 100644 --- a/src/wm/wm.c +++ b/src/wm/wm.c @@ -444,10 +444,6 @@ static void wm_reparent_no_flush(struct wm *wm, struct x_connection *c, struct a /// included in a query tree reply, so we must initiate its import process /// explicitly. if (window == NULL) { - if (wm_is_consistent(wm)) { - log_error("Window %#010x reparented, but it's not in our tree.", wid); - assert(false); - } if (new_parent_imported) { wm_import_start_no_flush(wm, c, atoms, wid, new_parent); } From 5912359d821c36b439db07491cf47e127e50656a Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Wed, 4 Sep 2024 08:22:41 +0100 Subject: [PATCH 10/24] wm: fix an incorrect BUG_ON It is possible for a window in the process of being queried to have an incomplete children list, in which case wm_disconnect might see child/parent mismatches. Signed-off-by: Yuxuan Shui --- src/wm/wm.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/wm/wm.c b/src/wm/wm.c index 913cce0f2c..d689a905e0 100644 --- a/src/wm/wm.c +++ b/src/wm/wm.c @@ -372,7 +372,10 @@ void wm_disconnect(struct wm *wm, xcb_window_t child, xcb_window_t parent) { return; } - BUG_ON(child_node->parent != parent_node); + // If child_node->parent is not parent_node, then the parent must be in the + // process of being queried. In that case the child_node must be orphaned. + BUG_ON(child_node->parent != parent_node && + (parent_node->req == NULL || child_node->parent != &wm->orphan_root)); log_debug("Disconnecting window %#010x from window %#010x", child, parent); wm_detach_inner(wm, child_node); From cd231e4db34583e1ff5ee456050fdc50f4cdde8a Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Wed, 4 Sep 2024 09:12:01 +0100 Subject: [PATCH 11/24] wm: don't reap orphans Orphan windows were once imported, so should have their event masks set, which means we should be getting DestroyNotify when they are destroyed. That should be enough to guarantee that orphans will be destroyed eventually, we don't need to reap them actively. Signed-off-by: Yuxuan Shui --- src/atom.c | 1 - src/wm/wm.c | 32 -------------------------------- 2 files changed, 33 deletions(-) diff --git a/src/atom.c b/src/atom.c index 01b09484c9..cd93e90769 100644 --- a/src/atom.c +++ b/src/atom.c @@ -6,7 +6,6 @@ #include #include "atom.h" -#include "common.h" #include "compiler.h" #include "log.h" #include "utils/cache.h" diff --git a/src/wm/wm.c b/src/wm/wm.c index d689a905e0..f6f2394260 100644 --- a/src/wm/wm.c +++ b/src/wm/wm.c @@ -7,7 +7,6 @@ #include "common.h" #include "log.h" -#include "utils/dynarr.h" #include "utils/list.h" #include "x.h" @@ -308,31 +307,6 @@ void wm_free(struct wm *wm) { free(wm); } -/// Once the window tree reaches a consistent state, we know any tree nodes that are not -/// reachable from the root must have been destroyed, so we can safely free them. -/// -/// There are cases where we won't receive DestroyNotify events for these windows. For -/// example, if a window is reparented to a window that is not yet in our tree, then -/// destroyed, we won't receive a DestroyNotify event for it. -static void wm_reap_orphans(struct wm *wm) { - // Reap orphaned windows - while (!list_is_empty(&wm->orphan_root.children)) { - auto node = - list_entry(wm->orphan_root.children.next, struct wm_tree_node, siblings); - list_remove(&node->siblings); - if (!list_is_empty(&node->children)) { - log_error("Orphaned window %#010x still has children", node->id.x); - list_foreach(struct wm_tree_node, i, &node->children, siblings) { - log_error(" Child: %#010x", i->id.x); - } - list_splice(&node->children, &wm->orphan_root.children); - } - log_debug("Reaped orphaned window %#010x", node->id.x); - HASH_DEL(wm->tree.nodes, node); - free(node); - } -} - /// Move `from->win` to `to->win`, update `win->tree_ref`. static void wm_move_win(struct wm_tree_node *from, struct wm_tree_node *to) { if (from->win != NULL) { @@ -412,9 +386,6 @@ void wm_destroy(struct wm *wm, xcb_window_t wid) { } HASH_DEL(wm->tree.nodes, node); free(node); - if (wm_is_consistent(wm)) { - wm_reap_orphans(wm); - } } void wm_reap_zombie(struct wm_ref *zombie) { @@ -562,9 +533,6 @@ wm_handle_query_tree_reply(struct x_connection *c, struct x_async_request_base * out: wm->n_pending_query_trees--; - if (wm_is_consistent(wm)) { - wm_reap_orphans(wm); - } } static void wm_handle_get_wm_state_reply(struct x_connection * /*c*/, From c1987ab14568798242b6002d6730856157618e2b Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Wed, 4 Sep 2024 16:36:26 +0100 Subject: [PATCH 12/24] core: don't manage window if it has changed while awaiting X reply The first thing we do when we get a new toplevel, is sending a GetWindowAttributes request, and based on its reply, we may allocate a managed window struct for it. Currently we find which tree node to attach this managed window struct by its X window ID only, but we should use the full treeid. When a new toplevel is added, we queue a creation tree change for it which contains a pointer to the tree node. If the tree node is destroyed before the creation tree change is dequeued, we must "cancel" that creation tree change, because otherwise the pointer in it would be dangling. This also means there will be no kill tree change for it either. As a consequence, we have no way to tell anybody about its zombie node if we create one. So we just don't. If we don't create a zombie node for a destroyed node, there will be nowhere to put the managed window struct attached to it if there is one. Which means there cannot be a one. Summing it all up - a toplevel cannot have a managed window struct, if its creation tree change was never dequeued. You can imagine the creation tree change is handing out a "key" to the tree node, and this tree node can only have managed window attached to it via this key. However, if we are getting the tree node via its X window ID only, we bypass this "key" mechanism, essentially we will find tree node with the same X ID even if we don't have the key for it yet. This happens if the window was destroyed, then another window took its ID while we were waiting for the GetWindowAttributes reply, we shouldn't start managing it. Signed-off-by: Yuxuan Shui --- src/picom.c | 27 ++++++++++++++++++--------- src/region.h | 2 +- 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/src/picom.c b/src/picom.c index f18a8de171..da840a71ec 100644 --- a/src/picom.c +++ b/src/picom.c @@ -1488,7 +1488,7 @@ static void handle_x_events_ev(EV_P attr_unused, ev_prepare *w, int revents attr struct new_window_attributes_request { struct x_async_request_base base; struct session *ps; - xcb_window_t wid; + wm_treeid id; }; static void handle_new_window_attributes_reply(struct x_connection * /*c*/, @@ -1496,8 +1496,8 @@ static void handle_new_window_attributes_reply(struct x_connection * /*c*/, xcb_raw_generic_event_t *reply_or_error) { auto req = (struct new_window_attributes_request *)req_base; auto ps = req->ps; - auto wid = req->wid; - auto new_window = wm_find(ps->wm, wid); + auto id = req->id; + auto new_window = wm_find(ps->wm, id.x); free(req); if (reply_or_error == NULL) { @@ -1508,7 +1508,7 @@ static void handle_new_window_attributes_reply(struct x_connection * /*c*/, if (reply_or_error->response_type == 0) { log_debug("Failed to get window attributes for newly created window " "%#010x", - wid); + id.x); return; } @@ -1517,17 +1517,26 @@ static void handle_new_window_attributes_reply(struct x_connection * /*c*/, // created with the same window ID before this request completed, and the // latter window isn't in our tree yet. if (wm_is_consistent(ps->wm)) { - log_error("Newly created window %#010x is not in the window tree", wid); + log_error("Newly created window %#010x is not in the window tree", + id.x); assert(false); } return; } + auto current_id = wm_ref_treeid(new_window); + if (!wm_treeid_eq(current_id, id)) { + log_debug("Window %#010x was not the window we queried anymore. Current " + "gen %" PRIu64 ", expected %" PRIu64, + id.x, current_id.gen, id.gen); + return; + } + auto toplevel = wm_ref_toplevel_of(ps->wm, new_window); if (toplevel != new_window) { log_debug("Newly created window %#010x was moved away from toplevel " "while we were waiting for its attributes", - wid); + id.x); return; } if (wm_ref_deref(toplevel) != NULL) { @@ -1538,7 +1547,7 @@ static void handle_new_window_attributes_reply(struct x_connection * /*c*/, // toplevel. But there is another get attributes request sent the // second time it became a toplevel. When we get the reply for the second // request, we will reach here. - log_debug("Newly created window %#010x is already managed", wid); + log_debug("Newly created window %#010x is already managed", id.x); return; } @@ -1566,11 +1575,11 @@ static void handle_new_windows(session_t *ps) { // number of things could happen before we get the reply. The // window can be reparented, destroyed, then get its window ID // reused, etc. - req->wid = wm_ref_win_id(wm_change.toplevel); + req->id = wm_ref_treeid(wm_change.toplevel); req->ps = ps; req->base.callback = handle_new_window_attributes_reply, req->base.sequence = - xcb_get_window_attributes(ps->c.c, req->wid).sequence; + xcb_get_window_attributes(ps->c.c, req->id.x).sequence; x_await_request(&ps->c, &req->base); break; case WM_TREE_CHANGE_TOPLEVEL_KILLED: diff --git a/src/region.h b/src/region.h index a57bf01121..cde4179d25 100644 --- a/src/region.h +++ b/src/region.h @@ -100,7 +100,7 @@ static inline void _resize_region(const region_t *region, region_t *output, int int nrects; int nnewrects = 0; const rect_t *rects = pixman_region32_rectangles((region_t *)region, &nrects); - auto newrects = ccalloc(nrects, rect_t); + rect_t *newrects = calloc((size_t)nrects, sizeof(rect_t)); for (int i = 0; i < nrects; i++) { int x1 = rects[i].x1 - dx; int y1 = rects[i].y1 - dy; From 743176379352f284caf0d234d33b492d48dcde25 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Wed, 4 Sep 2024 17:10:55 +0100 Subject: [PATCH 13/24] wm/tree: bump node's gen if it's removed from toplevel If a node is removed from toplevel then comes back, it will generate a new creation tree change. Because of the assumption explained in the previous commit, we can only attach managed window struct to it using the "key" from the creation tree change. But the node's key hasn't changed! Since it was toplevel, it will have a previous, older creation tree change which also handed out this key, which can be used to attach managed window to it, thus breaks the assumption. This means every time a node becomes a toplevel, it must have a different key. We bump its gen number when it's removed from toplevel to ensure that. Signed-off-by: Yuxuan Shui --- src/wm/tree.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/wm/tree.c b/src/wm/tree.c index a34966a7f4..ebb62e1eb1 100644 --- a/src/wm/tree.c +++ b/src/wm/tree.c @@ -334,6 +334,11 @@ struct wm_tree_node *wm_tree_detach(struct wm_tree *tree, struct wm_tree_node *s if (wm_tree_enqueue_toplevel_killed(tree, subroot->id, zombie)) { zombie = NULL; } + + // Gen bump must happen after enqueuing the change, because otherwise the + // kill change won't cancel out a previous new change because the IDs will + // be different. + subroot->id.gen = tree->gen++; } subroot->parent = NULL; return zombie; From e045cd0298ce3e8bc320fd0fca7e72ed335b1e12 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Thu, 5 Sep 2024 08:12:28 +0100 Subject: [PATCH 14/24] x: simplify x_poll_for_event Signed-off-by: Yuxuan Shui --- src/event.c | 8 +- src/picom.c | 6 +- src/wm/win.c | 6 +- src/wm/win.h | 2 +- src/wm/wm.c | 8 +- src/x.c | 226 ++++++++++++++++++++------------------------------- src/x.h | 5 +- 7 files changed, 106 insertions(+), 155 deletions(-) diff --git a/src/event.c b/src/event.c index 217a9cd3fc..2940d4b034 100644 --- a/src/event.c +++ b/src/event.c @@ -201,7 +201,7 @@ struct ev_ewmh_active_win_request { /// returned could not be found. static void update_ewmh_active_win(struct x_connection * /*c*/, struct x_async_request_base *req_base, - xcb_raw_generic_event_t *reply_or_error) { + const xcb_raw_generic_event_t *reply_or_error) { auto ps = ((struct ev_ewmh_active_win_request *)req_base)->ps; free(req_base); @@ -219,7 +219,7 @@ update_ewmh_active_win(struct x_connection * /*c*/, struct x_async_request_base } // Search for the window - auto reply = (xcb_get_property_reply_t *)reply_or_error; + auto reply = (const xcb_get_property_reply_t *)reply_or_error; if (reply->type == XCB_NONE || xcb_get_property_value_length(reply) < 4) { log_debug("EWMH _NET_ACTIVE_WINDOW not set."); return; @@ -248,7 +248,7 @@ struct ev_recheck_focus_request { * @return struct _win of currently focused window, NULL if not found */ static void recheck_focus(struct x_connection * /*c*/, struct x_async_request_base *req_base, - xcb_raw_generic_event_t *reply_or_error) { + const xcb_raw_generic_event_t *reply_or_error) { auto ps = ((struct ev_ewmh_active_win_request *)req_base)->ps; free(req_base); @@ -268,7 +268,7 @@ static void recheck_focus(struct x_connection * /*c*/, struct x_async_request_ba return; } - auto reply = (xcb_get_input_focus_reply_t *)reply_or_error; + auto reply = (const xcb_get_input_focus_reply_t *)reply_or_error; xcb_window_t wid = reply->focus; log_debug("Current focused window is %#010x", wid); if (wid == XCB_NONE || wid == XCB_INPUT_FOCUS_POINTER_ROOT || diff --git a/src/picom.c b/src/picom.c index da840a71ec..aa1880cc13 100644 --- a/src/picom.c +++ b/src/picom.c @@ -1493,7 +1493,7 @@ struct new_window_attributes_request { static void handle_new_window_attributes_reply(struct x_connection * /*c*/, struct x_async_request_base *req_base, - xcb_raw_generic_event_t *reply_or_error) { + const xcb_raw_generic_event_t *reply_or_error) { auto req = (struct new_window_attributes_request *)req_base; auto ps = req->ps; auto id = req->id; @@ -1551,8 +1551,8 @@ static void handle_new_window_attributes_reply(struct x_connection * /*c*/, return; } - auto w = win_maybe_allocate(ps, toplevel, - (xcb_get_window_attributes_reply_t *)reply_or_error); + auto w = win_maybe_allocate( + ps, toplevel, (const xcb_get_window_attributes_reply_t *)reply_or_error); if (w != NULL && w->a.map_state == XCB_MAP_STATE_VIEWABLE) { win_set_flags(w, WIN_FLAGS_MAPPED); ps->pending_updates = true; diff --git a/src/wm/win.c b/src/wm/win.c index dd65e63065..16e10396fb 100644 --- a/src/wm/win.c +++ b/src/wm/win.c @@ -1279,7 +1279,7 @@ void free_win_res(session_t *ps, struct win *w) { /// Query the Xorg for information about window `win`, and assign a window to `cursor` if /// this window should be managed. struct win *win_maybe_allocate(session_t *ps, struct wm_ref *cursor, - xcb_get_window_attributes_reply_t *attrs) { + const xcb_get_window_attributes_reply_t *attrs) { static const struct win win_def = { .frame_opacity = 1.0, .in_openclose = true, // set to false after first map is done, @@ -2088,7 +2088,7 @@ struct win_get_geometry_request { static void win_handle_get_geometry_reply(struct x_connection * /*c*/, struct x_async_request_base *req_base, - xcb_raw_generic_event_t *reply_or_error) { + const xcb_raw_generic_event_t *reply_or_error) { auto req = (struct win_get_geometry_request *)req_base; auto wid = req->wid; auto ps = req->ps; @@ -2123,7 +2123,7 @@ static void win_handle_get_geometry_reply(struct x_connection * /*c*/, return; } - auto r = (xcb_get_geometry_reply_t *)reply_or_error; + auto r = (const xcb_get_geometry_reply_t *)reply_or_error; ps->pending_updates |= win_set_pending_geometry(w, win_geometry_from_get_geometry(r)); } diff --git a/src/wm/win.h b/src/wm/win.h index d17f35c5e3..d79fdf6e4e 100644 --- a/src/wm/win.h +++ b/src/wm/win.h @@ -471,7 +471,7 @@ region_t win_get_region_frame_local_by_val(const struct win *w); /// Query the Xorg for information about window `win` /// `win` pointer might become invalid after this function returns struct win *win_maybe_allocate(session_t *ps, struct wm_ref *cursor, - xcb_get_window_attributes_reply_t *attrs); + const xcb_get_window_attributes_reply_t *attrs); /** * Release a destroyed window that is no longer needed. diff --git a/src/wm/wm.c b/src/wm/wm.c index f6f2394260..0f838e61df 100644 --- a/src/wm/wm.c +++ b/src/wm/wm.c @@ -475,7 +475,7 @@ static const xcb_event_mask_t WM_IMPORT_EV_MASK = XCB_EVENT_MASK_SUBSTRUCTURE_NO static void wm_handle_query_tree_reply(struct x_connection *c, struct x_async_request_base *base, - xcb_raw_generic_event_t *reply_or_error) { + const xcb_raw_generic_event_t *reply_or_error) { auto req = (struct wm_query_tree_request *)base; auto atoms = req->atoms; auto wm = req->wm; @@ -517,7 +517,7 @@ wm_handle_query_tree_reply(struct x_connection *c, struct x_async_request_base * BUG_ON(false); } - xcb_query_tree_reply_t *reply = (xcb_query_tree_reply_t *)reply_or_error; + auto reply = (const xcb_query_tree_reply_t *)reply_or_error; log_debug("Finished querying tree for window %#010x", node->id.x); auto children = xcb_query_tree_children(reply); @@ -537,7 +537,7 @@ wm_handle_query_tree_reply(struct x_connection *c, struct x_async_request_base * static void wm_handle_get_wm_state_reply(struct x_connection * /*c*/, struct x_async_request_base *base, - xcb_raw_generic_event_t *reply_or_error) { + const xcb_raw_generic_event_t *reply_or_error) { auto req = (struct wm_get_property_request *)base; if (reply_or_error == NULL) { free(req); @@ -569,7 +569,7 @@ static void wm_handle_get_wm_state_reply(struct x_connection * /*c*/, free(req); return; } - auto reply = (xcb_get_property_reply_t *)reply_or_error; + auto reply = (const xcb_get_property_reply_t *)reply_or_error; wm_tree_set_wm_state(&req->wm->tree, node, reply->type != XCB_NONE); free(req); } diff --git a/src/x.c b/src/x.c index c13eb41475..15f5a4e97b 100644 --- a/src/x.c +++ b/src/x.c @@ -949,7 +949,7 @@ struct x_update_monitors_request { static void x_handle_update_monitors_reply(struct x_connection * /*c*/, struct x_async_request_base *req_base, - xcb_raw_generic_event_t *reply_or_error) { + const xcb_raw_generic_event_t *reply_or_error) { auto m = ((struct x_update_monitors_request *)req_base)->monitors; free(req_base); @@ -966,7 +966,7 @@ static void x_handle_update_monitors_reply(struct x_connection * /*c*/, x_free_monitor_info(m); - auto reply = (xcb_randr_get_monitors_reply_t *)reply_or_error; + auto reply = (const xcb_randr_get_monitors_reply_t *)reply_or_error; m->count = xcb_randr_get_monitors_monitors_length(reply); m->regions = ccalloc(m->count, region_t); @@ -997,166 +997,120 @@ void x_free_monitor_info(struct x_monitors *m) { m->count = 0; } -static uint32_t x_get_full_sequence(struct x_connection *c, uint16_t sequence) { - auto last_sequence_low = c->last_sequence & 0xffff; - // sequence < last_sequence16 means the lower 16 bits overflowed, which should - // carry to the higher 16 bits - auto sequence_high = c->last_sequence & 0xffff0000; - if (sequence < last_sequence_low) { - sequence_high += 0x10000; +static inline xcb_raw_generic_event_t * +x_ingest_event(struct x_connection *c, xcb_generic_event_t *event) { + if (event != NULL) { + x_discard_pending_errors(c, event->full_sequence); + c->last_sequence = event->full_sequence; } - return sequence_high | sequence; + return (xcb_raw_generic_event_t *)event; } -static int64_t x_compare_sequence(struct x_connection *c, uint32_t a, uint32_t b) { - bool a_overflown = a < c->last_sequence, b_overflown = b < c->last_sequence; - if (a_overflown == b_overflown) { - return (int64_t)a - (int64_t)b; - } - return a_overflown ? 1 : -1; -} - -static xcb_raw_generic_event_t * -x_poll_for_event_impl(struct x_connection *c, struct x_async_request_base **out_req) { +/// Read the X connection once, and return the first event or unchecked error. If any +/// replies to pending X requests are read, they will be processed and callbacks will be +/// called. +/// +/// `retry` will be set to true if we might have read more replies than we have processed +/// in this function. This means if this function returned NULL with `retry` being true +/// and there are more pending requests in the list, the caller should call this function +/// again. This is because `x_poll_for_event` should never return NULL if there is any +/// pending events/errors/replies in the queue. +static const xcb_raw_generic_event_t * +x_poll_for_event_impl(struct x_connection *c, bool *retry) { struct x_async_request_base *first_pending_request = NULL; if (!list_is_empty(&c->pending_x_requests)) { first_pending_request = list_entry(c->pending_x_requests.next, struct x_async_request_base, siblings); } - bool on_hold_is_reply; - if (c->message_on_hold == NULL) { - // Nothing on hold, we need to read new information from the X connection. - // We must only read from the X connection once in this function to keep - // things consistent. The only way to do that is reading the connection - // with `xcb_poll_for_reply`, and then check for events with - // `xcb_poll_for_queued_event`, because there is no - // `xcb_poll_for_queued_reply`. Unless we are not waiting for any replies, - // in which case a simple `xcb_poll_for_event` is enough. - if (first_pending_request != NULL) { - xcb_generic_error_t *err = NULL; - on_hold_is_reply = - xcb_poll_for_reply(c->c, first_pending_request->sequence, - (void **)&c->message_on_hold, &err) == 1; - if (err != NULL) { - c->message_on_hold = (xcb_raw_generic_event_t *)err; - } - if (!on_hold_is_reply) { - // We didn't get a reply, but did we get an event? - c->message_on_hold = - (xcb_raw_generic_event_t *)xcb_poll_for_queued_event(c->c); - } - } else { - c->message_on_hold = - (xcb_raw_generic_event_t *)xcb_poll_for_event(c->c); - on_hold_is_reply = false; - } - } else if (first_pending_request != NULL) { - // response_type 0 is error, 1 is reply. - on_hold_is_reply = c->message_on_hold->response_type < 2 && - x_get_full_sequence(c, c->message_on_hold->sequence) == - first_pending_request->sequence; - } else { - on_hold_is_reply = false; + if (first_pending_request == NULL) { + // No reply to wait for, just poll for events. + return x_ingest_event(c, xcb_poll_for_event(c->c)); } + + // Now we need to first check if a reply to `first_pending_request` is available. if (c->message_on_hold == NULL) { - // Didn't get any new information from the X connection, nothing to - // return. - return NULL; - } + xcb_generic_error_t *err = NULL; + auto has_reply = xcb_poll_for_reply(c->c, first_pending_request->sequence, + (void *)&c->message_on_hold, &err); + if (has_reply == 0) { + // If we haven't got a reply to `first_pending_request`, any + // currently queued event must have a lower sequence number than + // it. + auto event = xcb_poll_for_queued_event(c->c); + assert(event == NULL || + event->full_sequence < first_pending_request->sequence); + return x_ingest_event(c, event); + } - // From this point, no more reading from the X connection is allowed. - xcb_generic_event_t *next_event = NULL; - if (on_hold_is_reply) { - next_event = xcb_poll_for_queued_event(c->c); - assert(next_event == NULL || next_event->response_type != 1); - } else { - next_event = (xcb_generic_event_t *)c->message_on_hold; + if (c->message_on_hold == NULL) { + c->message_on_hold = (xcb_raw_generic_event_t *)err; + } } - // `next_event == c->message_on_hold` iff `on_hold_is_reply` is false. + // So here, we have received a reply or error to `first_pending_request`, we still + // have to check if there are any events with a lower sequence number. + auto event = xcb_poll_for_queued_event(c->c); - bool should_return_event = false; - if (first_pending_request == NULL) { - // Here `on_hold_is_reply` must be false, therefore `next_event == - // c->message_on_hold` must be true, therefore `next_event` cannot be - // NULL. - should_return_event = true; - } else if (next_event != NULL) { - auto ordering = x_compare_sequence(c, next_event->full_sequence, - first_pending_request->sequence); - // If next_event is a true event, it might share a sequence number with a - // reply. But if it's an error (i.e. response_type == 0), its sequence - // number must be different from any reply. - assert(next_event->response_type != 0 || ordering != 0); - should_return_event = ordering < 0; - } - - if (should_return_event) { - x_discard_pending_errors(c, next_event->full_sequence); - c->last_sequence = next_event->full_sequence; - if (!on_hold_is_reply) { - c->message_on_hold = NULL; + // We have a reply to `first_pending_request`, but we don't know how many replies + // we have actually read, so after we returned we might need to retry this + // function. + *retry = true; + + // Now, if we get an event, that means any pending requests with a lower or equal + // sequence number should have their replies ready (which could be none of them, + // if the event has a lower sequence number than the first pending request). If we + // didn't get an event, then we know the reply to `first_pending_request` is the + // only thing we have received, and nothing comes before it. + unsigned target_sequence; + if (event != NULL) { + target_sequence = event->full_sequence; + } else { + target_sequence = first_pending_request->sequence; + } + x_discard_pending_errors(c, target_sequence); + while (first_pending_request->sequence <= target_sequence) { + if (c->message_on_hold == NULL) { + // This is a special case. We have already received an event with + // a greater or equal sequence number than + // `first_pending_request`, so we know the reply must be in xcb's + // queue. + xcb_generic_error_t *err = NULL; + BUG_ON(xcb_poll_for_reply(c->c, first_pending_request->sequence, + (void *)&c->message_on_hold, &err) == 0); + if (c->message_on_hold == NULL) { + c->message_on_hold = (xcb_raw_generic_event_t *)err; + } } - return (xcb_raw_generic_event_t *)next_event; - } + list_remove(&first_pending_request->siblings); + first_pending_request->callback(c, first_pending_request, c->message_on_hold); + free(c->message_on_hold); + c->message_on_hold = NULL; - // We should return the reply to the first pending request. - xcb_raw_generic_event_t *ret = NULL; - if (!on_hold_is_reply) { - xcb_generic_error_t *err = NULL; - // This is a very special case. Because we have already received an event - // with a greater or equal sequence number than the reply, we _know_ the - // reply must also have already arrived. We can safely call - // `xcb_poll_for_reply` here because we know it will not read from the X - // connection again. - BUG_ON(xcb_poll_for_reply(c->c, first_pending_request->sequence, - (void **)&ret, &err) == 0); - if (err != NULL) { - ret = (xcb_raw_generic_event_t *)err; + if (list_is_empty(&c->pending_x_requests)) { + break; } - } else { - ret = c->message_on_hold; - c->message_on_hold = (xcb_raw_generic_event_t *)next_event; - } - x_discard_pending_errors(c, first_pending_request->sequence + 1); - c->last_sequence = first_pending_request->sequence; - *out_req = first_pending_request; - list_remove(&first_pending_request->siblings); - return ret; + first_pending_request = list_entry(c->pending_x_requests.next, + struct x_async_request_base, siblings); + } + return (xcb_raw_generic_event_t *)event; } xcb_generic_event_t *x_poll_for_event(struct x_connection *c) { - xcb_raw_generic_event_t *ret = NULL; + const xcb_raw_generic_event_t *ret = NULL; while (true) { - struct x_async_request_base *req = NULL; - ret = x_poll_for_event_impl(c, &req); - if (ret == NULL) { - break; + bool retry = false; + ret = x_poll_for_event_impl(c, &retry); + if (ret == NULL && !list_is_empty(&c->pending_x_requests) && retry) { + continue; } - - if (req != NULL) { - req->callback(c, req, ret); - } else if (ret->response_type == 0) { - x_handle_error(c, (xcb_generic_error_t *)ret); - } else { + if (ret == NULL || ret->response_type != 0) { break; } - free(ret); - } - return (xcb_generic_event_t *)ret; -} -void x_cancel_request(struct x_connection *c, struct x_async_request_base *req) { - list_remove(&req->siblings); - if (c->message_on_hold == NULL) { - return; - } - if (c->message_on_hold->response_type >= 2 || - x_get_full_sequence(c, c->message_on_hold->sequence) != req->sequence) { - return; + x_handle_error(c, (xcb_generic_error_t *)ret); + free((void *)ret); } - free(c->message_on_hold); - c->message_on_hold = NULL; + return (xcb_generic_event_t *)ret; } diff --git a/src/x.h b/src/x.h index 8e90558f57..7835ecbe4d 100644 --- a/src/x.h +++ b/src/x.h @@ -194,7 +194,7 @@ struct x_async_request_base { /// The callback function to call when the reply is received. If `reply_or_error` /// is NULL, it means the X connection is closed while waiting for the reply. void (*callback)(struct x_connection *, struct x_async_request_base *, - xcb_raw_generic_event_t *reply_or_error); + const xcb_raw_generic_event_t *reply_or_error); /// The sequence number of the X request. unsigned int sequence; }; @@ -450,9 +450,6 @@ static inline void x_await_request(struct x_connection *c, struct x_async_reques list_insert_before(&c->pending_x_requests, &req->siblings); } -/// Cancel an async request. -void x_cancel_request(struct x_connection *c, struct x_async_request_base *req); - /// Poll for the next X event. This is like `xcb_poll_for_event`, but also includes /// machinery for handling async replies. Calling `xcb_poll_for_event` directly will /// cause replies to async requests to be lost, so that should never be called. From 2692fe7ce3f082bd605950b714c7e3f98cb00807 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Thu, 5 Sep 2024 13:29:16 +0100 Subject: [PATCH 15/24] x: support async requests with no reply Signed-off-by: Yuxuan Shui --- src/x.c | 224 ++++++++++++++++++++++++++++++++++++-------------------- src/x.h | 20 +++-- 2 files changed, 159 insertions(+), 85 deletions(-) diff --git a/src/x.c b/src/x.c index 15f5a4e97b..4ab2f60473 100644 --- a/src/x.c +++ b/src/x.c @@ -31,6 +31,14 @@ #include "utils/misc.h" #include "x.h" +static inline uint64_t x_widen_sequence(struct x_connection *c, uint32_t sequence) { + if (sequence < c->last_sequence) { + // The sequence number has wrapped around + return (uint64_t)sequence + UINT32_MAX + 1; + } + return (uint64_t)sequence; +} + // === Error handling === /// Discard pending error handlers. @@ -38,21 +46,9 @@ /// We have received reply with sequence number `sequence`, which means all pending /// replies with sequence number strictly less than `sequence` will never be received. So /// discard them. -static void x_discard_pending_errors(struct x_connection *c, uint32_t sequence) { - if (sequence < c->last_sequence) { - // Overflown, drop from `pending_x_errors` until its sequence number - // decreases. - log_debug("X sequence number overflown, %u -> %u", c->last_sequence, sequence); - list_foreach_safe(struct pending_x_error, i, &c->pending_x_errors, siblings) { - if (sequence >= i->sequence) { - break; - } - list_remove(&i->siblings); - free(i); - } - } +static void x_discard_pending_errors(struct x_connection *c, uint64_t sequence) { list_foreach_safe(struct pending_x_error, i, &c->pending_x_errors, siblings) { - if (sequence <= i->sequence) { + if (x_widen_sequence(c, i->sequence) >= sequence) { break; } list_remove(&i->siblings); @@ -1000,106 +996,174 @@ void x_free_monitor_info(struct x_monitors *m) { static inline xcb_raw_generic_event_t * x_ingest_event(struct x_connection *c, xcb_generic_event_t *event) { if (event != NULL) { - x_discard_pending_errors(c, event->full_sequence); + assert(event->response_type != 1); + uint64_t seq = x_widen_sequence(c, event->full_sequence); + if (event->response_type != 0) { + // For true events, we can discard pending errors with a lower or + // equal sequence number. This is because only a reply or an error + // increments the sequence number. + seq += 1; + } + x_discard_pending_errors(c, seq); c->last_sequence = event->full_sequence; } return (xcb_raw_generic_event_t *)event; } +static const xcb_raw_generic_event_t no_reply_success = {.response_type = 1}; + /// Read the X connection once, and return the first event or unchecked error. If any /// replies to pending X requests are read, they will be processed and callbacks will be /// called. -/// -/// `retry` will be set to true if we might have read more replies than we have processed -/// in this function. This means if this function returned NULL with `retry` being true -/// and there are more pending requests in the list, the caller should call this function -/// again. This is because `x_poll_for_event` should never return NULL if there is any -/// pending events/errors/replies in the queue. static const xcb_raw_generic_event_t * x_poll_for_event_impl(struct x_connection *c, bool *retry) { - struct x_async_request_base *first_pending_request = NULL; - if (!list_is_empty(&c->pending_x_requests)) { - first_pending_request = list_entry(c->pending_x_requests.next, - struct x_async_request_base, siblings); - } - - if (first_pending_request == NULL) { + // This whole thing, this whole complexity arises from libxcb's idiotic API. + // What we need is a call to give us the next message from X, regardless if + // it's a reply, an error, or an event. But what we get is two different calls: + // `xcb_poll_for_event` and `xcb_poll_for_reply`. And there is this weird + // asymmetry: `xcb_poll_for_event` has a version that doesn't read from X, but + // `xcb_poll_for_reply` doesn't. + // + // This whole thing we are doing here, is trying to emulate the desired behavior + // with what we got. + if (c->first_request_with_reply == NULL) { // No reply to wait for, just poll for events. + BUG_ON(!list_is_empty(&c->pending_x_requests)); return x_ingest_event(c, xcb_poll_for_event(c->c)); } - // Now we need to first check if a reply to `first_pending_request` is available. + // Now we need to first check if a reply to `first_request_with_reply` is + // available. if (c->message_on_hold == NULL) { xcb_generic_error_t *err = NULL; - auto has_reply = xcb_poll_for_reply(c->c, first_pending_request->sequence, + auto has_reply = xcb_poll_for_reply(c->c, c->first_request_with_reply->sequence, (void *)&c->message_on_hold, &err); - if (has_reply == 0) { - // If we haven't got a reply to `first_pending_request`, any - // currently queued event must have a lower sequence number than - // it. - auto event = xcb_poll_for_queued_event(c->c); - assert(event == NULL || - event->full_sequence < first_pending_request->sequence); - return x_ingest_event(c, event); - } - - if (c->message_on_hold == NULL) { + if (has_reply != 0 && c->message_on_hold == NULL) { c->message_on_hold = (xcb_raw_generic_event_t *)err; } + c->sequence_on_hold = c->first_request_with_reply->sequence; } - // So here, we have received a reply or error to `first_pending_request`, we still - // have to check if there are any events with a lower sequence number. + // Even if we don't get a reply for `first_request_with_reply`, some of the + // `no_reply` requests might have completed. We will see if we got an event, and + // deduce that information from that. auto event = xcb_poll_for_queued_event(c->c); - // We have a reply to `first_pending_request`, but we don't know how many replies - // we have actually read, so after we returned we might need to retry this - // function. - *retry = true; - - // Now, if we get an event, that means any pending requests with a lower or equal - // sequence number should have their replies ready (which could be none of them, - // if the event has a lower sequence number than the first pending request). If we - // didn't get an event, then we know the reply to `first_pending_request` is the - // only thing we have received, and nothing comes before it. - unsigned target_sequence; - if (event != NULL) { - target_sequence = event->full_sequence; - } else { - target_sequence = first_pending_request->sequence; - } - x_discard_pending_errors(c, target_sequence); - while (first_pending_request->sequence <= target_sequence) { - if (c->message_on_hold == NULL) { - // This is a special case. We have already received an event with - // a greater or equal sequence number than - // `first_pending_request`, so we know the reply must be in xcb's - // queue. - xcb_generic_error_t *err = NULL; - BUG_ON(xcb_poll_for_reply(c->c, first_pending_request->sequence, - (void *)&c->message_on_hold, &err) == 0); - if (c->message_on_hold == NULL) { - c->message_on_hold = (xcb_raw_generic_event_t *)err; + // As long as we have read one reply, we could have read an indefinite number of + // replies, so after we returned we might need to retry this function. + *retry = c->message_on_hold != NULL; + + while (!list_is_empty(&c->pending_x_requests) && + (event != NULL || c->message_on_hold != NULL)) { + auto head = list_entry(c->pending_x_requests.next, + struct x_async_request_base, siblings); + auto head_seq = x_widen_sequence(c, head->sequence); + auto event_seq = UINT64_MAX; + if (event != NULL) { + event_seq = x_widen_sequence(c, event->full_sequence); + BUG_ON(event->response_type == 1); + if (head_seq > event_seq) { + // The event comes before the first pending request, we + // can return it first. + break; } } - list_remove(&first_pending_request->siblings); - first_pending_request->callback(c, first_pending_request, c->message_on_hold); - free(c->message_on_hold); - c->message_on_hold = NULL; - if (list_is_empty(&c->pending_x_requests)) { - break; + // event == NULL || head_seq <= event_seq + // If event == NULL, we have message_on_hold != NULL. And message_on_hold + // has a sequence number equals to first_request_with_reply, which must be + // greater or equal to head_seq; If event != NULL, we have head_seq <= + // event_seq. Either case indicates `head` has completed, so we can remove + // it from the list. + list_remove(&head->siblings); + if (c->first_request_with_reply == head) { + BUG_ON(head->no_reply); + c->first_request_with_reply = NULL; + list_foreach(struct x_async_request_base, i, + &c->pending_x_requests, siblings) { + if (!i->no_reply) { + c->first_request_with_reply = i; + break; + } + } } + x_discard_pending_errors(c, head_seq); + c->last_sequence = head->sequence; + + if (event != NULL) { + if (event->response_type == 0 && head_seq == event_seq) { + // `event` is an error response to `head`. `head` must be + // `no_reply`, otherwise its error will be returned by + // `xcb_poll_for_reply`. + BUG_ON(!head->no_reply); + head->callback(c, head, (xcb_raw_generic_event_t *)event); + free(event); + + event = xcb_poll_for_queued_event(c->c); + continue; + } - first_pending_request = list_entry(c->pending_x_requests.next, - struct x_async_request_base, siblings); + // Here, we have 2 cases: + // a. event is an error && head_seq < event_seq + // b. event is a true event && head_seq <= event_seq + // In either case, we know `head` has completed. If it has a + // reply, the reply should be in xcb's queue. So we can call + // `xcb_poll_for_reply` safely without reading from X. + if (head->no_reply) { + head->callback(c, head, &no_reply_success); + continue; + } + if (c->message_on_hold == NULL) { + xcb_generic_error_t *err = NULL; + BUG_ON(xcb_poll_for_reply(c->c, head->sequence, + (void *)&c->message_on_hold, + NULL) == 0); + if (c->message_on_hold == NULL) { + c->message_on_hold = (xcb_raw_generic_event_t *)err; + } + c->sequence_on_hold = head->sequence; + } + BUG_ON(c->sequence_on_hold != head->sequence); + head->callback(c, head, c->message_on_hold); + free(c->message_on_hold); + c->message_on_hold = NULL; + } + // event == NULL => c->message_on_hold != NULL + else if (x_widen_sequence(c, c->sequence_on_hold) > head_seq) { + BUG_ON(!head->no_reply); + head->callback(c, head, &no_reply_success); + } else { + BUG_ON(c->sequence_on_hold != head->sequence); + BUG_ON(head->no_reply); + head->callback(c, head, c->message_on_hold); + free(c->message_on_hold); + c->message_on_hold = NULL; + } } - return (xcb_raw_generic_event_t *)event; + + return x_ingest_event(c, event); +} + +static void +x_dummy_async_callback(struct x_connection * /*c*/, struct x_async_request_base *req_base, + const xcb_raw_generic_event_t * /*reply_or_error*/) { + free(req_base); } xcb_generic_event_t *x_poll_for_event(struct x_connection *c) { const xcb_raw_generic_event_t *ret = NULL; while (true) { + if (c->first_request_with_reply == NULL && + !list_is_empty(&c->pending_x_requests)) { + // All requests we are waiting for are no_reply. We would have no + // idea if any of them completed, until a subsequent event is + // received, which can take an indefinite amount of time. So we + // insert a GetInputFocus request to ensure we get a reply. + auto req = ccalloc(1, struct x_async_request_base); + req->sequence = xcb_get_input_focus(c->c).sequence; + req->callback = x_dummy_async_callback; + x_await_request(c, req); + } bool retry = false; ret = x_poll_for_event_impl(c, &retry); if (ret == NULL && !list_is_empty(&c->pending_x_requests) && retry) { diff --git a/src/x.h b/src/x.h index 7835ecbe4d..2308cbeba4 100644 --- a/src/x.h +++ b/src/x.h @@ -53,7 +53,7 @@ enum x_error_action { /// Represents a X request we sent that might error. struct pending_x_error { - unsigned long sequence; + uint32_t sequence; enum x_error_action action; // Debug information, where in the code was this request sent. @@ -81,18 +81,22 @@ struct x_connection { /// The list of pending async requests that we have /// yet to receive a reply for. struct list_node pending_x_requests; + /// The first request that has a reply. + struct x_async_request_base *first_request_with_reply; /// A message, either an event or a reply, that is currently being held, because /// there are messages of the opposite type with lower sequence numbers that we /// need to return first. xcb_raw_generic_event_t *message_on_hold; - /// The sequence number of the last message returned by - /// `x_poll_for_message`. Used for sequence number overflow - /// detection. - uint32_t last_sequence; /// Previous handler of X errors XErrorHandler previous_xerror_handler; /// Information about the default screen xcb_screen_t *screen_info; + /// The sequence number of the last message returned by + /// `x_poll_for_message`. Used for sequence number overflow + /// detection. + uint32_t last_sequence; + /// The sequence number of `message_on_hold` + uint32_t sequence_on_hold; }; /// Monitor info @@ -197,6 +201,9 @@ struct x_async_request_base { const xcb_raw_generic_event_t *reply_or_error); /// The sequence number of the X request. unsigned int sequence; + /// This request doesn't expect a reply. If this is true, in the success case, + /// `callback` will be called with a dummy reply whose `response_type` is 1. + bool no_reply; }; static inline void attr_unused free_x_connection(struct x_connection *c) { @@ -447,6 +454,9 @@ void x_request_vblank_event(struct x_connection *c, xcb_window_t window, uint64_ /// `req` store information about the request, including the callback. The callback is /// responsible for freeing `req`. static inline void x_await_request(struct x_connection *c, struct x_async_request_base *req) { + if (c->first_request_with_reply == NULL && !req->no_reply) { + c->first_request_with_reply = req; + } list_insert_before(&c->pending_x_requests, &req->siblings); } From 7b7168b891afc03d61194b08a4c33f14d8487557 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Thu, 5 Sep 2024 14:07:21 +0100 Subject: [PATCH 16/24] wm: make setting event masks async We care about whether setting the event masks succeeded, we can't deduced that from the query tree request because of race conditions and window ID reuse. So we have to check if the request for setting the event masks itself succeeded or not. But `xcb_request_check` is out of order w.r.t. to other X events/replies. So essentially we are "looking into the future" with it, sensing that a window no longer exists before the usual events and replies we received from X would have told us so. This breaks assumptions we based many of our assertions on. For example, in many places when we can't find a window in our tree, we assume we must have an incomplete tree (i.e. based on the events/replies, there are windows on X's side we haven't queried). But it's actually possible that we looked into the future and saw it no longer exists. We have updated the async request mechanism so it can support requests that don't expect replies. This commit uses that to make setting event masks async, so it no longer uses `xcb_request_check` and is no longer out of order. Also updated how query tree requests are handled to make it work together with this change. Signed-off-by: Yuxuan Shui --- src/event.c | 4 +- src/wm/tree.c | 3 + src/wm/wm.c | 213 ++++++++++++++++++++++++++++--------------- src/wm/wm.h | 6 +- src/wm/wm_internal.h | 9 +- src/x.c | 12 ++- src/x.h | 7 +- 7 files changed, 169 insertions(+), 85 deletions(-) diff --git a/src/event.c b/src/event.c index 2940d4b034..065e75b76e 100644 --- a/src/event.c +++ b/src/event.c @@ -415,7 +415,7 @@ static inline void ev_destroy_notify(session_t *ps, xcb_destroy_notify_event_t * // If we hit an ABA problem, it is possible to get a DestroyNotify event from a // parent for its child, but not from the child for itself. if (ev->event != ev->window) { - wm_disconnect(ps->wm, ev->window, ev->event); + wm_disconnect(ps->wm, ev->window, ev->event, XCB_NONE); } else { wm_destroy(ps->wm, ev->window); } @@ -501,7 +501,7 @@ static inline void ev_reparent_notify(session_t *ps, xcb_reparent_notify_event_t return; } if (ev->parent != ev->event) { - wm_disconnect(ps->wm, ev->window, ev->event); + wm_disconnect(ps->wm, ev->window, ev->event, ev->parent); } else { wm_reparent(ps->wm, &ps->c, ps->atoms, ev->window, ev->parent); } diff --git a/src/wm/tree.c b/src/wm/tree.c index ebb62e1eb1..be750440aa 100644 --- a/src/wm/tree.c +++ b/src/wm/tree.c @@ -284,6 +284,9 @@ struct wm_tree_node *wm_tree_new_window(struct wm_tree *tree, xcb_window_t id) { node->id.x = id; node->id.gen = tree->gen++; node->has_wm_state = false; + node->receiving_events = false; + node->is_zombie = false; + node->visited = false; node->leader = id; list_init_head(&node->children); return node; diff --git a/src/wm/wm.c b/src/wm/wm.c index 0f838e61df..da6652b107 100644 --- a/src/wm/wm.c +++ b/src/wm/wm.c @@ -16,9 +16,9 @@ struct wm_query_tree_request { struct x_async_request_base base; - struct wm_tree_node *node; struct wm *wm; struct atom *atoms; + xcb_window_t wid; }; struct wm_get_property_request { @@ -42,11 +42,10 @@ struct wm { /// created its list of children is always up to date. struct wm_tree_node orphan_root; - /// Number of pending async query tree requests. We also have async get - /// property requests, but they are not tracked because they don't - /// affect the tree structure. We guarantee that when there are pending - /// query tree requests, no tree nodes will be freed. - unsigned n_pending_query_trees; + /// Number of pending async imports. This tracks the async event mask setups and + /// query tree queries. We also have async get property requests, but they are not + /// tracked because they don't affect the tree structure. + unsigned n_pending_imports; /// Whether cached window leaders should be recalculated. Following tree changes /// will trigger a leader refresh: @@ -277,7 +276,7 @@ struct wm *wm_new(void) { auto wm = ccalloc(1, struct wm); wm_tree_init(&wm->tree); list_init_head(&wm->orphan_root.children); - wm->n_pending_query_trees = 0; + wm->n_pending_imports = 0; return wm; } @@ -327,8 +326,10 @@ static void wm_detach_inner(struct wm *wm, struct wm_tree_node *child) { /// Disconnect `child` from `parent`. This is called when a window is removed from its /// parent's children list. This can be a result of the window being destroyed, or /// reparented to another window. This `child` is attached to the orphan root. -void wm_disconnect(struct wm *wm, xcb_window_t child, xcb_window_t parent) { +void wm_disconnect(struct wm *wm, xcb_window_t child, xcb_window_t parent, + xcb_window_t new_parent) { auto parent_node = wm_tree_find(&wm->tree, parent); + auto new_parent_node = wm_tree_find(&wm->tree, new_parent); BUG_ON_NULL(parent_node); auto child_node = wm_tree_find(&wm->tree, child); @@ -349,11 +350,26 @@ void wm_disconnect(struct wm *wm, xcb_window_t child, xcb_window_t parent) { // If child_node->parent is not parent_node, then the parent must be in the // process of being queried. In that case the child_node must be orphaned. BUG_ON(child_node->parent != parent_node && - (parent_node->req == NULL || child_node->parent != &wm->orphan_root)); + (parent_node->tree_queried || child_node->parent != &wm->orphan_root)); - log_debug("Disconnecting window %#010x from window %#010x", child, parent); wm_detach_inner(wm, child_node); - wm_tree_attach(&wm->tree, child_node, &wm->orphan_root); + if ((new_parent_node != NULL && new_parent_node->receiving_events) || + child_node->receiving_events) { + // We need to be sure we will be able to keep track of all orphaned + // windows to know none of them will be destroyed without us knowing. If + // we have already set up event mask for the new parent, we will be + // getting a reparent event from the new parent, which means we will have + // an eye on `child` at all times, so that's fine. If we have already set + // up event mask on the child itself, that's even better. + log_debug("Disconnecting window %#010x from window %#010x", child, parent); + wm_tree_attach(&wm->tree, child_node, &wm->orphan_root); + } else { + // Otherwise, we might potentially lose track of this window, so we have + // to destroy it. + log_debug("Destroy window %#010x because we can't keep track of it", child); + HASH_DEL(wm->tree.nodes, child_node); + free(child_node); + } } void wm_destroy(struct wm *wm, xcb_window_t wid) { @@ -378,12 +394,6 @@ void wm_destroy(struct wm *wm, xcb_window_t wid) { wm_detach_inner(wm, node); - if (node->req != NULL) { - // Effectively "cancel" the query tree request by setting `node->req` to - // NULL. This will cause its reply to be ignored. - node->req->node = NULL; - wm->n_pending_query_trees--; - } HASH_DEL(wm->tree.nodes, node); free(node); } @@ -411,7 +421,7 @@ static void wm_reparent_no_flush(struct wm *wm, struct x_connection *c, struct a xcb_window_t wid, xcb_window_t parent) { auto window = wm_tree_find(&wm->tree, wid); auto new_parent = wm_tree_find(&wm->tree, parent); - bool new_parent_imported = new_parent != NULL && new_parent->req == NULL; + bool new_parent_imported = new_parent != NULL && new_parent->tree_queried; /// If a previously unseen window is reparented to a window that has been fully /// imported, we must treat it as a newly created window. Because it will not be @@ -479,7 +489,17 @@ wm_handle_query_tree_reply(struct x_connection *c, struct x_async_request_base * auto req = (struct wm_query_tree_request *)base; auto atoms = req->atoms; auto wm = req->wm; - auto node = req->node; + auto node = wm_tree_find(&wm->tree, req->wid); + free(req); + + wm->n_pending_imports--; + + if (reply_or_error == NULL) { + // The program is quitting... If this happens when wm is in an + // inconsistent state, there could be errors. + // TODO(yshui): fix this + return; + } if (node == NULL) { // If the node was previously destroyed, then it means a newly created @@ -488,35 +508,39 @@ wm_handle_query_tree_reply(struct x_connection *c, struct x_async_request_base * // able to stay up-to-date with this window, and would have to re-query in // `wm_import_start_no_flush` anyway. And we don't have a node to attach // the children to anyway. - if (reply_or_error != NULL && reply_or_error->response_type != 0) { - log_debug("Ignoring query tree reply for unknown window, it has " - "been destroyed."); + if (reply_or_error->response_type != 0) { + log_debug("Ignoring query tree reply for window not in our " + "tree."); + BUG_ON(wm_is_consistent(wm)); } - free(req); return; } - BUG_ON(node->req != req); - node->req = NULL; - free(req); - - if (reply_or_error == NULL) { - // The program is quitting... If this happens when wm is in an - // inconsistent state, there could be errors. - // TODO(yshui): fix this - goto out; + if (!node->receiving_events) { + log_debug("Window ID %#010x is destroyed then reused, and hasn't had " + "event mask set yet.", + node->id.x); + return; } if (reply_or_error->response_type == 0) { // This is an error, most likely the window is gone when we tried // to query it. + // A window we are tracking died without us knowing, this should + // be impossible. xcb_generic_error_t *err = (xcb_generic_error_t *)reply_or_error; - log_error("Query tree request for window %#010x failed with " - "error %s, this query should have been cancelled.", + log_error("Query tree request for window %#010x failed with error %s.", node == NULL ? 0 : node->id.x, x_strerror(err)); BUG_ON(false); } + if (node->tree_queried) { + log_debug("Window %#010x has already been queried.", node->id.x); + return; + } + + node->tree_queried = true; + auto reply = (const xcb_query_tree_reply_t *)reply_or_error; log_debug("Finished querying tree for window %#010x", node->id.x); @@ -530,9 +554,6 @@ wm_handle_query_tree_reply(struct x_connection *c, struct x_async_request_base * wm_reparent_no_flush(wm, c, atoms, child, node->id.x); } x_flush(c); // Actually send the requests - -out: - wm->n_pending_query_trees--; } static void wm_handle_get_wm_state_reply(struct x_connection * /*c*/, @@ -574,6 +595,77 @@ static void wm_handle_get_wm_state_reply(struct x_connection * /*c*/, free(req); } +struct wm_set_event_mask_request { + struct x_async_request_base base; + struct wm *wm; + struct atom *atoms; + xcb_window_t wid; +}; + +static void +wm_handle_set_event_mask_reply(struct x_connection *c, struct x_async_request_base *base, + const xcb_raw_generic_event_t *reply_or_error) { + auto req = (struct wm_set_event_mask_request *)base; + auto wm = req->wm; + auto atoms = req->atoms; + auto wid = req->wid; + free(req); + + if (reply_or_error == NULL) { + goto end_import; + } + if (reply_or_error->response_type == 0) { + log_debug("Failed to set event mask for window %#010x: %s, ignoring this " + "window.", + wid, x_strerror((const xcb_generic_error_t *)reply_or_error)); + goto end_import; + } + + auto node = wm_tree_find(&wm->tree, wid); + if (node == NULL) { + // The window initiated this request is gone, but a window with this wid + // does exist - we just haven't gotten the event for its creation yet. We + // create a placeholder for it. + node = wm_tree_new_window(&wm->tree, wid); + wm_tree_add_window(&wm->tree, node); + wm_tree_attach(&wm->tree, node, &wm->orphan_root); + } + + if (node->receiving_events) { + // This means another set event mask request was already completed before + // us. We don't need to do anything. + log_debug("Event mask already set for window %#010x", wid); + goto end_import; + } + + log_debug("Event mask set for window %#010x, sending query tree.", wid); + node->receiving_events = true; + + { + auto req2 = ccalloc(1, struct wm_query_tree_request); + req2->base.callback = wm_handle_query_tree_reply; + req2->wid = node->id.x; + req2->wm = wm; + req2->atoms = atoms; + x_async_query_tree(c, node->id.x, &req2->base); + } + + // (It's OK to resend the get property request even if one is already in-flight, + // unlike query tree.) + { + auto req2 = ccalloc(1, struct wm_get_property_request); + req2->base.callback = wm_handle_get_wm_state_reply; + req2->wm = wm; + req2->wid = node->id.x; + x_async_get_property(c, node->id.x, atoms->aWM_STATE, XCB_ATOM_ANY, 0, 2, + &req2->base); + } + return; + +end_import: + wm->n_pending_imports--; +} + /// Create a window for `wid`. Send query tree and get property requests for the window if /// it is possible that it is a new window. /// @@ -598,50 +690,29 @@ static void wm_import_start_no_flush(struct wm *wm, struct x_connection *c, stru return; } - auto e = x_change_window_attributes(c, wid, XCB_CW_EVENT_MASK, - (const uint32_t[]){WM_IMPORT_EV_MASK}); - if (e) { - log_debug("Failed to set event mask for window %#010x: %s, ignoring this " - "window.", - wid, x_strerror(e)); - free(e); - return; - } - + // We need to create a tree node immediate before we even know if it still exists. + // Because otherwise we have nothing to keep track of its stacking order. new = wm_tree_new_window(&wm->tree, wid); wm_tree_add_window(&wm->tree, new); + wm_tree_attach(&wm->tree, new, parent); log_debug("Starting import process for window %#010x", new->id.x); - wm_tree_attach(&wm->tree, new, parent); - - { - auto req = ccalloc(1, struct wm_query_tree_request); - req->base.callback = wm_handle_query_tree_reply; - req->node = new; - req->wm = wm; - req->atoms = atoms; - new->req = req; - wm->n_pending_query_trees++; - x_async_query_tree(c, new->id.x, &req->base); - } + auto req = ccalloc(1, struct wm_set_event_mask_request); + req->base.callback = wm_handle_set_event_mask_reply; + req->wid = wid; + req->atoms = atoms; + req->wm = wm; + x_async_change_window_attributes( + c, wid, XCB_CW_EVENT_MASK, (const uint32_t[]){WM_IMPORT_EV_MASK}, &req->base); - // (It's OK to resend the get property request even if one is already in-flight, - // unlike query tree.) - { - auto req = ccalloc(1, struct wm_get_property_request); - req->base.callback = wm_handle_get_wm_state_reply; - req->wm = wm; - req->wid = new->id.x; - x_async_get_property(c, new->id.x, atoms->aWM_STATE, XCB_ATOM_ANY, 0, 2, - &req->base); - } + wm->n_pending_imports++; } void wm_import_start(struct wm *wm, struct x_connection *c, struct atom *atoms, xcb_window_t wid, struct wm_ref *parent) { struct wm_tree_node *parent_node = parent != NULL ? to_tree_node_mut(parent) : NULL; - if (parent_node != NULL && parent_node->req != NULL) { + if (parent_node != NULL && !parent_node->tree_queried) { // Parent node is currently being queried, we can't attach the new window // to it as that will change its children list. return; @@ -651,7 +722,7 @@ void wm_import_start(struct wm *wm, struct x_connection *c, struct atom *atoms, } bool wm_is_consistent(const struct wm *wm) { - return wm->n_pending_query_trees == 0; + return wm->n_pending_imports == 0; } bool wm_has_tree_changes(const struct wm *wm) { diff --git a/src/wm/wm.h b/src/wm/wm.h index d5b1646a3d..7bb0facc32 100644 --- a/src/wm/wm.h +++ b/src/wm/wm.h @@ -165,7 +165,11 @@ void wm_destroy(struct wm *wm, xcb_window_t wid); void wm_reap_zombie(struct wm_ref *zombie); void wm_reparent(struct wm *wm, struct x_connection *c, struct atom *atoms, xcb_window_t wid, xcb_window_t parent); -void wm_disconnect(struct wm *wm, xcb_window_t child, xcb_window_t parent); +/// Disconnect `child` from its `parent`. If `new_parent_known` is true, the new parent +/// is a fully imported window in our tree. Otherwise, the new parent is either unknown, +/// or in the process of being imported. +void wm_disconnect(struct wm *wm, xcb_window_t child, xcb_window_t parent, + xcb_window_t new_parent); void wm_set_has_wm_state(struct wm *wm, struct wm_ref *cursor, bool has_wm_state); /// Start the import process for `wid`. diff --git a/src/wm/wm_internal.h b/src/wm/wm_internal.h index a1050f8c55..612d9f198c 100644 --- a/src/wm/wm_internal.h +++ b/src/wm/wm_internal.h @@ -60,9 +60,6 @@ struct wm_tree_node { struct wm_tree_node *leader_final; xcb_window_t leader; - /// The current in-flight query tree request for this window. - struct wm_query_tree_request *req; - bool has_wm_state : 1; /// Whether this window exists only on our side. A zombie window is a toplevel /// that has been destroyed or reparented (i.e. no long a toplevel) on the X @@ -70,6 +67,12 @@ struct wm_tree_node { /// window cannot be found in the wm_tree hash table. bool is_zombie : 1; bool visited : 1; + /// Whether we have set up event masks on this window. This means we can reliably + /// detect if the window is destroyed. + bool receiving_events : 1; + /// If the initial query tree request has completed. This means the children list + /// of this window is complete w.r.t. the event stream. + bool tree_queried : 1; }; /// Describe a change of a toplevel's client window. diff --git a/src/x.c b/src/x.c index 4ab2f60473..74417f0c8e 100644 --- a/src/x.c +++ b/src/x.c @@ -669,10 +669,12 @@ uint32_t x_create_region(struct x_connection *c, const region_t *reg) { return ret; } -xcb_generic_error_t *x_change_window_attributes(struct x_connection *c, xcb_window_t wid, - uint32_t mask, const uint32_t *values) { - return xcb_request_check( - c->c, xcb_change_window_attributes_checked(c->c, wid, mask, values)); +void x_async_change_window_attributes(struct x_connection *c, xcb_window_t wid, + uint32_t mask, const uint32_t *values, + struct x_async_request_base *req) { + req->sequence = xcb_change_window_attributes(c->c, wid, mask, values).sequence; + req->no_reply = true; + x_await_request(c, req); } void x_async_query_tree(struct x_connection *c, xcb_window_t wid, @@ -749,7 +751,7 @@ void x_free_picture(struct x_connection *c, xcb_render_picture_t p) { * @return a pointer to a string. this pointer shouldn NOT be freed, same buffer is used * for multiple calls to this function, */ -const char *x_strerror(xcb_generic_error_t *e) { +const char *x_strerror(const xcb_generic_error_t *e) { if (!e) { return "No error"; } diff --git a/src/x.h b/src/x.h index 2308cbeba4..3d264d0795 100644 --- a/src/x.h +++ b/src/x.h @@ -340,8 +340,9 @@ bool x_set_region(struct x_connection *c, xcb_xfixes_region_t dst, const region_ /// Create a X region from a pixman region uint32_t x_create_region(struct x_connection *c, const region_t *reg); -xcb_generic_error_t *x_change_window_attributes(struct x_connection *c, xcb_window_t wid, - uint32_t mask, const uint32_t *values); +void x_async_change_window_attributes(struct x_connection *c, xcb_window_t wid, + uint32_t mask, const uint32_t *values, + struct x_async_request_base *req); void x_async_query_tree(struct x_connection *c, xcb_window_t wid, struct x_async_request_base *req); void x_async_get_property(struct x_connection *c, xcb_window_t wid, xcb_atom_t atom, @@ -377,7 +378,7 @@ void x_print_error_impl(unsigned long serial, uint8_t major, uint16_t minor, * @return a pointer to a string. this pointer shouldn NOT be freed, same buffer is used * for multiple calls to this function, */ -const char *x_strerror(xcb_generic_error_t *e); +const char *x_strerror(const xcb_generic_error_t *e); void x_flush(struct x_connection *c); From 1a4948d80628724d0e0fb485a42a6ab07aaba62c Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Thu, 5 Sep 2024 15:45:17 +0100 Subject: [PATCH 17/24] wm: remove unnecessary x_flush Signed-off-by: Yuxuan Shui --- src/wm/wm.c | 26 +++++++++----------------- 1 file changed, 9 insertions(+), 17 deletions(-) diff --git a/src/wm/wm.c b/src/wm/wm.c index da6652b107..8bd273dbb9 100644 --- a/src/wm/wm.c +++ b/src/wm/wm.c @@ -414,11 +414,11 @@ struct wm_wid_or_node { /// reusing the same window ID as a previously destroyed window, and that destroyed window /// is in our orphan tree. In this case, we revive the orphaned window instead of creating /// a new one. -static void wm_import_start_no_flush(struct wm *wm, struct x_connection *c, struct atom *atoms, - xcb_window_t wid, struct wm_tree_node *parent); +static void wm_import_start_inner(struct wm *wm, struct x_connection *c, struct atom *atoms, + xcb_window_t wid, struct wm_tree_node *parent); -static void wm_reparent_no_flush(struct wm *wm, struct x_connection *c, struct atom *atoms, - xcb_window_t wid, xcb_window_t parent) { +void wm_reparent(struct wm *wm, struct x_connection *c, struct atom *atoms, + xcb_window_t wid, xcb_window_t parent) { auto window = wm_tree_find(&wm->tree, wid); auto new_parent = wm_tree_find(&wm->tree, parent); bool new_parent_imported = new_parent != NULL && new_parent->tree_queried; @@ -429,7 +429,7 @@ static void wm_reparent_no_flush(struct wm *wm, struct x_connection *c, struct a /// explicitly. if (window == NULL) { if (new_parent_imported) { - wm_import_start_no_flush(wm, c, atoms, wid, new_parent); + wm_import_start_inner(wm, c, atoms, wid, new_parent); } return; } @@ -469,12 +469,6 @@ static void wm_reparent_no_flush(struct wm *wm, struct x_connection *c, struct a wm_tree_attach(&wm->tree, window, new_parent); } -void wm_reparent(struct wm *wm, struct x_connection *c, struct atom *atoms, - xcb_window_t wid, xcb_window_t parent) { - wm_reparent_no_flush(wm, c, atoms, wid, parent); - x_flush(c); -} - void wm_set_has_wm_state(struct wm *wm, struct wm_ref *cursor, bool has_wm_state) { wm_tree_set_wm_state(&wm->tree, to_tree_node_mut(cursor), has_wm_state); } @@ -551,9 +545,8 @@ wm_handle_query_tree_reply(struct x_connection *c, struct x_async_request_base * auto child = children[i]; // wm_reparent handles both the case where child is new, and the case // where the child is an known orphan. - wm_reparent_no_flush(wm, c, atoms, child, node->id.x); + wm_reparent(wm, c, atoms, child, node->id.x); } - x_flush(c); // Actually send the requests } static void wm_handle_get_wm_state_reply(struct x_connection * /*c*/, @@ -670,8 +663,8 @@ wm_handle_set_event_mask_reply(struct x_connection *c, struct x_async_request_ba /// it is possible that it is a new window. /// /// Note this function does not flush the X connection. -static void wm_import_start_no_flush(struct wm *wm, struct x_connection *c, struct atom *atoms, - xcb_window_t wid, struct wm_tree_node *parent) { +static void wm_import_start_inner(struct wm *wm, struct x_connection *c, struct atom *atoms, + xcb_window_t wid, struct wm_tree_node *parent) { auto new = wm_tree_find(&wm->tree, wid); if (new != NULL) { // This happens if a window A is created, destroyed, and then another @@ -717,8 +710,7 @@ void wm_import_start(struct wm *wm, struct x_connection *c, struct atom *atoms, // to it as that will change its children list. return; } - wm_import_start_no_flush(wm, c, atoms, wid, parent_node); - x_flush(c); + wm_import_start_inner(wm, c, atoms, wid, parent_node); } bool wm_is_consistent(const struct wm *wm) { From 28281bfdda0310d578ec369269f4b9ad52323042 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Thu, 5 Sep 2024 20:14:44 +0100 Subject: [PATCH 18/24] wm: update client window when a window loses/gains toplevel status If a toplevel loses toplevel status, its "client window" will no longer be updated. If the client window is then destroyed, it becomes a dangling pointer. Signed-off-by: Yuxuan Shui --- src/wm/tree.c | 8 +++++++- src/wm/wm.c | 1 + 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/src/wm/tree.c b/src/wm/tree.c index be750440aa..ad55b23e11 100644 --- a/src/wm/tree.c +++ b/src/wm/tree.c @@ -310,6 +310,9 @@ wm_tree_refresh_client_and_queue_change(struct wm_tree *tree, struct wm_tree_nod if (new_client != NULL) { new_client_id = new_client->id; } + log_debug("Toplevel window %#010x had client window %#010x, now has " + "%#010x.", + toplevel->id.x, old_client_id.x, new_client_id.x); toplevel->client_window = new_client; wm_tree_enqueue_client_change(tree, toplevel, old_client_id, new_client_id); } @@ -328,6 +331,7 @@ struct wm_tree_node *wm_tree_detach(struct wm_tree *tree, struct wm_tree_node *s } } else { // Detached a toplevel, create a zombie for it + log_debug("Detaching toplevel window %#010x.", subroot->id.x); zombie = ccalloc(1, struct wm_tree_node); zombie->parent = subroot->parent; zombie->id = subroot->id; @@ -342,6 +346,7 @@ struct wm_tree_node *wm_tree_detach(struct wm_tree *tree, struct wm_tree_node *s // kill change won't cancel out a previous new change because the IDs will // be different. subroot->id.gen = tree->gen++; + subroot->client_window = NULL; } subroot->parent = NULL; return zombie; @@ -364,7 +369,8 @@ void wm_tree_attach(struct wm_tree *tree, struct wm_tree_node *child, auto toplevel = wm_tree_find_toplevel_for(tree, child); if (child == toplevel) { wm_tree_enqueue_toplevel_new(tree, child); - } else if (toplevel != NULL) { + } + if (toplevel != NULL) { wm_tree_refresh_client_and_queue_change(tree, toplevel); } } diff --git a/src/wm/wm.c b/src/wm/wm.c index 8bd273dbb9..75c2f9d361 100644 --- a/src/wm/wm.c +++ b/src/wm/wm.c @@ -714,6 +714,7 @@ void wm_import_start(struct wm *wm, struct x_connection *c, struct atom *atoms, } bool wm_is_consistent(const struct wm *wm) { + assert(wm->n_pending_imports > 0 || list_is_empty(&wm->orphan_root.children)); return wm->n_pending_imports == 0; } From 8ddab6dd56bf29e1fc891ec9dccf401ae74eadae Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Thu, 5 Sep 2024 20:24:51 +0100 Subject: [PATCH 19/24] wm: setting WM_STATE on toplevel should trigger refresh Previously we would just ignore these updates. But `wm_tree_find_client` does treat a toplevel with WM_STATE set as its own client window. So to keep things consistent, client window refresh is still needed in this case. Signed-off-by: Yuxuan Shui --- src/wm/tree.c | 3 +-- src/wm/wm_internal.h | 1 + 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/wm/tree.c b/src/wm/tree.c index ad55b23e11..885804cad5 100644 --- a/src/wm/tree.c +++ b/src/wm/tree.c @@ -196,7 +196,7 @@ struct wm_tree_node *wm_tree_next(struct wm_tree_node *node, struct wm_tree_node /// Find a client window under a toplevel window. If there are multiple windows with /// `WM_STATE` set under the toplevel window, we will return an arbitrary one. -static struct wm_tree_node *attr_pure wm_tree_find_client(struct wm_tree_node *subroot) { +struct wm_tree_node *attr_pure wm_tree_find_client(struct wm_tree_node *subroot) { if (subroot->has_wm_state) { log_debug("Toplevel %#010x has WM_STATE set, weird. Using itself as its " "client window.", @@ -257,7 +257,6 @@ void wm_tree_set_wm_state(struct wm_tree *tree, struct wm_tree_node *node, bool if (toplevel == node) { log_debug("Setting WM_STATE on a toplevel window %#010x, weird.", node->id.x); - return; } if (!has_wm_state && toplevel->client_window == node) { diff --git a/src/wm/wm_internal.h b/src/wm/wm_internal.h index 612d9f198c..04b91589dd 100644 --- a/src/wm/wm_internal.h +++ b/src/wm/wm_internal.h @@ -133,6 +133,7 @@ void wm_tree_move_to_end(struct wm_tree *tree, struct wm_tree_node *node, bool t struct wm_tree_change wm_tree_dequeue_change(struct wm_tree *tree); void wm_tree_reap_zombie(struct wm_tree_node *zombie); void wm_tree_set_wm_state(struct wm_tree *tree, struct wm_tree_node *node, bool has_wm_state); +struct wm_tree_node *attr_pure wm_tree_find_client(struct wm_tree_node *subroot); static inline void wm_tree_init(struct wm_tree *tree) { tree->nodes = NULL; From f8828eb23d60f791394d0a998a5ee2c3f598f738 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Thu, 5 Sep 2024 20:45:04 +0100 Subject: [PATCH 20/24] wm: don't report children of the orphan root as toplevel This is already accounted for by wm_tree_find_toplevel_for, but in wm_tree_set_wm_state, there is a bit of copy-and-pasted code that didn't. Signed-off-by: Yuxuan Shui --- src/wm/tree.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/wm/tree.c b/src/wm/tree.c index 885804cad5..96924b68fa 100644 --- a/src/wm/tree.c +++ b/src/wm/tree.c @@ -250,9 +250,9 @@ void wm_tree_set_wm_state(struct wm_tree *tree, struct wm_tree_node *node, bool node->has_wm_state = has_wm_state; BUG_ON(node->parent == NULL); // Trying to set WM_STATE on the root window - struct wm_tree_node *toplevel; - for (auto cur = node; cur->parent != NULL; cur = cur->parent) { - toplevel = cur; + struct wm_tree_node *toplevel = wm_tree_find_toplevel_for(tree, node); + if (toplevel == NULL) { + return; } if (toplevel == node) { From 9ab644726254035245507f54938212eaf39dfd64 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Fri, 6 Sep 2024 00:24:24 +0100 Subject: [PATCH 21/24] wm: update wm_is_consistent condition Since we know have split reparenting into 2 steps, the first of which will create an orphaned window, we now can have orphaned windows when there are no query trees in-flight. Update wm_is_consistent condition to account for that. Signed-off-by: Yuxuan Shui --- src/wm/wm.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/wm/wm.c b/src/wm/wm.c index 75c2f9d361..087efbd0c9 100644 --- a/src/wm/wm.c +++ b/src/wm/wm.c @@ -714,8 +714,7 @@ void wm_import_start(struct wm *wm, struct x_connection *c, struct atom *atoms, } bool wm_is_consistent(const struct wm *wm) { - assert(wm->n_pending_imports > 0 || list_is_empty(&wm->orphan_root.children)); - return wm->n_pending_imports == 0; + return wm->n_pending_imports == 0 && list_is_empty(&wm->orphan_root.children); } bool wm_has_tree_changes(const struct wm *wm) { From cd5b136240f95ab7803d142a3f1a7eb8451ac0f8 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Fri, 6 Sep 2024 00:39:45 +0100 Subject: [PATCH 22/24] wm: avoid repeated unnecessary wm_tree_find Pass tree node instead of window ID. Signed-off-by: Yuxuan Shui --- src/wm/wm.c | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/src/wm/wm.c b/src/wm/wm.c index 087efbd0c9..e24a548ffb 100644 --- a/src/wm/wm.c +++ b/src/wm/wm.c @@ -417,18 +417,17 @@ struct wm_wid_or_node { static void wm_import_start_inner(struct wm *wm, struct x_connection *c, struct atom *atoms, xcb_window_t wid, struct wm_tree_node *parent); -void wm_reparent(struct wm *wm, struct x_connection *c, struct atom *atoms, - xcb_window_t wid, xcb_window_t parent) { +static void wm_reparent_inner(struct wm *wm, struct x_connection *c, struct atom *atoms, + xcb_window_t wid, struct wm_tree_node *new_parent) { + BUG_ON_NULL(new_parent); auto window = wm_tree_find(&wm->tree, wid); - auto new_parent = wm_tree_find(&wm->tree, parent); - bool new_parent_imported = new_parent != NULL && new_parent->tree_queried; /// If a previously unseen window is reparented to a window that has been fully /// imported, we must treat it as a newly created window. Because it will not be /// included in a query tree reply, so we must initiate its import process /// explicitly. if (window == NULL) { - if (new_parent_imported) { + if (new_parent->tree_queried) { wm_import_start_inner(wm, c, atoms, wid, new_parent); } return; @@ -437,7 +436,7 @@ void wm_reparent(struct wm *wm, struct x_connection *c, struct atom *atoms, if (window->parent == new_parent) { // Reparent to the same parent moves the window to the top of the // stack - BUG_ON(!new_parent_imported); + BUG_ON(!new_parent->tree_queried); wm_tree_move_to_end(&wm->tree, window, false); return; } @@ -457,18 +456,24 @@ void wm_reparent(struct wm *wm, struct x_connection *c, struct atom *atoms, // a destroyed window's ID, then we know we will re-query the new parent later // when we encounter it in a query tree reply, so we orphan the window in this // case as well. - if (!new_parent_imported) { - log_debug("Window %#010x is attached to window %#010x which is " - "currently been queried, orphaning.", - window->id.x, parent); + if (!new_parent->tree_queried) { + log_debug("Window %#010x is attached to window %#010x that is currently " + "been queried, orphaning.", + window->id.x, new_parent->id.x); return; } - log_debug("Reparented window %#010x to window %#010x", window->id.x, parent); + log_debug("Reparented window %#010x to window %#010x", window->id.x, + new_parent->id.x); BUG_ON(wm_tree_detach(&wm->tree, window) != NULL); wm_tree_attach(&wm->tree, window, new_parent); } +void wm_reparent(struct wm *wm, struct x_connection *c, struct atom *atoms, + xcb_window_t wid, xcb_window_t parent) { + return wm_reparent_inner(wm, c, atoms, wid, wm_tree_find(&wm->tree, parent)); +} + void wm_set_has_wm_state(struct wm *wm, struct wm_ref *cursor, bool has_wm_state) { wm_tree_set_wm_state(&wm->tree, to_tree_node_mut(cursor), has_wm_state); } @@ -545,7 +550,7 @@ wm_handle_query_tree_reply(struct x_connection *c, struct x_async_request_base * auto child = children[i]; // wm_reparent handles both the case where child is new, and the case // where the child is an known orphan. - wm_reparent(wm, c, atoms, child, node->id.x); + wm_reparent_inner(wm, c, atoms, child, node); } } From 570381fe7703debae1874855186d4faf489f3677 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Sat, 7 Sep 2024 09:14:15 +0100 Subject: [PATCH 23/24] wm: remove an unreachable branch See comments for proof why it's not reachable. Signed-off-by: Yuxuan Shui --- src/wm/wm.c | 35 ++++++++++++++--------------------- 1 file changed, 14 insertions(+), 21 deletions(-) diff --git a/src/wm/wm.c b/src/wm/wm.c index e24a548ffb..29b66a994e 100644 --- a/src/wm/wm.c +++ b/src/wm/wm.c @@ -428,6 +428,7 @@ static void wm_reparent_inner(struct wm *wm, struct x_connection *c, struct atom /// explicitly. if (window == NULL) { if (new_parent->tree_queried) { + // Contract: we just checked `wid` is not in the tree. wm_import_start_inner(wm, c, atoms, wid, new_parent); } return; @@ -664,33 +665,15 @@ wm_handle_set_event_mask_reply(struct x_connection *c, struct x_async_request_ba wm->n_pending_imports--; } -/// Create a window for `wid`. Send query tree and get property requests for the window if -/// it is possible that it is a new window. +/// Create a window for `wid`. Send query tree and get property requests, for this new +/// window. Caller must guarantee `wid` isn't already in the tree. /// /// Note this function does not flush the X connection. static void wm_import_start_inner(struct wm *wm, struct x_connection *c, struct atom *atoms, xcb_window_t wid, struct wm_tree_node *parent) { - auto new = wm_tree_find(&wm->tree, wid); - if (new != NULL) { - // This happens if a window A is created, destroyed, and then another - // window B reuses the same window ID. We will receive a CreateNotify for - // A, but will import B. Then we will be here again when we receive the - // CreateNotify for B, which we have already imported. - log_debug("Window %#010x is already in our tree, reparenting.", wid); - if (new->parent != &wm->orphan_root) { - log_error("Window %#010x appeared twice in our tree. Once as a " - "child of %#010x, once as a child of %#010x", - wid, new->parent->id.x, parent->id.x); - BUG_ON(true); - } - BUG_ON(wm_tree_detach(&wm->tree, new) != NULL); - wm_tree_attach(&wm->tree, new, parent); - return; - } - // We need to create a tree node immediate before we even know if it still exists. // Because otherwise we have nothing to keep track of its stacking order. - new = wm_tree_new_window(&wm->tree, wid); + auto new = wm_tree_new_window(&wm->tree, wid); wm_tree_add_window(&wm->tree, new); wm_tree_attach(&wm->tree, new, parent); @@ -715,6 +698,16 @@ void wm_import_start(struct wm *wm, struct x_connection *c, struct atom *atoms, // to it as that will change its children list. return; } + // Contract: how do we know `wid` is not in the tree? First of all, if `wid` is in + // the tree, it must be an orphan node, otherwise that would mean the same `wid` + // is children of two different windows. Notice a node is added to the orphan root + // only after we have confirmed its existence by setting up its event mask. Also + // notice replies to event mask setup requests are processed in order. So if we + // haven't received a CreateNotify before a window enters the orphan tree, we will + // _never_ get a CreateNotify for it. + // + // We get to here by receiving a CreateNotify for `wid`, therefore `wid` cannot be + // in the orphan tree. wm_import_start_inner(wm, c, atoms, wid, parent_node); } From 25000a8b81ad817c0e80199e821087c4b32134fd Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Sun, 8 Sep 2024 00:31:53 +0100 Subject: [PATCH 24/24] misc: update CHANGELOG.md Signed-off-by: Yuxuan Shui --- CHANGELOG.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 54d5ef8ce5..0735bea124 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,8 +1,10 @@ -# Unreleased +# v12-rc4 (2024-Sep-08) ## Bug fixes -* Windows become completely black when `rules` and `inactive-dim` are set at the same time. +* Windows become completely black when `rules` and `inactive-dim` are set at the same time +* Fix segmentation fault during unredirection if the geometry change animation is used (#1333, thanks to @monsterovich) +* Fix many rare race conditions in the window management code (#1334) # v12-rc3 (2024-Aug-30)