|Submitter||Diff||Changes||Open Issues||Last Updated|
|Error loading review requests:|
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 . This makes it much harder to diagnose crashes, both locally and on CI. We should prioritize fixing it.  https://treeherder.mozilla.org/logviewer.html#?job_id=107837618&repo=autoland&lineNumber=31124
Ted, do you have time to look at this, or should I ask Chris to find somebody?
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?
(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.
This is still happening - see  where I don't seem to have any useful stacks to go on.  https://treeherder.mozilla.org/#/jobs?repo=try&revision=080057dee6846a3a03e6d05018884b5e9430df89&selectedJob=111590500
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".
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.
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 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+
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.
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/a2f8483b6226 Call default panic hook after crashreporter. r=rillian
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
(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 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+
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.
You need to log in before you can comment on or make changes to this bug.