Closed
Bug 1373881
Opened 7 years ago
Closed 7 years ago
Rust panic messages stopped showing up in console spew
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox54 | --- | unaffected |
firefox55 | --- | fixed |
firefox56 | --- | fixed |
People
(Reporter: bholley, Assigned: jryans)
References
(Blocks 2 open bugs)
Details
(Keywords: regression, Whiteboard: [Stylo][fuzzblocker])
Attachments
(1 file)
(deleted),
text/x-review-board-request
|
rillian
:
review+
jcristau
:
approval-mozilla-beta+
|
Details |
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•7 years ago
|
||
Ted, do you have time to look at this, or should I ask Chris to find somebody?
Flags: needinfo?(ted)
Reporter | ||
Comment 2•7 years 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...
Reporter | ||
Updated•7 years ago
|
Blocks: 1275780
Keywords: regression
Reporter | ||
Comment 4•7 years 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•7 years 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)
Comment 6•7 years ago
|
||
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)
Comment 7•7 years ago
|
||
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•7 years ago
|
||
This bug makes Stylo fuzzing more difficult because the fuzzers can't capture Rust panic backtraces.
Updated•7 years ago
|
Whiteboard: [Stylo] → [Stylo][fuzzblocker]
Assignee | ||
Comment 9•7 years ago
|
||
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) |
Assignee | ||
Comment 11•7 years ago
|
||
Comment 12•7 years 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•7 years 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•7 years ago
|
||
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a2f8483b6226
Call default panic hook after crashreporter. r=rillian
Comment 15•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment 16•7 years ago
|
||
Is this something we need to backport to Beta?
status-firefox54:
--- → unaffected
status-firefox55:
--- → affected
status-firefox-esr52:
--- → unaffected
Flags: needinfo?(jryans)
Assignee | ||
Comment 17•7 years ago
|
||
(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)
Assignee | ||
Comment 18•7 years ago
|
||
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 19•7 years ago
|
||
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•7 years ago
|
||
bugherder uplift |
Comment 21•7 years ago
|
||
(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•7 years ago
|
Blocks: rust-great
You need to log in
before you can comment on or make changes to this bug.
Description
•