add "std-unwrap" feature to defer to std's unwrap in unwrap_throw

[alecmocatta]

In our release binaries .unwrap() gives a useful error:

Error: panicked at crates/lang/src/count.rs:22:64:
called `Option::unwrap()` on a `None` value

Whereas .unwrap_throw() does not:

Error: `unwrap_throw` failed

error.stack

Error: `unwrap_throw` failed
wasm_bindgen::throw_str::h52b6facbb28affa2@[wasm code]
<futures_util::stream::stream::for_each::ForEach<St,Fut,F> as core::future::future::Future>::poll::h63f690969fd636cf@[wasm code]
tokio::runtime::task::raw::poll::h14ea459da51d02f6@[wasm code]
concurrency_utils::try_future::try_::{{closure}}::hef4fe39654304a7f@[wasm code]
<dyn core::ops::function::FnMut<()>+Output = R as wasm_bindgen::closure::WasmClosure>::describe::invoke::h4114b59fa0fc4be3@[wasm code]
A@https://tably.com/xxx:1:1125947
_dyn_core__ops__function__FnMut_____Output___R_as_wasm_bindgen__closure__WasmClosure___describe__invoke__h4114b59fa0fc4be3@https://tably.com/xxx:1:1128611
x@https://tably.com/xxx:1:1179277
__wbg_try_b1e23a7f21a743ba@https://tably.com/xxx:1:1128574
<futures_util::future::maybe_done::MaybeDone<Fut> as core::future::future::Future>::poll::h685a6becb168fdad@[wasm code]
<concurrency_utils::task::DropFuture<F> as core::future::future::Future>::poll::h0af0a15d821db4ce@[wasm code]
wasm_bindgen_futures::queue::Queue::new::{{closure}}::h46010d614ba0d67e@[wasm code]
<dyn core::ops::function::FnMut<(A,)>+Output = R as wasm_bindgen::closure::WasmClosure>::describe::invoke::h018270bf1cf615d6@[wasm code]
E@https://tably.com/xxx:1:1126097
s@https://tably.com/xxx:1:1125704

Currently, enabling debug_assertions is the only way to have .unwrap_throw() print the call site:

wasm-bindgen/src/lib.rs

Lines 1330 to 1344 in 78463a2

	#[cfg_attr(debug_assertions, track_caller)]
	fn unwrap_throw(self) -> T {
	if cfg!(all(debug_assertions, feature = "std")) {
	let loc = core::panic::Location::caller();
	let msg = std::format!(
	"`unwrap_throw` failed ({}:{}:{})",
	loc.file(),
	loc.line(),
	loc.column()
	);
	self.expect_throw(&msg)
	} else {
	self.expect_throw("`unwrap_throw` failed")
	}
	}

Gating on debug_assertions was intended to avoid increasing code size in release mode. Unfortunately though it makes debugging panics in release binaries significantly harder, given many fundamental libs including wasm-bindgen-futures, yew and leptos use .unwrap_throw().

This PR adds a feature "std-unwrap" to make fn unwrap_throw and fn expect_throw defer to fn unwrap and fn expect respectively. This allows applications to trade off a potentially negligible increase in code size for useful panic messages. Some measurements from our production application:

	call site in panic message	Wasm bin compressed size	% increase
debug_assertions = false	no	4,237,448	-
debug_assertions = true	yes	4,269,427	0.75%
debug_assertions = false, features = ["std-unwrap"]	yes	4,239,307	0.04%

[daxpedda]

I'm not in favor of adding a crate feature for this kind of adjustment.

However, it would be great if you could dig into why this was guarded behind cfg(debug_assertions) in the first place, then we could just remove the guard.

[alecmocatta]

The original motivation in #1219 for these traits was to create variants with "a smaller code size footprint than the normal Option::unwrap and Option::expect methods".

Printing the call site was added in #2995. That PR initially unconditionally printed the call site but that was objected to on code size grounds. A compromise was suggested:

I think it would be really useful if unwrap_throw displayed filename and line numbers in debug mode (but not release mode).

and this was implemented by gating on cfg(debug_assertions).

