Closed Bug 761859 Opened 8 years ago Closed 8 years ago

mozalloc_abort() should use MOZ_CRASH(), not roll its own crash behavior

Categories

(Core :: mozglue, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: Waldo, Assigned: Waldo)

References

Details

Attachments

(2 files)

Attached patch Patch with testSplinter Review
Better to have only one way to crash, than to have many, given that every way has to be Breakpad-compatible, and that's not entirely trivial.
Attachment #630369 - Flags: review?(ted.mielczarek)
Oh, this patch (plus the one in bug 761857, joined together) passed try.

https://tbpl.mozilla.org/?tree=Try&rev=deb12dbebfc7
Comment on attachment 630369 [details] [diff] [review]
Patch with test

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

::: memory/mozalloc/mozalloc_abort.cpp
@@ -26,5 @@
> -static int gDummyCounter;
> -
> -// Not inlining this function avoids the compiler making optimizations
> -// that end up corrupting stack traces.
> -MOZ_NEVER_INLINE static void

Wonder if we need to shift this annotation up to mozalloc_abort?

::: toolkit/crashreporter/test/unit/test_crash_moz_crash.js
@@ +2,5 @@
> +{
> +  if (!("@mozilla.org/toolkit/crash-reporter;1" in Components.classes)) {
> +    dump("INFO | test_crash_moz_crash.js | Can't test crashreporter in a non-libxul build.\n");
> +    return;
> +  }

You can ditch this bit now (I know it's in all the other tests).
Attachment #630369 - Flags: review?(ted.mielczarek) → review+
I don't think it should be needed as mozalloc_abort is exported, but we can keep an eye on this.  I ditched the if you mentioned, too.

https://hg.mozilla.org/integration/mozilla-inbound/rev/379e323cede9
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/379e323cede9
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
This checkin introduced a new build warning:
> memory/mozalloc/mozalloc_abort.cpp: In function 'void mozalloc_abort(const char*)':
> memory/mozalloc/mozalloc_abort.cpp:20:1: warning: 'noreturn' function does return

Linux 64-bit debug clobber-build from the push where this made it to m-c:
  https://tbpl.mozilla.org/php/getParsedLog.php?id=12447312&tree=Firefox
Linux 32-bit debug clobber-build from the previous push:
  https://tbpl.mozilla.org/php/getParsedLog.php?id=12443806&tree=Firefox
Depends on: 762675
Depends on: 762880
This patch changed the "crash reason" in crash reports from EXCEPTION_BREAKPOINT to EXCEPTION_ACCESS_VIOLATION_WRITE, which makes searching for these in crash-stats much more difficult. I believe that we should either fix MOZ_CRASH to have the prior more correct behavior of mozalloc_abort ASAP, or revert this patch.
This is atop the patch in bug 763000, which is involved enough that I'd rather not push this above it unless absolutely desired.
Attachment #634124 - Flags: review?(benjamin)
Attachment #634124 - Flags: review?(benjamin) → review+
You need to log in before you can comment on or make changes to this bug.