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

RESOLVED FIXED in mozilla16

Status

()

Core
mozglue
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

unspecified
mozilla16
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Created attachment 630369 [details] [diff] [review]
Patch with test

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

Comment 4

5 years ago
https://hg.mozilla.org/mozilla-central/rev/379e323cede9
Status: ASSIGNED → RESOLVED
Last Resolved: 5 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
https://hg.mozilla.org/mozilla-central/rev/93214bac07b2
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.
In particular the call to __debugbreak at http://hg.mozilla.org/releases/mozilla-release/annotate/2b643ea8edf9/memory/mozalloc/mozalloc_abort.cpp#l79
Created attachment 634124 [details] [diff] [review]
Insert __debugbreak() at the start of MOZ_CRASH() on Windows

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+
https://hg.mozilla.org/integration/mozilla-inbound/rev/229623f7ea72
https://hg.mozilla.org/mozilla-central/rev/229623f7ea72
You need to log in before you can comment on or make changes to this bug.