1311016 - Make Firefox honor MOZ_CRASHREPORTER_SHUTDOWN environment variable to quit browser on content process crash

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Similar to bug 976120 for B2G we would like to have Firefox respect the crash environment especially for MOZ_CRASHREPORTER_SHUTDOWN which should cause Firefox to quit. A patch should be similar to the following changeset:

https://hg.mozilla.org/mozilla-central/rev/471b293539c6

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ted, do you know where we would have to get this code added?

Comment 2

[(not currently active) Ted Mielczarek]

We'd want to handle this in the existing browser code that handles ipc:content-shutdown:
https://dxr.mozilla.org/mozilla-central/rev/7452437b3ab571b1d60aed4e973d82a1471f72b2/browser/modules/ContentCrashHandlers.jsm#79

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Looks like an easy thing to do. I will take it.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1311016 - Make Firefox honor MOZ_CRASHREPORTER_SHUTDOWN environment variable to quit browser on content process crash

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

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Tested and works perfectly fine locally with the environment variable set. Ted, please let me know if that is the right place to insert this shutdown. If yes, I will request review from a browser peer. Thanks.

Comment 6

[(not currently active) Ted Mielczarek]

Comment on attachment 8803899 [details]
Bug 1311016 - Make Firefox honor MOZ_CRASHREPORTER_SHUTDOWN environment variable to quit browser on content process crash

https://reviewboard.mozilla.org/r/88110/#review87288

That looks like the correct patch.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8803899 [details]
Bug 1311016 - Make Firefox honor MOZ_CRASHREPORTER_SHUTDOWN environment variable to quit browser on content process crash

https://reviewboard.mozilla.org/r/88110/#review87330

Thanks!

::: browser/modules/ContentCrashHandlers.jsm:121
(Diff revision 1)
>          }
> +
> +        // check for environment affecting crash reporting
> +        let env = Cc["@mozilla.org/process/environment;1"]
> +                    .getService(Ci.nsIEnvironment);
> +        let shutdown = env.get("MOZ_CRASHREPORTER_SHUTDOWN");

Note that the documentation in `nsIEnvironment` [suggests we use env.exists for this use case instead](http://searchfox.org/mozilla-central/rev/e3e8571c5378ac92663d4f583ccc4ad0a3019716/xpcom/threads/nsIEnvironment.idl#29-31).

::: browser/modules/ContentCrashHandlers.jsm:124
(Diff revision 1)
> +          let appStartup = Cc["@mozilla.org/toolkit/app-startup;1"]
> +                             .getService(Ci.nsIAppStartup);
> +          appStartup.quit(Ci.nsIAppStartup.eForceQuit);

You should be able to use `Services.startup.quit` here instead.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8803899 [details]
Bug 1311016 - Make Firefox honor MOZ_CRASHREPORTER_SHUTDOWN environment variable to quit browser on content process crash

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f7a9e4b09055
Make Firefox honor MOZ_CRASHREPORTER_SHUTDOWN environment variable to quit browser on content process crash r=mconley

Comment 10

[Phil Ringnalda (:philor)]

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

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

Does this, and should it, work with the GPU process?

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I think that Ted might better know the details if the GPU process is also covered.

Comment 13

[David Anderson [:dvander] - inactive, e-mail if emergency]

This patch probably doesn't cover the GPU process. But I don't know why we'd want to shutdown the UI for a gpu process crash, unless we're specifically interested in stopping testing when that happens (we definitely would not normally want to).

Comment 14

[(not currently active) Ted Mielczarek]

I assume this does not cover GPU process crashes, since the notification it keys off of is "ipc:content-shutdown". dvander: this is intended solely for testing, where we likely want to stop testing when we experience an unexpected crash since it's unlikely that things will continue to work as expected.

Comment 15

[David Anderson [:dvander] - inactive, e-mail if emergency]

For the GPU process it's the opposite: things are likely to keep working and if not, that's a problem. We want to know about those failures.