fix: propagate unwind safety to user callbacks across all gloo crates#562
Conversation
wasm-bindgen 0.2.117+ added `MaybeUnwindSafe` bounds on `Closure::wrap` and `Closure::once`, because closures handed to JS are invoked across a `catch_unwind` boundary under `panic = "unwind"`. `gloo-timers` accepts arbitrary user `FnOnce` / `FnMut` callbacks, but its `Closure::wrap(...)` / `Closure::once(...)` calls fail to compile under panic=unwind because the `Box::new(callback) as Box<dyn FnMut()>` coercion (and the `FnOnce` trait selection) erase the static `UnwindSafe` bound that wasm-bindgen requires. This breaks every downstream crate that pulls in `gloo-timers` as a `dev-dependency` and tests under `-Cpanic=unwind` \u2014 see the failure on MattiasBuelens/wasm-streams#35. Fix: - Surface the requirement at the public API: `Timeout::new` / `Interval::new` now require `F: CallbackUnwindSafe`, a marker that resolves to `std::panic::UnwindSafe` under `panic = "unwind"` on wasm and to a no-op blanket otherwise. Callers with non-`UnwindSafe` captures must wrap them in `std::panic::AssertUnwindSafe` at the call site, which is where the invariants can actually be reasoned about. - Internally use `Closure::wrap_assert_unwind_safe` / `Closure::once_assert_unwind_safe` under panic=unwind to acknowledge the dyn-erasure explicitly. The public bound has already enforced the requirement at the call site, so the internal assertion is sound. - Bump the minimum `wasm-bindgen` requirement to `0.2.117` (where the `_assert_unwind_safe` helpers were added). - Add a `panic_unwind_build` CI job that builds `gloo-timers` with `-Cpanic=unwind` so this regression cannot recur silently.
cd8cde8 to
82b17da
Compare
Madoshakalaka
left a comment
There was a problem hiding this comment.
The futures feature is broken now:
RUSTFLAGS='-Cpanic=unwind' cargo +nightly check -p gloo-timers --features futures --target wasm32-unknown-unknown -Zbuild-std=std,panic_unwind
fails with
error[E0277]: the type `UnsafeCell<Option<Waker>>` may contain interior mutability and a reference
may not be safely transferrable across a catch_unwind boundary
--> crates/timers/src/future.rs:143:43
...
note: required for `{closure@crates/timers/src/future.rs:143:43: 143:50}`
to implement `CallbackUnwindSafe`
The CI job runs cargo build -p gloo-timers --target wasm32-unknown-unknown -Zbuild-std=std,panic_unwind with default features only so this went undetected.
The previous commit propagated `CallbackUnwindSafe` through `Timeout::new` / `Interval::new`, but the `futures` module's own closures capture `oneshot::Sender` and `mpsc::UnboundedSender`, both of which hold an `Arc<Inner<T>>` with `UnsafeCell` interior and so are not `UnwindSafe`. Building `gloo-timers --features futures` under `-Cpanic=unwind` failed against the new bound; the CI job only exercised default features so the regression went undetected. Wrap each sender in `AssertUnwindSafe` and ensure the closure captures the wrapper rather than the inner sender. RFC 2229 disjoint capture projects through any explicit field access (`tx.0`) or irrefutable destructure (`let AssertUnwindSafe(x) = tx`), defeating the wrapper. The `TimeoutFuture` case (FnOnce, `send` consumes `self`) routes the unwrap through a tiny helper so the captured path stays at the wrapper. The `IntervalStream` case (FnMut, `unbounded_send` takes `&self`) already autoderefs through `AssertUnwindSafe<T>: Deref`, so just wrapping at the bind site is sufficient. Soundness: for `TimeoutFuture` the sender is consumed and never observed again, so any panic inside `send` cannot expose torn state. For `IntervalStream` `unbounded_send` is a lock-free push whose only realistic panic site is allocation, which aborts under default config; the worst-case observable consequence of an interrupted push is a hung stream, not a memory-safety violation. Extend the panic=unwind CI job to also build with `--features futures` so this path is exercised.
|
I added the fix for the futures case as well and a CI test to check that in future. We have since landed the wasm-bindgen change that does these stricter assertions which will be going out in wasm-bindgen@0.2.122 - would be great to have this landed by then! |
The unwind-safety bound added in wasm-bindgen makes the published gloo-timers 0.3.0 fail to compile against newer wasm-bindgen. Patch to the gloo fork branch which propagates UnwindSafe through the timer callbacks until ranile/gloo#562 lands and a new gloo-timers is published.
Madoshakalaka
left a comment
There was a problem hiding this comment.
Additionally, I note all sibling crates fail with the flag:
RUSTFLAGS='-Cpanic=unwind' cargo +nightly check -p gloo-{events,render,net,worker} --target wasm32-unknown-unknown -Zbuild-std=std,panic_unwind
But it's out of the scope for this PR.
- Drop the wasm-bindgen floor bump back to 0.2; the _assert_unwind_safe paths are cfg-gated on panic = "unwind" so default builds remain compatible with any 0.2.x release. - Deduplicate the CallbackUnwindSafe doc block (the no-op blanket now carries #[doc(hidden)] instead of a copy of the marker docs). - Fix stale PR reference in the AssertUnwindSafe capture rationale.
Apply the same fix as gloo-timers to every sibling crate that hosts wasm-bindgen `Closure::wrap` / `Closure::once` calls. Without this, each of these crates fails to build under `-Cpanic=unwind` against wasm-bindgen 0.2.117+ because the `Box<F> as Box<dyn Fn*>` coercion strips the static `MaybeUnwindSafe` bound the closure constructors now require. - gloo-events: surface `CallbackUnwindSafe` on the four public `EventListener` constructors and route through `Closure::wrap_assert_unwind_safe` / `once_assert_unwind_safe` under panic=unwind. - gloo-render: same on `request_animation_frame`. The internal trampoline captures `Rc<RefCell<Option<CallbackWrapper>>>` and is audited unwind-safe (borrow released before invoking the user closure; Drop sees a valid post-fire `None` state on panic). - gloo-net: internal-only closures in the websocket and eventsource futures. No public bound needed — every callback forwards to lock-free `mpsc::UnboundedSender` pushes and single-shot `Option<Waker>` takes on state we exclusively own. A shared `wrap_internal!` macro at the crate root keeps the cfg branches in one place. - gloo-worker: internal `set_on_packed_message` trampoline; the spawner/registrar handlers release `RefCell` borrows before invoking nested user code so the assertion is sound. All under-panic=unwind code paths are cfg-gated, so default panic=abort builds are unchanged on any 0.2.x wasm-bindgen.
Previously this CI job only built gloo-timers with default features, which let the futures-feature regression go undetected during review and would not catch the same regression in any sibling crate. Build every gloo crate that wraps user closures via wasm-bindgen under `-Cpanic=unwind` so any future change that breaks `MaybeUnwindSafe` compatibility fails CI loudly. Also drop the nightly-2026-01-28 pin: the upstream LLVM 22 + `panic=unwind` exception-handling-tag breakage (wasm-bindgen#4929) is fixed.
6e6bb48 to
e3def81
Compare
|
Hi, can we have a patch release with this? |
wasm-bindgen 0.2.117+ added
MaybeUnwindSafebounds onClosure::wrapandClosure::oncebecause closures handed to JS are invoked across acatch_unwindboundary underpanic = "unwind". Every gloo crate that wraps user (or internal)Fn/FnMut/FnOncecallbacks fails to compile under-Cpanic=unwindbecause theBox::new(callback) as Box<dyn Fn*>coercion (and theFnOncetrait selection) erases the staticUnwindSafebound that wasm-bindgen requires.This breaks every downstream crate that pulls in gloo and tests under
-Cpanic=unwind— see the failure on MattiasBuelens/wasm-streams#35.The fix follows the same pattern in every affected crate:
Where user callbacks flow through the public API (gloo-timers, gloo-events, gloo-render), surface the requirement via a
CallbackUnwindSafemarker trait. It resolves tostd::panic::UnwindSafeunderpanic = "unwind"on wasm and to a no-op blanket otherwise. Callers with non-UnwindSafecaptures must wrap them instd::panic::AssertUnwindSafeat the call site, which is where the invariants can actually be reasoned about.Where the closures are purely internal (gloo-net websocket/eventsource, gloo-worker actor trampoline, gloo-timers futures wrappers, gloo-render's internal slot), use
Closure::wrap_assert_unwind_safe/Closure::once_assert_unwind_safeunder panic=unwind. The unwind-safety assertion is audited per call site: every internal closure either touches lock-freempsc::UnboundedSenderpushes, single-shotOption<Waker>takes, orRc<RefCell<...>>slots where the borrow is released before invoking nested user code. A panic mid-callback leaves observers seeing only states already reachable by normal operation (a missing wake, a dropped event).All under-panic=unwind code paths are cfg-gated, so default
panic = "abort"builds are byte-for-byte unchanged and remain compatible with any 0.2.x wasm-bindgen. No version-requirement bumps anywhere.This is not a breaking change. Under
panic = "abort"(the default)CallbackUnwindSafeis a blanket implemented for everyT, so the bound is invisible. Every existing caller continues to compile unchanged. Underpanic = "unwind"gloo previously did not compile at all against wasm-bindgen 0.2.117+, so there are no existing panic=unwind callers to break. The change unblocks the configuration; it does not regress it.The
panic_unwind_buildCI job now exercises every affected crate, not just gloo-timers default-features, so the regression cannot recur silently in any sibling crate. The nightly-pin workaround is also dropped: the upstream LLVM 22 +panic=unwindissue (wasm-bindgen#4929) is resolved.Tested locally:
RUSTFLAGS="-Cpanic=unwind" cargo +nightly build -p gloo-timers -p gloo-events -p gloo-render -p gloo-net -p gloo-worker --target wasm32-unknown-unknown -Zbuild-std=std,panic_unwind— passes.cargo build -p gloo-timers --features futures --target wasm32-unknown-unknownunder both panic strategies — passes.cargo check+cargo clippyon all touched crates — clean.