From a56d66b4f35d975276935305dc381dc070f39922 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Fri, 27 Nov 2020 05:34:54 +0000 Subject: [PATCH 1/7] win: rework how events for unmapped windows are handled Make unmapped window events work mostly like a mapped window, except flags set on unmapped windows aren't processed until the window is mapped. Hopefully this unifies some of the code paths and reduce corner cases. Should fix #525 Signed-off-by: Yuxuan Shui --- src/event.c | 81 +++++++++++----------- src/picom.c | 31 +++------ src/win.c | 195 +++++++++++++++++++++++++++------------------------- src/win.h | 8 ++- 4 files changed, 157 insertions(+), 158 deletions(-) diff --git a/src/event.c b/src/event.c index 12c03045c2..88bcf31a4b 100644 --- a/src/event.c +++ b/src/event.c @@ -201,58 +201,55 @@ static void configure_win(session_t *ps, xcb_configure_notify_event_t *ce) { auto mw = (struct managed_win *)w; - if (mw->state == WSTATE_UNMAPPED || mw->state == WSTATE_UNMAPPING || - mw->state == WSTATE_DESTROYING) { - // Only restack the window to make sure we can handle future restack - // notification correctly - restack_above(ps, w, ce->above_sibling); - } else { - restack_above(ps, w, ce->above_sibling); - - // If window geometry change, free old extents - if (mw->g.x != ce->x || mw->g.y != ce->y || mw->g.width != ce->width || - mw->g.height != ce->height || mw->g.border_width != ce->border_width) { - // We don't mark the old region as damaged if we have stale - // shape/size/position information. The old region should have - // already been add to damage when the information became stale. - if (!win_check_flags_any( - mw, WIN_FLAGS_SIZE_STALE | WIN_FLAGS_POSITION_STALE)) { - // Mark the old extents as damaged. - // The new extents will be marked damaged when processing - // window flags. + restack_above(ps, w, ce->above_sibling); + + // If window geometry change, free old extents + if (mw->g.x != ce->x || mw->g.y != ce->y || mw->g.width != ce->width || + mw->g.height != ce->height || mw->g.border_width != ce->border_width) { + // We don't mark the old region as damaged if we have stale + // shape/size/position information. The old region should have + // already been add to damage when the information became stale. + if (!win_check_flags_any(mw, WIN_FLAGS_SIZE_STALE | WIN_FLAGS_POSITION_STALE)) { + if (mw->state != WSTATE_UNMAPPED && mw->state != WSTATE_UNMAPPING && + mw->state != WSTATE_DESTROYING) { + // Mark the old extents as damaged. The new extents will + // be marked damaged when processing window flags. + // If the window is not mapped, we don't care region_t damage; pixman_region32_init(&damage); win_extents(mw, &damage); add_damage(ps, &damage); pixman_region32_fini(&damage); } + } - // Queue pending updates - win_set_flags(mw, WIN_FLAGS_FACTOR_CHANGED); - ps->pending_updates = true; - - // At least one of the following if's is true - if (mw->g.x != ce->x || mw->g.y != ce->y) { - log_trace("Window position changed, %dx%d -> %dx%d", - mw->g.x, mw->g.y, ce->x, ce->y); - mw->g.x = ce->x; - mw->g.y = ce->y; - win_set_flags(mw, WIN_FLAGS_POSITION_STALE); - } + // Queue pending updates + win_set_flags(mw, WIN_FLAGS_FACTOR_CHANGED); + // TODO(yshui) don't set pending_updates if the window is not + // visible/mapped + ps->pending_updates = true; - if (mw->g.width != ce->width || mw->g.height != ce->height || - mw->g.border_width != ce->border_width) { - log_trace("Window size changed, %dx%d -> %dx%d", - mw->g.width, mw->g.height, ce->width, ce->height); - mw->g.width = ce->width; - mw->g.height = ce->height; - mw->g.border_width = ce->border_width; - win_set_flags(mw, WIN_FLAGS_SIZE_STALE); - } + // At least one of the following if's is true + if (mw->g.x != ce->x || mw->g.y != ce->y) { + log_trace("Window position changed, %dx%d -> %dx%d", mw->g.x, + mw->g.y, ce->x, ce->y); + mw->g.x = ce->x; + mw->g.y = ce->y; + win_set_flags(mw, WIN_FLAGS_POSITION_STALE); + } - // Recalculate which screen this window is on - win_update_screen(ps->xinerama_nscrs, ps->xinerama_scr_regs, mw); + if (mw->g.width != ce->width || mw->g.height != ce->height || + mw->g.border_width != ce->border_width) { + log_trace("Window size changed, %dx%d -> %dx%d", mw->g.width, + mw->g.height, ce->width, ce->height); + mw->g.width = ce->width; + mw->g.height = ce->height; + mw->g.border_width = ce->border_width; + win_set_flags(mw, WIN_FLAGS_SIZE_STALE); } + + // Recalculate which screen this window is on + win_update_screen(ps->xinerama_nscrs, ps->xinerama_scr_regs, mw); } // override_redirect flag cannot be changed after window creation, as far diff --git a/src/picom.c b/src/picom.c index 27598edc66..97a1bbe726 100644 --- a/src/picom.c +++ b/src/picom.c @@ -405,8 +405,12 @@ static void destroy_backend(session_t *ps) { if (w->state == WSTATE_MAPPED) { win_release_images(ps->backend_data, w); } else { + // Unmapped windows could still have shadow images. + // And they might have image stale flags set by events. assert(!w->win_image); - assert(!w->shadow_image); + win_clear_flags( + w, WIN_FLAGS_PIXMAP_STALE | WIN_FLAGS_SHADOW_STALE); + win_release_images(ps->backend_data, w); } } free_paint(ps, &w->paint); @@ -495,25 +499,12 @@ static bool initialize_backend(session_t *ps) { } auto w = (struct managed_win *)_w; assert(w->state == WSTATE_MAPPED || w->state == WSTATE_UNMAPPED); - if (w->state == WSTATE_MAPPED) { - // We need to reacquire image - log_debug("Marking window %#010x (%s) for update after " - "redirection", - w->base.id, w->name); - if (w->shadow) { - struct color c = { - .red = ps->o.shadow_red, - .green = ps->o.shadow_green, - .blue = ps->o.shadow_blue, - .alpha = ps->o.shadow_opacity, - }; - win_bind_shadow(ps->backend_data, w, c, - ps->gaussian_map); - } - - w->flags |= WIN_FLAGS_PIXMAP_STALE; - ps->pending_updates = true; - } + // We need to reacquire image + log_debug("Marking window %#010x (%s) for update after " + "redirection", + w->base.id, w->name); + win_set_flags(w, WIN_FLAGS_IMAGES_STALE); + ps->pending_updates = true; } } diff --git a/src/win.c b/src/win.c index d32ec978fb..bb08e05ae7 100644 --- a/src/win.c +++ b/src/win.c @@ -125,8 +125,9 @@ static void win_update_focused(session_t *ps, struct managed_win *w) { (ps->o.mark_wmwin_focused && w->wmwin) || (ps->o.mark_ovredir_focused && w->base.id == w->client_win && !w->wmwin) || (w->a.map_state == XCB_MAP_STATE_VIEWABLE && - c2_match(ps, w, ps->o.focus_blacklist, NULL))) + c2_match(ps, w, ps->o.focus_blacklist, NULL))) { w->focused = true; + } // If window grouping detection is enabled, mark the window active if // its group is @@ -411,6 +412,12 @@ void win_process_update_flags(session_t *ps, struct managed_win *w) { win_clear_flags(w, WIN_FLAGS_MAPPED); } + if (w->state == WSTATE_UNMAPPED || w->state == WSTATE_DESTROYING || + w->state == WSTATE_UNMAPPING) { + // Flags of invisible windows are processed when they are mapped + return; + } + // Check client first, because later property updates need accurate client window // information if (win_check_flags_all(w, WIN_FLAGS_CLIENT_STALE)) { @@ -452,6 +459,12 @@ void win_process_update_flags(session_t *ps, struct managed_win *w) { void win_process_image_flags(session_t *ps, struct managed_win *w) { assert(!win_check_flags_all(w, WIN_FLAGS_MAPPED)); + if (w->state == WSTATE_UNMAPPED || w->state == WSTATE_DESTROYING || + w->state == WSTATE_UNMAPPING) { + // Flags of invisible windows are processed when they are mapped + return; + } + // Not a loop while (win_check_flags_any(w, WIN_FLAGS_IMAGES_STALE) && !win_check_flags_all(w, WIN_FLAGS_IMAGE_ERROR)) { @@ -804,15 +817,9 @@ 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(win_check_flags_all(w, WIN_FLAGS_IMAGES_NONE)); - return; - } + // We don't handle property updates of non-visible windows until they are mapped. + assert(w->state != WSTATE_UNMAPPED && w->state != WSTATE_DESTROYING && + w->state != WSTATE_UNMAPPING); // Keep a copy of window extent before the shadow change. Will be used for // calculation of damaged region @@ -856,6 +863,9 @@ static void win_set_shadow(session_t *ps, struct managed_win *w, bool shadow_new // 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. win_set_flags(w, WIN_FLAGS_SHADOW_STALE); + + // Only set pending_updates if we are redirected. Otherwise change of a + // shadow won't have influence on whether we should redirect. ps->pending_updates = true; } @@ -991,8 +1001,9 @@ win_set_blur_background(session_t *ps, struct managed_win *w, bool blur_backgrou */ static void win_determine_blur_background(session_t *ps, struct managed_win *w) { log_debug("Determining blur-background of window %#010x (%s)", w->base.id, w->name); - if (w->a.map_state != XCB_MAP_STATE_VIEWABLE) + if (w->a.map_state != XCB_MAP_STATE_VIEWABLE) { return; + } bool blur_background_new = ps->o.blur_method != BLUR_METHOD_NONE; if (blur_background_new) { @@ -1190,8 +1201,9 @@ static xcb_window_t find_client_win(session_t *ps, xcb_window_t w) { xcb_query_tree_reply_t *reply = xcb_query_tree_reply(ps->c, xcb_query_tree(ps->c, w), NULL); - if (!reply) + if (!reply) { return 0; + } xcb_window_t *children = xcb_query_tree_children(reply); int nchildren = xcb_query_tree_children_length(reply); @@ -1199,8 +1211,9 @@ static xcb_window_t find_client_win(session_t *ps, xcb_window_t w) { xcb_window_t ret = 0; for (i = 0; i < nchildren; ++i) { - if ((ret = find_client_win(ps, children[i]))) + if ((ret = find_client_win(ps, children[i]))) { break; + } } free(reply); @@ -1237,8 +1250,9 @@ void win_recheck_client(session_t *ps, struct managed_win *w) { } // Unmark the old one - if (w->client_win && w->client_win != cw) + if (w->client_win && w->client_win != cw) { win_unmark_client(ps, w); + } // Mark the new one win_mark_client(ps, w, cw); @@ -1455,17 +1469,41 @@ struct win *fill_win(session_t *ps, struct win *w) { free(a); + xcb_generic_error_t *e; + auto g = xcb_get_geometry_reply(ps->c, xcb_get_geometry(ps->c, w->id), &e); + if (!g) { + log_error_x_error(e, "Failed to get geometry of window %#010x", w->id); + free(e); + free(new); + return w; + } + new->g = *g; + free(g); + + win_update_screen(ps->xinerama_nscrs, ps->xinerama_scr_regs, new); + // Create Damage for window (if not Input Only) new->damage = x_new_id(ps->c); - xcb_generic_error_t *e = xcb_request_check( + e = xcb_request_check( ps->c, xcb_damage_create_checked(ps->c, new->damage, w->id, XCB_DAMAGE_REPORT_LEVEL_NON_EMPTY)); if (e) { + log_error_x_error(e, "Failed to create damage"); free(e); free(new); return w; } + // Set window event mask + xcb_change_window_attributes( + ps->c, new->base.id, XCB_CW_EVENT_MASK, + (const uint32_t[]){determine_evmask(ps, new->base.id, WIN_EVMODE_FRAME)}); + + // Get notification when the shape of a window changes + if (ps->shape_exists) { + xcb_shape_select_input(ps->c, new->base.id, 1); + } + new->pictfmt = x_get_pictform_for_visual(ps->c, new->a.visual); new->client_pictfmt = NULL; @@ -1475,6 +1513,20 @@ struct win *fill_win(session_t *ps, struct win *w) { assert(replaced == w); free(w); + // Set all the stale flags on this new window, so it's properties will get updated + // when it's mapped + win_set_flags(new, WIN_FLAGS_CLIENT_STALE | WIN_FLAGS_SIZE_STALE | + WIN_FLAGS_POSITION_STALE | WIN_FLAGS_PROPERTY_STALE | + WIN_FLAGS_FACTOR_CHANGED); + xcb_atom_t init_stale_props[] = { + ps->atoms->a_NET_WM_WINDOW_TYPE, ps->atoms->a_NET_WM_WINDOW_OPACITY, + ps->atoms->a_NET_FRAME_EXTENTS, ps->atoms->aWM_NAME, + ps->atoms->a_NET_WM_NAME, ps->atoms->aWM_CLASS, + ps->atoms->aWM_WINDOW_ROLE, ps->atoms->a_COMPTON_SHADOW, + ps->atoms->aWM_CLIENT_LEADER, ps->atoms->aWM_TRANSIENT_FOR, + }; + win_set_properties_stale(new, init_stale_props, ARR_SIZE(init_stale_props)); + #ifdef CONFIG_DBUS // Send D-Bus signal if (ps->o.dbus) { @@ -1856,7 +1908,11 @@ static void unmap_win_finish(session_t *ps, struct managed_win *w) { // We are in unmap_win, this window definitely was viewable if (ps->backend_data) { - win_release_images(ps->backend_data, w); + // Only the pixmap needs to be freed and reacquired when mapping. + // Shadow image can be preserved. + if (!win_check_flags_all(w, WIN_FLAGS_PIXMAP_NONE)) { + win_release_pixmap(ps->backend_data, w); + } } else { assert(!w->win_image); assert(!w->shadow_image); @@ -1889,6 +1945,12 @@ static void destroy_win_finish(session_t *ps, struct win *w) { unmap_win_finish(ps, mw); } + // Unmapping preserves the shadow image, so free it here + if (!win_check_flags_all(mw, WIN_FLAGS_SHADOW_NONE)) { + assert(mw->shadow_image != NULL); + win_release_shadow(ps->backend_data, mw); + } + // Invalidate reg_ignore of windows below this one // TODO(yshui) what if next_w is not mapped?? /* TODO(yshui) seriously figure out how reg_ignore behaves. @@ -2117,13 +2179,6 @@ void unmap_win_start(session_t *ps, struct managed_win *w) { w->opacity_target_old = fmax(w->opacity_target, w->opacity_target_old); w->opacity_target = win_calc_opacity_target(ps, w); - // Clear PIXMAP_STALE flag, since the window is unmapped there is no pixmap - // available so STALE doesn't make sense. - win_clear_flags(w, WIN_FLAGS_PIXMAP_STALE); - - // don't care about properties anymore - win_ev_stop(ps, &w->base); - #ifdef CONFIG_DBUS // Send D-Bus signal if (ps->o.dbus) { @@ -2226,23 +2281,6 @@ void map_win_start(session_t *ps, struct managed_win *w) { } assert(w->state == WSTATE_UNMAPPED); - assert(win_check_flags_all(w, WIN_FLAGS_IMAGES_NONE) || !ps->o.experimental_backends); - - // We stopped processing window size change when we were unmapped, refresh the - // size of the window - xcb_get_geometry_cookie_t gcookie = xcb_get_geometry(ps->c, w->base.id); - xcb_get_geometry_reply_t *g = xcb_get_geometry_reply(ps->c, gcookie, NULL); - - if (!g) { - log_error("Failed to get the geometry of window %#010x", w->base.id); - return; - } - - w->g = *g; - free(g); - - win_on_win_size_change(ps, w); - log_trace("Window size: %dx%d", w->g.width, w->g.height); // Rant: window size could change after we queried its geometry here and before // we get its pixmap. Later, when we get back to the event processing loop, we @@ -2254,48 +2292,11 @@ void map_win_start(session_t *ps, struct managed_win *w) { // XXX Can we assume map_state is always viewable? w->a.map_state = XCB_MAP_STATE_VIEWABLE; - win_update_screen(ps->xinerama_nscrs, ps->xinerama_scr_regs, w); - - // Set window event mask before reading properties so that no property - // changes are lost - xcb_change_window_attributes( - ps->c, w->base.id, XCB_CW_EVENT_MASK, - (const uint32_t[]){determine_evmask(ps, w->base.id, WIN_EVMODE_FRAME)}); - - // Get notification when the shape of a window changes - if (ps->shape_exists) { - xcb_shape_select_input(ps->c, w->base.id, 1); - } - // Update window mode here to check for ARGB windows w->mode = win_calc_mode(w); - // Detect client window here instead of in add_win() as the client - // window should have been prepared at this point - if (!w->client_win) { - win_recheck_client(ps, w); - } else { - // Re-mark client window here - win_mark_client(ps, w, w->client_win); - } - assert(w->client_win); - log_debug("Window (%#010x) has type %s", w->base.id, WINTYPES[w->window_type]); - // TODO(yshui) can we just replace calls below with win_on_factor_change? - - // Update window focus state - win_update_focused(ps, w); - - // Update opacity and dim state - win_update_opacity_prop(ps, w); - - // Check for _COMPTON_SHADOW - win_update_prop_shadow_raw(ps, w); - - // Many things above could affect shadow - win_determine_shadow(ps, w); - // XXX We need to make sure that win_data is available // iff `state` is MAPPED w->state = WSTATE_MAPPING; @@ -2305,23 +2306,15 @@ void map_win_start(session_t *ps, struct managed_win *w) { log_debug("Window %#010x has opacity %f, opacity target is %f", w->base.id, w->opacity, w->opacity_target); - win_determine_blur_background(ps, w); - // Cannot set w->ever_damaged = false here, since window mapping could be // delayed, so a damage event might have already arrived before this function // is called. But this should be unnecessary in the first place, since // ever_damaged is set to false in unmap_win_finish anyway. - // We stopped listening on ShapeNotify events - // when the window is unmapped (XXX we shouldn't), - // so the shape of the window might have changed, - // update. (Issue #35) - // - // Also this sets the WIN_FLAGS_IMAGES_STALE flag so later in the critical section + // Sets the WIN_FLAGS_IMAGES_STALE flag so later in the critical section // the window's image will be bound - win_update_bounding_shape(ps, w); - assert(win_check_flags_all(w, WIN_FLAGS_IMAGES_STALE)); + win_set_flags(w, WIN_FLAGS_PIXMAP_STALE); #ifdef CONFIG_DBUS // Send D-Bus signal @@ -2538,19 +2531,33 @@ void win_clear_flags(struct managed_win *w, uint64_t flags) { w->flags = w->flags & (~flags); } -void win_set_property_stale(struct managed_win *w, xcb_atom_t prop) { +void win_set_properties_stale(struct managed_win *w, const xcb_atom_t *props, int nprops) { const auto bits_per_element = sizeof(*w->stale_props) * 8; - if (prop >= w->stale_props_capacity * bits_per_element) { - const auto new_size = prop / bits_per_element + 1; - w->stale_props = realloc(w->stale_props, new_size * sizeof(*w->stale_props)); + size_t new_capacity = w->stale_props_capacity; + + // Calculate the new capacity of the properties array + for (int i = 0; i < nprops; i++) { + if (props[i] >= new_capacity * bits_per_element) { + new_capacity = props[i] / bits_per_element + 1; + } + } + + // Reallocate if necessary + if (new_capacity > w->stale_props_capacity) { + w->stale_props = + realloc(w->stale_props, new_capacity * sizeof(*w->stale_props)); // Clear the content of the newly allocated bytes memset(w->stale_props + w->stale_props_capacity, 0, - (new_size - w->stale_props_capacity) * sizeof(*w->stale_props)); - w->stale_props_capacity = new_size; + (new_capacity - w->stale_props_capacity) * sizeof(*w->stale_props)); + w->stale_props_capacity = new_capacity; } - w->stale_props[prop / bits_per_element] |= 1UL << (prop % bits_per_element); + // Set the property bits + for (int i = 0; i < nprops; i++) { + w->stale_props[props[i] / bits_per_element] |= + 1UL << (props[i] % bits_per_element); + } win_set_flags(w, WIN_FLAGS_PROPERTY_STALE); } diff --git a/src/win.h b/src/win.h index 3439acf27f..83cebec56c 100644 --- a/src/win.h +++ b/src/win.h @@ -428,8 +428,12 @@ void win_clear_flags(struct managed_win *w, uint64_t flags); bool win_check_flags_any(struct managed_win *w, uint64_t flags); /// Returns true if all of the flags in `flags` are set bool win_check_flags_all(struct managed_win *w, uint64_t flags); -/// Mark a property as stale for a window -void win_set_property_stale(struct managed_win *w, xcb_atom_t prop); +/// Mark properties as stale for a window +void win_set_properties_stale(struct managed_win *w, const xcb_atom_t *prop, int nprops); + +static inline attr_unused void win_set_property_stale(struct managed_win *w, xcb_atom_t prop) { + return win_set_properties_stale(w, (xcb_atom_t[]){prop}, 1); +} /// Free all resources in a struct win void free_win_res(session_t *ps, struct managed_win *w); From 2d54942295c5100753b2faf2d0a2b70ef706e06f Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Fri, 27 Nov 2020 20:35:09 +0000 Subject: [PATCH 2/7] testcase: add redirect_when_unmapped_window_has_shadow While working on this PR, I introduced a bug where shadow images for unmapped windows aren't properly recreated after unredirect/redirect. The shadow image is freed during unredirect, OTOH redirect only set IMAGE_STALE flags for mapped window, thus the shadow images for unmapped windows will be missing. This bug is already fixed in the previous commit. But the testcase is good to keep nonetheless. Signed-off-by: Yuxuan Shui --- tests/run_tests.sh | 1 + ...edirect_when_unmapped_window_has_shadow.py | 65 +++++++++++++++++++ 2 files changed, 66 insertions(+) create mode 100755 tests/testcases/redirect_when_unmapped_window_has_shadow.py diff --git a/tests/run_tests.sh b/tests/run_tests.sh index 6c871dec3d..3eec37891f 100755 --- a/tests/run_tests.sh +++ b/tests/run_tests.sh @@ -17,4 +17,5 @@ eval `dbus-launch --sh-syntax` ./run_one_test.sh $exe /dev/null testcases/issue299.py ./run_one_test.sh $exe configs/issue465.conf testcases/issue465.py ./run_one_test.sh $exe configs/clear_shadow_unredirected.conf testcases/clear_shadow_unredirected.py +./run_one_test.sh $exe configs/clear_shadow_unredirected.conf testcases/redirect_when_unmapped_window_has_shadow.py ./run_one_test.sh $exe configs/issue394.conf testcases/issue394.py diff --git a/tests/testcases/redirect_when_unmapped_window_has_shadow.py b/tests/testcases/redirect_when_unmapped_window_has_shadow.py new file mode 100755 index 0000000000..aeceb33c4a --- /dev/null +++ b/tests/testcases/redirect_when_unmapped_window_has_shadow.py @@ -0,0 +1,65 @@ +#!/usr/bin/env python + +import xcffib.xproto as xproto +import xcffib +import time +from common import set_window_name + +conn = xcffib.connect() +setup = conn.get_setup() +root = setup.roots[0].root +visual = setup.roots[0].root_visual +depth = setup.roots[0].root_depth + +name = "_NET_WM_STATE" +name_atom = conn.core.InternAtom(False, len(name), name).reply().atom +atom = "ATOM" +atom_atom = conn.core.InternAtom(False, len(atom), atom).reply().atom +fs = "_NET_WM_STATE_FULLSCREEN" +fs_atom = conn.core.InternAtom(False, len(fs), fs).reply().atom + +wid1 = conn.generate_id() +print("Window 1 id is ", hex(wid1)) + +# Create a window +conn.core.CreateWindowChecked(depth, wid1, root, 0, 0, 100, 100, 0, xproto.WindowClass.InputOutput, visual, 0, []).check() + +# Map the window +print("mapping 1") +conn.core.MapWindowChecked(wid1).check() + +time.sleep(0.5) + +print("unmapping 1") +# Unmap the window +conn.core.UnmapWindowChecked(wid1).check() + +time.sleep(0.5) + +# create and map a second window +wid2 = conn.generate_id() +print("Window 2 id is ", hex(wid2)) +conn.core.CreateWindowChecked(depth, wid2, root, 200, 0, 100, 100, 0, xproto.WindowClass.InputOutput, visual, 0, []).check() +print("mapping 2") +conn.core.MapWindowChecked(wid2).check() +time.sleep(0.5) + +# Set fullscreen property on the second window, causing screen to be unredirected +conn.core.ChangePropertyChecked(xproto.PropMode.Replace, wid2, name_atom, atom_atom, 32, 1, [fs_atom]).check() + +time.sleep(0.5) + +# Unset fullscreen property on the second window, causing screen to be redirected +conn.core.ChangePropertyChecked(xproto.PropMode.Replace, wid2, name_atom, atom_atom, 32, 0, []).check() + +time.sleep(0.5) + +# map the first window again +print("mapping 1") +conn.core.MapWindowChecked(wid1).check() + +time.sleep(0.5) + +# Destroy the windows +conn.core.DestroyWindowChecked(wid1).check() +conn.core.DestroyWindowChecked(wid2).check() From 1b4dacf2c277f16a32a8d5e698569fc2dd220db4 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Sat, 28 Nov 2020 02:11:17 +0000 Subject: [PATCH 3/7] win: add assertion ensuring flags of unmapped windows aren't processed Signed-off-by: Yuxuan Shui --- src/win.c | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/src/win.c b/src/win.c index bb08e05ae7..5bdaacc83e 100644 --- a/src/win.c +++ b/src/win.c @@ -1082,14 +1082,13 @@ void win_on_win_size_change(session_t *ps, struct managed_win *w) { w->shadow_width = w->widthb + ps->o.shadow_radius * 2; w->shadow_height = w->heightb + ps->o.shadow_radius * 2; + // We don't handle property updates of non-visible windows until they are mapped. + assert(w->state != WSTATE_UNMAPPED && w->state != WSTATE_DESTROYING && + w->state != WSTATE_UNMAPPING); + // Invalidate the shadow we built - if (w->state == WSTATE_MAPPED || w->state == WSTATE_MAPPING || - w->state == WSTATE_FADING) { - win_set_flags(w, WIN_FLAGS_IMAGES_STALE); - ps->pending_updates = true; - } else { - assert(w->state == WSTATE_UNMAPPED); - } + win_set_flags(w, WIN_FLAGS_IMAGES_STALE); + ps->pending_updates = true; free_paint(ps, &w->shadow_paint); } @@ -1744,6 +1743,10 @@ void win_update_bounding_shape(session_t *ps, struct managed_win *w) { w->bounding_shaped = win_bounding_shaped(ps, w->base.id); } + // We don't handle property updates of non-visible windows until they are mapped. + assert(w->state != WSTATE_UNMAPPED && w->state != WSTATE_DESTROYING && + w->state != WSTATE_UNMAPPING); + pixman_region32_clear(&w->bounding_shape); // Start with the window rectangular region win_get_region_local(w, &w->bounding_shape); @@ -1795,13 +1798,9 @@ void win_update_bounding_shape(session_t *ps, struct managed_win *w) { // Window shape changed, we should free old wpaint and shadow pict // log_trace("free out dated pict"); - if (w->state != WSTATE_UNMAPPED) { - // Note we only do this when screen is redirected, because - // otherwise win_data is not valid - assert(w->state != WSTATE_UNMAPPING && w->state != WSTATE_DESTROYING); - win_set_flags(w, WIN_FLAGS_IMAGES_STALE); - ps->pending_updates = true; - } + win_set_flags(w, WIN_FLAGS_IMAGES_STALE); + ps->pending_updates = true; + free_paint(ps, &w->paint); free_paint(ps, &w->shadow_paint); From 862d96d6093bbaa0158046d017f582905cd697c2 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Sat, 28 Nov 2020 03:07:59 +0000 Subject: [PATCH 4/7] testcases: add a test case for #525 Signed-off-by: Yuxuan Shui --- tests/run_tests.sh | 1 + tests/testcases/issue525.py | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+) create mode 100755 tests/testcases/issue525.py diff --git a/tests/run_tests.sh b/tests/run_tests.sh index 3eec37891f..6a5a6b3ffc 100755 --- a/tests/run_tests.sh +++ b/tests/run_tests.sh @@ -19,3 +19,4 @@ eval `dbus-launch --sh-syntax` ./run_one_test.sh $exe configs/clear_shadow_unredirected.conf testcases/clear_shadow_unredirected.py ./run_one_test.sh $exe configs/clear_shadow_unredirected.conf testcases/redirect_when_unmapped_window_has_shadow.py ./run_one_test.sh $exe configs/issue394.conf testcases/issue394.py +./run_one_test.sh $exe configs/issue239.conf testcases/issue525.py diff --git a/tests/testcases/issue525.py b/tests/testcases/issue525.py new file mode 100755 index 0000000000..317e30882d --- /dev/null +++ b/tests/testcases/issue525.py @@ -0,0 +1,36 @@ +#!/usr/bin/env python + +import xcffib.xproto as xproto +import xcffib +import time +from common import set_window_name + +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 525 happens when a window is unmapped with pixmap stale flag set +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() + +# Map the window +print("mapping") +conn.core.MapWindowChecked(wid).check() + +time.sleep(0.5) + +# change window size, invalidate the pixmap +conn.core.ConfigureWindow(wid, xproto.ConfigWindow.X | xproto.ConfigWindow.Width, [100, 200]) + +# unmap the window immediately after +conn.core.UnmapWindowChecked(wid).check() + +time.sleep(0.1) + +# Destroy the window +conn.core.DestroyWindowChecked(wid).check() From e49ef2ccd87302e1a18f7e95bbe399c9203a1113 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Sat, 28 Nov 2020 19:31:43 +0000 Subject: [PATCH 5/7] core: always clear stale flags in destroy_backend Reference: https://github.com/yshui/picom/pull/546#discussion_r532054960 Signed-off-by: Yuxuan Shui --- src/picom.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/src/picom.c b/src/picom.c index 97a1bbe726..a394a309ff 100644 --- a/src/picom.c +++ b/src/picom.c @@ -402,16 +402,17 @@ static void destroy_backend(session_t *ps) { } if (ps->backend_data) { - if (w->state == WSTATE_MAPPED) { - win_release_images(ps->backend_data, w); - } else { - // Unmapped windows could still have shadow images. - // And they might have image stale flags set by events. - assert(!w->win_image); - win_clear_flags( - w, WIN_FLAGS_PIXMAP_STALE | WIN_FLAGS_SHADOW_STALE); - win_release_images(ps->backend_data, w); - } + // Unmapped windows could still have shadow images, but not pixmap + // images + assert(!w->win_image || w->state != WSTATE_UNMAPPED); + // Windows can still have stale flags set. For mapped windows, + // this can happen when destroy_backend is called before the stale + // flags are handled (e.g. when destroy_backend is called in + // session_destroy, or configure_root); for unmapped windows, this + // is because their stale flags aren't handled until they are + // mapped. + win_clear_flags(w, WIN_FLAGS_IMAGES_STALE); + win_release_images(ps->backend_data, w); } free_paint(ps, &w->paint); } From 60f733d17c395e08226b0788790954d1c64065e7 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Sat, 28 Nov 2020 22:48:01 +0000 Subject: [PATCH 6/7] core: don't call map_win_start in handle_new_windows Set WIN_FLAGS_MAPPED instead, it will be handled later. Previously, we call map_win_start because we need the geometry of the window, which is updated in map_win_start. Now, we get the geometry in fill_win, so map_win_start is not needed anymore. Eliminate a case where destroy_backend could see IMAGES_STALE flags set on windows. Signed-off-by: Yuxuan Shui --- src/picom.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/picom.c b/src/picom.c index a394a309ff..439c624ce8 100644 --- a/src/picom.c +++ b/src/picom.c @@ -1346,10 +1346,7 @@ static void handle_new_windows(session_t *ps) { } auto mw = (struct managed_win *)new_w; if (mw->a.map_state == XCB_MAP_STATE_VIEWABLE) { - // Have to map immediately instead of queue window update - // because we need the window's extent right now. - // We can do this because we are in the critical section. - map_win_start(ps, mw); + win_set_flags(mw, WIN_FLAGS_MAPPED); // This window might be damaged before we called fill_win // and created the damage handle. And there is no way for From 0f975616d63a53aa8404546e50375dd7a8b4e0f2 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Sun, 29 Nov 2020 00:12:40 +0000 Subject: [PATCH 7/7] core: ensure stale flags aren't set in destroy_backend After 60f733d17c395e08226b0788790954d1c64065e7, we can be sure stale flags won't be seen in destroy_backend. Signed-off-by: Yuxuan Shui --- src/picom.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/picom.c b/src/picom.c index 439c624ce8..17005c384a 100644 --- a/src/picom.c +++ b/src/picom.c @@ -405,12 +405,15 @@ static void destroy_backend(session_t *ps) { // Unmapped windows could still have shadow images, but not pixmap // images assert(!w->win_image || w->state != WSTATE_UNMAPPED); - // Windows can still have stale flags set. For mapped windows, - // this can happen when destroy_backend is called before the stale - // flags are handled (e.g. when destroy_backend is called in - // session_destroy, or configure_root); for unmapped windows, this - // is because their stale flags aren't handled until they are - // mapped. + if (win_check_flags_any(w, WIN_FLAGS_IMAGES_STALE) && + w->state == WSTATE_MAPPED) { + log_warn("Stale flags set for mapped window %#010x " + "during backend destruction", + w->base.id); + assert(false); + } + // Unmapped windows can still have stale flags set, because their + // stale flags aren't handled until they are mapped. win_clear_flags(w, WIN_FLAGS_IMAGES_STALE); win_release_images(ps->backend_data, w); }