LogPlugin silently overwrites the panic hook in WASM

[simbleau]

The LogPlugin silently overwrites the panic handler on WASM. This means if you attempt to set your own panic handler, or use something like https://github.com/vectorgameexperts/web_panic_report, you have to overwrite it on Startup (thus, your panic handler can't be added until Startup)

Problematic code:

bevy/crates/bevy_log/src/lib.rs

Lines 206 to 212 in 5d0ff60

	#[cfg(target_arch = "wasm32")]
	{
	console_error_panic_hook::set_once();
	finished_subscriber = subscriber.with(tracing_wasm::WASMLayer::new(
	tracing_wasm::WASMLayerConfig::default(),
	));
	}

This should not be considered acceptable default behavior.

3 options here, open to discussion.

• (1) Provide a hook to the LogPlugin
• (2) Take it out (my preferred answer - Logging really has nothing to do with panicking.)
• (3) Consume the previous hook and wrap it, e.g.
  

  bevy/crates/bevy_log/src/lib.rs

  Lines 136 to 140 in 5d0ff60

  	let old_handler = panic::take_hook();
  	panic::set_hook(Box::new(move |infos| {
  	eprintln!("{}", tracing_error::SpanTrace::capture());
  	old_handler(infos);
  	}));

[alice-i-cecile]

Why was this added in the first place? Can you check through the git history and link the PR?

[simbleau]

Why was this added in the first place? Can you check through the git history and link the PR?

I believe it was added as a default to give WASM users the stack trace in their console, instead of the cryptic unreachable error. (see image)

It's been untouched since the original LogPlugin PR, 4 years ago, by @cart

I think the reason it has gone unnoticed for so long is probably that it's uncommon and fairly technical to implement your own WASM panic handler. @Vrixyz and I just made the web_panic_report crate yesterday and we found no similar crates.

I would love to hear @cart 's original intention with this - and whether we can remove it. Should this logic be a separate PanicHandlerPlugin in the DefaultPlugins instead?

[alice-i-cecile]

Thanks for the detective work! Splitting this out so it can be disabled or overwritten seems like a sensible direction to me. No strong feelings on the exact architecture though.

[james7132]

I think the reason it has gone unnoticed for so long is probably that it's uncommon and fairly technical to implement your own WASM panic handler. Vrixyz and I just made the web_panic_report crate yesterday and we found no similar crates.

This is probably the case. According to console_error_panic_handler's README, the alternative is an opaque error message that is borderline unusable to figure out what caused a panic.

[simbleau]

Thanks for the detective work! Splitting this out so it can be disabled or overwritten seems like a sensible direction to me. No strong feelings on the exact architecture though.

@alice-i-cecile - Went ahead with the separate plugin strategy outlined above. PR #12557