fix(modal): drop the PANELS borrow before closing drawer windows (#627) by vibechoom · Pull Request #630 · vibec0re/trollshell

vibechoom · 2026-07-30T12:12:24Z

Closes #627

Mechanism

close_all held panels.borrow_mut() (the drain() temporary) across every panel.window.close() call in its loop, while wire_retract_finish's child-revealed handler takes its own panels.borrow() of the same RefCell — if close() ever emitted that signal synchronously, the handler would run inside the still-active borrow_mut() and panic, which aborts the process (a panic unwinding through a glib callback). Fixed by draining PANELS into a local Vec first (dropping the borrow), then closing each owned panel outside the RefCell — strictly safer, no behavior change, matching the issue's proposed shape.

Verified the #624 reset_drawer_open_states() ordering is untouched: every panel is still dropped (via the Vec's consuming for loop) before reset_drawer_open_states() runs, exactly as before, so its documented "clear before prune" invariant holds. The plugin-panel clear and recompute_gates() afterward don't touch PANELS's borrow state, so they're unaffected by the reordering.

Blast-radius findings (step 3 — grepped `modal.rs` and `overlays/`, fixed only `modal.rs` per scope)

The identical shape (map.borrow_mut().drain() held across a .window.close() loop) also exists, unfixed, in:

trollshell/src/overlays/sidebar.rs::close_all (lines 594-621)
trollshell/src/overlays/frame.rs::close_all (lines 186-197)
trollshell/src/overlays/notifications.rs::close_all (lines 151-157)
trollshell/src/overlays/osd.rs::close_all (lines 192-209)

For each of these, I found no signal handler that currently takes a competing borrow()/borrow_mut() of that module's own map, so — like the modal.rs case before this fix — they appear dormant today, resting on the same overlays/sidebar.rs GTK-timing assumption. Not fixed here; flagging for a follow-up decision since other files may be claimed by other in-flight work.

A weaker, different-shaped instance: overlays/prompt.rs::close_prompt and two sites in overlays/consent.rs::close_all/request use if let Some(x) = cell.borrow_mut().take() { x.close(); } — Rust's temporary-lifetime-extension rule for if let scrutinees keeps the RefMut alive for the whole arm, so the borrow is technically still held across .close() even though the value itself was already moved out. No known reentrant borrower today either; noted for completeness rather than fixed.

Test

No test added. A hermetic reproduction isn't feasible — the hazard requires GTK to emit child-revealed synchronously from inside gtk_window_close, which can't be triggered on demand outside a live GTK main loop / display server, and the existing close_all_resets_drawer_open_state test already runs close_all against an empty PANELS (no display needed) so it doesn't exercise the borrow-timing question either way. Confirmed the existing test suite (cargo test -p trollshell, 233 tests) still passes unchanged.

Gates run

cargo clippy -p trollshell --all-targets -- -D warnings (RUSTC_WRAPPER unset) — clean
cargo test -p trollshell — 233 passed
nix fmt twice — 1 file changed then 0 changed

🤖 Generated with Claude Code

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) <[email protected]>

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) <[email protected]>

…630/#634 (#636) PR #634 (closes #613) makes two existing entries actively wrong rather than merely stale, which is worse than a gap: a verifier following the old text would read a successful self-heal as a regression. Corrects both, per #635: - The #610 "restart NetworkManager" caveat no longer says the backend probe runs once at startup; it retries while inconclusive. The deferred re-probe gap now points at #633 (the genuinely-unsolved half — switching backends after one is already committed) instead of the closed #613. - The #609 entry no longer expects an `error!` pointing at `systemctl --user restart trollshell` on a transient probe failure. The shipped retry policy is unbounded and self-heals into a `RECOVERED` line instead of giving up. Also folds in the three PRs that merged alongside #634 since the doc's last refresh (#628): - #629 (closes #614): docs/plugin-env.md publishing to the Pages options-doc site, new "Documentation site (GitHub Pages)" section (deploy itself is outside `nix flake check` by design — flagged as a manual check). - #630 (closes #627): modal::close_all's borrow-timing fix, added to Shell chrome as a regression-only check — explicitly states that no observable change is the pass condition. - #634 itself: new Wi-Fi entry covering the INCONCLUSIVE/RECOVERED journal pair and the incidental startup-freeze fix. - #628: docs-only refresh of this file, noted in the closing section. Coverage line bumped to #458 through #634. Documentation-only change. Co-authored-by: Claude Opus 5 (1M context) <[email protected]>

