Last Comment Bug 761859 - mozalloc_abort() should use MOZ_CRASH(), not roll its own crash behavior
: mozalloc_abort() should use MOZ_CRASH(), not roll its own crash behavior
Product: Core
Classification: Components
Component: mozglue (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
Depends on: 762675 762880
  Show dependency treegraph
Reported: 2012-06-05 16:49 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2012-06-19 01:19 PDT (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch with test (5.30 KB, patch)
2012-06-05 16:49 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
ted: review+
Details | Diff | Review
Insert __debugbreak() at the start of MOZ_CRASH() on Windows (1.33 KB, patch)
2012-06-18 11:53 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
benjamin: review+
Details | Diff | Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2012-06-05 16:49:00 PDT
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.
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2012-06-05 16:52:41 PDT
Oh, this patch (plus the one in bug 761857, joined together) passed try.
Comment 2 Ted Mielczarek [:ted.mielczarek] 2012-06-06 04:29:48 PDT
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).
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2012-06-06 11:29:36 PDT
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.
Comment 4 Ed Morley [:emorley] 2012-06-07 05:51:39 PDT
Comment 5 Daniel Holbert [:dholbert] 2012-06-07 11:47:07 PDT
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:
Comment 6 Graeme McCutcheon [:graememcc] 2012-06-12 03:07:07 PDT
Comment 7 Benjamin Smedberg [:bsmedberg] 2012-06-18 10:03:04 PDT
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 [:bsmedberg] 2012-06-18 10:03:54 PDT
In particular the call to __debugbreak at
Comment 9 Jeff Walden [:Waldo] (remove +bmo to email) 2012-06-18 11:53:09 PDT
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.
Comment 10 Jeff Walden [:Waldo] (remove +bmo to email) 2012-06-18 12:35:07 PDT
Comment 11 Ed Morley [:emorley] 2012-06-19 01:19:26 PDT

Note You need to log in before you can comment on or make changes to this bug.