Rust panic messages stopped showing up in console spew

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: bholley, Assigned: jryans)

Tracking

(Blocks: 2 bugs, {regression})

unspecified
mozilla56
regression
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 unaffected, firefox55 fixed, firefox56 fixed)

Details

(Whiteboard: [Stylo][fuzzblocker])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a year ago
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
(Reporter)

Comment 1

a year ago
Ted, do you have time to look at this, or should I ask Chris to find somebody?
Flags: needinfo?(ted)
(Reporter)

Comment 2

a year ago
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...
Did bug 1275780 break this?
Flags: needinfo?(ted)
(Reporter)

Updated

a year ago
Blocks: 1275780
Keywords: regression
(Reporter)

Comment 4

a year ago
(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.
(Reporter)

Comment 5

a year ago
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
Flags: needinfo?(ted)
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".
Flags: needinfo?(ted)
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!)
This bug makes Stylo fuzzing more difficult because the fuzzers can't capture Rust panic backtraces.
Whiteboard: [Stylo] → [Stylo][fuzzblocker]
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.
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)

Comment 12

a year ago
mozreview-review
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?
Attachment #8884470 - Flags: review?(giles) → review+
(Assignee)

Comment 13

a year ago
mozreview-review-reply
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

a year ago
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a2f8483b6226
Call default panic hook after crashreporter. r=rillian
https://hg.mozilla.org/mozilla-central/rev/a2f8483b6226
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Is this something we need to backport to Beta?
status-firefox54: --- → unaffected
status-firefox55: --- → affected
status-firefox-esr52: --- → unaffected
Flags: needinfo?(jryans)
(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.
Flags: needinfo?(jryans)
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
Attachment #8884470 - Flags: approval-mozilla-beta?
Comment on attachment 8884470 [details]
Bug 1373881 - Call default panic hook after crashreporter.

fair enough.  let's take this for 55.0b9
Attachment #8884470 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 20

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/5e694949cac6
status-firefox55: affected → fixed
(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.
Flags: qe-verify-
(Assignee)

Updated

a year ago
Blocks: 1380210
You need to log in before you can comment on or make changes to this bug.