I understand the motivation but debug_assertions is orthogonal to "give useful panic messages". In a production application we need useful panic messages but would rather not take the unpredictable performance and code size hit of enabling debug_assertions. As there's no way around this we're using and maintaining this fork.

I understand the reticence to adding a crate feature so if you have any alternative suggestions please do say.

[daxpedda]

The original motivation in #1219 for these traits was to create variants with "a smaller code size footprint than the normal Option::unwrap and Option::expect methods".

Printing the call site was added in #2995. That PR initially unconditionally printed the call site but that was objected to on code size grounds. A compromise was suggested:

I think it would be really useful if unwrap_throw displayed filename and line numbers in debug mode (but not release mode).

and this was implemented by gating on cfg(debug_assertions).

Thank you for the research!

I understand the reticence to adding a crate feature so if you have any alternative suggestions please do say.

I would recommend you to explore generating source maps as this would significantly improve your debugging experience in many ways. Let me know if you have any trouble with it.

If that doesn't really work for you, I think the alternative to a crate feature would be something like cfg(wasm_bindgen_unwrap_throw_debug), which I think would be acceptable for wasm-bindgen.

[alecmocatta]

I would recommend you to explore generating source maps as this would significantly improve your debugging experience in many ways. Let me know if you have any trouble with it.

Source maps would help debugging on our machines right? It's specifically crashes that occur on user machines where we need this. For crashes on our machines Chrome's DWARF extension gives us a pretty decent debugging experience. For the JS we've not got around to source maps yet, but as it's only wasm-bindgen shims I don't expect it to be too helpful for debugging.

Also in favour of printing the call location: there are a couple of reasons the backtrace can be incomplete (regardless of source maps / DWARF): inlining, and lack of pcOffset on Safari meaning we can't get files+line numbers for each frame.

If that doesn't really work for you, I think the alternative to a crate feature would be something like cfg(wasm_bindgen_unwrap_throw_debug), which I think would be acceptable for wasm-bindgen.

Ah, good idea! Thanks, I'll go ahead and made this change shortly.

[daxpedda]

Source maps would help debugging on our machines right? It's specifically crashes that occur on user machines where we need this. For crashes on our machines Chrome's DWARF extension gives us a pretty decent debugging experience.

Source maps work on user machines as well, they also work on all current browsers without the need for any extension.

If you don't want to expose any debug information to users, you may want to take a look at Wasm coredumps.

[daxpedda]

A changelog entry would be necessary as well.

[daxpedda]

I'm aware that this is the change proposed in OP, but is the debug_assertions output not good enough, or why do you actually need unwrap()?

[alecmocatta]

I spend quite a bit of time triaging errors, and in that context consistency and recognisability is important. In this instance, deferring to std maintains consistency:

• across build targets. same error under wasm, wasi, emscripten, linux, macos and so on.

• across calls to the _throw or non-_throw variant. same error and importantly same behaviour, panicking rather than throwing.

We've implemented manual poisoning in our panic hook until #3687 is resolved. The throw inside .unwrap_throw() bypasses that and results in unsoundness that is unavoidable without patching leptos wasm-bindgen-futures and so on to respect our poison. That's rather painful, hence this fork where we panic rather than throw. We do also implement poison in window.addEventListener("error", ...) and window.addEventListener("unhandledrejection", ...) for such throws but I believe e.g. wasm-bindgen-futures calling queueMicrotask can preempt that callback, so can't fully mitigate the unsoundness.

Anecdotally since switching to this fork various "impossible" errors e.g. rust-lang/regex#1176 have stopped occurring, so in retrospect I think the throwing plus #3687 plus leptos or wasm-bindgen-futures calling queueMicrotask and similar was likely the cause.

[daxpedda]

I'm questioning if wasm-bindgen should do something here, unwrap_throw() should explicitly throw, if it panics it would be doing something different then advertised.

I do agree that the lack of module poisoning is quite problematic, unfortunately I don't know a "quick" fix for it apart from what you have been doing.

I have a couple of suggestions:

