fix: the RefCell-across-a-GTK-call pattern also lives outside overlays/ — reactive_list.rs, calendar/tasks/connections rows, control-center, hytte-ui popup (#631 blast radius)

[vibechoom]

Found while sweeping #631 (the RefCell-borrow-held-across-a-GTK-call pattern). #631's fix covered nine sites; grepping the same two shapes turns up more, unfixed, outside that issue's file list. Filing rather than fixing — same as how #632/#638 spun out of earlier sweeps.

The pattern (unchanged from #631/#630/#627)

A RefCell borrow held across a GTK call that can emit a signal. If the emission is ever synchronous and the handler borrows the same cell, it panics — and a panic unwinding through a glib callback aborts the process. Reachability is not verified either way for any of these; that's the same standard #631 used, not a downgrade.

Form A — the borrow spans a loop of GTK teardown calls (.remove() / .destroy())

• crates/hytte-ui/src/popup.rs:286 (close_catchers) — for win in catchers.borrow_mut().drain(..) { win.destroy(); }. Notable because it's in hytte-ui, not trollshell, and because fix(hytte-ui): destroy() the three remaining close()-teardown sites (#638) #639 already touched this exact function (converting close() → destroy()) without addressing this borrow.
• trollshell/src/components/reactive_list.rs:88 — for row in rows_track.borrow_mut().drain(..) { container_for_bind.remove_row(&row); }, inside the shared generic list-diff helper (RowContainer/bind_rows). This one backs 8 consumers (panels/{appearance,vpn,clipboard,displays,stats}.rs, panels/network/{wired,wifi,connection}.rs), so it's the highest-blast-radius instance found.
• trollshell/src/panels/stats.rs:632 and trollshell/src/components/power_profile.rs:49 — same shape, each inside a bind() reactive closure with its own hand-rolled rows_track.
• trollshell/src/widgets/calendar.rs:328 — for (_d, row) in rows_track.borrow_mut().drain(..) { group.remove(&row); }.
• trollshell/src/widgets/tasks.rs:139 — for row in rows_track.borrow_mut().drain(..) { group.remove(&row); }.
• trollshell/src/panels/connections.rs:67-72 — two sites, owned/others, structured as a named let mut owned = owned_for_bind.borrow_mut(); bound across the loop rather than an anonymous drain() temporary — same hazard, just spelled with an explicit binding instead of a chained call.
• crates/trollshell-control-center/src/main.rs:449 (clear_rows) — for row in state.rows.borrow_mut().drain(..) { state.group.remove(&row); }. In the control-center binary, not the shell — the pattern isn't scoped to trollshell/src/overlays/.

Form B — if let Some(x) = cell.borrow_mut().take() with a GTK call in the then-block

• trollshell/src/widgets/calendar.rs:331 — if let Some(p) = placeholder_track.borrow_mut().take() { group.remove(&p); }, immediately after the Form A site above in the same function.
• trollshell/src/widgets/tasks.rs:142 and :145 — placeholder_track and overflow_track, same shape, same function as the Form A site above.
• trollshell/src/panels/connections.rs:75 — overflow_for_bind, same shape, same function as the two Form A sites above.

Not sites (checked, ruled out)

Grepping .borrow_mut().take() and .borrow_mut().drain() more broadly also turns up several look-alikes that are not hazards, for the same reason #631 excluded prompt.rs's abort_subscription (no GTK call in the body) or because they use let-else (whose scrutinee temporary drops at the end of the let statement, not across a following GTK call, unlike a same-statement if let then-block):

• trollshell/src/overlays/fullscreen.rs:60, trollshell/src/components/focus.rs:146, crates/trollshell-control-center/src/main.rs:114, trollshell/src/plugins/mod.rs:651, crates/hytte-ui/src/app.rs:80,95 — each either aborts a task/timer (no GTK) or uses let-else.

Recommendation

Same as #631: for the Form A sites, prefer RefCell::take() (or, where the container also needs post-drain access like connections.rs's named bindings, bind-then-loop) over holding the borrow across the removal calls; for Form B, bind-then-act. One mechanical PR per the cluster convention, reactive_list.rs first given its blast radius.

Refs #631, #630, #627, #632, #638.

[vibechoom]

Do not execute this issue as written. An adversarial review of PR #644 — run specifically to validate the pattern before replicating it here — found that the sweep definition is narrower than the hazard. Converting 12+ sites under the current definition would leave half the hazard behind and declare the files clean.

I verified the load-bearing findings against source before rewriting this.

The definition is wrong: it's any borrow held across a GTK call, not borrow_mut()

Both #631 and this issue describe the pattern as borrow_mut() held across a GTK call. That grep provably misses three other spellings — all three present in the seven files #644 already edits:

(a) A named binding rather than a chained temporary. overlays/notifications.rs:227 (on #644's branch):

let mut map = view.card_map.borrow_mut();

held live to :307, across view.vbox.remove(&entry.widget) (:252), replace_card/mount_card (:275-276), further remove/append (:293-300), and set_visible (:307).

This is the most important site in the whole cluster, because the synchronicity question every other site leaves open is settled in the affirmative here, in-tree. notifications.rs:259 says destroying a card "fires connect_unmap", and :527 says vbox.remove(&old.widget) "Fires old's connect_unmap → resume_expiry". #593's entire keep-the-card-mounted design is built on that being synchronous.

So #644 fixes nine sites where emission is unverified while leaving untouched the one site where the tree documents that the call does emit. It doesn't panic today only because that handler touches resume_expiry rather than card_map — which is exactly the "no known reentrant borrower" standard the nine fixed sites were held to. By this issue's own standard it qualifies.

(b) A shared borrow() held across GTK work, where a borrow_mut() exists on another path. notifications.rs:202-214 (TOAST_WINDOWS.borrow() across apply_emission), osd.rs:416-431 (OSDS.borrow() across show()), and modal.rs:1108/1124/1135/1152/1170/1190/1210/1253/1373 (PANELS.borrow() across set_stack_page / set_reveal_child / show_panel / on_page_show). close_all and install are the borrow_mut() counterparties.

(c) An argument-position temporary. notifications.rs:306-307:

view.window.set_visible(!map.is_empty() || view.overflow_card.borrow().is_some());

The Ref is a statement temporary, alive across set_visible.

The install insert-drop shape exists at four more sites, and merging #644 makes them less visible

#644 adds a twelve-line comment to modal.rs:570-580 explaining why dropping a replaced value inside a borrow is a hazard. The identical shape survives, unannotated, at:

• overlays/sidebar.rs:242 — drops Option<SidebarPanel> (a gtk::Window, a gtk::Revealer, a Mutable<bool>, two JoinHandles) inside PANELS.borrow_mut()
• overlays/frame.rs:171 — drops Option<FrameView> inside FRAMES.borrow_mut()
• overlays/osd.rs:172 — drops Option<Rc<OsdView>> inside OSDS.borrow_mut()
• overlays/consent.rs:71 — drops Option<Monitor> inside MONITORS.borrow_mut() (weakest; a GdkMonitor unref emits nothing)

Reachability is identical to the site that was fixed. Three of the four are in files #644 edits.

The semicolon rule — write this down or the sweep will be applied wrongly

notifications.rs:134 is the same call:

TOAST_WINDOWS.with(|map| map.borrow_mut().insert(connector, view));

and it is safe — purely because it is a closure tail expression, so the returned Option<ToastView> is moved out past the RefMut. Add a semicolon and it becomes a statement temporary dropped inside the borrow.

Safety here turns on a semicolon. Anyone mechanically applying drop(X.with(...)) across these sites will either wrap safe ones needlessly or "fix" one, see the file look clean, and move on. This distinction appears nowhere in #631, #644, or this issue's original body.

Also worth carrying

A hermetic test is feasible for any site whose reentrancy trigger is a Mutable wake rather than a GTK call. futures-signals invokes Waker::wake() synchronously from notify (mutable.rs:66-72), so a RawWaker whose wake re-enters the cell, plus one poll of a pinned MutableSignal, reproduces the panic with pure std and no display. #644 is adding exactly this for reset_drawer_open_states. Use it wherever it applies here rather than defaulting to "not testable".

prompt.rs/consent.rs keeping .close() was correct, not an oversight — their windows are realized before storage (prompt.rs:225-226, consent.rs:244-245), so #632's unrealized-early-return argument doesn't apply. One residual nobody has settled: on hot-plug these run after the output may have vanished, and if zwlr_layer_surface_v1.closed leaves the window unrealized, close() early-returns with the slot already take()n and nothing left to destroy it. Worth checking gtk4-layer-shell's closed handling when someone is in there.

Revised scope

1. Correct the definition to any borrow across a GTK call, and enumerate all four spellings.
2. Add the four install insert sites.
3. Add the notifications.rs named-binding cluster — highest priority, since it is the only site with documented synchronous emission.
4. Record the semicolon rule with notifications.rs:134 as the worked counter-example.
5. Use the Waker-based hermetic test wherever the trigger is a Mutable wake.

Still start with components/reactive_list.rs:88 among the originally-listed sites — it backs eight panels — but the notifications cluster now outranks it on evidence.

Refs #644, #631, #630, #632, #593.

[vibechoom]

The non-overlay half of this issue shipped as PR #663 (Refs, not Closes — the overlay half below remains). It fixed 22 sites across the eight in-scope files; this issue listed 12, and applying the corrected four-spelling definition turned up 10 more in those same files.

The cluster's central unverified question is now answered — for one call, empirically

Every issue in this cluster (#627, #630, #631, #643, and PR #644) has carried the same honest caveat: reachability is not verified either way, and a BorrowMutError panicking through a glib callback would abort the process. Both halves of that were reasoning, not measurement.

PR #663 added a gated GTK test that reproduces the abort for real, and falsified it against the pre-fix code:

RefCell already borrowed → panic in a function that cannot unwind
→ thread caused non-unwinding panic. aborting. → SIGABRT

So two things are now established rather than assumed:

1. GtkWidget::destroy emits connect_destroy synchronously (from dispose), so a handler re-entering the same cell hits a live borrow.
2. The resulting BorrowMutError really does abort the process, not merely fail the operation. The unwind-through-FFI reasoning was correct.

Being precise about the scope of that result, because it would be easy to over-read: this was demonstrated for destroy. It does not transitively prove synchronous emission for remove(), append(), set_visible(), or set_active(). What it removes is the doubt about the consequence — we no longer need to hedge on whether one of these panics would be survivable. It wouldn't be. That raises the value of fixing the remaining sites even where the emission is still unproven.

Related: the parent comment suggested a Waker-based hermetic harness. It does not apply here — no site in the non-overlay half touches a Mutable inside the borrow; every trigger is a GTK emission. The destroy route was the right substitute. Worth knowing before anyone reaches for the Waker shape on the remaining sites.

Two sites worth calling out from the extra ten

• crates/trollshell-control-center/src/main.rs:427 — the strongest-evidence site in that PR. state.by_id.borrow() held across update_plugin_row, which ends in set_active; GObject property-notify is synchronous, so the row's own connect_active_notify handler runs inside that call. It survives today only because that handler early-returns on the syncing guard — exactly the "no known reentrant borrower" standard the rest of the cluster was held to.
• trollshell/src/panels/stats.rs:756 (build_live_per_core_row) — a named RefMut held live across remove(), append(), set_fraction() and set_tooltip_text(). Spelling (2). This issue's original body missed it.

What remains here, and a correction about #644

Still open, and deliberately deferred because PR #644 owns those files:

• the notifications.rs:227 named-binding cluster — highest priority, since notifications.rs:259/:527 document that emission as synchronous in-tree
• the four install insert-drop sites: sidebar.rs:242, frame.rs:171, osd.rs:172, consent.rs:71
• the spelling-(3) shared-borrow() sites across modal.rs, osd.rs, notifications.rs

A correction to how that gap gets described. It would be easy to read the above as "#644 didn't finish its own job" — it isn't that. #644 was built against #631's nine sites and did exactly those. The additional sites were discovered by the adversarial review of #644 itself, and were filed into this issue afterwards. They were never in that PR's scope, and its +6/-1 diffs on frame/notifications/osd/sidebar are what its nine mechanical take() conversions are supposed to look like.

The operative consequence is unchanged and worth stating plainly: this issue must not be closed when #644 merges. Dispatching the remainder is sequenced behind that merge purely to avoid a file collision.

[vibechoom]

An adversarial review of PR #663 found the 22 conversions sound — but it also found that the claim I relayed in my previous comment is false. Correcting it here, because that comment is what whoever picks up the overlay half will read.

Correction: control-center/src/main.rs is NOT "the strongest-evidence site"

I wrote that state.by_id.borrow() is held across update_plugin_row → set_active, that the row's connect_active_notify therefore runs inside that call, and that the site survives only because the handler early-returns on the syncing guard.

The last part is wrong, and I verified it against source rather than taking the review's word for it. The handler (crates/trollshell-control-center/src/main.rs:493-505) does this:

row.connect_active_notify(move |sw| {
    if state.syncing.get() { return; }
    let want_on = sw.is_active();
    let (state, id) = (state.clone(), id.clone());
    spawn_on_runtime(set_plugin_state(id, want_on), move |res| { … refresh_plugins_soon(&state); });
});

spawn_on_runtime (:891-906) spawns onto the tokio runtime and delivers its continuation through a oneshot awaited inside glib::spawn_future_local. Everything the handler does is deferred to a later main-loop iteration. So even with the syncing guard removed, it would never touch state.by_id while the borrow is live, and no BorrowMutError would occur.

That site is exactly as speculative as the other 21 — no better evidenced, and no worse. The syncing guard is there to stop a programmatic set_active from firing a spurious Start/Stop, not to prevent a borrow collision. I should not have promoted it to "strongest evidence" on a single reading.

The site was still worth converting — the cluster's standard is deliberately "fix it even where emission is unverified" — so nothing about PR #663 changes. Only the justification does.

A live site PR #663 missed, inside a range its body certifies as clean

trollshell/src/widgets/tasks.rs:688 (refresh_summary) is a spelling-(3) site that survives on the PR branch:

match *self.mode.borrow() {
    …
    DueMode::Pick => {
        let label = self.selected.borrow().map_or_else(…);
        self.summary_label.set_visible(true);
        self.summary_label.set_text(&label);

The Ref from *self.mode.borrow() is held across every arm's set_visible / set_text, and there are three borrow_mut() counterparties on the same cell (:667, :748, :769). By this issue's own four-spelling definition it qualifies. Reachability is unverified either way, same as everywhere else in this cluster.

Add it to the remaining scope.

Three audit-trail corrections, so the next pass isn't misled

• The do_create exclusion's stated reason is wrong. drop(lists) at tasks.rs:399 comes after DropDown::selected() at :394, so the exclusion is only correct on getter-versus-setter grounds — which the four-spelling definition never states. If getters are categorically safe, that belongs in the definition explicitly; right now it is folklore.
• The let displaced hazard note describes a widget teardown; it is actually a refcount decrement. Different mechanism, different reachability argument.
• stats.rs:645 appears in neither the fixed list nor the ruled-out list. It should be in one of them.

The new hytte-ui test — genuine, but its doc comment overstates

close_catchers_tolerates_a_reentrant_teardown_from_destroy really does falsify against the pre-fix code, and the SIGABRT result stands: destroy emits synchronously from dispose, and a BorrowMutError through a glib callback does abort the process. That part of my previous comment holds.

What does not hold is any implication that it demonstrates a reachable bug in popup.rs. Nothing in the tree installs a connect_destroy on a catcher window. The test is a synthetic probe of the mechanism — which is exactly what the cluster needed, since the mechanism was the open question — but it is not evidence that this particular site was live. Worth stating plainly so the result gets cited for what it proves.

What still stands

The structural finding that makes the whole conversion safe: bind's apply loop (crates/hytte-reactive/src/bind.rs:38-46) runs inside a glib::MainContext::spawn_local poll and cannot re-enter itself. That rules out the entire "nested emission repopulates the cell, then the write-back clobbers it" failure family for every bind-driven site in the PR — which is most of them.

Refs #663, #644, #631.