1379857 - Rust panic messages from child processes aren't being captured in crash reports

Status: VERIFIED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Nicholas Nethercote [inactive]]

Bug 1275780 comment 17 says:

> Panic report like bp-2abff06f-d969-4ba5-845b-a98410170708 doesn't have the
> MOZ_CRASH Reason field. And in report bp-03718a9c-9d98-4832-b8a6-026220170706,
> there is such field, but it contains nothing useful.

And from bug 1379101, we have https://crash-stats.mozilla.com/report/index/4f8bc612-65c6-4959-93f4-fd2ce0170708 which is an expect() failure that likewise lacks a MOZ_CRASH Reason field.

Ted, I've assigned to you given that this is a follow-up to bug 1275780. Hope that's ok!

Comment 1

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

I believe I see the issue here.  We appear to only watch for Rust panics in the main process at the moment, so we need to also hook child processes as well.

Comment 2

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

Here's a test report from the content process:

https://crash-stats.mozilla.com/report/index/dde8784d-71cb-495e-ae20-5e0ba0170712

(No symbols uploaded, but the panic message appears.)

Comment 3

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

Attachment: Bug 1379857 - Record Rust panics for child process crashes.

Install crash reporter's panic hook in child processes (and also delay the main
process installation until we know crash reporter is enabled).

When collecting child crash annotations, read the Rust panic message if it
exists.

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

Comment 4

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

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

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

Great! Thanks for working on this!

Comment 6

[Nicholas Nethercote [inactive]]

Comment on attachment 8885922 [details]
Bug 1379857 - Record Rust panics for child process crashes.

https://reviewboard.mozilla.org/r/156710/#review161836

Thank you for taking this bug. This patch looks plausible to me but I'm not an appropriate reviewer for crash reporter changes. I would normally suggest Ted, but I believe he's on PTO this week. Maybe gsvelto could review?

Comment 7

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

:gsvelto seems to be on PTO as well.  Maybe :dmajor?  Let's try it...

Comment 8

[(Away)]

Comment on attachment 8885922 [details]
Bug 1379857 - Record Rust panics for child process crashes.

https://reviewboard.mozilla.org/r/156710/#review162126

Thank you for including a test!

::: toolkit/crashreporter/nsExceptionHandler.cpp:1620
(Diff revision 1)
>    const char *envvar = PR_GetEnv("MOZ_CRASHREPORTER");
>    if ((!envvar || !*envvar) && !force)
>      return NS_OK;
>  #endif
>  
> +  install_rust_panic_hook();

I wonder if we should move this down even more, near the end next to mozalloc_set_oom_abort_handler and oldTerminateHandler, in order to follow the same pattern as the child functions?

Comment 9

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

Comment on attachment 8885922 [details]
Bug 1379857 - Record Rust panics for child process crashes.

https://reviewboard.mozilla.org/r/156710/#review162126

> I wonder if we should move this down even more, near the end next to mozalloc_set_oom_abort_handler and oldTerminateHandler, in order to follow the same pattern as the child functions?

Yes, seems resonable!  Looks like there are a few more abort conditions in the middle, and it makes sense to only install the panic hook if we're really going to report things.

Comment 10

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5180575653a890bc6b5cd8a6c15005b7889c5d42

Comment 11

[Ryan VanderMeulen [:RyanVM]]

I assume we'll want this on Beta for 55 as well. Please nominate it for uplift when you're comfortable doing so.

Comment 12

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

Comment on attachment 8885922 [details]
Bug 1379857 - Record Rust panics for child process crashes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156710/diff/1-2/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b972eadf54e8
Record Rust panics for child process crashes. r=dmajor

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

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

Comment on attachment 8885922 [details]
Bug 1379857 - Record Rust panics for child process crashes.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1275780 started to capture Rust panic messages in crash reports in 55, but it only did so for the parent process.
[User impact if declined]: If declined, content process crash reports will miss Rust panic messages.  For Stylo at least, we don't _have_ to have this, as it's not enabled for Beta 55, but there might be other Rust code that this code be helpful for.
[Is this code covered by automated tests?]: Yes
[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?]: Reuses the existing panic handling from the parent process in the child.  We've confirmed it's working in Nightly crash reports.
[String changes made/needed]: None

Comment 16

[Julien Cristau [:jcristau]]

Comment on attachment 8885922 [details]
Bug 1379857 - Record Rust panics for child process crashes.

looks harmless enough and possibly useful, let's get this in 55.0b11

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ed5cfbc47793