Fix freezes

src/event.c

@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: MPL-2.0
 // Copyright (c) 2019, Yuxuan Shui <[email protected]>
 
+#include <stdint.h>
 #include <stdio.h>
 
 #include <X11/Xlibint.h>
@@ -47,8 +48,14 @@
 /// When top half finished, we enter the render stage, where no server state should be
 /// queried. All rendering should be done with our internal knowledge of the server state.
 ///
+/// P.S. There is another reason to avoid sending any request to the server as much as
+/// possible. To make sure requests are sent, flushes are needed. And `xcb_flush`/`XFlush`
+/// functions may read more events from the server into their queues. This is
+/// undesirable, see the comments on `handle_queued_x_events` in picom.c for more details.
 
-// TODO(yshui) the things described above
+// TODO(yshui) the things described above. This is mostly done, maybe some of
+//             the functions here is still making unnecessary queries, we need
+//             to do some auditing to be sure.
 
 /**
  * Get a window's name from window ID.
@@ -350,19 +357,14 @@
 		}
 
 		// Reset event mask in case something wrong happens
-		xcb_change_window_attributes(
-		    ps->c.c, ev->window, XCB_CW_EVENT_MASK,
-		    (const uint32_t[]){determine_evmask(ps, ev->window, WIN_EVMODE_UNKNOWN)});
+		uint32_t evmask = determine_evmask(ps, ev->window, WIN_EVMODE_UNKNOWN);
 
 		if (!wid_has_prop(ps, ev->window, ps->atoms->aWM_STATE)) {
 			log_debug("Window %#010x doesn't have WM_STATE property, it is "
 			          "probably not a client window. But we will listen for "
 			          "property change in case it gains one.",
 			          ev->window);
-			xcb_change_window_attributes(
-			    ps->c.c, ev->window, XCB_CW_EVENT_MASK,
-			    (const uint32_t[]){determine_evmask(ps, ev->window, WIN_EVMODE_UNKNOWN) |
-			                       XCB_EVENT_MASK_PROPERTY_CHANGE});
+			evmask |= XCB_EVENT_MASK_PROPERTY_CHANGE;
 		} else {
 			auto w_real_top = find_managed_window_or_parent(ps, ev->parent);
 			if (w_real_top && w_real_top->state != WSTATE_UNMAPPED &&
@@ -386,6 +388,8 @@
 				}
 			}
 		}
+		XCB_AWAIT_VOID(xcb_change_window_attributes, ps->c.c, ev->window,
+		               XCB_CW_EVENT_MASK, (const uint32_t[]){evmask});
 	}
 }
 
@@ -475,9 +479,10 @@
 		// Check whether it could be a client window
 		if (!find_toplevel(ps, ev->window)) {
 			// Reset event mask anyway
-			xcb_change_window_attributes(ps->c.c, ev->window, XCB_CW_EVENT_MASK,
-			                             (const uint32_t[]){determine_evmask(
-			                                 ps, ev->window, WIN_EVMODE_UNKNOWN)});
+			const uint32_t evmask =
+			    determine_evmask(ps, ev->window, WIN_EVMODE_UNKNOWN);
+			XCB_AWAIT_VOID(xcb_change_window_attributes, ps->c.c, ev->window,
+			               XCB_CW_EVENT_MASK, (const uint32_t[]){evmask});
 
 			auto w_top = find_managed_window_or_parent(ps, ev->window);
 			// ev->window might have not been managed yet, in that case w_top
@@ -492,8 +497,8 @@
 	// If _NET_WM_WINDOW_TYPE changes... God knows why this would happen, but
 	// there are always some stupid applications. (#144)
 	if (ev->atom == ps->atoms->a_NET_WM_WINDOW_TYPE) {
-		struct managed_win *w = NULL;
-		if ((w = find_toplevel(ps, ev->window))) {
+		struct managed_win *w = find_toplevel(ps, ev->window);
+		if (w) {
 			win_set_property_stale(w, ev->atom);
 		}
 	}
@@ -586,16 +591,28 @@
 	region_t parts;
 	pixman_region32_init(&parts);
 
+	// If this is the first time this window is damaged, we would redraw the
+	// whole window, so we don't need to fetch the damage region. But we still need
+	// to make sure the X server receives the DamageSubtract request, hence the
+	// `xcb_request_check` here.
+	// Otherwise, we fetch the damage regions. That means we will receive a reply
+	// from the X server, which implies it has received our DamageSubtract request.
 	if (!w->ever_damaged) {
-		win_extents(w, &parts);
-		if (!ps->o.show_all_xerrors) {
-			set_ignore_cookie(&ps->c, xcb_damage_subtract(ps->c.c, w->damage,
-			                                              XCB_NONE, XCB_NONE));
+		auto e = xcb_request_check(
+		    ps->c.c, xcb_damage_subtract(ps->c.c, w->damage, XCB_NONE, XCB_NONE));
+		if (e) {
+			if (ps->o.show_all_xerrors) {
+				x_print_error(e->sequence, e->major_code, e->minor_code,
+				              e->error_code);
+			}
+			free(e);
 		}
+		win_extents(w, &parts);
 	} else {
+		auto cookie =
+		    xcb_damage_subtract(ps->c.c, w->damage, XCB_NONE, ps->damaged_region);
 		if (!ps->o.show_all_xerrors) {
-			set_ignore_cookie(&ps->c, xcb_damage_subtract(ps->c.c, w->damage, XCB_NONE,
-			                                              ps->damaged_region));
+			set_ignore_cookie(&ps->c, cookie);
 		}
 		x_fetch_region(&ps->c, ps->damaged_region, &parts);
 		pixman_region32_translate(&parts, w->g.x + w->g.border_width,

src/picom.c

@@ -1607,9 +1607,32 @@ static void unredirect(session_t *ps) {
 	log_debug("Screen unredirected.");
 }
 
-// Handle queued events before we go to sleep
+/// Handle queued events before we go to sleep.
+///
+/// This function is called by ev_prepare watcher, which is called just before
+/// the event loop goes to sleep. X damage events are incremental, which means
+/// if we don't handle the ones X server already sent us, we won't get new ones.
+/// And if we don't get new ones, we won't render, i.e. we would freeze. libxcb
+/// keeps an internal queue of events, so we have to be 100% sure no events are
+/// left in that queue before we go to sleep.
 static void handle_queued_x_events(EV_P attr_unused, ev_prepare *w, int revents attr_unused) {
 	session_t *ps = session_ptr(w, event_check);
+	// Flush because if we go into sleep when there is still requests in the
+	// outgoing buffer, they will not be sent for an indefinite amount of
+	// time. Use XFlush here too, we might still use some Xlib functions
+	// because OpenGL.
+	//
+	// Also note, after we have flushed here, we won't flush again in this
+	// function before going into sleep. This is because `xcb_flush`/`XFlush`
+	// may _read_ more events from the server (yes, this is ridiculous, I
+	// know). And we can't have that, see the comments above this function.
+	//
+	// This means if functions called ev_handle need to send some events,
+	// they need to carefully make sure those events are flushed, one way or
+	// another.
+	XFlush(ps->c.dpy);
+	xcb_flush(ps->c.c);
+
 	if (ps->vblank_scheduler) {
 		vblank_handle_x_events(ps->vblank_scheduler);
 	}
@@ -1619,13 +1642,6 @@ static void handle_queued_x_events(EV_P attr_unused, ev_prepare *w, int revents
 		ev_handle(ps, ev);
 		free(ev);
 	};
-	// Flush because if we go into sleep when there is still
-	// requests in the outgoing buffer, they will not be sent
-	// for an indefinite amount of time.
-	// Use XFlush here too, we might still use some Xlib functions
-	// because OpenGL.
-	XFlush(ps->c.dpy);
-	xcb_flush(ps->c.c);
 	int err = xcb_connection_has_error(ps->c.c);
 	if (err) {
 		log_fatal("X11 server connection broke (error %d)", err);