1373881 - Rust panic messages stopped showing up in console spew

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Bobby Holley (:bholley)]

I noticed this today in a local build, and assumed I'd done something wrong. However, I then saw the same behavior on a treeherder build [1].

This makes it much harder to diagnose crashes, both locally and on CI. We should prioritize fixing it.

[1] https://treeherder.mozilla.org/logviewer.html#?job_id=107837618&repo=autoland&lineNumber=31124

Comment 1

[Bobby Holley (:bholley)]

Ted, do you have time to look at this, or should I ask Chris to find somebody?

Comment 2

[Bobby Holley (:bholley)]

Oh interesting! It actually works for e10s, and I just happened to have clicked on the non-e10s build in the log, and happened to be debugging with --disable-e10s locally.

So less urgent, since non-e10s is going away, though would be good to fix if we can...

Comment 3

[(not currently active) Ted Mielczarek]

Did bug 1275780 break this?

Comment 4

[Bobby Holley (:bholley)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #3)
> Did bug 1275780 break this?

Yes - reverting that patch fixes the problem.

I also seem to have the problem intermittently with e10s as well. Seems like something we need to fix.

Comment 5

[Bobby Holley (:bholley)]

This is still happening - see [1] where I don't seem to have any useful stacks to go on.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=080057dee6846a3a03e6d05018884b5e9430df89&selectedJob=111590500

Comment 6

[(not currently active) Ted Mielczarek]

If this is really breaking your workflow feel free to back that patch out and reopen the bug. I won't have time to look into this right now, I'm on vacation next week. I had not realized that the default "print a backtrace" error was implemented as a panic hook, and that using `set_hook` would override that behavior. I think the simplest way to have our cake and eat it too would be to call `panic::take_hook` before we call `panic::set_hook`, stash that in a global, and then call that function from our hook:
https://doc.rust-lang.org/std/panic/fn.take_hook.html

I looked at the source, and it will return the default hook if no other hook is registered. There doesn't seem to be any public API for "call the default panic hook".

Comment 7

[(not currently active) Ted Mielczarek]

Specifically, we'd call `panic::take_hook` in `install_rust_panic_hook`:
https://dxr.mozilla.org/mozilla-central/rev/211d4dd61025c0a40caea7a54c9066e051bdde8c/toolkit/library/rust/shared/lib.rs#52

I guess we might not even need to put it in a global, since the new hook is a closure we could just use the local variable (yay Rust ownership!)

Comment 8

[Chris Peterson [:cpeterson]]

This bug makes Stylo fuzzing more difficult because the fuzzers can't capture Rust panic backtraces.

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I had quite a bit of trouble reproducing this at first... then I realized I build with --disable-crashreporter locally. X_X

Anyway, I think I've got the fix outlined in comment 7 working.

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1373881 - Call default panic hook after crashreporter.

Crash reporter installs a special Rust panic hook to grab the panic reason.
However, we still want to call the default hook as well, so that we still print
the reason and backtrace to the console.

Review commit: https://reviewboard.mozilla.org/r/155372/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/155372/

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=106e7df5298b210a791836b40082e93bc605ab94

Comment 12

[Ralph Giles (:rillian)]

Comment on attachment 8884470 [details]
Bug 1373881 - Call default panic hook after crashreporter.

https://reviewboard.mozilla.org/r/155372/#review160464

::: toolkit/library/rust/shared/lib.rs:71
(Diff revision 1)
>              // in practice.
>              println!("Unhandled panic payload!");
>          }
> +        // Fall through to the default hook so we still print the reason and
> +        // backtrace to the console.
> +        default_hook(info);

Neat that you can call through the `Box` here. I guess that's auto-Deref?

Comment 13

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8884470 [details]
Bug 1373881 - Call default panic hook after crashreporter.

https://reviewboard.mozilla.org/r/155372/#review160464

> Neat that you can call through the `Box` here. I guess that's auto-Deref?

Right!  It's seen more commonly with function args, but also works with the function itself.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a2f8483b6226
Call default panic hook after crashreporter. r=rillian

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/a2f8483b6226

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Is this something we need to backport to Beta?

Comment 17

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #16)
> Is this something we need to backport to Beta?

Probably not critical on Beta for Stylo specifically, but it's a good fix to have when trying to diagnose anything Rust related, and it's a small patch.

I'll file an uplift request.

Comment 18

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8884470 [details]
Bug 1373881 - Call default panic hook after crashreporter.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1275780 change Rust crash handling, and caused crashes to no longer be logged to the console
[User impact if declined]: If declined, it will be harder to investigate Rust crash issues, since panic messages and backtraces are no longer printed
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: It's a straightforward change that only impacts Rust crash handling.
[String changes made/needed]: None

Comment 19

[Julien Cristau [:jcristau]]

Comment on attachment 8884470 [details]
Bug 1373881 - Call default panic hook after crashreporter.

fair enough.  let's take this for 55.0b9

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5e694949cac6

Comment 21

[Andrei Vaida [:avaida]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #18)
> [Is this code covered by automated tests?]: No
> [Has the fix been verified in Nightly?]: Yes
> [Needs manual test from QE? If yes, steps to reproduce]: No

Setting qe-verify- based on J. Ryan Stinnett's assessment on manual testing needs.