…e-enter them (#631) (#644) * fix(overlays): release RefCell borrows before the GTK call that can re-enter them (#631) A RefCell borrow held across a GTK call (close()/destroy(), or a futures-signals set_neq() that wakes a subscriber) is a latent reentrancy hazard: if the emission is ever synchronous and a handler borrows the same cell, it panics, and a panic unwinding through a glib callback aborts the process. PR #630 fixed the first instance in modal::close_all; this sweeps the rest. Form A (borrow_mut().drain() held across the loop's teardown call) in sidebar.rs, frame.rs, notifications.rs, osd.rs: switched to RefCell::take(), which moves the whole map out (leaving Default) and releases the borrow inside the call — no intermediate Vec, and keeps keys where the loop needs them (sidebar.rs). Form B (if let Some(x) = cell.borrow_mut().take() with a GTK call in the then-block) in prompt.rs's close_prompt and consent.rs's close_all plus its supersede-in-request site: bind the taken value first, then act on it outside the borrow. Rust 2024 only changed when an if let scrutinee's temporary drops relative to an else block; the then-block lifetime this hazard depends on is unaffected. Two more in modal.rs the original survey missed: - reset_drawer_open_states held DRAWER_OPEN borrowed across set_neq() for every entry; now collects the Mutable handles first and calls set_neq() on them with no borrow held. - install's panels.borrow_mut().insert(...) dropped the replaced Option<ModalPanel> as a same-statement temporary of the borrow_mut() RefMut — statement temporaries drop in reverse creation order, so the old panel dropped before the RefMut, i.e. still inside the borrow. Wrapped in drop(PANELS.with(...)) so it drops after with() returns. prompt.rs's abort_subscription has the same take()-shape but aborts a tokio AbortHandle, not GTK — deliberately left alone, as the issue calls out. Reachability is unverified either way, not dormant: sidebar.rs's settle-timer note is about a polling loop, not signal synchronicity, and notifications.rs documents a case where a signal (connect_unmap) does fire synchronously from inside a teardown. This removes the dependency on GTK's emission timing regardless of which way that resolves. No hermetic test: reproducing the hazard needs GTK to emit a signal synchronously from inside destroy()/close() against a real display, and trollshell has no display-test harness to fake that. Also grepped for the same two shapes outside this issue's site list and found more, unfixed — filed as #643 rather than fixed here. Closes #631 Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> * fix(modal): correct a stale doc claim + add the one hermetically-testable reentrancy site (#631 follow-up) Adversarial review of #644 confirmed reset_drawer_open_states is correct, but flagged two gaps: 1. The doc comment on close_all/reset_drawer_open_states still said "the PANELS drain above has already dropped every ModalPanel::open_state, so by the time we run, this map holds the last Mutable for each key" — true of the pre-#631 shape, false now. Cloning into `states` before set_neq bumps senders 1->2, so the map's copy is never the last survivor: retain's drop of it is an ordinary decrement, and the actual last-sender drop happens only when `states` goes out of scope at the end of the function, after both the reset and the prune, with no DRAWER_OPEN borrow held. Rewrote the comment to say that, and to note explicitly that this is the safe direction relative to the pre-#631 shape (whose last-sender drop happened at retain, inside the borrow). 2. reset_drawer_open_states has no GTK in it at all — its reentrancy trigger is plain futures-signals Waker::wake(), invoked synchronously from Mutable::set_neq's notify. That's testable with nothing but std::task: a std::task::Wake impl (not a raw RawWaker/RawWakerVTable — the workspace forbids unsafe_code project-wide, and Wake's Arc<W> -> Waker conversion is the safe, stable route) whose wake() re-enters DRAWER_OPEN, registered on a signal for one key, then reset_drawer_open_states run under catch_unwind. Falsified against the pre-#631 shape before restoring: it doesn't just fail the assert. The BorrowMutError panics through Mutable's internal Mutex mid-notify, poisoning it; catch_unwind catches that first panic, but the poisoned Mutable is still sitting in DRAWER_OPEN, and the worker thread's later TLS teardown re-panics dropping it -- from inside a destructor, where an escaping panic is fatal. `cargo test -p trollshell modal::` aborts the whole binary (SIGABRT), not just one test failing. Restored the fix: 234 tests pass (233 + this one), clippy clean, nix fmt 0 changes. This is the one site in the #631 sweep whose regression would otherwise be silent -- every other test in the module would keep passing while this function quietly reopened the reentrancy hazard. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> --------- Co-authored-by: Claude Opus 5 (1M context) <[email protected]>

…urst falsified (#672) Closes #660 Bring docs/live-verify.md current across the ten PRs that merged after the last refresh (#634): #637, #639, #644, #645, #662, #663, #664, #666, #668, plus #642 (coverage bump only, per the issue's own conclusion — its default station-id fix needs no live-verify caveat since write_default_config only touches an absent places.toml). Corrected the #630 entry: it told a verifier to expect modal::close_all tearing down with window.close(); #637 changed that call to destroy() after #630 had already merged, so the entry named a call the code no longer makes (exactly the issue's finding). Also added a new #645 entry to the Network panel section for the networkd startup-refresh retry, since that path used to latch "no link manager has answered yet" forever on a transient first-refresh failure (curable only by a shell restart) and now self-heals — swept the rest of the doc for the same class of falsified expectation (inert-until-restart) and found nothing else affected; the wifi-side #609/#634 entries describe a different code path and are unaffected. New entries added: - #637/#639 — the destroy()-not-close() sweep across the five trollshell overlays and the three remaining hytte-ui sites (bar.rs, popup.rs), including #637's WeakRef/toplevels()-count kanshi hot-plug gesture. - #644/#663 — the RefCell-borrow-across-GTK-call sweep (31 sites total across overlays/modal.rs and the rest of the UI, incl. an 8-panel-wide reactive_list.rs helper and the control-center). Both documented with the same "nothing changes" honest pass condition #630 already used. - #664 — mpris Auto chip fix + bare-string artist widening. Needs two live MPRIS players and a shell restart for the new CSS class, both called out. - #666 — new top-level "Claude bridge" section for the keyless loopback shim, nobody has run this service live yet. Carries the PR's flagged riskiest assumption (hive-claude's SessionNotFound classification) with its journal tell. - #668 — new top-level "D-Bus name ownership" section. Documents that recovery going from ~250ms to up to 5 minutes after a squatter exits is the expected cost of the fix, not a regression to file. Left alone: #642 (no doc change needed beyond the coverage line, matching the issue's own recommendation after checking places.toml's write-once behavior); #637's abort_subscription/prompt.rs exclusions and #644/#663's "deliberately not touched" sites (no live-visible behavior, nothing to verify). Gates: markdown-only diff (git diff --stat: 1 file, docs/live-verify.md). nix fmt run twice (1 file reformatted first pass — a prose reflow — 0 changed second pass). Clippy/tests deliberately skipped: nix/package.nix's crane filter excludes markdown, so the packaged derivation is unaffected and a nix build would burn a full workspace recompile for zero signal (the same crane-filter trap #662 already documented). nix build was not run, per instructions. Co-authored-by: Claude Opus 5 (1M context) <[email protected]>

…uster, install inserts, shared borrows (#643) (#673) The overlay half of #643, sequenced behind #644/#663 which owned these files. 22 sites, all four spellings of the corrected definition (*any* borrow held across a GTK call, not just `borrow_mut()`). notifications.rs `apply_emission` is the headline: two named `RefMut`s plus the overflow slot held live across `vbox.remove`, `replace_card`/ `mount_card`, the overflow `remove`/`append` and the closing `set_visible`, whose argument was itself an `overflow_card.borrow()` temporary. It is the one site in the cluster whose emission the tree *documents* as synchronous (`connect_unmap` → `resume_expiry`, the mechanism #593's design rests on); it survives today only because that handler touches the expiry bookkeeping rather than `card_map`. Converted with take-and-restore, the `panels/stats.rs` idiom from #663. `TOAST_WINDOWS` and `PANELS` (modal) now store `Rc<_>` so `route_emission` and modal's nine entry points can clone a handle out instead of holding a shared borrow across `show_panel` / `set_stack_page` / `set_reveal_child` / `on_page_show` — mirroring `overlays::osd`'s existing `Rc<OsdView>`. `modal::live_panel`/`live_panels` are the snapshot helpers. The four `install` insert-drop sites (sidebar/frame/osd/consent) get the tail-expression + `drop(...)` shape #644 annotated at `modal::install`. Their comments state the mechanism precisely per the #663 review's correction: a refcount decrement, not a widget teardown. consent's is the weakest of the four — a `GdkMonitor` unref emits nothing — and says so. Also `tasks.rs::refresh_summary` (a `match *self.mode.borrow()` holding across every arm's `set_visible`/`set_text`, which #663 missed inside a range its body certified clean), `tasks.rs::do_create`'s getter-ordering folklore, and four sites the closing re-grep turned up in `widgets/{workspaces,tray,window_list}.rs`. No new tests: `trollshell` is a binary crate with no `system-tests`/xvfb bucket, and every site needs a live GTK widget tree to reach. The `Waker`-based harness doesn't apply — the two sites here with a `Mutable` wake inside the borrow still need a populated `PANELS`, i.e. GTK windows. Closes #643. Refs #663, #644, #631, #630, #627, #593. Co-authored-by: Claude Opus 5 (1M context) <[email protected]>

vibechoom merged commit aaba27a into main Jul 30, 2026
1 check passed

This was referenced Jul 30, 2026

docs(live-verify): correct two entries #634 made false, fold in #628-#630/#634 #636

Merged

fix(overlays): destroy unrealized layer windows on teardown, not close() (#632) #637

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(modal): drop the PANELS borrow before closing drawer windows (#627) - #630

fix(modal): drop the PANELS borrow before closing drawer windows (#627)#630
vibechoom merged 2 commits into
mainfrom
fix/modal-borrow-across-close-627

vibechoom commented Jul 30, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

vibechoom commented Jul 30, 2026

Mechanism

Blast-radius findings (step 3 — grepped modal.rs and overlays/, fixed only modal.rs per scope)

Test

Gates run

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Blast-radius findings (step 3 — grepped `modal.rs` and `overlays/`, fixed only `modal.rs` per scope)