From de52cbd0e617d31b9a67ede71e6da4b0cb4e8268 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Mon, 24 Jun 2024 09:47:10 +0100 Subject: [PATCH 1/7] x: add support for receiving replies as part of the event stream Why do we need this? This is because, firstly, in the X protocol, events, errors, and replies are in a single stream to begin with, so it's not "unnatural" to put them back. And, some replies have to be handled in the correct "context". Say, the X window tree look like this: A / \ B C `C` is the new window, so send a query tree request for it, and you receive: > Event: Reparent B to C > Query tree reply: C has child B (Remember events, and replies are all in a single stream) If you handle the reply before the event, which is what will happen if you do `xcb_query_tree_reply(c, xcb_query_tree(c, ..))`, you will think window `B` appeared in 2 places. Also with this change the `x_handle_error` in `ev_handle` is move into `x_poll_for_event`. It just makes more sense to there. Signed-off-by: Yuxuan Shui --- src/event.c | 5 -- src/picom.c | 6 +- src/x.c | 230 +++++++++++++++++++++++++++++++++++++++++++++++----- src/x.h | 58 ++++++++++--- 4 files changed, 259 insertions(+), 40 deletions(-) diff --git a/src/event.c b/src/event.c index 953c882feb..dd113c99b3 100644 --- a/src/event.c +++ b/src/event.c @@ -629,10 +629,6 @@ ev_selection_clear(session_t *ps, xcb_selection_clear_event_t attr_unused *ev) { } void ev_handle(session_t *ps, xcb_generic_event_t *ev) { - if (XCB_EVENT_RESPONSE_TYPE(ev) != KeymapNotify) { - x_discard_pending_errors(&ps->c, ev->full_sequence); - } - xcb_window_t wid = ev_window(ps, ev); if (ev->response_type != ps->damage_event + XCB_DAMAGE_NOTIFY) { log_debug("event %10.10s serial %#010x window %#010x \"%s\"", @@ -702,7 +698,6 @@ void ev_handle(session_t *ps, xcb_generic_event_t *ev) { case XCB_SELECTION_CLEAR: ev_selection_clear(ps, (xcb_selection_clear_event_t *)ev); break; - case 0: x_handle_error(&ps->c, (xcb_generic_error_t *)ev); break; default: if (ps->shape_exists && ev->response_type == ps->shape_event) { ev_shape_notify(ps, (xcb_shape_notify_event_t *)ev); diff --git a/src/picom.c b/src/picom.c index ca8bdde6a0..a06fdb019c 100644 --- a/src/picom.c +++ b/src/picom.c @@ -1558,8 +1558,8 @@ static void handle_x_events(struct session *ps) { } xcb_generic_event_t *ev; - while ((ev = xcb_poll_for_event(ps->c.c))) { - ev_handle(ps, ev); + while ((ev = x_poll_for_event(&ps->c))) { + ev_handle(ps, (xcb_generic_event_t *)ev); free(ev); }; int err = xcb_connection_has_error(ps->c.c); @@ -1978,7 +1978,7 @@ static void draw_callback(EV_P_ ev_timer *w, int revents) { static void x_event_callback(EV_P attr_unused, ev_io *w, int revents attr_unused) { session_t *ps = (session_t *)w; - xcb_generic_event_t *ev = xcb_poll_for_event(ps->c.c); + xcb_generic_event_t *ev = x_poll_for_event(&ps->c); if (ev) { ev_handle(ps, ev); free(ev); diff --git a/src/x.c b/src/x.c index 256ba42ba7..6291306980 100644 --- a/src/x.c +++ b/src/x.c @@ -18,6 +18,8 @@ #include #include #include +#include +#include #include #include "atom.h" @@ -31,26 +33,24 @@ // === Error handling === -/** - * Xlib error handler function. - */ -static int xerror(Display attr_unused *dpy, XErrorEvent *ev) { - if (!ps_g) { - // Do not ignore errors until the session has been initialized - return 0; +/// Discard pending error handlers. +/// +/// 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); + } } - - // Fake a xcb error, fill in just enough information - xcb_generic_error_t xcb_err; - xcb_err.full_sequence = (uint32_t)ev->serial; - xcb_err.major_code = ev->request_code; - xcb_err.minor_code = ev->minor_code; - xcb_err.error_code = ev->error_code; - x_handle_error(&ps_g->c, &xcb_err); - return 0; -} - -void x_discard_pending_errors(struct x_connection *c, uint32_t sequence) { list_foreach_safe(struct pending_x_error, i, &c->pending_x_errors, siblings) { if (sequence <= i->sequence) { break; @@ -168,7 +168,7 @@ void x_print_error_impl(unsigned long serial, uint8_t major, uint16_t minor, /// Handle X errors. /// /// This function logs X errors, or aborts the program based on severity of the error. -void x_handle_error(struct x_connection *c, xcb_generic_error_t *ev) { +static void x_handle_error(struct x_connection *c, xcb_generic_error_t *ev) { x_discard_pending_errors(c, ev->full_sequence); struct pending_x_error *first_error_action = NULL; if (!list_is_empty(&c->pending_x_errors)) { @@ -204,6 +204,25 @@ void x_handle_error(struct x_connection *c, xcb_generic_error_t *ev) { ev->error_code)); } +/** + * Xlib error handler function. + */ +static int xerror(Display attr_unused *dpy, XErrorEvent *ev) { + if (!ps_g) { + // Do not ignore errors until the session has been initialized + return 0; + } + + // Fake a xcb error, fill in just enough information + xcb_generic_error_t xcb_err; + xcb_err.full_sequence = (uint32_t)ev->serial; + xcb_err.major_code = ev->request_code; + xcb_err.minor_code = ev->minor_code; + xcb_err.error_code = ev->error_code; + x_handle_error(&ps_g->c, &xcb_err); + return 0; +} + /// Initialize x_connection struct from an Xlib Display. /// /// Note this function doesn't take ownership of the Display, the caller is still @@ -212,10 +231,17 @@ void x_connection_init(struct x_connection *c, Display *dpy) { c->dpy = dpy; c->c = XGetXCBConnection(dpy); list_init_head(&c->pending_x_errors); + list_init_head(&c->pending_x_requests); c->previous_xerror_handler = XSetErrorHandler(xerror); c->screen = DefaultScreen(dpy); c->screen_info = xcb_aux_get_screen(c->c, c->screen); + c->message_on_hold = NULL; + + // Do a round trip to fetch the current sequence number + auto cookie = xcb_get_input_focus(c->c); + free(xcb_get_input_focus_reply(c->c, cookie, NULL)); + c->last_sequence = cookie.sequence; } /** @@ -909,3 +935,167 @@ 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; + } + 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) { + 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 (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; + } + + 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; + } + 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; + } + } 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; +} + +xcb_generic_event_t *x_poll_for_event(struct x_connection *c) { + 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 (req != NULL) { + req->callback(c, req, ret); + } else if (ret->response_type == 0) { + x_handle_error(c, (xcb_generic_error_t *)ret); + } else { + 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; + } + free(c->message_on_hold); + c->message_on_hold = NULL; +} diff --git a/src/x.h b/src/x.h index caaf4b4e65..6afc5ddcab 100644 --- a/src/x.h +++ b/src/x.h @@ -78,6 +78,17 @@ struct x_connection { // Private fields /// The error handling list. struct list_node pending_x_errors; + /// The list of pending async requests that we have + /// yet to receive a reply for. + struct list_node pending_x_requests; + /// 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 @@ -178,11 +189,25 @@ x_set_error_action(struct x_connection *c, uint32_t sequence, enum x_error_actio ((void)(cookie)) #endif +struct x_async_request_base { + struct list_node siblings; + /// 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); + /// The sequence number of the X request. + unsigned int sequence; +}; + static inline void attr_unused free_x_connection(struct x_connection *c) { list_foreach_safe(struct pending_x_error, i, &c->pending_x_errors, siblings) { list_remove(&i->siblings); free(i); } + list_foreach_safe(struct x_async_request_base, i, &c->pending_x_requests, siblings) { + list_remove(&i->siblings); + i->callback(c, i, NULL); + } XSetErrorHandler(c->previous_xerror_handler); } @@ -193,18 +218,6 @@ static inline void attr_unused free_x_connection(struct x_connection *c) { /// responsible for closing it after `free_x_connection` is called. void x_connection_init(struct x_connection *c, Display *dpy); -/// Discard queued pending replies. -/// -/// We have received reply with sequence number `sequence`, which means all pending -/// replies with sequence number less than `sequence` will never be received. So discard -/// them. -void x_discard_pending_errors(struct x_connection *c, uint32_t sequence); - -/// Handle X errors. -/// -/// This function logs X errors, or aborts the program based on severity of the error. -void x_handle_error(struct x_connection *c, xcb_generic_error_t *ev); - /** * Get a specific attribute of a window. * @@ -415,3 +428,24 @@ uint32_t attr_deprecated xcb_generate_id(xcb_connection_t *c); /// Ask X server to send us a notification for the next end of vblank. void x_request_vblank_event(struct x_connection *c, xcb_window_t window, uint64_t msc); + +/// Register an X request as async request. Its reply will be processed as part of the +/// event stream. i.e. the registered callback will only be called when all preceding +/// events have been retrieved via `x_poll_for_event`. +/// `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) { + 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. +xcb_generic_event_t *x_poll_for_event(struct x_connection *c); + +static inline bool x_has_pending_requests(struct x_connection *c) { + return !list_is_empty(&c->pending_x_requests); +} From 7d216657d0fc5e0abf0596b7fa34e48bcf3f1e72 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Tue, 25 Jun 2024 18:51:27 +0100 Subject: [PATCH 2/7] wm/tree: split wm_tree_new_window and wm_tree_reparent wm_tree_new_window is splitted into 3 steps: creating the window object (wm_tree_new_window), adding it to the hash table (wm_tree_add_window), and attaching it to its parent (wm_tree_attach). wm_tree_reparent is splitted into wm_tree_detach and then wm_tree_attach. Signed-off-by: Yuxuan Shui --- src/wm/tree.c | 108 +++++++++++++++++++------------------------ src/wm/wm.c | 19 ++++++-- src/wm/wm_internal.h | 10 ++-- 3 files changed, 69 insertions(+), 68 deletions(-) diff --git a/src/wm/tree.c b/src/wm/tree.c index 3c4c493234..32b93b6f29 100644 --- a/src/wm/tree.c +++ b/src/wm/tree.c @@ -234,31 +234,17 @@ void wm_tree_set_wm_state(struct wm_tree *tree, struct wm_tree_node *node, bool } } -struct wm_tree_node * -wm_tree_new_window(struct wm_tree *tree, xcb_window_t id, struct wm_tree_node *parent) { +struct wm_tree_node *wm_tree_new_window(struct wm_tree *tree, xcb_window_t id) { auto node = ccalloc(1, struct wm_tree_node); node->id.x = id; node->id.gen = tree->gen++; node->has_wm_state = false; list_init_head(&node->children); + return node; +} - BUG_ON(parent == NULL && tree->nodes != NULL); // Trying to create a second - // root window +void wm_tree_add_window(struct wm_tree *tree, struct wm_tree_node *node) { HASH_ADD_INT(tree->nodes, id.x, node); - - node->parent = parent; - if (parent != NULL) { - list_insert_after(&parent->children, &node->siblings); - if (parent->parent == NULL) { - // Parent is root, this is a new toplevel window - wm_tree_enqueue_change(tree, (struct wm_tree_change){ - .toplevel = node->id, - .type = WM_TREE_CHANGE_TOPLEVEL_NEW, - .new_ = node, - }); - } - } - return node; } static void @@ -307,6 +293,30 @@ void wm_tree_detach(struct wm_tree *tree, struct wm_tree_node *subroot) { .killed = zombie, }); } + subroot->parent = NULL; +} + +void wm_tree_attach(struct wm_tree *tree, struct wm_tree_node *child, + struct wm_tree_node *parent) { + BUG_ON_NULL(parent); + BUG_ON(child->parent != NULL); // Trying to attach a window that's already + // attached + child->parent = parent; + list_insert_after(&parent->children, &child->siblings); + + auto toplevel = wm_tree_find_toplevel_for(child); + if (child == toplevel) { + // This node could have a stale `->win` if it was a toplevel at + // some point in the past. + child->win = NULL; + wm_tree_enqueue_change(tree, (struct wm_tree_change){ + .toplevel = child->id, + .type = WM_TREE_CHANGE_TOPLEVEL_NEW, + .new_ = child, + }); + } else { + wm_tree_refresh_client_and_queue_change(tree, toplevel); + } } void wm_tree_destroy_window(struct wm_tree *tree, struct wm_tree_node *node) { @@ -386,38 +396,6 @@ void wm_tree_move_to_above(struct wm_tree *tree, struct wm_tree_node *node, } } -void wm_tree_reparent(struct wm_tree *tree, struct wm_tree_node *node, - struct wm_tree_node *new_parent) { - BUG_ON(node == NULL); - BUG_ON(new_parent == NULL); // Trying make `node` a root window - - if (node->parent == new_parent) { - // Reparent to the same parent moves the window to the top of the stack - wm_tree_move_to_end(tree, node, false); - return; - } - - wm_tree_detach(tree, node); - - // Reparented window always becomes the topmost child of the new parent - list_insert_after(&new_parent->children, &node->siblings); - node->parent = new_parent; - - auto toplevel = wm_tree_find_toplevel_for(node); - if (node == toplevel) { - // This node could have a stale `->win` if it was a toplevel at - // some point in the past. - node->win = NULL; - wm_tree_enqueue_change(tree, (struct wm_tree_change){ - .toplevel = node->id, - .type = WM_TREE_CHANGE_TOPLEVEL_NEW, - .new_ = node, - }); - } else { - wm_tree_refresh_client_and_queue_change(tree, toplevel); - } -} - void wm_tree_clear(struct wm_tree *tree) { struct wm_tree_node *cur, *tmp; HASH_ITER(hh, tree->nodes, cur, tmp) { @@ -438,7 +416,7 @@ TEST_CASE(tree_manipulation) { struct wm_tree tree; wm_tree_init(&tree); - wm_tree_new_window(&tree, 1, NULL); + wm_tree_add_window(&tree, wm_tree_new_window(&tree, 1)); auto root = wm_tree_find(&tree, 1); assert(root != NULL); assert(root->parent == NULL); @@ -446,9 +424,11 @@ TEST_CASE(tree_manipulation) { auto change = wm_tree_dequeue_change(&tree); assert(change.type == WM_TREE_CHANGE_NONE); - wm_tree_new_window(&tree, 2, root); - auto node2 = wm_tree_find(&tree, 2); + auto node2 = wm_tree_new_window(&tree, 2); + wm_tree_add_window(&tree, node2); + wm_tree_attach(&tree, node2, root); assert(node2 != NULL); + assert(node2 == wm_tree_find(&tree, 2)); assert(node2->parent == root); change = wm_tree_dequeue_change(&tree); @@ -456,15 +436,16 @@ TEST_CASE(tree_manipulation) { assert(change.type == WM_TREE_CHANGE_TOPLEVEL_NEW); assert(wm_treeid_eq(node2->id, change.toplevel)); - wm_tree_new_window(&tree, 3, root); - auto node3 = wm_tree_find(&tree, 3); - assert(node3 != NULL); + auto node3 = wm_tree_new_window(&tree, 3); + wm_tree_add_window(&tree, node3); + wm_tree_attach(&tree, node3, root); change = wm_tree_dequeue_change(&tree); assert(change.toplevel.x == 3); assert(change.type == WM_TREE_CHANGE_TOPLEVEL_NEW); - wm_tree_reparent(&tree, node2, node3); + wm_tree_detach(&tree, node2); + wm_tree_attach(&tree, node2, node3); assert(node2->parent == node3); assert(node3->children.next == &node2->siblings); @@ -481,8 +462,9 @@ TEST_CASE(tree_manipulation) { assert(wm_treeid_eq(change.client.old, WM_TREEID_NONE)); assert(change.client.new_.x == 2); - wm_tree_new_window(&tree, 4, node3); - auto node4 = wm_tree_find(&tree, 4); + auto node4 = wm_tree_new_window(&tree, 4); + wm_tree_add_window(&tree, node4); + wm_tree_attach(&tree, node4, node3); change = wm_tree_dequeue_change(&tree); assert(change.type == WM_TREE_CHANGE_NONE); @@ -500,7 +482,9 @@ TEST_CASE(tree_manipulation) { // Test window ID reuse wm_tree_destroy_window(&tree, node4); - node4 = wm_tree_new_window(&tree, 4, node3); + node4 = wm_tree_new_window(&tree, 4); + wm_tree_add_window(&tree, node4); + wm_tree_attach(&tree, node4, node3); wm_tree_set_wm_state(&tree, node4, true); change = wm_tree_dequeue_change(&tree); @@ -509,7 +493,9 @@ TEST_CASE(tree_manipulation) { assert(change.client.old.x == 4); assert(change.client.new_.x == 4); - auto node5 = wm_tree_new_window(&tree, 5, root); + auto node5 = wm_tree_new_window(&tree, 5); + wm_tree_add_window(&tree, node5); + wm_tree_attach(&tree, node5, root); wm_tree_destroy_window(&tree, node5); change = wm_tree_dequeue_change(&tree); assert(change.type == WM_TREE_CHANGE_NONE); // Changes cancelled out diff --git a/src/wm/wm.c b/src/wm/wm.c index c8d37156b0..51f05f4c7e 100644 --- a/src/wm/wm.c +++ b/src/wm/wm.c @@ -286,7 +286,16 @@ void wm_reparent(struct wm *wm, xcb_window_t wid, xcb_window_t parent) { return; } - wm_tree_reparent(&wm->tree, window, new_parent); + if (new_parent == window->parent) { + log_debug("Reparenting window %#010x to its current parent %#010x, " + "moving it to the top.", + wid, parent); + wm_tree_move_to_end(&wm->tree, window, false); + return; + } + + wm_tree_detach(&wm->tree, window); + wm_tree_attach(&wm->tree, window, new_parent); } void wm_set_has_wm_state(struct wm *wm, struct wm_ref *cursor, bool has_wm_state) { @@ -313,7 +322,9 @@ void wm_import_incomplete(struct wm *wm, xcb_window_t wid, xcb_window_t parent) } } log_debug("Importing window %#010x with parent %#010x", wid, parent); - auto new = wm_tree_new_window(&wm->tree, wid, parent_cursor); + auto new = wm_tree_new_window(&wm->tree, wid); + wm_tree_add_window(&wm->tree, new); + wm_tree_attach(&wm->tree, new, parent_cursor); dynarr_push(wm->incompletes, new); if (parent == XCB_NONE) { BUG_ON(wm->root != NULL); // Can't have more than one root @@ -383,7 +394,9 @@ static bool wm_complete_import_subtree(struct wm *wm, struct x_connection *c, children[i], curr->id.x); wm_tree_destroy_window(&wm->tree, existing); } - existing = wm_tree_new_window(&wm->tree, children[i], curr); + existing = wm_tree_new_window(&wm->tree, children[i]); + wm_tree_add_window(&wm->tree, existing); + wm_tree_attach(&wm->tree, existing, curr); wm_complete_import_single(wm, c, atoms, existing); } free(tree); diff --git a/src/wm/wm_internal.h b/src/wm/wm_internal.h index 9e33808a9b..e155e39031 100644 --- a/src/wm/wm_internal.h +++ b/src/wm/wm_internal.h @@ -24,6 +24,7 @@ struct wm_tree { uint64_t gen; /// wm tree nodes indexed by their X window ID. struct wm_tree_node *nodes; + struct wm_tree_node *root; struct list_node changes; struct list_node free_changes; @@ -85,15 +86,16 @@ struct wm_tree_node *attr_pure wm_tree_next(struct wm_tree_node *node, /// permitted, and the root window cannot be destroyed once created, until /// `wm_tree_clear` is called. If `parent` is not NULL, the new node will be put at the /// top of the stacking order among its siblings. -struct wm_tree_node * -wm_tree_new_window(struct wm_tree *tree, xcb_window_t id, struct wm_tree_node *parent); +struct wm_tree_node *wm_tree_new_window(struct wm_tree *tree, xcb_window_t id); +void wm_tree_add_window(struct wm_tree *tree, struct wm_tree_node *node); void wm_tree_destroy_window(struct wm_tree *tree, struct wm_tree_node *node); /// Detach the subtree rooted at `subroot` from `tree`. The subtree root is removed from /// its parent, and the disconnected tree nodes won't be able to be found via /// `wm_tree_find`. Relevant events will be generated. void wm_tree_detach(struct wm_tree *tree, struct wm_tree_node *subroot); -void wm_tree_reparent(struct wm_tree *tree, struct wm_tree_node *node, - struct wm_tree_node *new_parent); +/// Attach `node` to `parent`. `node` becomes the topmost child of `parent`. +void wm_tree_attach(struct wm_tree *tree, struct wm_tree_node *child, + struct wm_tree_node *parent); void wm_tree_move_to_above(struct wm_tree *tree, struct wm_tree_node *node, struct wm_tree_node *other); /// Move `node` to the top or the bottom of its parent's child window stack. From bb5d026a6f87be43bf9567bb3f05b6d890ea2855 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Wed, 26 Jun 2024 21:44:29 +0100 Subject: [PATCH 3/7] wm/tree: fix potential use-after-free in tree changes If a toplevel is reparented, then destroy, any WM_TREE_CHANGE_CLIENT events currently queued will reference the freed toplevel node. Note this doesn't happen when the toplevel is destroyed directly. Because in that case it will be turned into a zombie, and is guaranteed to only be freed after the client change event is handled. With this commit, when a toplevel is killed (i.e. destroyed or reparented), all currently queued tree changes will be updated to reference the zombie instead. Signed-off-by: Yuxuan Shui --- src/wm/tree.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/src/wm/tree.c b/src/wm/tree.c index 32b93b6f29..b851e89d47 100644 --- a/src/wm/tree.c +++ b/src/wm/tree.c @@ -40,16 +40,25 @@ static void wm_tree_enqueue_change(struct wm_tree *tree, struct wm_tree_change c // `WM_TREE_CHANGE_TOPLEVEL_NEW` change in the queue. bool found = false; list_foreach_safe(struct wm_tree_change_list, i, &tree->changes, siblings) { - if (i->item.type == WM_TREE_CHANGE_TOPLEVEL_NEW && - wm_treeid_eq(i->item.toplevel, change.toplevel)) { + if (!wm_treeid_eq(i->item.toplevel, change.toplevel)) { + continue; + } + if (i->item.type == WM_TREE_CHANGE_TOPLEVEL_NEW) { list_remove(&i->siblings); list_insert_after(&tree->free_changes, &i->siblings); found = true; - } else if (wm_treeid_eq(i->item.toplevel, change.toplevel) && found) { - // We also need to delete all other changes related to - // this toplevel in between the new and gone changes. + } else if (found) { + // We also need to delete all other changes + // related to this toplevel in between the new and + // gone changes. list_remove(&i->siblings); list_insert_after(&tree->free_changes, &i->siblings); + } else if (i->item.type == WM_TREE_CHANGE_CLIENT) { + // Need to update client changes, so they points to the + // zombie instead of the old toplevel node, since the old + // toplevel node could be freed before tree changes are + // processed. + i->item.client.toplevel = change.killed; } } if (found) { From 7675a015bd7393a3cf705ef9c3599296cd2d6cb6 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Wed, 26 Jun 2024 22:09:48 +0100 Subject: [PATCH 4/7] wm/tree: clear managed window object when a toplevel node is detached Signed-off-by: Yuxuan Shui --- src/wm/tree.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/wm/tree.c b/src/wm/tree.c index b851e89d47..e45810fa9d 100644 --- a/src/wm/tree.c +++ b/src/wm/tree.c @@ -294,6 +294,7 @@ void wm_tree_detach(struct wm_tree *tree, struct wm_tree_node *subroot) { zombie->id = subroot->id; zombie->is_zombie = true; zombie->win = subroot->win; + subroot->win = NULL; list_init_head(&zombie->children); list_replace(&subroot->siblings, &zombie->siblings); wm_tree_enqueue_change(tree, (struct wm_tree_change){ @@ -315,9 +316,6 @@ void wm_tree_attach(struct wm_tree *tree, struct wm_tree_node *child, auto toplevel = wm_tree_find_toplevel_for(child); if (child == toplevel) { - // This node could have a stale `->win` if it was a toplevel at - // some point in the past. - child->win = NULL; wm_tree_enqueue_change(tree, (struct wm_tree_change){ .toplevel = child->id, .type = WM_TREE_CHANGE_TOPLEVEL_NEW, From b85d3e26555c33cfbeef68ed579bff631ab32faa Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Wed, 26 Jun 2024 14:43:20 +0100 Subject: [PATCH 5/7] wm: no longer depends on server grabbing for tree consistency This is part of the effort to remove reliance on the X critical section for internal consistency. The rational will be explained in later commits when it is complete. Instead of using server grabbing to guarantee that our tree is synchronized with the X server, this commit moves the wm tree to an "eventually consistent" model, utilizing the newly introduced async X request infrastructure (See commit: "x: add support for receiving replies as part of the event stream"). The algorithm is like this: let's look at each tree node individually, every node has a list of children. If this list consistent with the X server for each of the tree nodes, the entire tree is consistent. Let's also define a "partial consistency" concept. A tree is partially consistent, if for all the nodes in the tree, their lists of chilren are either consistent, or empty. Meaning some nodes might exist on the X server, but not replicated on our side. If a node's parent isn't in our tree, we call that node "orphaned". (Conveniently, a toplevel can never be orphaned, because the root window is always in the tree.) To achieve partial consistency, when each node is discovered (either via a CreateNotify from the server, or via a QueryTree request, you will see later), we set up an event mask on it to receive further updates, and then send a QueryTree request for it. The trick is that the reply to the QueryTree request will be processed in order with all the other events, errors and replies. Assuming the tree started out partially consistent, it will still be partially consistent after we processed the reply. Because the reply contains up-to-date information about that node at that point in time; and we have also processed all events that came before that reply, so all other nodes are consistent too. Further more, the tree will stay partially consistent, because the event mask we set up will keep bringing us new information for the newly discovered node. This partial consistency will eventually become full consistency after all of our requests to the X server have completed. Note this means we might start rendering with a partial window tree. In practice, that should hardly be noticeable. (The explanation above glossed over many implementation concerns, some of those are documentation in code comments.) Signed-off-by: Yuxuan Shui --- src/c2.c | 5 +- src/event.c | 68 +++--- src/event.h | 1 + src/inspect.c | 37 +++- src/picom.c | 54 +---- src/wm/tree.c | 78 +++---- src/wm/win.c | 2 +- src/wm/wm.c | 509 ++++++++++++++++++++++++++----------------- src/wm/wm.h | 93 ++++---- src/wm/wm_internal.h | 9 +- 10 files changed, 481 insertions(+), 375 deletions(-) diff --git a/src/c2.c b/src/c2.c index b8891b7006..08733c1454 100644 --- a/src/c2.c +++ b/src/c2.c @@ -442,11 +442,11 @@ TEST_CASE(c2_parse) { TEST_STREQUAL3(str, "name = \"xterm\"", len); struct wm *wm = wm_new(); - wm_import_incomplete(wm, 1, XCB_NONE); + struct wm_ref *node = wm_new_mock_window(wm, 1); struct win test_win = { .name = "xterm", - .tree_ref = wm_find(wm, 1), + .tree_ref = node, }; TEST_TRUE(c2_match(state, &test_win, cond, NULL)); c2_list_postprocess(state, NULL, cond); @@ -568,6 +568,7 @@ TEST_CASE(c2_parse) { TEST_STREQUAL3(str, rule, len); c2_list_free(&cond, NULL); + wm_free_mock_window(wm, test_win.tree_ref); wm_free(wm); } diff --git a/src/event.c b/src/event.c index dd113c99b3..77b20cadaf 100644 --- a/src/event.c +++ b/src/event.c @@ -203,10 +203,14 @@ static inline void ev_focus_out(session_t *ps, xcb_focus_out_event_t *ev) { } static inline void ev_create_notify(session_t *ps, xcb_create_notify_event_t *ev) { - if (wm_is_wid_masked(ps->wm, ev->parent)) { - return; + auto parent = wm_find(ps->wm, ev->parent); + if (parent == NULL) { + log_error("Create notify received for window %#010x, but its parent " + "window %#010x is not in our tree. Expect malfunction.", + ev->window, ev->parent); + assert(false); } - wm_import_incomplete(ps->wm, ev->window, ev->parent); + wm_import_start(ps->wm, &ps->c, ps->atoms, ev->window, parent); } /// Handle configure event of a regular window @@ -215,7 +219,11 @@ static void configure_win(session_t *ps, xcb_configure_notify_event_t *ce) { auto below = wm_find(ps->wm, ce->above_sibling); if (!cursor) { - log_error("Configure event received for unknown window %#010x", ce->window); + if (wm_is_consistent(ps->wm)) { + log_error("Configure event received for unknown window %#010x", + ce->window); + assert(false); + } return; } @@ -223,6 +231,7 @@ static void configure_win(session_t *ps, xcb_configure_notify_event_t *ce) { log_error("Configure event received for window %#010x, but its sibling " "window %#010x is not in our tree. Expect malfunction.", ce->window, ce->above_sibling); + assert(false); } else if (below != NULL) { wm_stack_move_to_above(ps->wm, cursor, below); } else { @@ -288,7 +297,7 @@ static inline void ev_configure_notify(session_t *ps, xcb_configure_notify_event if (ev->window == ps->c.screen_info->root) { set_root_flags(ps, ROOT_FLAGS_CONFIGURED); - } else if (!wm_is_wid_masked(ps->wm, ev->event)) { + } else { configure_win(ps, ev); } } @@ -314,15 +323,14 @@ static inline void ev_map_notify(session_t *ps, xcb_map_notify_event_t *ev) { return; } - if (wm_is_wid_masked(ps->wm, ev->event)) { - return; - } - auto cursor = wm_find(ps->wm, ev->window); if (cursor == NULL) { - log_error("Map event received for unknown window %#010x, overlay is " - "%#010x", - ev->window, ps->overlay); + if (wm_is_consistent(ps->wm)) { + log_debug("Map event received for unknown window %#010x, overlay " + "is %#010x", + ev->window, ps->overlay); + assert(false); + } return; } @@ -349,13 +357,12 @@ static inline void ev_unmap_notify(session_t *ps, xcb_unmap_notify_event_t *ev) return; } - if (wm_is_wid_masked(ps->wm, ev->event)) { - return; - } - auto cursor = wm_find(ps->wm, ev->window); if (cursor == NULL) { - log_error("Unmap event received for unknown window %#010x", ev->window); + if (wm_is_consistent(ps->wm)) { + log_error("Unmap event received for unknown window %#010x", ev->window); + assert(false); + } return; } auto w = wm_ref_deref(cursor); @@ -372,14 +379,14 @@ static inline void ev_reparent_notify(session_t *ps, xcb_reparent_notify_event_t } static inline void ev_circulate_notify(session_t *ps, xcb_circulate_notify_event_t *ev) { - if (wm_is_wid_masked(ps->wm, ev->event)) { - return; - } - auto cursor = wm_find(ps->wm, ev->window); if (cursor == NULL) { - log_error("Circulate event received for unknown window %#010x", ev->window); + if (wm_is_consistent(ps->wm)) { + log_debug("Circulate event received for unknown window %#010x", + ev->window); + assert(false); + } return; } @@ -451,18 +458,18 @@ static inline void ev_property_notify(session_t *ps, xcb_property_notify_event_t return; } - if (wm_is_wid_masked(ps->wm, ev->window)) { - return; - } - ps->pending_updates = true; auto cursor = wm_find(ps->wm, ev->window); if (cursor == NULL) { - log_error("Property notify received for unknown window %#010x", ev->window); + if (wm_is_consistent(ps->wm)) { + log_error("Property notify received for unknown window %#010x", + ev->window); + assert(false); + } return; } - auto toplevel_cursor = wm_ref_toplevel_of(cursor); + auto toplevel_cursor = wm_ref_toplevel_of(ps->wm, cursor); if (ev->atom == ps->atoms->aWM_STATE) { log_debug("WM_STATE changed for window %#010x (%s): %s", ev->window, ev_window_name(ps, ev->window), @@ -470,6 +477,11 @@ static inline void ev_property_notify(session_t *ps, xcb_property_notify_event_t wm_set_has_wm_state(ps->wm, cursor, ev->state != XCB_PROPERTY_DELETE); } + if (toplevel_cursor == NULL) { + assert(!wm_is_consistent(ps->wm)); + return; + } + // We only care if the property is set on the toplevel itself, or on its // client window if it has one. WM_STATE is an exception, it is handled // always because it is what determines if a window is a client window. diff --git a/src/event.h b/src/event.h index 629dec0f3f..675b4d8c10 100644 --- a/src/event.h +++ b/src/event.h @@ -1,6 +1,7 @@ // SPDX-License-Identifier: MPL-2.0 // Copyright (c) 2019, Yuxuan Shui +#pragma once #include #include "common.h" diff --git a/src/inspect.c b/src/inspect.c index 33a646af70..d6e0160225 100644 --- a/src/inspect.c +++ b/src/inspect.c @@ -34,10 +34,13 @@ setup_window(struct x_connection *c, struct atom *atoms, struct options *options return NULL; } - auto toplevel = wm_ref_toplevel_of(cursor); + auto toplevel = wm_ref_toplevel_of(wm, cursor); + BUG_ON_NULL(toplevel); struct win *w = ccalloc(1, struct win); w->state = WSTATE_MAPPED; w->tree_ref = toplevel; + log_debug("Toplevel is %#010x", wm_ref_win_id(toplevel)); + log_debug("Client is %#010x", win_client_id(w, true)); win_update_wintype(c, atoms, w); win_update_frame_extents(c, atoms, w, win_client_id(w, /*fallback_to_self=*/true), options->frame_opacity); @@ -195,14 +198,40 @@ int inspect_main(int argc, char **argv, const char *config_file) { return 1; } - auto atoms attr_unused = init_atoms(c.c); + auto atoms = init_atoms(c.c); auto state = c2_state_new(atoms); options_postprocess_c2_lists(state, &c, &options); struct wm *wm = wm_new(); - wm_import_incomplete(wm, c.screen_info->root, XCB_NONE); - wm_complete_import(wm, &c, atoms); + wm_import_start(wm, &c, atoms, c.screen_info->root, NULL); + // Process events until the window tree is consistent + while (x_has_pending_requests(&c)) { + auto ev = x_poll_for_event(&c); + if (ev == NULL) { + continue; + } + switch (ev->response_type) { + case XCB_CREATE_NOTIFY:; + auto create = (xcb_create_notify_event_t *)ev; + auto parent = wm_find(wm, create->parent); + wm_import_start(wm, &c, atoms, + ((xcb_create_notify_event_t *)ev)->window, parent); + break; + case XCB_DESTROY_NOTIFY: + wm_destroy(wm, ((xcb_destroy_notify_event_t *)ev)->window); + break; + case XCB_REPARENT_NOTIFY:; + auto reparent = (xcb_reparent_notify_event_t *)ev; + wm_reparent(wm, reparent->window, reparent->parent); + break; + default: + // Ignore ConfigureNotify and CirculateNotify, because we don't + // use stacking order for window rules. + break; + } + free(ev); + } auto target = select_window(&c); log_info("Target window: %#x", target); diff --git a/src/picom.c b/src/picom.c index a06fdb019c..843972f61e 100644 --- a/src/picom.c +++ b/src/picom.c @@ -509,7 +509,11 @@ static void recheck_focus(session_t *ps) { return; } - cursor = wm_ref_toplevel_of(cursor); + cursor = wm_ref_toplevel_of(ps->wm, cursor); + if (cursor == NULL) { + assert(!wm_is_consistent(ps->wm)); + return; + } // And we set the focus state here auto w = wm_ref_deref(cursor); @@ -1575,10 +1579,6 @@ static void handle_x_events_ev(EV_P attr_unused, ev_prepare *w, int revents attr } static void handle_new_windows(session_t *ps) { - while (!wm_complete_import(ps->wm, &ps->c, ps->atoms)) { - handle_x_events(ps); - } - // Check tree changes first, because later property updates need accurate // client window information struct win *w = NULL; @@ -1676,8 +1676,7 @@ static void handle_pending_updates(struct session *ps, double delta_t) { /// and forces us to use some hacky code to recover when we discover we are /// out-of-sync.) handle_x_events(ps); - if (ps->pending_updates || wm_has_incomplete_imports(ps->wm) || - wm_has_tree_changes(ps->wm)) { + if (ps->pending_updates || wm_has_tree_changes(ps->wm)) { log_debug("Delayed handling of events, entering critical section"); // Process new windows, and maybe allocate struct managed_win for them handle_new_windows(ps); @@ -1804,9 +1803,9 @@ static void draw_callback_impl(EV_P_ session_t *ps, int revents attr_unused) { log_fatal("Couldn't find specified benchmark window."); exit(1); } - w = wm_ref_toplevel_of(w); + w = wm_ref_toplevel_of(ps->wm, w); - auto win = wm_ref_deref(w); + auto win = w == NULL ? NULL : wm_ref_deref(w); if (win != NULL) { add_damage_from_win(ps, win); } else { @@ -2511,38 +2510,7 @@ static session_t *session_init(int argc, char **argv, Display *dpy, } ps->wm = wm_new(); - wm_import_incomplete(ps->wm, ps->c.screen_info->root, XCB_NONE); - - e = xcb_request_check(ps->c.c, xcb_grab_server_checked(ps->c.c)); - if (e) { - log_fatal_x_error(e, "Failed to grab X server"); - free(e); - goto err; - } - - ps->server_grabbed = true; - - // We are going to pull latest information from X server now, events sent by X - // earlier is irrelevant at this point. - // A better solution is probably grabbing the server from the very start. But I - // think there still could be race condition that mandates discarding the events. - x_discard_events(&ps->c); - - wm_complete_import(ps->wm, &ps->c, ps->atoms); - - e = xcb_request_check(ps->c.c, xcb_ungrab_server_checked(ps->c.c)); - if (e) { - log_fatal_x_error(e, "Failed to ungrab server"); - free(e); - goto err; - } - - ps->server_grabbed = false; - - log_debug("Initial stack:"); - wm_stack_foreach(ps->wm, i) { - log_debug(" %#010x", wm_ref_win_id(i)); - } + 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); @@ -2737,8 +2705,10 @@ static void session_destroy(session_t *ps) { ev_signal_stop(ps->loop, &ps->usr1_signal); ev_signal_stop(ps->loop, &ps->int_signal); - wm_free(ps->wm); + // The X connection could hold references to wm if there are pending async + // requests. Therefore the wm must be freed after the X connection. free_x_connection(&ps->c); + wm_free(ps->wm); } /** diff --git a/src/wm/tree.c b/src/wm/tree.c index e45810fa9d..99e195d30f 100644 --- a/src/wm/tree.c +++ b/src/wm/tree.c @@ -135,6 +135,10 @@ struct wm_tree_change wm_tree_dequeue_change(struct wm_tree *tree) { /// Return the next node in the subtree after `node` in a pre-order traversal. Returns /// NULL if `node` is the last node in the traversal. struct wm_tree_node *wm_tree_next(struct wm_tree_node *node, struct wm_tree_node *subroot) { + if (node == NULL) { + return NULL; + } + if (!list_is_empty(&node->children)) { // Descend if there are children return list_entry(node->children.next, struct wm_tree_node, siblings); @@ -180,7 +184,8 @@ struct wm_tree_node *wm_tree_find(const struct wm_tree *tree, xcb_window_t id) { return node; } -struct wm_tree_node *wm_tree_find_toplevel_for(struct wm_tree_node *node) { +struct wm_tree_node * +wm_tree_find_toplevel_for(const struct wm_tree *tree, struct wm_tree_node *node) { BUG_ON_NULL(node); BUG_ON_NULL(node->parent); // Trying to find toplevel for the root // window @@ -189,7 +194,7 @@ struct wm_tree_node *wm_tree_find_toplevel_for(struct wm_tree_node *node) { for (auto curr = node; curr->parent != NULL; curr = curr->parent) { toplevel = curr; } - return toplevel; + return toplevel->parent == tree->root ? toplevel : NULL; } /// Change whether a tree node has the `WM_STATE` property set. @@ -283,10 +288,12 @@ void wm_tree_detach(struct wm_tree *tree, struct wm_tree_node *subroot) { BUG_ON(subroot == NULL); BUG_ON(subroot->parent == NULL); // Trying to detach the root window?! - auto toplevel = wm_tree_find_toplevel_for(subroot); + auto toplevel = wm_tree_find_toplevel_for(tree, subroot); if (toplevel != subroot) { list_remove(&subroot->siblings); - wm_tree_refresh_client_and_queue_change(tree, toplevel); + if (toplevel != NULL) { + wm_tree_refresh_client_and_queue_change(tree, toplevel); + } } else { // Detached a toplevel, create a zombie for it auto zombie = ccalloc(1, struct wm_tree_node); @@ -308,57 +315,30 @@ void wm_tree_detach(struct wm_tree *tree, struct wm_tree_node *subroot) { void wm_tree_attach(struct wm_tree *tree, struct wm_tree_node *child, struct wm_tree_node *parent) { - BUG_ON_NULL(parent); BUG_ON(child->parent != NULL); // Trying to attach a window that's already // attached child->parent = parent; + if (parent == NULL) { + BUG_ON(tree->root != NULL); // Trying to create a second root + // window + tree->root = child; + return; + } + list_insert_after(&parent->children, &child->siblings); - auto toplevel = wm_tree_find_toplevel_for(child); + auto toplevel = wm_tree_find_toplevel_for(tree, child); if (child == toplevel) { wm_tree_enqueue_change(tree, (struct wm_tree_change){ .toplevel = child->id, .type = WM_TREE_CHANGE_TOPLEVEL_NEW, .new_ = child, }); - } else { + } else if (toplevel != NULL) { wm_tree_refresh_client_and_queue_change(tree, toplevel); } } -void wm_tree_destroy_window(struct wm_tree *tree, struct wm_tree_node *node) { - BUG_ON(node == NULL); - BUG_ON(node->parent == NULL); // Trying to destroy the root window?! - - if (node->has_wm_state) { - wm_tree_set_wm_state(tree, node, false); - } - - HASH_DEL(tree->nodes, node); - - if (!list_is_empty(&node->children)) { - log_error("Window %#010x is destroyed, but it still has children. Expect " - "malfunction.", - node->id.x); - list_foreach_safe(struct wm_tree_node, i, &node->children, siblings) { - log_error(" Child window %#010x", i->id.x); - wm_tree_destroy_window(tree, i); - } - } - - if (node->parent->parent == NULL) { - node->is_zombie = true; - wm_tree_enqueue_change(tree, (struct wm_tree_change){ - .toplevel = node->id, - .type = WM_TREE_CHANGE_TOPLEVEL_KILLED, - .killed = node, - }); - } else { - list_remove(&node->siblings); - free(node); - } -} - void wm_tree_move_to_end(struct wm_tree *tree, struct wm_tree_node *node, bool to_bottom) { BUG_ON(node == NULL); BUG_ON(node->parent == NULL); // Trying to move the root window @@ -374,7 +354,7 @@ void wm_tree_move_to_end(struct wm_tree *tree, struct wm_tree_node *node, bool t } else { list_insert_after(&node->parent->children, &node->siblings); } - if (node->parent->parent == NULL) { + if (node->parent == tree->root) { wm_tree_enqueue_change(tree, (struct wm_tree_change){ .type = WM_TREE_CHANGE_TOPLEVEL_RESTACKED, }); @@ -396,7 +376,7 @@ void wm_tree_move_to_above(struct wm_tree *tree, struct wm_tree_node *node, list_remove(&node->siblings); list_insert_before(&other->siblings, &node->siblings); - if (node->parent->parent == NULL) { + if (node->parent == tree->root) { wm_tree_enqueue_change(tree, (struct wm_tree_change){ .type = WM_TREE_CHANGE_TOPLEVEL_RESTACKED, }); @@ -428,6 +408,8 @@ TEST_CASE(tree_manipulation) { assert(root != NULL); assert(root->parent == NULL); + tree.root = root; + auto change = wm_tree_dequeue_change(&tree); assert(change.type == WM_TREE_CHANGE_NONE); @@ -480,7 +462,9 @@ TEST_CASE(tree_manipulation) { // node3 already has node2 as its client window, so the new one should be ignored. assert(change.type == WM_TREE_CHANGE_NONE); - wm_tree_destroy_window(&tree, node2); + wm_tree_detach(&tree, node2); + HASH_DEL(tree.nodes, node2); + free(node2); change = wm_tree_dequeue_change(&tree); assert(change.toplevel.x == 3); assert(change.type == WM_TREE_CHANGE_CLIENT); @@ -488,7 +472,9 @@ TEST_CASE(tree_manipulation) { assert(change.client.new_.x == 4); // Test window ID reuse - wm_tree_destroy_window(&tree, node4); + wm_tree_detach(&tree, node4); + HASH_DEL(tree.nodes, node4); + free(node4); node4 = wm_tree_new_window(&tree, 4); wm_tree_add_window(&tree, node4); wm_tree_attach(&tree, node4, node3); @@ -503,7 +489,9 @@ TEST_CASE(tree_manipulation) { auto node5 = wm_tree_new_window(&tree, 5); wm_tree_add_window(&tree, node5); wm_tree_attach(&tree, node5, root); - wm_tree_destroy_window(&tree, node5); + wm_tree_detach(&tree, node5); + HASH_DEL(tree.nodes, node5); + free(node5); change = wm_tree_dequeue_change(&tree); assert(change.type == WM_TREE_CHANGE_NONE); // Changes cancelled out diff --git a/src/wm/win.c b/src/wm/win.c index 43cb5629d8..af3bde3055 100644 --- a/src/wm/win.c +++ b/src/wm/win.c @@ -1591,7 +1591,7 @@ static struct wm_ref *win_get_leader_raw(session_t *ps, struct win *w, int recur // ancestors if (w->cache_leader && w->cache_leader != client_win && w->cache_leader != w->tree_ref) { - auto parent = wm_ref_toplevel_of(w->cache_leader); + auto parent = wm_ref_toplevel_of(ps->wm, w->cache_leader); auto wp = parent ? wm_ref_deref(parent) : NULL; if (wp) { // Dead loop? diff --git a/src/wm/wm.c b/src/wm/wm.c index 51f05f4c7e..58e75c5eac 100644 --- a/src/wm/wm.c +++ b/src/wm/wm.c @@ -15,6 +15,20 @@ #include "wm.h" #include "wm_internal.h" +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; +}; + +struct wm_get_property_request { + struct x_async_request_base base; + struct wm *wm; + xcb_window_t wid; +}; + struct wm { /// Pointer to win of current active window. Used by /// EWMH _NET_ACTIVE_WINDOW focus detection. In theory, @@ -27,14 +41,20 @@ struct wm { struct wm_tree_node *active_leader; struct wm_tree tree; - struct wm_tree_node *root; - - /// Incomplete imports. See `wm_import_incomplete` for an explanation. - /// This is a dynarr. - struct wm_tree_node **incompletes; - /// Tree nodes that we have chosen to forget, but we might still receive some - /// events from, we keep them here to ignore those events. - struct wm_tree_node **masked; + /// This is a virtual root for all "orphaned" windows. A window is orphaned + /// if it is not reachable from the root node. This can only be non-empty if + /// the tree is not consistent, i.e. there are pending async query tree requests. + /// + /// Note an orphaned window cannot be a toplevel. This is trivially true because + /// a toplevel has the root window as its parent, and once the root window is + /// 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; }; // TODO(yshui): this is a bit weird and I am not decided on it yet myself. Maybe we can @@ -86,19 +106,6 @@ void wm_ref_set(struct wm_ref *cursor, struct win *w) { to_tree_node_mut(cursor)->win = w; } -static ptrdiff_t wm_find_masked(struct wm *wm, xcb_window_t wid) { - dynarr_foreach(wm->masked, m) { - if ((*m)->id.x == wid) { - return m - wm->masked; - } - } - return -1; -} - -bool wm_is_wid_masked(struct wm *wm, xcb_window_t wid) { - return wm_find_masked(wm, wid) != -1; -} - struct win *wm_active_win(struct wm *wm) { return wm->active_win; } @@ -132,7 +139,7 @@ struct wm_ref *wm_ref_above(const struct wm_ref *cursor) { } struct wm_ref *wm_root_ref(const struct wm *wm) { - return (struct wm_ref *)&wm->root->siblings; + return (struct wm_ref *)&wm->tree.root->siblings; } struct wm_ref *wm_ref_topmost_child(const struct wm_ref *cursor) { @@ -152,12 +159,16 @@ struct wm_ref *wm_find(const struct wm *wm, xcb_window_t id) { struct wm_ref *wm_find_by_client(const struct wm *wm, xcb_window_t client) { auto node = wm_tree_find(&wm->tree, client); - return node != NULL ? (struct wm_ref *)&wm_tree_find_toplevel_for(node)->siblings - : NULL; + if (node == NULL) { + return NULL; + } + auto toplevel = wm_tree_find_toplevel_for(&wm->tree, node); + return toplevel != NULL ? (struct wm_ref *)&toplevel->siblings : NULL; } -struct wm_ref *wm_ref_toplevel_of(struct wm_ref *cursor) { - return (struct wm_ref *)&wm_tree_find_toplevel_for(to_tree_node_mut(cursor))->siblings; +struct wm_ref *wm_ref_toplevel_of(const struct wm *wm, struct wm_ref *cursor) { + auto toplevel = wm_tree_find_toplevel_for(&wm->tree, to_tree_node_mut(cursor)); + return toplevel != NULL ? (struct wm_ref *)&toplevel->siblings : NULL; } struct wm_ref *wm_ref_client_of(struct wm_ref *cursor) { @@ -165,29 +176,44 @@ struct wm_ref *wm_ref_client_of(struct wm_ref *cursor) { return client != NULL ? (struct wm_ref *)&client->siblings : NULL; } -void wm_remove(struct wm *wm, struct wm_ref *w) { - wm_tree_destroy_window(&wm->tree, (struct wm_tree_node *)w); +struct wm_ref *wm_stack_end(struct wm *wm) { + return (struct wm_ref *)&wm->tree.root->children; } -struct wm_ref *wm_stack_end(struct wm *wm) { - return (struct wm_ref *)&wm->root->children; +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) { - wm_tree_move_to_above(&wm->tree, to_tree_node_mut(cursor), to_tree_node_mut(below)); + auto node = to_tree_node_mut(cursor); + if (node->parent == &wm->orphan_root) { + // If this window is orphaned, moving it around its siblings is + // meaningless. Same below. + return; + } + wm_tree_move_to_above(&wm->tree, node, to_tree_node_mut(below)); } void wm_stack_move_to_end(struct wm *wm, struct wm_ref *cursor, bool to_bottom) { - wm_tree_move_to_end(&wm->tree, to_tree_node_mut(cursor), to_bottom); + auto node = to_tree_node_mut(cursor); + if (node->parent == &wm->orphan_root) { + return; + } + wm_tree_move_to_end(&wm->tree, node, to_bottom); } struct wm *wm_new(void) { auto wm = ccalloc(1, struct wm); wm_tree_init(&wm->tree); - wm->incompletes = dynarr_new(struct wm_tree_node *, 4); - wm->masked = dynarr_new(struct wm_tree_node *, 8); + list_init_head(&wm->orphan_root.children); + wm->pending_query_trees = dynarr_new(struct wm_query_tree_request *, 0); return wm; } @@ -195,47 +221,72 @@ void wm_free(struct wm *wm) { // Free all `struct win`s associated with tree nodes, this leaves dangling // pointers, but we are freeing the tree nodes immediately after, so everything // is fine (TM). - wm_stack_foreach_safe(wm, i, next) { - auto w = wm_ref_deref(i); - auto tree_node = to_tree_node_mut(i); - free(w); - - if (tree_node->is_zombie) { - // This mainly happens on `session_destroy`, e.g. when there's - // ongoing animations. - log_debug("Leftover zombie node for window %#010x", tree_node->id.x); - wm_tree_reap_zombie(tree_node); + if (wm->tree.root != NULL) { + wm_stack_foreach_safe(wm, i, next) { + auto w = wm_ref_deref(i); + auto tree_node = to_tree_node_mut(i); + free(w); + + if (tree_node->is_zombie) { + // This mainly happens on `session_destroy`, e.g. when + // there's ongoing animations. + log_debug("Leftover zombie node for window %#010x", + tree_node->id.x); + wm_tree_reap_zombie(tree_node); + } } } wm_tree_clear(&wm->tree); - dynarr_free_pod(wm->incompletes); - dynarr_free_pod(wm->masked); + assert(wm_is_consistent(wm)); + assert(list_is_empty(&wm->orphan_root.children)); + dynarr_free_pod(wm->pending_query_trees); free(wm); } -void wm_destroy(struct wm *wm, xcb_window_t wid) { - auto masked = wm_find_masked(wm, wid); - if (masked != -1) { - free(wm->masked[masked]); - dynarr_remove_swap(wm->masked, (size_t)masked); - return; +/// 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); } +} +void wm_destroy(struct wm *wm, xcb_window_t wid) { struct wm_tree_node *node = wm_tree_find(&wm->tree, wid); if (!node) { - log_error("Trying to destroy window %#010x, but it's not in the tree. " - "Expect malfunction.", - wid); + if (wm_is_consistent(wm)) { + log_error("Window %#010x destroyed, but it's not in our tree.", wid); + } return; } - auto index = dynarr_find_pod(wm->incompletes, node); - if (index != -1) { - dynarr_remove_swap(wm->incompletes, (size_t)index); - } + log_debug("Destroying window %#010x", wid); - wm_tree_destroy_window(&wm->tree, node); + if (!list_is_empty(&node->children)) { + log_error("Window %#010x is destroyed but it still has children", wid); + } + wm_tree_detach(&wm->tree, node); + // 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); + } } void wm_reap_zombie(struct wm_ref *zombie) { @@ -245,185 +296,229 @@ void wm_reap_zombie(struct wm_ref *zombie) { void wm_reparent(struct wm *wm, 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); - // We delete the window here if parent is not found, or if the parent is - // an incomplete import. We will rediscover this window later in - // `wm_complete_import`. Keeping it around will only confuse us. - bool should_forget = - new_parent == NULL || dynarr_find_pod(wm->incompletes, new_parent) != -1; + + // 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) { - log_debug("Reparenting window %#010x which is not in our tree. Assuming " - "it came from fog of war.", - wid); - if (!should_forget) { - wm_import_incomplete(wm, wid, parent); - } - return; - } - if (should_forget) { - log_debug("Window %#010x reparented to window %#010x which is " - "%s, forgetting and masking instead.", - window->id.x, parent, - new_parent == NULL ? "not in our tree" : "an incomplete import"); - - wm_tree_detach(&wm->tree, window); - for (auto curr = window; curr != NULL;) { - auto next = wm_tree_next(curr, window); - HASH_DEL(wm->tree.nodes, curr); - auto incomplete_index = dynarr_find_pod(wm->incompletes, curr); - if (incomplete_index != -1) { - dynarr_remove_swap(wm->incompletes, (size_t)incomplete_index); - // Incomplete imports cannot have children. - assert(list_is_empty(&curr->children)); - - // Incomplete imports won't have event masks set, so we - // don't need to mask them. - free(curr); - } else { - dynarr_push(wm->masked, curr); - } - curr = next; + if (wm_is_consistent(wm)) { + log_error("Window %#010x reparented, but it's not in " + "our tree.", + wid); } return; } - if (new_parent == window->parent) { - log_debug("Reparenting window %#010x to its current parent %#010x, " - "moving it to the top.", - wid, parent); + if (window->parent == new_parent) { + // Reparent to the same parent moves the window to the top of the + // stack wm_tree_move_to_end(&wm->tree, window, false); return; } wm_tree_detach(&wm->tree, window); - wm_tree_attach(&wm->tree, window, new_parent); + + // 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 + // so will create an overlap. In other words, `window` will appear in the query + // tree reply too. Generally speaking, we want to keep a node's children list + // empty while there is a pending query tree request for it. (Imagine sending the + // query tree "locks" the children list until the reply is processed). Same logic + // 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.", + 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); + } } 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); } -void wm_import_incomplete(struct wm *wm, xcb_window_t wid, xcb_window_t parent) { - auto masked = wm_find_masked(wm, wid); - if (masked != -1) { - // A new window created with the same wid means the window we chose to - // forget has been deleted (without us knowing), and its ID was then - // reused. - free(wm->masked[masked]); - dynarr_remove_swap(wm->masked, (size_t)masked); - } - auto parent_cursor = NULL; - if (parent != XCB_NONE) { - parent_cursor = wm_tree_find(&wm->tree, parent); - if (parent_cursor == NULL) { - log_error("Importing window %#010x, but its parent %#010x is not " - "in our tree, ignoring. Expect malfunction.", - wid, parent); - return; - } - } - log_debug("Importing window %#010x with parent %#010x", wid, parent); - auto new = wm_tree_new_window(&wm->tree, wid); - wm_tree_add_window(&wm->tree, new); - wm_tree_attach(&wm->tree, new, parent_cursor); - dynarr_push(wm->incompletes, new); - if (parent == XCB_NONE) { - BUG_ON(wm->root != NULL); // Can't have more than one root - wm->root = new; - } -} static const xcb_event_mask_t WM_IMPORT_EV_MASK = XCB_EVENT_MASK_SUBSTRUCTURE_NOTIFY | XCB_EVENT_MASK_PROPERTY_CHANGE; -static void wm_complete_import_single(struct wm *wm, struct x_connection *c, - struct atom *atoms, struct wm_tree_node *node) { - log_debug("Finishing importing window %#010x with parent %#010lx.", node->id.x, - node->parent != NULL ? node->parent->id.x : XCB_NONE); - x_set_error_action_ignore( - c, xcb_change_window_attributes(c->c, node->id.x, XCB_CW_EVENT_MASK, - (const uint32_t[]){WM_IMPORT_EV_MASK})); - if (wid_has_prop(c->c, node->id.x, atoms->aWM_STATE)) { - wm_tree_set_wm_state(&wm->tree, node, true); + +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_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 masked = wm_find_masked(wm, node->id.x); - if (masked != -1) { - free(wm->masked[masked]); - dynarr_remove_swap(wm->masked, (size_t)masked); + if (reply_or_error == NULL) { + goto out; } -} -static bool wm_complete_import_subtree(struct wm *wm, struct x_connection *c, - struct atom *atoms, struct wm_tree_node *subroot) { - bool out_of_sync = false; - wm_complete_import_single(wm, c, atoms, subroot); + auto node = req->node; - for (auto curr = subroot; curr != NULL; curr = wm_tree_next(curr, subroot)) { - // Surprise! This newly imported window might already have children. - // Although we haven't setup SubstructureNotify for it yet, it's still - // possible for another window to be reparented to it. + 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)); + goto out; + } - xcb_query_tree_reply_t *tree = XCB_AWAIT(xcb_query_tree, c->c, curr->id.x); - if (!tree) { - log_error("Disappearing window subtree rooted at %#010x. We are " - "out-of-sync.", - curr->id.x); - out_of_sync = true; + 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 children = xcb_query_tree_children(reply); + log_debug("Window %#010x has %d children", node->id.x, + 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; } - auto children = xcb_query_tree_children(tree); - auto children_len = xcb_query_tree_children_length(tree); - for (int i = 0; i < children_len; i++) { - // `children` goes from bottom to top, and `wm_tree_new_window` - // puts the new window at the top of the stacking order, which - // means the windows will naturally be in the correct stacking - // order. - auto existing = wm_tree_find(&wm->tree, children[i]); - if (existing != NULL) { - // This should never happen: we haven't subscribed to - // child creation events yet, and any window reparented to - // an incomplete is deleted. report an error and try to - // recover. - auto index = dynarr_find_pod(wm->incompletes, existing); - if (index != -1) { - dynarr_remove_swap(wm->incompletes, (size_t)index); - } - log_error("Window %#010x already exists in the tree, but " - "it appeared again as a child of window " - "%#010x. Deleting the old one, but expect " - "malfunction.", - children[i], curr->id.x); - wm_tree_destroy_window(&wm->tree, existing); - } - existing = wm_tree_new_window(&wm->tree, children[i]); - wm_tree_add_window(&wm->tree, existing); - wm_tree_attach(&wm->tree, existing, curr); - wm_complete_import_single(wm, c, atoms, existing); - } - free(tree); + // If child node exists, it must be a previously orphaned node. + assert(child_node->parent == &wm->orphan_root); + wm_tree_detach(&wm->tree, child_node); + 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); + } +} + +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) { + auto req = (struct wm_get_property_request *)base; + if (reply_or_error == NULL) { + free(req); + return; + } + + // We guarantee that if a query tree request is pending, its corresponding + // window tree node won't be reaped. But we don't guarantee the same for + // get property requests. So we need to search the node by window ID again. + + if (reply_or_error->response_type == 0) { + // This is an error, most likely the window is gone when we tried + // to query it. (Note the tree node might have been freed at this + // point if the query tree request also failed earlier.) + xcb_generic_error_t *err = (xcb_generic_error_t *)reply_or_error; + log_debug("Get WM_STATE request for window %#010x failed with " + "error %s", + req->wid, x_strerror(err)); + free(req); + return; } - return !out_of_sync; + + 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; + wm_tree_set_wm_state(&req->wm->tree, node, reply->type != XCB_NONE); + free(req); } -bool wm_complete_import(struct wm *wm, struct x_connection *c, struct atom *atoms) { - while (!dynarr_is_empty(wm->incompletes)) { - auto i = dynarr_pop(wm->incompletes); - // This function modifies `wm->incompletes`, so we can't use - // `dynarr_foreach`. - if (!wm_complete_import_subtree(wm, c, atoms, i)) { - log_debug("Out-of-sync with the X server, try to resync."); - return false; +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})); + + // 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); } + + wm_tree_detach(&wm->tree, new); + // 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) { + return; + } + + { + 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); + } + + // (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); } +} - dynarr_foreach(wm->masked, m) { - free(*m); +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) { + // Parent node is currently being queried, we can't attach the new window + // to it as that will change its children list. + return; } - dynarr_clear_pod(wm->masked); - return true; + wm_import_start_no_flush(wm, c, atoms, wid, parent_node); + xcb_flush(c->c); // Actually send the requests } -bool wm_has_incomplete_imports(const struct wm *wm) { - return !dynarr_is_empty(wm->incompletes); +bool wm_is_consistent(const struct wm *wm) { + return dynarr_is_empty(wm->pending_query_trees); } bool wm_has_tree_changes(const struct wm *wm) { @@ -452,3 +547,11 @@ struct wm_change wm_dequeue_change(struct wm *wm) { } return ret; } + +struct wm_ref *wm_new_mock_window(struct wm *wm, xcb_window_t wid) { + auto node = wm_tree_new_window(&wm->tree, wid); + return (struct wm_ref *)&node->siblings; +} +void wm_free_mock_window(struct wm * /*wm*/, struct wm_ref *cursor) { + free(to_tree_node_mut(cursor)); +} diff --git a/src/wm/wm.h b/src/wm/wm.h index 7c8f58f651..7fa9391bb8 100644 --- a/src/wm/wm.h +++ b/src/wm/wm.h @@ -98,6 +98,9 @@ static inline bool wm_treeid_eq(wm_treeid a, wm_treeid b) { return a.gen == b.gen && a.x == b.x; } +/// Create a new window management object. +/// Caller is expected to first call `wm_import_start` with the root window after +/// creating the object. Otherwise many operations will fail or crash. struct wm *wm_new(void); void wm_free(struct wm *wm); @@ -115,12 +118,10 @@ void wm_set_active_leader(struct wm *wm, struct wm_ref *leader); /// Find a window in the hash table from window id. struct wm_ref *attr_pure wm_find(const struct wm *wm, xcb_window_t id); -/// Remove a window from the hash table. -void wm_remove(struct wm *wm, struct wm_ref *w); // Find the WM frame of a client window. `id` is the client window id. struct wm_ref *attr_pure wm_find_by_client(const struct wm *wm, xcb_window_t client); /// Find the toplevel of a window by going up the window tree. -struct wm_ref *attr_pure wm_ref_toplevel_of(struct wm_ref *cursor); +struct wm_ref *attr_pure wm_ref_toplevel_of(const struct wm *wm, struct wm_ref *cursor); /// Return the client window of a window. Must be called with a cursor to a toplevel. /// Returns NULL if there is no client window. struct wm_ref *attr_pure wm_ref_client_of(struct wm_ref *cursor); @@ -154,59 +155,57 @@ void wm_reap_zombie(struct wm_ref *zombie); void wm_reparent(struct wm *wm, 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); -/// Create a tree node for `wid`, with `parent` as its parent. The parent must already -/// be in the window tree. This function creates a placeholder tree node, without -/// contacting the X server, thus can be called outside of the X critical section. The -/// expectation is that the caller will later call `wm_complete_import` inside the -/// X critical section to complete the import. +/// Start the import process for `wid`. /// -/// ## NOTE +/// This function sets up event masks for `wid`, and start an async query tree request on +/// it. When the query tree request is completed, `wm_handle_query_tree_reply` will be +/// called to actually insert the window into the window tree. +/// `wm_handle_query_tree_reply` will also in turn start the import process for all +/// children of `wid`. /// -/// The reason for this complicated dance is because we want to catch all windows ever -/// created on X server's side. For a newly created windows, we will setup a -/// SubstructureNotify event mask to catch any new windows created under it. But between -/// the time we received the creation event and the time we setup the event mask, if any -/// windows were created under the new window, we will miss them. Therefore we have to -/// scan the new windows in X critical section so they won't change as we scan. +/// The reason for this two step process is because we want to catch all windows ever +/// created on X server's side. It's not enough to just set up event masks and wait for +/// events. Because at the point in time we set up the event mask, some child windows +/// could have already been created. It would have been nice if there is a way to listen +/// for events on the whole window tree, for all present and future windows. But alas, the +/// X11 protocol is lacking in this area. /// -/// On the other hand, we can't push everything to the X critical section, because -/// updating the window stack requires knowledge of all windows in the stack. Think -/// ConfigureNotify, if we don't know about the `above_sibling` window, we don't know -/// where to put the window. So we need to create an incomplete import first. +/// The best thing we can do is set up the event mask to catch all future events, and then +/// query the current state. But there are complications with this approach, too. Because +/// there is no way to atomically do these two things in one go, things could happen +/// between these two steps, for which we will receive events. Some of these are easy to +/// deal with, e.g. if a window is created, we will get an event for that, and later we +/// will see that window again in the query tree reply. These are easy to ignore. Some are +/// more complex. Because there could be some child windows we are not aware of. We could +/// get events for windows that we don't know about. We try our best to ignore those +/// events. /// -/// But wait, this is actually way more involved. Because we choose to not set up event -/// masks for incomplete imports (we can also choose otherwise, but that also has its own -/// set of problems), there is a whole subtree of windows we don't know about. And those -/// windows might involve in reparent events. To handle that, we essentially put "fog of -/// war" under any incomplete imports, anything reparented into the fog is lost, and will -/// be rediscovered later during subtree scans. If a window is reparented out of the fog, -/// then it's treated as if a brand new window was created. -/// -/// But wait again, there's more. We can delete "lost" windows on our side and unset event -/// masks, but again because this is racy, we might still receive some events for those -/// windows. So we have to keep a list of "lost" windows, and correctly ignore events sent -/// for them. (And since we are actively ignoring events from these windows, we might as -/// well not unset their event masks, saves us a trip to the X server). +/// Another problem with this is, the usual way we send a request then process the reply +/// is synchronous. i.e. with `xcb__reply(xcb_(...))`. As previously +/// mentioned, we might receive events before the reply. And in this case we would process +/// the reply _before_ any of those events! This might be benign, but it is difficult to +/// audit all the possible cases and events to make sure this always work. One example is, +/// imagine a window A is being imported, and another window B, which is already in the +/// tree got reparented to A. We would think B appeared in two places if we processed +/// query tree reply before the reparent event. For that, we introduce the concept of +/// "async requests". Replies to these requests are received and processed like other X +/// events. With that, we don't need to worry about the order of events and replies. /// /// (Now you have a glimpse of how much X11 sucks.) -void wm_import_incomplete(struct wm *wm, xcb_window_t wid, xcb_window_t parent); - -/// Check if there are any incomplete imports in the window tree. -bool wm_has_incomplete_imports(const struct wm *wm); +void wm_import_start(struct wm *wm, struct x_connection *c, struct atom *atoms, + xcb_window_t wid, struct wm_ref *parent); /// Check if there are tree change events bool wm_has_tree_changes(const struct wm *wm); -/// Complete the previous incomplete imports by querying the X server. This function will -/// recursively import all children of previously created placeholders, and add them to -/// the window tree. This function must be called from the X critical section. This -/// function also subscribes to SubstructureNotify events for all newly imported windows, -/// as well as for the (now completed) placeholder windows. -/// -/// Returns false if our internal state is behind the X server's. In other words, if a -/// window we think should be there, does not in fact exist. Returns true otherwise. -bool wm_complete_import(struct wm *wm, struct x_connection *c, struct atom *atoms); +struct wm_change wm_dequeue_change(struct wm *wm); -bool wm_is_wid_masked(struct wm *wm, xcb_window_t wid); +/// Whether the window tree should be in a consistent state. When the window tree is +/// consistent, we should not be receiving X events that refer to windows that we don't +/// know about. And we also should not have any orphaned windows in the tree. +bool wm_is_consistent(const struct wm *wm); -struct wm_change wm_dequeue_change(struct wm *wm); +// Unit testing helpers + +struct wm_ref *wm_new_mock_window(struct wm *wm, xcb_window_t wid); +void wm_free_mock_window(struct wm *wm, struct wm_ref *cursor); diff --git a/src/wm/wm_internal.h b/src/wm/wm_internal.h index e155e39031..d70db89aca 100644 --- a/src/wm/wm_internal.h +++ b/src/wm/wm_internal.h @@ -1,6 +1,5 @@ #pragma once -#include #include #include @@ -78,7 +77,10 @@ struct wm_tree_change { /// shutting down. void wm_tree_clear(struct wm_tree *tree); struct wm_tree_node *attr_pure wm_tree_find(const struct wm_tree *tree, xcb_window_t id); -struct wm_tree_node *attr_pure wm_tree_find_toplevel_for(struct wm_tree_node *node); +/// Find the toplevel that is an ancestor of `node` or `node` itself. Returns NULL if +/// `node` is part of an orphaned subtree. +struct wm_tree_node *attr_pure wm_tree_find_toplevel_for(const struct wm_tree *tree, + struct wm_tree_node *node); struct wm_tree_node *attr_pure wm_tree_next(struct wm_tree_node *node, struct wm_tree_node *subroot); /// Create a new window node in the tree, with X window ID `id`, and parent `parent`. If @@ -93,7 +95,8 @@ void wm_tree_destroy_window(struct wm_tree *tree, struct wm_tree_node *node); /// its parent, and the disconnected tree nodes won't be able to be found via /// `wm_tree_find`. Relevant events will be generated. void wm_tree_detach(struct wm_tree *tree, struct wm_tree_node *subroot); -/// Attach `node` to `parent`. `node` becomes the topmost child of `parent`. +/// Attach `node` to `parent`. `node` becomes the topmost child of `parent`. If `parent` +/// is NULL, `node` becomes the root window. void wm_tree_attach(struct wm_tree *tree, struct wm_tree_node *child, struct wm_tree_node *parent); void wm_tree_move_to_above(struct wm_tree *tree, struct wm_tree_node *node, From 15ed5bd79ff2ca7966500344ee8ee4d50f56ea1f Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Wed, 26 Jun 2024 15:24:43 +0100 Subject: [PATCH 6/7] misc: fix comment formatting in handle_pending_updates Signed-off-by: Yuxuan Shui --- src/picom.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/picom.c b/src/picom.c index 843972f61e..c05d2d7dca 100644 --- a/src/picom.c +++ b/src/picom.c @@ -1666,15 +1666,15 @@ static void handle_pending_updates(struct session *ps, double delta_t) { ps->server_grabbed = true; // Catching up with X server - /// Handle all events X server has sent us so far, so that our internal state will - /// catch up with the X server's state. It only makes sense to call this function - /// in the X critical section, otherwise we will be chasing a moving goal post. - /// - /// (Disappointingly, grabbing the X server doesn't actually prevent the X server - /// state from changing. It should, but in practice we have observed window still - /// being destroyed while we have the server grabbed. This is very disappointing - /// and forces us to use some hacky code to recover when we discover we are - /// out-of-sync.) + // Handle all events X server has sent us so far, so that our internal state will + // catch up with the X server's state. It only makes sense to call this function + // in the X critical section, otherwise we will be chasing a moving goal post. + // + // (Disappointingly, grabbing the X server doesn't actually prevent the X server + // state from changing. It should, but in practice we have observed window still + // being destroyed while we have the server grabbed. This is very disappointing + // and forces us to use some hacky code to recover when we discover we are + // out-of-sync.) handle_x_events(ps); if (ps->pending_updates || wm_has_tree_changes(ps->wm)) { log_debug("Delayed handling of events, entering critical section"); From f126d47539a649fef2bc992f0e3307cef3fbae95 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Wed, 26 Jun 2024 22:30:29 +0100 Subject: [PATCH 7/7] wm/tree: fix uninitialized tree->gen in unit test Signed-off-by: Yuxuan Shui --- src/wm/wm_internal.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/wm/wm_internal.h b/src/wm/wm_internal.h index d70db89aca..f9a8c9ffb7 100644 --- a/src/wm/wm_internal.h +++ b/src/wm/wm_internal.h @@ -109,6 +109,7 @@ void wm_tree_set_wm_state(struct wm_tree *tree, struct wm_tree_node *node, bool static inline void wm_tree_init(struct wm_tree *tree) { tree->nodes = NULL; + tree->gen = 1; list_init_head(&tree->changes); list_init_head(&tree->free_changes); }