Closed Bug 1393716 Opened 5 years ago Closed 5 years ago

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

Categories

(Toolkit :: Crash Reporting, defect)

All
Windows
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- fixed

People

(Reporter: gsvelto, Assigned: gsvelto)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

+++ 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.
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
Blocks: 1396509
Ted, will you be able to get to this any time soon? This bug blocks a topcrasher bug.
Flags: needinfo?(ted)
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)
(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)
(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.
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.
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
https://hg.mozilla.org/mozilla-central/rev/d1a21fb93a04
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
See Also: → 1400294
See Also: 1400294
Too late for 56. Mass won't fix for 56.
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.