From 97fb7289d02412ad9c6d065c558d984bcc37d51f Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 14:12:02 +0200 Subject: [PATCH 1/2] fix(modal): drop the PANELS borrow before closing drawer windows (#627) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit close_all held panels.borrow_mut() (the drain() temporary) across every panel.window.close() call in its loop. wire_retract_finish's child-revealed handler takes its own panels.borrow() on the same RefCell; if a close() ever emitted that signal synchronously, the handler would run inside the still-active borrow_mut() and panic — and a panic unwinding through a glib callback aborts the process. Believed dormant today only because of a GTK-internals assumption documented in a different module (overlays/sidebar.rs's note on a closed window's revealer never settling), which is a fragile thing to lean on. Fixed per the issue's proposal: drain PANELS into a local Vec, letting the borrow end before any window is closed, then close each owned panel outside the RefCell. Verified the #624 reset_drawer_open_states() ordering still holds — every panel is dropped (via the Vec's consuming for-loop) before reset_drawer_open_states() runs, same as before, so its "clear before prune" invariant is untouched. The plugin-panel clear and recompute_gates afterward are unaffected; they don't touch PANELS's borrow state. Closes #627 Co-Authored-By: Claude Opus 5 (1M context) --- trollshell/src/modal.rs | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/trollshell/src/modal.rs b/trollshell/src/modal.rs index 6df45d5..b4adad3 100644 --- a/trollshell/src/modal.rs +++ b/trollshell/src/modal.rs @@ -973,12 +973,24 @@ fn wire_retract_finish(revealer: >k::Revealer, key: String) { } /// Close and remove all drawers (called before rebuilding bars on hot-plug). +/// +/// Drains `PANELS` into a local `Vec` and drops the borrow *before* calling +/// [`gtk::Window::close`] on each entry, rather than closing from inside the +/// `borrow_mut()` (#627). `wire_retract_finish`'s `child-revealed` handler +/// takes its own `borrow()` of the same `RefCell`; if a `close()` ever +/// triggered that signal synchronously, running it under an active +/// `borrow_mut()` would panic, and a panic unwinding through a glib callback +/// aborts the process. Believed unreachable today (see +/// `overlays::sidebar.rs`'s note on a closed window's revealer never +/// settling), but that's a GTK-internals assumption documented in a +/// different module — not worth leaving as a load-bearing dependency here +/// when not holding the borrow is free. pub fn close_all() { - PANELS.with(|panels| { - for (_, panel) in panels.borrow_mut().drain() { - panel.window.close(); - } - }); + let panels: Vec<_> = + PANELS.with(|panels| panels.borrow_mut().drain().map(|(_, v)| v).collect()); + for panel in panels { + panel.window.close(); + } reset_drawer_open_states(); // A monitor teardown that held the open plugin panel must clear the // selection too, so the v1 "hot-unplug just closes the plugin page with the From b7e0760d3aaf2b3c19da3f12ed7e50caf9f041bd Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 14:58:43 +0200 Subject: [PATCH 2/2] docs(modal): correct the dormancy claim, note the new None branch (#627) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up on #630, comment-only: - close_all's doc cited overlays/sidebar.rs's settle-timer note as grounds for the hazard being dormant. That note is about a polling loop never terminating, not about whether close() emits notify::child-revealed synchronously — the actual open question. And overlays/notifications.rs documents a case where a destroy does fire a signal synchronously, so the citation oversold what's known. Reworded to "not verified either way" and dropped the citation; fixed the `overlays::sidebar.rs` malformed path/module mix in the process, and switched the `gtk::Window::close` intra-doc link to a plain backtick since `close` lives on the GtkWindowExt trait and may not resolve. - Added a comment at wire_retract_finish's `None` branch: draining PANELS before any close() now means a synchronous re-entrant emission always lands here (the key is already gone), which is benign because close_all's own tail duplicates everything the `Some` arm would have done. No code change; gates re-run (clippy clean, nix fmt 0 changed x2, the 14 modal:: tests unaffected). Co-Authored-By: Claude Opus 5 (1M context) --- trollshell/src/modal.rs | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/trollshell/src/modal.rs b/trollshell/src/modal.rs index b4adad3..df4caa4 100644 --- a/trollshell/src/modal.rs +++ b/trollshell/src/modal.rs @@ -950,6 +950,15 @@ fn wire_retract_finish(revealer: >k::Revealer, key: String) { } let closed_plugin_panel = PANELS.with(|panels| { let panels = panels.borrow(); + // Post-#627, `close_all` drains every entry out of `PANELS` before + // closing any window, so a synchronous `child-revealed` emission + // during one of those closes now always lands here (this key is + // already gone). Benign: `close_all`'s own tail performs + // everything the `Some` arm below would have — + // `set_visible(false)` on a window that's being dropped anyway, + // `*current = None`, `open_state.set(false)`, and + // (unconditionally) `set_active_panel(None)` — so returning + // `false` here just skips redundant work, not a missed teardown. let Some(panel) = panels.get(&key) else { return false; }; @@ -975,16 +984,16 @@ fn wire_retract_finish(revealer: >k::Revealer, key: String) { /// Close and remove all drawers (called before rebuilding bars on hot-plug). /// /// Drains `PANELS` into a local `Vec` and drops the borrow *before* calling -/// [`gtk::Window::close`] on each entry, rather than closing from inside the +/// `gtk::Window::close` on each entry, rather than closing from inside the /// `borrow_mut()` (#627). `wire_retract_finish`'s `child-revealed` handler -/// takes its own `borrow()` of the same `RefCell`; if a `close()` ever +/// takes its own `borrow()` of the same `RefCell`; if `close()` ever /// triggered that signal synchronously, running it under an active /// `borrow_mut()` would panic, and a panic unwinding through a glib callback -/// aborts the process. Believed unreachable today (see -/// `overlays::sidebar.rs`'s note on a closed window's revealer never -/// settling), but that's a GTK-internals assumption documented in a -/// different module — not worth leaving as a load-bearing dependency here -/// when not holding the borrow is free. +/// aborts the process. Whether `gtk_window_close`/`gtk_revealer_unmap` can +/// emit `notify::child-revealed` synchronously is not verified either way — +/// draining first removes the dependency on that question entirely, which is +/// the point: not holding the borrow is free, so there's no need to resolve +/// it first. pub fn close_all() { let panels: Vec<_> = PANELS.with(|panels| panels.borrow_mut().drain().map(|(_, v)| v).collect());