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

Status: RESOLVED FIXED

(Core :: mozglue, defect)

Description

[Jeff Walden [:Waldo]]

Attachment: 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.

Comment 1

[Jeff Walden [:Waldo]]

Oh, this patch (plus the one in bug 761857, joined together) passed try.

https://tbpl.mozilla.org/?tree=Try&rev=deb12dbebfc7

Comment 2

[(not currently active) Ted Mielczarek]

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).

Comment 3

[Jeff Walden [:Waldo]]

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

Comment 4

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/379e323cede9

Comment 5

[Daniel Holbert [:dholbert]]

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

Comment 6

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/93214bac07b2

Comment 7

[Benjamin Smedberg]

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.

Comment 8

[Benjamin Smedberg]

In particular the call to __debugbreak at http://hg.mozilla.org/releases/mozilla-release/annotate/2b643ea8edf9/memory/mozalloc/mozalloc_abort.cpp#l79

Comment 9

[Jeff Walden [:Waldo]]

Attachment: 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.

Comment 10

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/229623f7ea72

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/229623f7ea72