Merge branch 'yshui:next' into next

CHANGELOG.md

@@ -1,8 +1,10 @@
-# Unreleased
+# v12-rc4 (2024-Sep-08)
 
 ## Bug fixes
 
-* Windows become completely black when `rules` and `inactive-dim` are set at the same time.
+* Windows become completely black when `rules` and `inactive-dim` are set at the same time
+* Fix segmentation fault during unredirection if the geometry change animation is used (#1333, thanks to @monsterovich)
+* Fix many rare race conditions in the window management code (#1334)
 
 # v12-rc3 (2024-Aug-30)
 

src/atom.c

@@ -6,7 +6,6 @@
 #include <xcb/xcb.h>
 
 #include "atom.h"
-#include "common.h"
 #include "compiler.h"
 #include "log.h"
 #include "utils/cache.h"

src/event.c

@@ -201,7 +201,7 @@ struct ev_ewmh_active_win_request {
 /// returned could not be found.
 static void
 update_ewmh_active_win(struct x_connection * /*c*/, struct x_async_request_base *req_base,
-                       xcb_raw_generic_event_t *reply_or_error) {
+                       const xcb_raw_generic_event_t *reply_or_error) {
 	auto ps = ((struct ev_ewmh_active_win_request *)req_base)->ps;
 	free(req_base);
 
@@ -219,7 +219,7 @@ update_ewmh_active_win(struct x_connection * /*c*/, struct x_async_request_base
 	}
 
 	// Search for the window
-	auto reply = (xcb_get_property_reply_t *)reply_or_error;
+	auto reply = (const xcb_get_property_reply_t *)reply_or_error;
 	if (reply->type == XCB_NONE || xcb_get_property_value_length(reply) < 4) {
 		log_debug("EWMH _NET_ACTIVE_WINDOW not set.");
 		return;
@@ -248,7 +248,7 @@ struct ev_recheck_focus_request {
  * @return struct _win of currently focused window, NULL if not found
  */
 static void recheck_focus(struct x_connection * /*c*/, struct x_async_request_base *req_base,
-                          xcb_raw_generic_event_t *reply_or_error) {
+                          const xcb_raw_generic_event_t *reply_or_error) {
 	auto ps = ((struct ev_ewmh_active_win_request *)req_base)->ps;
 	free(req_base);
 
@@ -268,7 +268,7 @@ static void recheck_focus(struct x_connection * /*c*/, struct x_async_request_ba
 		return;
 	}
 
-	auto reply = (xcb_get_input_focus_reply_t *)reply_or_error;
+	auto reply = (const xcb_get_input_focus_reply_t *)reply_or_error;
 	xcb_window_t wid = reply->focus;
 	log_debug("Current focused window is %#010x", wid);
 	if (wid == XCB_NONE || wid == XCB_INPUT_FOCUS_POINTER_ROOT ||
@@ -399,6 +399,10 @@ static inline void ev_configure_notify(session_t *ps, xcb_configure_notify_event
 		return;
 	}
 
+	if (ev->window == ev->event) {
+		return;
+	}
+
 	if (ev->window == ps->c.screen_info->root) {
 		configure_root(ps);
 	} else {
@@ -408,10 +412,20 @@ static inline void ev_configure_notify(session_t *ps, xcb_configure_notify_event
 
 static inline void ev_destroy_notify(session_t *ps, xcb_destroy_notify_event_t *ev) {
 	log_debug("{ event: %#010x, id: %#010x }", ev->event, ev->window);
-	wm_destroy(ps->wm, ev->window);
+	// If we hit an ABA problem, it is possible to get a DestroyNotify event from a
+	// parent for its child, but not from the child for itself.
+	if (ev->event != ev->window) {
+		wm_disconnect(ps->wm, ev->window, ev->event, XCB_NONE);
+	} else {
+		wm_destroy(ps->wm, ev->window);
+	}
 }
 
 static inline void ev_map_notify(session_t *ps, xcb_map_notify_event_t *ev) {
+	if (ev->window == ev->event) {
+		return;
+	}
+
 	// Unmap overlay window if it got mapped but we are currently not
 	// in redirected state.
 	if (ps->overlay && ev->window == ps->overlay) {
@@ -461,6 +475,10 @@ static inline void ev_unmap_notify(session_t *ps, xcb_unmap_notify_event_t *ev)
 		return;
 	}
 
+	if (ev->event == ev->window) {
+		return;
+	}
+
 	auto cursor = wm_find(ps->wm, ev->window);
 	if (cursor == NULL) {
 		if (wm_is_consistent(ps->wm)) {
@@ -479,10 +497,21 @@ static inline void ev_reparent_notify(session_t *ps, xcb_reparent_notify_event_t
 	log_debug("Window %#010x has new parent: %#010x, override_redirect: %d, "
 	          "send_event: %#010x",
 	          ev->window, ev->parent, ev->override_redirect, ev->event);
-	wm_reparent(ps->wm, ev->window, ev->parent);
+	if (ev->event == ev->window) {
+		return;
+	}
+	if (ev->parent != ev->event) {
+		wm_disconnect(ps->wm, ev->window, ev->event, ev->parent);
+	} else {
+		wm_reparent(ps->wm, &ps->c, ps->atoms, ev->window, ev->parent);
+	}
 }
 
 static inline void ev_circulate_notify(session_t *ps, xcb_circulate_notify_event_t *ev) {
+	if (ev->event == ev->window) {
+		return;
+	}
+
 	auto cursor = wm_find(ps->wm, ev->window);
 
 	if (cursor == NULL) {

src/picom.c

@@ -1493,16 +1493,16 @@ static void handle_x_events_ev(EV_P attr_unused, ev_prepare *w, int revents attr
 struct new_window_attributes_request {
 	struct x_async_request_base base;
 	struct session *ps;
-	xcb_window_t wid;
+	wm_treeid id;
 };
 
 static void handle_new_window_attributes_reply(struct x_connection * /*c*/,
                                                struct x_async_request_base *req_base,
-                                               xcb_raw_generic_event_t *reply_or_error) {
+                                               const xcb_raw_generic_event_t *reply_or_error) {
 	auto req = (struct new_window_attributes_request *)req_base;
 	auto ps = req->ps;
-	auto wid = req->wid;
-	auto new_window = wm_find(ps->wm, wid);
+	auto id = req->id;
+	auto new_window = wm_find(ps->wm, id.x);
 	free(req);
 
 	if (reply_or_error == NULL) {
@@ -1513,7 +1513,7 @@ static void handle_new_window_attributes_reply(struct x_connection * /*c*/,
 	if (reply_or_error->response_type == 0) {
 		log_debug("Failed to get window attributes for newly created window "
 		          "%#010x",
-		          wid);
+		          id.x);
 		return;
 	}
 
@@ -1522,17 +1522,26 @@ static void handle_new_window_attributes_reply(struct x_connection * /*c*/,
 		// created with the same window ID before this request completed, and the
 		// latter window isn't in our tree yet.
 		if (wm_is_consistent(ps->wm)) {
-			log_error("Newly created window %#010x is not in the window tree", wid);
+			log_error("Newly created window %#010x is not in the window tree",
+			          id.x);
 			assert(false);
 		}
 		return;
 	}
 
+	auto current_id = wm_ref_treeid(new_window);
+	if (!wm_treeid_eq(current_id, id)) {
+		log_debug("Window %#010x was not the window we queried anymore. Current "
+		          "gen %" PRIu64 ", expected %" PRIu64,
+		          id.x, current_id.gen, id.gen);
+		return;
+	}
+
 	auto toplevel = wm_ref_toplevel_of(ps->wm, new_window);
 	if (toplevel != new_window) {
 		log_debug("Newly created window %#010x was moved away from toplevel "
 		          "while we were waiting for its attributes",
-		          wid);
+		          id.x);
 		return;
 	}
 	if (wm_ref_deref(toplevel) != NULL) {
@@ -1543,12 +1552,12 @@ static void handle_new_window_attributes_reply(struct x_connection * /*c*/,
 		// toplevel. But there is another get attributes request sent the
 		// second time it became a toplevel. When we get the reply for the second
 		// request, we will reach here.
-		log_debug("Newly created window %#010x is already managed", wid);
+		log_debug("Newly created window %#010x is already managed", id.x);
 		return;
 	}
 
-	auto w = win_maybe_allocate(ps, toplevel,
-	                            (xcb_get_window_attributes_reply_t *)reply_or_error);
+	auto w = win_maybe_allocate(
+	    ps, toplevel, (const xcb_get_window_attributes_reply_t *)reply_or_error);
 	if (w != NULL && w->a.map_state == XCB_MAP_STATE_VIEWABLE) {
 		win_set_flags(w, WIN_FLAGS_MAPPED);
 		ps->pending_updates = true;
@@ -1571,11 +1580,11 @@ static void handle_new_windows(session_t *ps) {
 			// number of things could happen before we get the reply. The
 			// window can be reparented, destroyed, then get its window ID
 			// reused, etc.
-			req->wid = wm_ref_win_id(wm_change.toplevel);
+			req->id = wm_ref_treeid(wm_change.toplevel);
 			req->ps = ps;
 			req->base.callback = handle_new_window_attributes_reply,
 			req->base.sequence =
-			    xcb_get_window_attributes(ps->c.c, req->wid).sequence;
+			    xcb_get_window_attributes(ps->c.c, req->id.x).sequence;
 			x_await_request(&ps->c, &req->base);
 			break;
 		case WM_TREE_CHANGE_TOPLEVEL_KILLED:

src/region.h

@@ -100,7 +100,7 @@ static inline void _resize_region(const region_t *region, region_t *output, int
 	int nrects;
 	int nnewrects = 0;
 	const rect_t *rects = pixman_region32_rectangles((region_t *)region, &nrects);
-	auto newrects = ccalloc(nrects, rect_t);
+	rect_t *newrects = calloc((size_t)nrects, sizeof(rect_t));
 	for (int i = 0; i < nrects; i++) {
 		int x1 = rects[i].x1 - dx;
 		int y1 = rects[i].y1 - dy;

src/wm/tree.c

@@ -196,7 +196,7 @@ struct wm_tree_node *wm_tree_next(struct wm_tree_node *node, struct wm_tree_node
 
 /// Find a client window under a toplevel window. If there are multiple windows with
 /// `WM_STATE` set under the toplevel window, we will return an arbitrary one.
-static struct wm_tree_node *attr_pure wm_tree_find_client(struct wm_tree_node *subroot) {
+struct wm_tree_node *attr_pure wm_tree_find_client(struct wm_tree_node *subroot) {
 	if (subroot->has_wm_state) {
 		log_debug("Toplevel %#010x has WM_STATE set, weird. Using itself as its "
 		          "client window.",
@@ -250,14 +250,13 @@ void wm_tree_set_wm_state(struct wm_tree *tree, struct wm_tree_node *node, bool
 	node->has_wm_state = has_wm_state;
 	BUG_ON(node->parent == NULL);        // Trying to set WM_STATE on the root window
 
-	struct wm_tree_node *toplevel;
-	for (auto cur = node; cur->parent != NULL; cur = cur->parent) {
-		toplevel = cur;
+	struct wm_tree_node *toplevel = wm_tree_find_toplevel_for(tree, node);
+	if (toplevel == NULL) {
+		return;
 	}
 
 	if (toplevel == node) {
 		log_debug("Setting WM_STATE on a toplevel window %#010x, weird.", node->id.x);
-		return;
 	}
 
 	if (!has_wm_state && toplevel->client_window == node) {
@@ -284,6 +283,9 @@ struct wm_tree_node *wm_tree_new_window(struct wm_tree *tree, xcb_window_t id) {
 	node->id.x = id;
 	node->id.gen = tree->gen++;
 	node->has_wm_state = false;
+	node->receiving_events = false;
+	node->is_zombie = false;
+	node->visited = false;
 	node->leader = id;
 	list_init_head(&node->children);
 	return node;
@@ -307,6 +309,9 @@ wm_tree_refresh_client_and_queue_change(struct wm_tree *tree, struct wm_tree_nod
 		if (new_client != NULL) {
 			new_client_id = new_client->id;
 		}
+		log_debug("Toplevel window %#010x had client window %#010x, now has "
+		          "%#010x.",
+		          toplevel->id.x, old_client_id.x, new_client_id.x);
 		toplevel->client_window = new_client;
 		wm_tree_enqueue_client_change(tree, toplevel, old_client_id, new_client_id);
 	}
@@ -325,6 +330,7 @@ struct wm_tree_node *wm_tree_detach(struct wm_tree *tree, struct wm_tree_node *s
 		}
 	} else {
 		// Detached a toplevel, create a zombie for it
+		log_debug("Detaching toplevel window %#010x.", subroot->id.x);
 		zombie = ccalloc(1, struct wm_tree_node);
 		zombie->parent = subroot->parent;
 		zombie->id = subroot->id;
@@ -334,6 +340,12 @@ struct wm_tree_node *wm_tree_detach(struct wm_tree *tree, struct wm_tree_node *s
 		if (wm_tree_enqueue_toplevel_killed(tree, subroot->id, zombie)) {
 			zombie = NULL;
 		}
+
+		// Gen bump must happen after enqueuing the change, because otherwise the
+		// kill change won't cancel out a previous new change because the IDs will
+		// be different.
+		subroot->id.gen = tree->gen++;
+		subroot->client_window = NULL;
 	}
 	subroot->parent = NULL;
 	return zombie;
@@ -356,7 +368,8 @@ void wm_tree_attach(struct wm_tree *tree, struct wm_tree_node *child,
 	auto toplevel = wm_tree_find_toplevel_for(tree, child);
 	if (child == toplevel) {
 		wm_tree_enqueue_toplevel_new(tree, child);
-	} else if (toplevel != NULL) {
+	}
+	if (toplevel != NULL) {
 		wm_tree_refresh_client_and_queue_change(tree, toplevel);
 	}
 }

src/wm/win.c

@@ -235,6 +235,13 @@ static inline void win_release_mask(backend_t *base, struct win *w) {
 	}
 }
 
+static inline void win_release_saved_win_image(backend_t *base, struct win *w) {
+	if (w->saved_win_image) {
+		base->ops.release_image(base, w->saved_win_image);
+		w->saved_win_image = NULL;
+	}
+}
+
 void win_release_images(struct backend_base *backend, struct win *w) {
 	// We don't want to decide what we should do if the image we want to
 	// release is stale (do we clear the stale flags or not?) But if we are
@@ -244,6 +251,7 @@ void win_release_images(struct backend_base *backend, struct win *w) {
 	win_release_pixmap(backend, w);
 	win_release_shadow(backend, w);
 	win_release_mask(backend, w);
+	win_release_saved_win_image(backend, w);
 }
 
 /// Returns true if the `prop` property is stale, as well as clears the stale
@@ -1277,7 +1285,7 @@ void free_win_res(session_t *ps, struct win *w) {
 /// Query the Xorg for information about window `win`, and assign a window to `cursor` if
 /// this window should be managed.
 struct win *win_maybe_allocate(session_t *ps, struct wm_ref *cursor,
-                               xcb_get_window_attributes_reply_t *attrs) {
+                               const xcb_get_window_attributes_reply_t *attrs) {
 	static const struct win win_def = {
 	    .frame_opacity = 1.0,
 	    .frame_opacity_for_same_colors = false,
@@ -1365,8 +1373,9 @@ struct win *win_maybe_allocate(session_t *ps, struct wm_ref *cursor,
 	}
 
 	// Set window event mask
-	uint32_t frame_event_mask =
-	    XCB_EVENT_MASK_PROPERTY_CHANGE | XCB_EVENT_MASK_SUBSTRUCTURE_NOTIFY;
+	uint32_t frame_event_mask = XCB_EVENT_MASK_PROPERTY_CHANGE |
+	                            XCB_EVENT_MASK_SUBSTRUCTURE_NOTIFY |
+	                            XCB_EVENT_MASK_STRUCTURE_NOTIFY;
 	if (!ps->o.use_ewmh_active_win) {
 		frame_event_mask |= XCB_EVENT_MASK_FOCUS_CHANGE;
 	}
@@ -2104,7 +2113,7 @@ struct win_get_geometry_request {
 
 static void win_handle_get_geometry_reply(struct x_connection * /*c*/,
                                           struct x_async_request_base *req_base,
-                                          xcb_raw_generic_event_t *reply_or_error) {
+                                          const xcb_raw_generic_event_t *reply_or_error) {
 	auto req = (struct win_get_geometry_request *)req_base;
 	auto wid = req->wid;
 	auto ps = req->ps;
@@ -2139,7 +2148,7 @@ static void win_handle_get_geometry_reply(struct x_connection * /*c*/,
 		return;
 	}
 
-	auto r = (xcb_get_geometry_reply_t *)reply_or_error;
+	auto r = (const xcb_get_geometry_reply_t *)reply_or_error;
 	ps->pending_updates |= win_set_pending_geometry(w, win_geometry_from_get_geometry(r));
 }
 

src/wm/win.h

@@ -477,7 +477,7 @@ region_t win_get_region_frame_local_by_val(const struct win *w);
 /// Query the Xorg for information about window `win`
 /// `win` pointer might become invalid after this function returns
 struct win *win_maybe_allocate(session_t *ps, struct wm_ref *cursor,
-                               xcb_get_window_attributes_reply_t *attrs);
+                               const xcb_get_window_attributes_reply_t *attrs);
 
 /**
  * Release a destroyed window that is no longer needed.