win: delayed release of shadow image

Previously win_set_shadow tries to release the shadow image when turning
off shadow for a window. When shadow is turned off _immediately_ after
it's turned on, picom won't have a chance to handle the delayed creation
of the shadow before win_set_shadow tries to release the shadow image,
causing a assertion failure because win_set_shadow tried to release a
non-existing image.

This commit makes releasing the shadow image delayed as well.

In theory, we could check the STALE flag in win_set_shadow before
release the image, but that duplicates the logic that is already in
win_process_flags.

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

src/win.c

@@ -695,34 +695,55 @@ static void win_set_shadow(session_t *ps, struct managed_win *w, bool shadow_new
 	log_debug("Updating shadow property of window %#010x (%s) to %d", w->base.id,
 	          w->name, shadow_new);
 
+	if (w->state == WSTATE_UNMAPPED) {
+		// No need to add damage or update shadow
+		// Unmapped window shouldn't have any images
+		w->shadow = shadow_new;
+		assert(!w->shadow_image);
+		assert(!w->win_image);
+		assert(w->flags & WIN_FLAGS_IMAGES_NONE);
+		return;
+	}
+
+	// Keep a copy of window extent before the shadow change. Will be used for
+	// calculation of damaged region
 	region_t extents;
 	pixman_region32_init(&extents);
 	win_extents(w, &extents);
 
+	// Apply the shadow change
 	w->shadow = shadow_new;
 
+	// Add damage for shadow change
+
 	// Window extents need update on shadow state change
 	// Shadow geometry currently doesn't change on shadow state change
 	// calc_shadow_geometry(ps, w);
-	// Mark the old extents as damaged if the shadow is removed
-	if (!w->shadow) {
-		add_damage(ps, &extents);
-		win_release_shadow(ps->backend_data, w);
-	}
 
-	pixman_region32_clear(&extents);
+	// Note: because the release and creation of the shadow images are delayed. When
-	// Mark the new extents as damaged if the shadow is added
+	// multiple shadow changes happen in a row, without rendering phase between them,
+	// there could be a stale shadow image attached to the window even if w->shadow
+	// was previously false. And vice versa. So we check the STALE flag before
+	// asserting the existence of the shadow image.
 	if (w->shadow) {
+		// Mark the new extents as damaged if the shadow is added
+		assert(!w->shadow_image || (w->flags & WIN_FLAGS_SHADOW_STALE));
+		pixman_region32_clear(&extents);
 		win_extents(w, &extents);
 		add_damage_from_win(ps, w);
-		if (w->state != WSTATE_UNMAPPED) {
+	} else {
-			assert(!w->shadow_image);
+		// Mark the old extents as damaged if the shadow is removed
-			// Delayed creation of shadow image
+		assert(w->shadow_image || (w->flags & WIN_FLAGS_SHADOW_STALE));
+		add_damage(ps, &extents);
+	}
+
+	pixman_region32_fini(&extents);
+
+	// Delayed update of shadow image
+	// By setting WIN_FLAGS_SHADOW_STALE, we ask win_process_flags to re-create or
+	// release the shaodw in based on whether w->shadow is set.
 	w->flags |= WIN_FLAGS_SHADOW_STALE;
 	ps->pending_updates = true;
-		}
-	}
-	pixman_region32_fini(&extents);
 }
 
 /**

tests/run_tests.sh

@@ -7,3 +7,4 @@ cd $(dirname $0)
 ./run_one_test.sh $exe configs/issue239.conf testcases/issue239.py
 ./run_one_test.sh $exe configs/issue239_2.conf testcases/issue239_2.py
 ./run_one_test.sh $exe configs/issue239_3.conf testcases/issue239_3.py
+./run_one_test.sh $exe configs/issue239_3.conf testcases/issue239_3_norefresh.py

tests/testcases/issue239_3.py

@@ -36,6 +36,8 @@ print("set new name")
 win_name = "NoShadow"
 conn.core.ChangePropertyChecked(xproto.PropMode.Replace, wid, name_atom, str_type_atom, 8, len(win_name), win_name).check()
 
+time.sleep(0.5)
+
 # Set the Window name so it gets a shadow
 print("set new name")
 win_name = "YesShadow"

tests/testcases/issue239_3_norefresh.py

@@ -0,0 +1,52 @@
+#!/usr/bin/env python
+
+import xcffib.xproto as xproto
+import xcffib
+import time
+
+conn = xcffib.connect()
+setup = conn.get_setup()
+root = setup.roots[0].root
+visual = setup.roots[0].root_visual
+depth = setup.roots[0].root_depth
+
+# issue 239 is caused by a window gaining a shadow during its fade-out transition
+wid = conn.generate_id()
+print("Window id is ", hex(wid))
+
+# Create a window
+conn.core.CreateWindowChecked(depth, wid, root, 0, 0, 100, 100, 0, xproto.WindowClass.InputOutput, visual, 0, []).check()
+
+# Set Window name so it doesn't get a shadow
+name = "_NET_WM_NAME"
+name_atom = conn.core.InternAtom(True, len(name), name).reply().atom
+str_type = "STRING"
+str_type_atom = conn.core.InternAtom(True, len(str_type), str_type).reply().atom
+
+win_name = "YesShadow"
+conn.core.ChangePropertyChecked(xproto.PropMode.Replace, wid, name_atom, str_type_atom, 8, len(win_name), win_name).check()
+
+# Map the window
+print("mapping")
+conn.core.MapWindowChecked(wid).check()
+
+time.sleep(0.5)
+
+print("set new name")
+win_name = "NoShadow"
+conn.core.ChangePropertyChecked(xproto.PropMode.Replace, wid, name_atom, str_type_atom, 8, len(win_name), win_name).check()
+
+# Set the Window name so it gets a shadow
+print("set new name")
+win_name = "YesShadow"
+conn.core.ChangePropertyChecked(xproto.PropMode.Replace, wid, name_atom, str_type_atom, 8, len(win_name), win_name).check()
+
+time.sleep(0.5)
+
+# Unmap the window
+conn.core.UnmapWindowChecked(wid).check()
+
+time.sleep(0.5)
+
+# Destroy the window
+conn.core.DestroyWindowChecked(wid).check()