core: re-run paint_preprocess after redirecting

paint_preprocess calls redir_start if it decides to redirect the screen.
redir_start might change the state of the windows. For example, when the
image of the window fails to bind, redir_start sets the error flag. And
those state changes might exclude the window from being rendered. Thus,
paint_preprocess should be re-run to make sure the new states are
honoured.

Fixes a crash when window images fail to bind after redirecting.

Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>

src/backend/backend.c

@@ -98,6 +98,7 @@ void paint_all_new(session_t *ps, win *const t, bool ignore_damage) {
 	// Whether this is beneficial is to be determined XXX
 	for (win *w = t; w; w = w->prev_trans) {
 		pixman_region32_subtract(&reg_visible, &ps->screen_reg, w->reg_ignore);
+		assert(!(w->flags & WIN_FLAGS_IMAGE_ERROR));
 
 		// The bounding shape of the window, in global/target coordinates
 		// reminder: bounding shape contains the WM frame

src/compton.c

@@ -24,8 +24,8 @@
 #include <xcb/sync.h>
 #include <xcb/xfixes.h>
 
-#include <test.h>
 #include <ev.h>
+#include <test.h>
 
 #include "common.h"
 #include "compiler.h"
@@ -605,12 +605,13 @@ static win *paint_preprocess(session_t *ps, bool *fade_running) {
 	rc_region_unref(&last_reg_ignore);
 
 	// If possible, unredirect all windows and stop painting
-	if (ps->o.redirected_force != UNSET)
+	if (ps->o.redirected_force != UNSET) {
 		unredir_possible = !ps->o.redirected_force;
-	else if (ps->o.unredir_if_possible && is_highest && !ps->redirected)
+	} else if (ps->o.unredir_if_possible && is_highest && !ps->redirected) {
 		// If there's no window to paint, and the screen isn't redirected,
 		// don't redirect it.
 		unredir_possible = true;
+	}
 	if (unredir_possible) {
 		if (ps->redirected) {
 			if (!ps->o.unredir_if_possible_delay || ps->tmout_unredir_hit)
@@ -623,10 +624,12 @@ static win *paint_preprocess(session_t *ps, bool *fade_running) {
 		}
 	} else {
 		ev_timer_stop(ps->loop, &ps->unredir_timer);
+		if (!ps->redirected) {
 			if (!redir_start(ps)) {
 				return NULL;
 			}
 		}
+	}
 
 	return t;
 }
@@ -1315,8 +1318,8 @@ static bool init_overlay(session_t *ps) {
  * @return whether the operation succeeded or not
  */
 static bool redir_start(session_t *ps) {
-	if (!ps->redirected) {
+	assert(!ps->redirected);
-		log_debug("Screen redirected.");
+	log_debug("Redirecting the screen.");
 
 	// Map overlay window. Done firstly according to this:
 	// https://bugzilla.gnome.org/show_bug.cgi?id=597014
@@ -1324,8 +1327,7 @@ static bool redir_start(session_t *ps) {
 		xcb_map_window(ps->c, ps->overlay);
 	}
 
-		xcb_composite_redirect_subwindows(ps->c, ps->root,
+	xcb_composite_redirect_subwindows(ps->c, ps->root, XCB_COMPOSITE_REDIRECT_MANUAL);
-		                                  XCB_COMPOSITE_REDIRECT_MANUAL);
 
 	x_sync(ps->c);
 
@@ -1354,7 +1356,7 @@ static bool redir_start(session_t *ps) {
 
 	// Repaint the whole screen
 	force_repaint(ps);
-	}
+	log_debug("Screen redirected.");
 	return true;
 }
 
@@ -1362,13 +1364,12 @@ static bool redir_start(session_t *ps) {
  * Unredirect all windows.
  */
 static void redir_stop(session_t *ps) {
-	if (ps->redirected) {
+	assert(ps->redirected);
-		log_debug("Screen unredirected.");
+	log_debug("Unredirecting the screen.");
 
 	destroy_backend(ps);
 
-		xcb_composite_unredirect_subwindows(ps->c, ps->root,
+	xcb_composite_unredirect_subwindows(ps->c, ps->root, XCB_COMPOSITE_REDIRECT_MANUAL);
-		                                    XCB_COMPOSITE_REDIRECT_MANUAL);
 	// Unmap overlay window
 	if (ps->overlay)
 		xcb_unmap_window(ps->c, ps->overlay);
@@ -1385,7 +1386,7 @@ static void redir_stop(session_t *ps) {
 	x_sync(ps->c);
 
 	ps->redirected = false;
-	}
+	log_debug("Screen unredirected.");
 }
 
 // Handle queued events before we go to sleep
@@ -1443,14 +1444,27 @@ static void _draw_callback(EV_P_ session_t *ps, int revents) {
 
 	handle_root_flags(ps);
 
+	// TODO have a stripped down version of paint_preprocess that is used when screen
+	// is not redirected. its sole purpose should be to decide whether the screen
+	// should be redirected.
 	bool fade_running = false;
+	bool was_redirected = ps->redirected;
 	win *t = paint_preprocess(ps, &fade_running);
 	ps->tmout_unredir_hit = false;
 
+	if (!was_redirected && ps->redirected) {
+		// paint_preprocess redirected the screen, which might change the state of
+		// some of the windows (e.g. the window image might fail to bind, and the
+		// window would be put into an error state). so we rerun paint_preprocess
+		// here to make sure the rendering decision we make is up-to-date
+		log_debug("Re-run paint_preprocess");
+		t = paint_preprocess(ps, &fade_running);
+	}
+
 	// Start/stop fade timer depends on whether window are fading
-	if (!fade_running && ev_is_active(&ps->fade_timer))
+	if (!fade_running && ev_is_active(&ps->fade_timer)) {
 		ev_timer_stop(ps->loop, &ps->fade_timer);
-	else if (fade_running && !ev_is_active(&ps->fade_timer)) {
+	} else if (fade_running && !ev_is_active(&ps->fade_timer)) {
 		ev_timer_set(&ps->fade_timer, fade_timeout(ps), 0);
 		ev_timer_start(ps->loop, &ps->fade_timer);
 	}
@@ -2144,7 +2158,9 @@ static session_t *session_init(int argc, char **argv, Display *dpy,
  * @param ps session to destroy
  */
 static void session_destroy(session_t *ps) {
+	if (ps->redirected) {
 		redir_stop(ps);
+	}
 
 	// Stop listening to events on root window
 	xcb_change_window_attributes(ps->c, ps->root, XCB_CW_EVENT_MASK,

src/win.c

@@ -196,7 +196,7 @@ _win_bind_image(session_t *ps, win *w, void **win_image, void **shadow_image) {
 	auto e = xcb_request_check(
 	    ps->c, xcb_composite_name_window_pixmap_checked(ps->c, w->id, pixmap));
 	if (e) {
-		log_error("Failed to get named pixmap");
+		log_error("Failed to get named pixmap for window %#010x(%s)", w->id, w->name);
 		free(e);
 		return false;
 	}