Merge pull request #1281 from yshui/eventually-consistent-tree

Eventually consistent tree

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);
 }
 

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,14 +219,19 @@ 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;
 	}
 
 	if (below == NULL && ce->above_sibling != XCB_NONE) {
 		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,25 +458,30 @@ 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),
 		          ev->state == XCB_PROPERTY_DELETE ? "deleted" : "set");
 		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.
@@ -629,10 +641,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 +710,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);

src/event.h

@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: MPL-2.0
 // Copyright (c) 2019, Yuxuan Shui <[email protected]>
 
+#pragma once
 #include <xcb/xcb.h>
 
 #include "common.h"

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);

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);
@@ -1558,8 +1562,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);
@@ -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;
@@ -1666,18 +1666,17 @@ 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_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 {
@@ -1978,7 +1977,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);
@@ -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);
 }
 
 /**