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) 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/event.c b/src/event.c index 788daa7ca3..be702ef660 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 || @@ -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,20 @@ 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); - wm_destroy(ps->wm, 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) { + wm_disconnect(ps->wm, ev->window, ev->event, XCB_NONE); + } else { + 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 +475,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 +497,21 @@ 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); + if (ev->event == ev->window) { + return; + } + if (ev->parent != 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); + } } 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 63f668689e..afeeb7718b 100644 --- a/src/picom.c +++ b/src/picom.c @@ -1493,16 +1493,16 @@ 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*/, 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 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) { @@ -1513,7 +1513,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; } @@ -1522,17 +1522,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) { @@ -1543,12 +1552,12 @@ 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; } - 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; @@ -1571,11 +1580,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; diff --git a/src/wm/tree.c b/src/wm/tree.c index a34966a7f4..96924b68fa 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.", @@ -250,14 +250,13 @@ 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) { log_debug("Setting WM_STATE on a toplevel window %#010x, weird.", node->id.x); - return; } if (!has_wm_state && toplevel->client_window == node) { @@ -284,6 +283,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; @@ -307,6 +309,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); } @@ -325,6 +330,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; @@ -334,6 +340,12 @@ 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->client_window = NULL; } subroot->parent = NULL; return zombie; @@ -356,7 +368,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/win.c b/src/wm/win.c index ac2c1f215d..029341c2fc 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 @@ -1277,7 +1285,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, .frame_opacity_for_same_colors = false, @@ -1365,8 +1373,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; } @@ -2104,7 +2113,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; @@ -2139,7 +2148,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 471d64aec3..2e7345ed15 100644 --- a/src/wm/win.h +++ b/src/wm/win.h @@ -477,7 +477,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 bbabb6545c..29b66a994e 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" @@ -17,10 +16,9 @@ struct wm_query_tree_request { struct x_async_request_base base; - struct wm_tree_node *node; struct wm *wm; struct atom *atoms; - size_t pending_index; + xcb_window_t wid; }; struct wm_get_property_request { @@ -44,11 +42,10 @@ 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 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: @@ -255,15 +252,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 +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->pending_query_trees = dynarr_new(struct wm_query_tree_request *, 0); + wm->n_pending_imports = 0; return wm; } @@ -314,32 +302,10 @@ 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); } -/// 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_splice(&node->children, &wm->orphan_root.children); - } - 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) { @@ -349,54 +315,121 @@ static void wm_move_win(struct wm_tree_node *from, struct wm_tree_node *to) { from->win = NULL; } -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); - } +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, + 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); + 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; } + // 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->tree_queried || child_node->parent != &wm->orphan_root)); + + wm_detach_inner(wm, child_node); + 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) { + struct wm_tree_node *node = wm_tree_find(&wm->tree, wid); + BUG_ON_NULL(node); + 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) { 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); - } - // 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); - } + wm_detach_inner(wm, node); + + HASH_DEL(wm->tree.nodes, node); + free(node); } 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_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_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); - // We orphan the window here if parent is not found. We will reconnect - // this window later as query tree requests being completed. + /// 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); + 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; } @@ -404,15 +437,12 @@ void wm_reparent(struct wm *wm, xcb_window_t wid, xcb_window_t parent) { if (window->parent == new_parent) { // Reparent to the same parent moves the window to the top of the // stack + BUG_ON(!new_parent->tree_queried); 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); - } + 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 @@ -423,55 +453,95 @@ 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) { - log_debug("Window %#010x is attached to window %#010x which is " - "currently been queried, orphaning.", + // 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->tree_queried) { + log_debug("Window %#010x is attached to window %#010x that is currently " + "been queried, orphaning.", window->id.x, new_parent->id.x); - wm_tree_attach(&wm->tree, window, &wm->orphan_root); - } else { - wm_tree_attach(&wm->tree, window, new_parent); + return; } + + 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); } -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); +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) { + 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; - { - 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 = wm_tree_find(&wm->tree, req->wid); + free(req); + + wm->n_pending_imports--; if (reply_or_error == NULL) { - goto out; + // The program is quitting... If this happens when wm is in an + // inconsistent state, there could be errors. + // TODO(yshui): fix this + return; } - 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 window not in our " + "tree."); + BUG_ON(wm_is_consistent(wm)); + } + return; + } + + 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_debug("Query tree request for window %#010x failed with " - "error %s", - node->id.x, x_strerror(err)); - goto out; + 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; } - xcb_query_tree_reply_t *reply = (xcb_query_tree_reply_t *)reply_or_error; + 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); auto children = xcb_query_tree_children(reply); @@ -479,35 +549,15 @@ 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); - } - -out: - free(req); - xcb_flush(c->c); // Actually send the requests - if (wm_is_consistent(wm)) { - wm_reap_orphans(wm); + // wm_reparent handles both the case where child is new, and the case + // where the child is an known orphan. + wm_reparent_inner(wm, c, atoms, child, node); } } 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); @@ -531,102 +581,138 @@ 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. - auto reply = (xcb_get_property_reply_t *)reply_or_error; + 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 = (const xcb_get_property_reply_t *)reply_or_error; wm_tree_set_wm_state(&req->wm->tree, node, reply->type != XCB_NONE); 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_set_error_action_ignore( - c, xcb_change_window_attributes(c->c, wid, XCB_CW_EVENT_MASK, - (const uint32_t[]){WM_IMPORT_EV_MASK})); +struct wm_set_event_mask_request { + struct x_async_request_base base; + struct wm *wm; + struct atom *atoms; + xcb_window_t wid; +}; - // 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. - auto new = wm_tree_find(&wm->tree, wid); - if (new == NULL) { - new = wm_tree_new_window(&wm->tree, 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); - 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); - assert(false); - } +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); - 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++; + if (reply_or_error == NULL) { + goto end_import; } - 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) { - return; + 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 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); + 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 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); + 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 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) { + // 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. + auto 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); + + 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); + + 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 && wm_find_pending_query_tree(wm, parent_node) != -1) { + 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; } - wm_import_start_no_flush(wm, c, atoms, wid, parent_node); - xcb_flush(c->c); // Actually send the requests + // 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); } bool wm_is_consistent(const struct wm *wm) { - return dynarr_is_empty(wm->pending_query_trees); + return wm->n_pending_imports == 0 && list_is_empty(&wm->orphan_root.children); } bool wm_has_tree_changes(const struct wm *wm) { diff --git a/src/wm/wm.h b/src/wm/wm.h index 893905972d..7bb0facc32 100644 --- a/src/wm/wm.h +++ b/src/wm/wm.h @@ -163,7 +163,13 @@ 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); +/// 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 a255edeb08..04b91589dd 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; @@ -65,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. @@ -125,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; diff --git a/src/x.c b/src/x.c index 3961c73789..74417f0c8e 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); @@ -673,6 +669,28 @@ uint32_t x_create_region(struct x_connection *c, const region_t *reg) { return ret; } +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, + 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)); @@ -733,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"; } @@ -741,6 +759,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. */ @@ -925,7 +947,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); @@ -942,7 +964,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); @@ -973,166 +995,188 @@ 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; - } - return sequence_high | sequence; -} - -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; +static inline xcb_raw_generic_event_t * +x_ingest_event(struct x_connection *c, xcb_generic_event_t *event) { + if (event != NULL) { + 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 a_overflown ? 1 : -1; + return (xcb_raw_generic_event_t *)event; } -static xcb_raw_generic_event_t * -x_poll_for_event_impl(struct x_connection *c, struct x_async_request_base **out_req) { - 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; +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. +static const xcb_raw_generic_event_t * +x_poll_for_event_impl(struct x_connection *c, bool *retry) { + // 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_request_with_reply` is + // available. 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); + xcb_generic_error_t *err = NULL; + 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 && 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; + } + + // 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); + + // 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; } - } 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 (c->message_on_hold == NULL) { - // Didn't get any new information from the X connection, nothing to - // return. - return NULL; - } - // 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; - } - - // `next_event == c->message_on_hold` iff `on_hold_is_reply` is false. - - 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; - } + // 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; + } - 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) { + // 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; } - return (xcb_raw_generic_event_t *)next_event; - } - - // 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; + // 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; } - } 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; + 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) { - 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; + 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); } - - if (req != NULL) { - req->callback(c, req, ret); - } else if (ret->response_type == 0) { - x_handle_error(c, (xcb_generic_error_t *)ret); - } else { + bool retry = false; + ret = x_poll_for_event_impl(c, &retry); + if (ret == NULL && !list_is_empty(&c->pending_x_requests) && retry) { + continue; + } + 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; + x_handle_error(c, (xcb_generic_error_t *)ret); + free((void *)ret); } - if (c->message_on_hold->response_type >= 2 || - x_get_full_sequence(c, c->message_on_hold->sequence) != req->sequence) { - return; - } - 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 1f8b0bead7..3d264d0795 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 @@ -194,9 +198,12 @@ 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; + /// 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) { @@ -333,6 +340,15 @@ 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_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, + 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); @@ -362,7 +378,9 @@ 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); xcb_pixmap_t x_create_pixmap(struct x_connection *, uint8_t depth, int width, int height); @@ -437,12 +455,12 @@ 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); } -/// 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.