• Maybe we can figure out a way to fix wasm-bindgen-futures:
  • Now that unwinding is actually supported in Rust Wasm we may want to think about the implementation of wasm-bindgen-futures more carefully.
  • If we can't find a good solution, I'm happy to add a cfg flag there to panic instead of throwing.
• Other libraries might do whats suggested here, e.g. switch to panicking behind a cfg flag.
• Module poisoning needs a solution, I'm happy to look at a PR here in wasm-bindgen, even if it should probably be solved in Rustc instead.

Please let me know what you think, I'm sympathetic to your goals but I would like to find a better solution.

[daxpedda]

As above. If you want to include the file, line and column as in unwrap_throw() I think it would be a nice improvement.

[daxpedda]

As above.

[daxpedda]

I think it would be best if you just remove all guards on track_caller, which would also improve native support.

[alecmocatta]

Source maps work on user machines as well, they also work on all current browsers without the need for any extension.

Is there any documentation that they give Wasm stack traces line numbers? I only found the opposite.

If you don't want to expose any debug information to users, you may want to take a look at Wasm coredumps.

I didn't think this was implemented in any browsers yet?

[daxpedda]

Source maps work on user machines as well, they also work on all current browsers without the need for any extension.

Is there any documentation that they give Wasm stack traces line numbers? I only found the opposite.

I'm not sure where to find any documentation on this, but source maps do include line numbers and from personal testing I can say that it looks just like native stacktraces.

If you don't want to expose any debug information to users, you may want to take a look at Wasm coredumps.

I didn't think this was implemented in any browsers yet?

I'm not aware of any actual proposal that needs implementing, these are just tool conventions. If you follow the links on this page you will even find a live demo to try it out yourself.

[alecmocatta]

Thanks for the pushback @daxpedda! I looked into this a little more and decided this approach doesn't sufficiently mitigate the unsoundness we've seen the effects of. As Alex said, Wasm-bindgen also offers the ability to throw a JS exception at any point from within wasm which is basically unsound and can't be used properly anyway. Instead we're strengthening our poisoning until #3687 is implemented. I'll go ahead and close this.

I'm not sure where to find any documentation on this, but source maps do include line numbers and from personal testing I can say that it looks just like native stacktraces.

I'm not seeing source maps affecting Error().stack for Wasm frames, unless I'm doing something wrong. Perhaps you're referring to the debugger's call stack, which is was I was referring to as workable on our but not on user machines.

I'm not aware of any actual proposal that needs implementing, these are just tool conventions. If you follow the links on this page you will even find a live demo to try it out yourself.

Thanks for making me aware of it but it's sadly not workable. It's not implemented by browsers yet and so requires a sketchy polyfill wasm-coredump-rewriter to "rewrite[s] [the] Wasm module and inject the core dump runtime functionality in the binary" per Cloudflare which will break the debuggability we already have from DWARF debuginfo.

[daxpedda]

I'm not sure where to find any documentation on this, but source maps do include line numbers and from personal testing I can say that it looks just like native stacktraces.

I'm not seeing source maps affecting Error().stack for Wasm frames, unless I'm doing something wrong. Perhaps you're referring to the debugger's call stack, which is was I was referring to as workable on our but not on user machines.

I just tested it with Error.stack and it works for me. Maybe something went wrong in your source map generation, I've used wasm2map, maybe that works for you as well.

I'm not aware of any actual proposal that needs implementing, these are just tool conventions. If you follow the links on this page you will even find a live demo to try it out yourself.

Thanks for making me aware of it but it's sadly not workable. It's not implemented by browsers yet and so requires a sketchy polyfill wasm-coredump-rewriter to "rewrite[s] [the] Wasm module and inject the core dump runtime functionality in the binary" per Cloudflare which will break the debuggability we already have from DWARF debuginfo.

The Cloudflare blog post you linked has a decent overview how to work with it. Breaking the DWARF debuginfo would defeat the purpose of a core dump, so it actually still works.

Wasm post-processing isn't ideal, but it is the status quo right now, wasm-bindgen itself is a Wasm post-processor, so hopefully that shouldn't be a blocker by principle.

---

Sorry for the delayed response!