Closed Bug 761859 Opened 11 years ago Closed 11 years ago

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


(Core :: mozglue, defect)

Not set





(Reporter: Waldo, Assigned: Waldo)




(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.
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 (!(";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.
Target Milestone: --- → mozilla16
Closed: 11 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:
Linux 32-bit debug clobber-build from the previous push:
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.