Intermittent toolkit/components/crashes/tests/xpcshell/test_crash_service.js | application crashed [@ test_app.exe + 0x429e]

RESOLVED FIXED in Firefox 58

Status

()

defect
P5
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: intermittent-bug-filer, Assigned: gsvelto)

Tracking

({crash, intermittent-failure})

unspecified
mozilla58
Points:
---

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox56 unaffected, firefox57 unaffected, firefox58 fixed)

Details

(crash signature)

Attachments

(1 attachment)

I think that this might be caused by the minidump analyzer finishing execution before we have a chance to kill it in the test. I'll try to reproduce it locally; if my assumption is correct this should be an easy fix.
I've confirmed that this happens when the call to kill() arrives too late and the process has already finished executing on its own.
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
Comment on attachment 8922503 [details]
Bug 1408806 - Gracefully deal with errors when killing the minidump-analyzer on shutdown;

https://reviewboard.mozilla.org/r/193574/#review200492

::: commit-message-a97fb:5
(Diff revision 1)
> +Bug 1408806 - Gracefully deal with errors when killing the minidump-analyzer on shutdown; r?mconley
> +
> +This solves two problems that were causing tests to fail intermittently. The
> +first issue is that calling nsIProcess.kill() on a process that had already
> +terminated would throw an exception which wouldn't be caught. Since might

Nit: "Since might" sounds wrong.

::: toolkit/components/crashes/CrashService.js:230
(Diff revision 1)
> +          try {
> -          process.kill();
> +            process.kill();
> +          } catch (e) {
> +            // If the process has already quit then kill() fails, but since
> +            // this failure is benign it is safe to silently ignore it.
> +          }

I think I'm okay wrapping this in a try/catch, but should we also attempt to deal with the "process-failed" topic in here: http://searchfox.org/mozilla-central/rev/aa1343961fca52e8c7dab16788530da31aab7c8d/toolkit/components/crashes/CrashService.js#61-71 by removing the process from gRunningProcesses?

Or maybe in the "default" handler, we should remove the process from the gRunningProcesses list.
Attachment #8922503 - Flags: review?(mconley) → review+
Comment on attachment 8922503 [details]
Bug 1408806 - Gracefully deal with errors when killing the minidump-analyzer on shutdown;

https://reviewboard.mozilla.org/r/193574/#review200492

> Nit: "Since might" sounds wrong.

Yeah, it should have been "Since this might..."

> I think I'm okay wrapping this in a try/catch, but should we also attempt to deal with the "process-failed" topic in here: http://searchfox.org/mozilla-central/rev/aa1343961fca52e8c7dab16788530da31aab7c8d/toolkit/components/crashes/CrashService.js#61-71 by removing the process from gRunningProcesses?
> 
> Or maybe in the "default" handler, we should remove the process from the gRunningProcesses list.

Good point, we should remove the process anyway since we're adding it unconditionally. The try/catch block is still required because the issue here is caused by a race happening after the process has finished but before the observer is notified.
Pushed by gsvelto@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbac4c5957cd
Gracefully deal with errors when killing the minidump-analyzer on shutdown; r=mconley
https://hg.mozilla.org/mozilla-central/rev/cbac4c5957cd
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.