1393716 - Do not run content crash analysis if shutdown has already started and cancel pending ones

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Gabriele Svelto [:gsvelto] (PTO)]

+++ This bug was initially created as a clone of Bug #1373958 +++

As per bug 1373958 comment 26 we should prevent the minidump-analyzer from running if we're already shutting down, and we should cancel it if it's taking too long.

Comment 1

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: Bug 1393716 - Don't run minidump analysis on content crashes if the browser is quitting .mielczarek

When a content crash is detected we run the minidump analyzer on it to extract
a stack trace that will be sent with the crash ping. Since this operation can
potentially take a long time this patch prevents it from happening if the
browser is already quitting. Already running instances of the minidump
analyzer are also killed. The patch includes test coverage for both the new
codepaths.

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

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

Ted, will you be able to get to this any time soon? This bug blocks a topcrasher bug.

Comment 3

[(not currently active) Ted Mielczarek]

Comment on attachment 8904077 [details]
Bug 1393716 - Don't run minidump analysis on content crashes if the browser is quitting .mielczarek

https://reviewboard.mozilla.org/r/175822/#review184506

::: toolkit/components/crashes/CrashService.js:135
(Diff revision 1)
>   *
>   * It is a service because some background activity will eventually occur.
>   */
> -this.CrashService = function() {};
> +this.CrashService = function() {
> +  quitting = false;
> +  runningProcesses = new Set();

Any reason not to just initialize these at the top level?

::: toolkit/components/crashes/tests/xpcshell/test_crash_service.js:119
(Diff revision 1)
> +  // Now wait for the crash to be recorded
> +  await addCrashPromise;
> +  let crashes = await Services.crashmanager.getCrashes();
> +  let crash = crashes.find(c => { return c.id === firstCrashId; });
> +  Assert.ok(crash, "Crash " + firstCrashId + " has been stored successfully.");
> +  Assert.ok(crash.metadata.StackTraces === undefined,

Is it possible for this to race with the minidump analyzer? I'd hate to see this test fail intermittently just because the minidump analyzer runs quickly enough to be done before shutdown.

Comment 4

[(not currently active) Ted Mielczarek]

Sorry, having a hard time keeping my review queue clean the past few weeks.

Comment 5

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #3)
> ::: toolkit/components/crashes/CrashService.js:135
> (Diff revision 1)
> >   *
> >   * It is a service because some background activity will eventually occur.
> >   */
> > -this.CrashService = function() {};
> > +this.CrashService = function() {
> > +  quitting = false;
> > +  runningProcesses = new Set();
> 
> Any reason not to just initialize these at the top level?

No, I first thought about storing them in the CrashService object and then decided it would be better to have them outside so that they couldn't be visible; I didn't move the initializations though.

> ::: toolkit/components/crashes/tests/xpcshell/test_crash_service.js:119
> (Diff revision 1)
> > +  // Now wait for the crash to be recorded
> > +  await addCrashPromise;
> > +  let crashes = await Services.crashmanager.getCrashes();
> > +  let crash = crashes.find(c => { return c.id === firstCrashId; });
> > +  Assert.ok(crash, "Crash " + firstCrashId + " has been stored successfully.");
> > +  Assert.ok(crash.metadata.StackTraces === undefined,
> 
> Is it possible for this to race with the minidump analyzer? I'd hate to see
> this test fail intermittently just because the minidump analyzer runs
> quickly enough to be done before shutdown.

Yes, it is potentially racy though rather unlikely. I haven't thought of a better alternative, save adding some test-only notification sent when the minidump-analyzer process gets killed. I would then add an observer in the test waiting on it instead. That would be clunky but non-racy and definitely not unheard of in our codebase, what do you think?

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #4)
> Sorry, having a hard time keeping my review queue clean the past few weeks.

No problem.

Comment 6

[(not currently active) Ted Mielczarek]

(In reply to Gabriele Svelto [:gsvelto] from comment #5)
> Yes, it is potentially racy though rather unlikely. I haven't thought of a
> better alternative, save adding some test-only notification sent when the
> minidump-analyzer process gets killed. I would then add an observer in the
> test waiting on it instead. That would be clunky but non-racy and definitely
> not unheard of in our codebase, what do you think?

Either that, or if there's already a notification for "minidump-analyzer finished successfully" you could have the test listen to that, and condition the check on whether that happened.

Comment 7

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #6)
> Either that, or if there's already a notification for "minidump-analyzer
> finished successfully" you could have the test listen to that, and condition
> the check on whether that happened.

OK, I'll update the test before landing then, thanks.

Comment 8

[Gabriele Svelto [:gsvelto] (PTO)]

I've updated the patch to use a dedicated test notification to verify that the minidump-analyzer has been killed, this makes the test completely race-free. The green try run is here:

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

I'm ready to land.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1a21fb93a04
Don't run minidump analysis on content crashes if the browser is quitting r=ted.mielczarek

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

[Gerry Chang [:gchang]]

Too late for 56. Mass won't fix for 56.

Comment 12

[Gabriele Svelto [:gsvelto] (PTO)]

The amount of crashes with the shutdownhang | ZwCreateUserProcess signature seem to be going down, maybe this time the issue was successfully mitigated. I'll keep monitoring it though.