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

RESOLVED FIXED in Firefox 57

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: gsvelto, Assigned: gsvelto)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla57
All
Windows
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox55 wontfix, firefox56 wontfix, firefox57 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

a year ago
+++ 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.
status-firefox55: --- → wontfix
status-firefox56: --- → affected
status-firefox57: --- → affected
(Assignee)

Updated

a year ago
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Blocks: 1396509
Ted, will you be able to get to this any time soon? This bug blocks a topcrasher bug.
Flags: needinfo?(ted)

Comment 3

a year ago
mozreview-review
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.
Attachment #8904077 - Flags: review?(ted) → review+
Sorry, having a hard time keeping my review queue clean the past few weeks.
Flags: needinfo?(ted)
(Assignee)

Comment 5

a year ago
(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.
Flags: needinfo?(ted)
(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.
Flags: needinfo?(ted)
(Assignee)

Comment 7

a year ago
(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.
(Assignee)

Comment 8

a year ago
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

a year ago
Pushed by gsvelto@mozilla.com:
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d1a21fb93a04
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox57: affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
(Assignee)

Updated

a year ago
See Also: → bug 1400294
(Assignee)

Updated

a year ago
See Also: bug 1400294
Too late for 56. Mass won't fix for 56.
status-firefox56: affected → wontfix
(Assignee)

Comment 12

a year ago
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.
Blocks: 1416078
You need to log in before you can comment on or make changes to this bug.