Skip to content

fix(hytte-ui): three more close()-instead-of-destroy() sites, and popup.rs's comment claims the semantics only destroy() provides (#632/#637 blast radius) #638

Description

@vibechoom

Found by PR #637's blast-radius sweep while fixing #632's five sites in trollshell. These three are in hytte-ui, a different crate, so they were deliberately left out of that PR's lane.

Filed with the same honesty as #632: the fix is unconditionally correct, but whether these are live leaks depends on a reachability question I could not settle read-only.

The pattern

Per GTK 4.22's GIR docs, gtk_window_close is "Requests that the window is closed… similar to what happens when a window manager close button is clicked" — a request routed through close-request, which the default handler turns into a destroy for a realized window. gtk_window_destroy is the call documented as "Drops the internal reference GTK holds on toplevel windows." On an unrealized window, close() does not tear anything down.

hytte-ui builds layer-shell windows the same way trollshell does — layer_window.rs:98 says outright "Construct the gtk::Window, wire up layer-shell, but don't show" — so the unrealized case is reachable here in principle too.

The three sites

  1. crates/hytte-ui/src/bar.rs:195BarHandle::close()self.window.close().
  2. crates/hytte-ui/src/bar.rs:208impl Drop for BarHandleself.window.close(). This is the more interesting one: a BarHandle dropped before its bar was ever shown (an error path during monitor setup, or a bar built for a monitor that vanished mid-setup) would hold an unrealized window that close() cannot dispose of.
  3. crates/hytte-ui/src/popup.rs:285close_catcherswin.close() for each drained catcher.

Site 3 has a documentation/implementation mismatch regardless of reachability

popup.rs:283-284's own doc comment describes what the code does as:

…takes them out of the shared cell and close()s them (removing each toplevel from GTK's window list), leaving the other a harmless no-op.

Removing a toplevel from GTK's window list is precisely what destroy() is documented to do and what close() only achieves indirectly, via the default close-request handler, and only when the window is realized. So the comment asserts the stronger semantics while the code uses the weaker call. That is worth fixing even if every catcher happens to be realized in practice.

Worth noting this neighbourhood already had a leak bug: #427, "popup dismiss-catcher can leak a fullscreen invisible click-eater on hot-plug teardown."

What I could not verify

Whether a BarHandle is ever dropped, or a catcher ever closed, while still unrealized. Dismiss-catchers are shown by definition (an invisible click-eater that was never mapped would not catch anything), so site 3 is probably always realized in the happy path — the mismatch there is about the comment and about robustness, not a demonstrated leak. Site 2's error paths I did not enumerate.

Severity

Low. No demonstrated leak, no reporter. The value is removing a dependency on "was this window realized?" from teardown paths where the answer is incidental, in a library crate whose callers may not share trollshell's assumptions.

Recommendation

Switch all three to destroy(), and fix popup.rs's comment to match. destroy() is right for the same three reasons it was right in #632: it works on unrealized windows, it is the documented ref-dropping call, and it cannot be vetoed by a future close-request handler returning true — these are library-owned surfaces with no user-facing close affordance.

BarHandle::close() (site 1) deserves one moment's thought before changing, since it is a pub fn on a public handle: its name promises "close" semantics to downstream callers. Changing the implementation to destroy() is almost certainly what every caller wants, but if any consumer relies on a close-request handler running, that would break silently. A grep of the workspace for close-request / connect_close_request will settle it in a minute.

Sequence after #637 merges, or alongside — different crate, no file conflict, but the same reviewer context is useful.

Refs #632, #637, #427.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions