Closed Bug 976863 Opened 6 years ago Closed 6 years ago

Cement over the CrashTestUtils.CRASH_ABORT hangs on Window crack

Categories

(Toolkit :: Crash Reporting, defect)

defect
Not set

Tracking

()

RESOLVED WONTFIX

People

(Reporter: gps, Assigned: gps)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

I just spent hours diagnosing a failing crash reporter test only to discover it was due to using CrashTestUitls.CRASH_ABORT on Windows, which apparently brings up the "Abort" "Retry" "Ignore" dialog.

I'm going to write a patch to ensure nobody falls through this crack again.
This is what I have in mind. Waiting on a Windows build to see if this
actually works.
Attachment #8381807 - Flags: feedback?(benjamin)
Assignee: nobody → gps
Status: NEW → ASSIGNED
This passes locally on Windows.

I changed a do_throw to a "throw new Error" so tests could actually
handle failures. This code path isn't taken unless tests are failing, so
I don't think it matters too much.

https://tbpl.mozilla.org/?tree=Try&rev=0e279991be40
Attachment #8382470 - Flags: review?(benjamin)
Attachment #8381807 - Attachment is obsolete: true
Attachment #8381807 - Flags: feedback?(benjamin)
Comment on attachment 8382470 [details] [diff] [review]
Prevent tests from using CrashTestUtils.CRASH_ABORT on Windows

Review of attachment 8382470 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/crashreporter/test/CrashTestUtils.jsm
@@ +37,5 @@
> +  // CRASH_ABORT on Windows will bring up a dialog and hang until a user
> +  // clicks a dialog. Tests are useless here. Detect that scenario and
> +  // fail fast.
> +  if (crashType == CrashTestUtils.CRASH_ABORT) {
> +    if ("@mozilla.org/windows-registry-key;1" in Components.classes) {

Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULRuntime).OS == "WINNT"
Comment on attachment 8382470 [details] [diff] [review]
Prevent tests from using CrashTestUtils.CRASH_ABORT on Windows

I'm fine with the temporary throw in CrashTestUtils.jsm. I really don't think we need the test_crash_abort_windows.js test for this. And maybe we should just fix the problem by calling http://msdn.microsoft.com/en-us/library/e631wekh.aspx within http://hg.mozilla.org/mozilla-central/annotate/c8bea55437c1/toolkit/crashreporter/nsExceptionHandler.cpp#l1157
Attachment #8382470 - Flags: review?(benjamin) → review-
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #4)
> Comment on attachment 8382470 [details] [diff] [review]
> Prevent tests from using CrashTestUtils.CRASH_ABORT on Windows
> 
> I really don't think we need the test_crash_abort_windows.js test for this.

OK. I'll remove the test.

> And maybe we
> should just fix the problem by calling
> http://msdn.microsoft.com/en-us/library/e631wekh.aspx within
> http://hg.mozilla.org/mozilla-central/annotate/c8bea55437c1/toolkit/
> crashreporter/nsExceptionHandler.cpp#l1157

I'm calling scope bloat. Follow-up.
Dropped the test coverage. Changed the "is Windows" check.
Attachment #8385736 - Flags: review?(benjamin)
Comment on attachment 8385736 [details] [diff] [review]
Prevent tests from using CrashTestUtils.CRASH_ABORT on Windows

Please file the followup.
Attachment #8385736 - Flags: review?(benjamin) → review+
Blocks: 980004
Not worth my time.

Bug 980004 has a better solution.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.