shell: drop the GdkSurface when a layer-shell window closes - #15
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4031172d53
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // gtk4-layer-shell makes in gtk_layer_surface_remap(). | ||
| // The cast is required: GtkWindow implements GtkNative, so a bare | ||
| // unrealize() binds to gtk_native_unrealize, not gtk_widget_unrealize. | ||
| ((Gtk.Widget) this).unrealize(); |
There was a problem hiding this comment.
When the launcher overview is dismissed through the four-finger gesture, both branches of end_gesture() call finish_gesture_hide(), which only invokes hide() and never drops the GdkSurface. Consequently this reset protects toggle-based dismissal but leaves the animated gesture path able to reuse the stale wl_surface and reproduce the same compositor protocol error; put the reset in the shared hide cleanup or add it to finish_gesture_hide().
Useful? React with 👍 / 👎.
| // Drop the GdkSurface so the next open gets a fresh wl_surface. | ||
| // Cast required: GtkWindow implements GtkNative, so a bare | ||
| // unrealize() binds to gtk_native_unrealize, not the widget one. | ||
| ((Gtk.Widget) this).unrealize(); |
There was a problem hiding this comment.
The fresh-surface behavior is added to the debug remap action, but the production autohide reveal still performs the same hide()/present() remap in animate_dock(false) at lines 1043-1044 without unrealizing; the overview-only close at line 1231 is similarly unprotected. These frequently used paths therefore continue recreating a layer surface over the existing GdkSurface and can still trigger the protocol failure this commit is intended to prevent.
Useful? React with 👍 / 👎.
4031172 to
57554ab
Compare
|
Both findings were correct. Verified each against the source before acting, and the gap turned out to be wider than reported — pushed in Gesture dismissal (overview.vala). Confirmed. Dock remap paths (dock.vala). Confirmed as to substance. Line 1039 is Root cause of both. The patch generator matched only a bare
Neither path was caught by testing because autohide and the four-finger gesture were never exercised in the reproduction. Review caught what testing did not. Worth noting for the reviewer's benefit: this has now missed close paths twice, which is an argument for the library-side fix proposed in wmww/gtk4-layer-shell#130 — clearing the stale buffer inside |
57554ab to
458d4d5
Compare
|
Pushed Gesture dismissal — private void finish_gesture_hide() {
hide();
// Drop the GdkSurface so the next open gets a fresh wl_surface.
((Gtk.Widget) this).unrealize();
PreviewCache.get_default().clear();Dock remaps — all five Totals across the six layer-shell files: 24 close paths, 24 unrealize calls. Was 18. Since this has now missed close paths once and been corrected, it is worth saying what the underlying fix is: I have proposed a library-side change at wmww/gtk4-layer-shell#130 that clears the stale buffer before If that lands, most of this PR becomes redundant. It is still worth having for shells running against the library as it currently ships, and I am happy to reshape it into a shared close helper (as suggested above) if you would rather have one call site than 24 — say the word and I will respin. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 458d4d5af2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Drop the GdkSurface so the next open gets a fresh wl_surface. | ||
| // Cast required: GtkWindow implements GtkNative, so a bare | ||
| // unrealize() binds to gtk_native_unrealize, not the widget one. | ||
| ((Gtk.Widget) this).unrealize(); |
There was a problem hiding this comment.
When bar-layout edit mode is enabled while the sidebar is open, sync_bar_layout_edit_mode() still calls sidebar?.hide() directly at src/core/main.vala:761-764. Because the new unrealize calls cover only Sidebar's internal close sites, this path retains the old GdkSurface; reopening the same sidebar through a panel or dock toggle calls present() and can reproduce the stale wl_surface protocol failure this change is intended to prevent. Route this close through a sidebar method that performs both operations.
Useful? React with 👍 / 👎.
Every layer-shell window in the shell can be killed by the compositor after a
variable number of open/close cycles:
Gdk-Message: Error 71 (Protocol error) dispatching to Wayland display.
wl_display error 2: zwlr_layer_surface_v1 has never been configured
Root cause, from WAYLAND_DEBUG=1 traces. GTK keeps ONE wl_surface alive across
hide/show. gtk4-layer-shell destroys the zwlr_layer_surface_v1 at unmap and
creates a NEW one over that same wl_surface on the next open. Comparing a
working cycle with a crashing one in a single session:
working (#77 -> #64) crashing (#64 -> #82)
destroy() destroy()
attach(nil); commit() attach(nil); commit()
<nothing> attach(wl_buffer#79); commit() <-- stray
get_layer_surface(#64) get_layer_surface(#82)
commit() -> configure OK commit() -> ERROR 2
The only difference is a frame queued by the closing animation that lands
~0.7ms AFTER the unmap and re-attaches a live buffer to the now-roleless
surface. wl_surface state is persistent, so that buffer is still current when
the next layer surface is created; its first commit therefore carries a buffer
before any configure, which wlroots rejects. Whether that frame lands is a race
on wl_buffer.release, which is why the shell died after a VARIABLE number of
opens rather than deterministically.
unrealize() after hide() drops the GdkSurface, so the next open allocates a
brand-new wl_surface that cannot carry a stale buffer. This is the same call
gtk4-layer-shell itself makes in gtk_layer_surface_remap(), which does
gtk_widget_unrealize() then gtk_widget_map() -- a supported, exercised path.
unrealize() on a never-realized widget is a documented no-op, so the
constructor-time hide() in workspace_overview is harmless.
The cast is required: GtkWindow implements GtkNative, so a bare unrealize()
binds to gtk_native_unrealize, an internal vfunc, not gtk_widget_unrealize.
Verified by inspecting the C that valac emits for each spelling.
This is not launcher-specific. An audit found 19 windows calling
init_for_window(), of which six hide and then re-show -- each one a distinct
instance of the same fault. Applied to all 24 close paths in those six files:
dock.vala 7
sidebar.vala 5
app_menu.vala 4
workspace_overview.vala 4
hot_corner_manager.vala 2
overview.vala 2
Note that a close path is spelled either `hide();` or
`((Gtk.Widget) this).hide();`, and both need the same treatment -- dock.vala
uses the cast form for five of its seven, including the production autohide
reveal, which does that then present(): a full remap.
Verified on CIX Sky1 (Radxa Orion O6N, labwc/wlroots, GTK4, GLES on libmali)
with a Wayland trace analyzer that flags a layer surface being re-created over
a wl_surface that still holds a buffer: 1 dangerous re-creation before the
change, 0 across 36 layer surfaces after it. Confirmed by hand over ~30 open
cycles and across a clean reboot, with zero shell restarts.
Upstream gtk4-layer-shell has no fix. Issue #94 is this exact bug (same error,
same "random after N opens", reproduces only on the gl/vulkan renderers) and
was closed because Hyprland changed; PR #119 reordered teardown and was
rejected as a smithay bug. A library-side fix is proposed separately in
wmww/gtk4-layer-shell#130, which would cover every close path in every client
without each one having to remember; this change is the client-side fix for
shells running against the library as it stands.
A shared close helper would be cleaner than 24 call sites; happy to respin that
way if preferred.
458d4d5 to
f453641
Compare
|
I re-checked both P1s from the latest review against the commit it was submitted on (458d4d5) and against the current head (f453641). Both were already fixed at that point — the review re-anchored the two original findings rather than re-reading the files. Quoting the current head so it's checkable: 1. "Unrealize the overview after gesture dismissal" — private void finish_gesture_hide() {
hide();
// Drop the GdkSurface so the next open gets a fresh wl_surface.
// Cast required: GtkWindow implements GtkNative, so a bare
// unrealize() binds to gtk_native_unrealize, not the widget one.
((Gtk.Widget) this).unrealize();Both branches of 2. "Reset the surface on production dock remaps" — the ((Gtk.Widget) this).hide();
// Drop the GdkSurface so the next open gets a fresh wl_surface.
// Cast required: GtkWindow implements GtkNative, so a bare
// unrealize() binds to gtk_native_unrealize, not the widget one.
((Gtk.Widget) this).unrealize();
present();and ((Gtk.Widget) this).hide();
// Drop the GdkSurface so the next open gets a fresh wl_surface.
// Cast required: GtkWindow implements GtkNative, so a bare
// unrealize() binds to gtk_native_unrealize, not the widget one.
((Gtk.Widget) this).unrealize();
present();
For completeness on the third finding, which was real: the external |
|
The bug is valid, but I do not want 24 manual unrealize calls. Please put this behind one shared close helper and trim the repeated comments, then I can merge it <3 |
Replaces 24 open-coded hide()+unrealize() pairs, and the explanatory comment repeated at each of them, with a single close_layer_window() in src/core/layer_window.vala. The rationale now lives once, next to the code that implements it. No behaviour change: the helper performs the same hide() then unrealize() in the same order, including the cast that makes unrealize() bind to gtk_widget_unrealize rather than GtkNative's internal vfunc. Verified every one of the 24 sites resolves to a Gtk.Window: two of them are in hot_corner_manager.vala, whose first class is HotCornerManager : Object, but both sit inside HotCornerHintWindow : Gtk.Window further down the file. overview.vala likewise contains a WorkspaceCard : Gtk.Box, and no call site falls inside it. Net -132/+25 lines across six files.
|
Done in No behaviour change: same Validation: confirmed all 24 sites resolve to a Separately: I reproduced the underlying protocol error standalone, no shell involved — it needs two or more layer windows cycling plus the unmap dispatched after a paint is queued, and it fails 5/5 within four layer surfaces on stock gtk4-layer-shell 1.3.0. Details and reproducer are in wmww/gtk4-layer-shell#130. If that lands, this workaround becomes unnecessary. |
f011b70 to
4730fea
Compare
|
The shared helper is good now. One issue remains before merge: the dock frame-clock pulse must end the same frame clock it started. After hide/unrealize/present, the timeout currently fetches the new clock and can end it twice during a quick pointer leave/re-enter. Please retain the original frame clock in the timeout. Also keep #18 stacked until this lands. |
Closing a layer window now drops the GdkSurface, so the next present allocates a NEW GdkFrameClock. The 350ms timeout re-fetched the clock via get_frame_clock(), which after a hide/unrealize/present cycle is not the clock begin_updating() was called on: the original is left permanently updating, and a quick pointer leave/re-enter can end the new one twice. Capture the clock at begin_updating() time and end that one. The identical re-fetch exists in panel.vala, but the panel never hides or unrealizes and does not go through close_layer_window(), so its frame clock does not change underneath the timeout. Left alone deliberately - it is not reachable from this change.
|
Done in var fc = get_frame_clock();
if (fc == null) return;
fc.begin_updating();
GLib.Timeout.add(350, () => {
fc.end_updating();
return GLib.Source.REMOVE;
});You were right about the mechanism — since this PR drops the The identical re-fetch exists in Pushed as a plain commit rather than a rebase — the branch still merges cleanly onto current |
2 participants
Summary
Every layer-shell window in the shell can be killed by the compositor after a variable number of open/close cycles:
Root cause
GTK keeps ONE
wl_surfacealive across hide/show. gtk4-layer-shell destroys thezwlr_layer_surface_v1at unmap and creates a NEW one over that same surface on the next open. Comparing a working cycle with a crashing one in a single session:The only difference is a frame queued by the closing animation landing ~0.7 ms after the unmap, re-attaching a live buffer to the now-roleless surface.
wl_surfacestate is persistent, so that buffer is still current when the next layer surface is created, and its first commit carries a buffer before any configure — which wlroots rejects. Whether that frame lands is a race onwl_buffer.release, which is why windows died after a variable number of opens.The change
close_layer_window()insrc/core/layer_window.valahides the window and drops itsGdkSurface, so the next open allocates a freshwl_surfacethat cannot carry a stale buffer. This is the same sequence gtk4-layer-shell performs internally ingtk_layer_surface_remap().All 24 close paths across six files call that one helper — the rationale lives once, next to the code that implements it, rather than being repeated at each site:
The cast inside the helper is required:
GtkWindowimplementsGtkNative, so a bareunrealize()binds togtk_native_unrealize, an internal vfunc, notgtk_widget_unrealize. Verified by inspecting the C valac emits for each spelling.Validation
Verified with a Wayland trace analyser that flags a layer surface being re-created over a
wl_surfacethat still holds a buffer: 1 dangerous re-creation before the change, 0 across 36 layer surfaces after it. Confirmed by hand over ~30 open cycles and across a clean reboot, with zero shell restarts. CIX Sky1, labwc/wlroots, GTK4, GLES on Mali.Every one of the 24 sites resolves to a
Gtk.Window: two are inhot_corner_manager.vala, whose first class isHotCornerManager : Object, but both sit insideHotCornerHintWindow : Gtk.Windowlater in the file;overview.valasimilarly contains aWorkspaceCard : Gtk.Boxand no call site falls inside it.Upstream
The underlying protocol error is reproducible without this shell — two layer windows cycling, with the unmap dispatched after a paint is queued, fails 5/5 within four layer surfaces on stock gtk4-layer-shell 1.3.0. Reproducer and analysis: wmww/gtk4-layer-shell#130. If that lands, this workaround becomes unnecessary.