Ensure the OSX crash catcher does not trigger on SIGABRT when disabled.

Ensure the OSX crash catcher does not trigger on SIGABRT when disabled.

This CL also re-enables NaClBrowserTestNewlib.Bad. This test was
disabled because there were unautomated "visual tests" embeded in the
test's html. These embeds declared a pdf content type.  When the PDF
plugin was open sourced, this content type started being recognized
and the test broke.

This test was historically flaky, however. One reason  was that pauses
caused by the OS crash catcher could cause the test to time out. The
OS crash catcher was triggering because NaCl calls abort() in response
to many fatal errors. This test is being re-enabled to show abort is
no longer triggering the crash catcher.

BUG=375103
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278617

[Nick Bray (chromium), 2014-06-19 18:05:53]

Blocking: https://codereview.chromium.org/338353008/

[Mark Seaborn, 2014-06-19 18:28:14]

https://codereview.chromium.org/347803004/diff/1/base/mac/os_crash_dumps.cc
File base/mac/os_crash_dumps.cc (right):

https://codereview.chromium.org/347803004/diff/1/base/mac/os_crash_dumps.cc#n...
base/mac/os_crash_dumps.cc:35: SIGABRT,  // EXC_CRASH
FWIW, the comment is misleading.

See
http://lackingrhoticity.blogspot.com/2013/08/handling-crashes-on-mac-os-x.html

Any hardware fault will trigger a Mach exception like EXC_BAD_INSTRUCTION,
EXC_ARITHMETIC, EXC_BAD_ACCESS, etc.

If the Mach exception handler passes the exception on, it gets converted to a
POSIX signal: SIGFPE, SIGBUS, etc.

If a POSIX signal is unhandled, it triggers the EXC_CRASH Mach exception.

So any fatal POSIX signal will be converted to EXC_CRASH.  SIGABRT is never
generated by a hardware fault, only by syscalls like kill() and raise().

So, can you write something like:
  const int signals_to_intercept[] = {
    // Hardware faults
    SIGILL,   // EXC_BAD_INSTRUCTION
    SIGTRAP,  // EXC_BREAKPOINT
    SIGFPE,   // EXC_ARITHMETIC
    SIGBUS,   // EXC_BAD_ACCESS
    SIGSEGV   // EXC_BAD_ACCESS
    // Not a hardware fault
    SIGABRT,
  };

https://codereview.chromium.org/347803004/diff/1/chrome/test/nacl/nacl_browse...
File chrome/test/nacl/nacl_browsertest.cc (left):

https://codereview.chromium.org/347803004/diff/1/chrome/test/nacl/nacl_browse...
chrome/test/nacl/nacl_browsertest.cc:91: // crbug.com/375103: fails on Linux too
after open sourcing PDF plugin.
This issue seems unrelated, since Linux wouldn't be affected by
base/mac/os_crash_dumps.cc.  Not sure if it's safe to re-enable this for Linux.

Any idea what was going on with the PDF plugin?

[Nick Bray (chromium), 2014-06-19 18:38:05]

https://codereview.chromium.org/347803004/diff/1/chrome/test/nacl/nacl_browse...
File chrome/test/nacl/nacl_browsertest.cc (left):

https://codereview.chromium.org/347803004/diff/1/chrome/test/nacl/nacl_browse...
chrome/test/nacl/nacl_browsertest.cc:91: // crbug.com/375103: fails on Linux too
after open sourcing PDF plugin.
On 2014/06/19 18:28:14, Mark Seaborn wrote:
> This issue seems unrelated, since Linux wouldn't be affected by
> base/mac/os_crash_dumps.cc.  Not sure if it's safe to re-enable this for
Linux.
> 
> Any idea what was going on with the PDF plugin?

The only reason open sourcing the PDF plugin affected this test was because
there were two "visual tests" that used a pdf mimetype.  I didn't bother digging
into it because  1) visual tests in an otherwise automated test are stupid and
2) trying to use a mimetype that may already be registered is stupid.  So, with
so much obvious stupid on the table, I saw no real reason to dig into this -
just remove the code in question.

That being said, this test is the best test we have for this bug, and it needed
to be re-enabled.  So yes, it's also being re-enabled for Linux but the reasons
it was disabled were trivial, so it's not a huge leap.

[Nick Bray (chromium), 2014-06-19 20:49:55]

PTAL

https://codereview.chromium.org/347803004/diff/1/base/mac/os_crash_dumps.cc
File base/mac/os_crash_dumps.cc (right):

https://codereview.chromium.org/347803004/diff/1/base/mac/os_crash_dumps.cc#n...
base/mac/os_crash_dumps.cc:35: SIGABRT,  // EXC_CRASH
On 2014/06/19 18:28:14, Mark Seaborn wrote:
> FWIW, the comment is misleading.
> 
> See
> http://lackingrhoticity.blogspot.com/2013/08/handling-crashes-on-mac-os-x.html
> 
> Any hardware fault will trigger a Mach exception like EXC_BAD_INSTRUCTION,
> EXC_ARITHMETIC, EXC_BAD_ACCESS, etc.
> 
> If the Mach exception handler passes the exception on, it gets converted to a
> POSIX signal: SIGFPE, SIGBUS, etc.
> 
> If a POSIX signal is unhandled, it triggers the EXC_CRASH Mach exception.
> 
> So any fatal POSIX signal will be converted to EXC_CRASH.  SIGABRT is never
> generated by a hardware fault, only by syscalls like kill() and raise().
> 
> So, can you write something like:
>   const int signals_to_intercept[] = {
>     // Hardware faults
>     SIGILL,   // EXC_BAD_INSTRUCTION
>     SIGTRAP,  // EXC_BREAKPOINT
>     SIGFPE,   // EXC_ARITHMETIC
>     SIGBUS,   // EXC_BAD_ACCESS
>     SIGSEGV   // EXC_BAD_ACCESS
>     // Not a hardware fault
>     SIGABRT,
>   };

Done.

[Nick Bray (chromium), 2014-06-19 20:54:28]

Switching reviewer to mark@ because thakis@ is semi-OOO.

[Mark Mentovai, 2014-06-19 21:12:44]

LGTM in base. Thanks to mseaborn for saying what I would have said during his
review of an earlier patch set.

[Mark Seaborn, 2014-06-19 22:14:32]

https://codereview.chromium.org/347803004/diff/1/chrome/test/nacl/nacl_browse...
File chrome/test/nacl/nacl_browsertest.cc (left):

https://codereview.chromium.org/347803004/diff/1/chrome/test/nacl/nacl_browse...
chrome/test/nacl/nacl_browsertest.cc:91: // crbug.com/375103: fails on Linux too
after open sourcing PDF plugin.
On 2014/06/19 18:38:05, Nick Bray (chromium) wrote:
> On 2014/06/19 18:28:14, Mark Seaborn wrote:
> > This issue seems unrelated, since Linux wouldn't be affected by
> > base/mac/os_crash_dumps.cc.  Not sure if it's safe to re-enable this for
> Linux.
> > 
> > Any idea what was going on with the PDF plugin?
> 
> The only reason open sourcing the PDF plugin affected this test was because
> there were two "visual tests" that used a pdf mimetype.  I didn't bother
digging
> into it because  1) visual tests in an otherwise automated test are stupid and
> 2) trying to use a mimetype that may already be registered is stupid.  So,
with
> so much obvious stupid on the table, I saw no real reason to dig into this -
> just remove the code in question.
> 
> That being said, this test is the best test we have for this bug, and it
needed
> to be re-enabled.  So yes, it's also being re-enabled for Linux but the
reasons
> it was disabled were trivial, so it's not a huge leap.

OK, I see now.  That wasn't obvious from the commit message.  Can you explain
that in the commit message, please?  i.e. Explain why changing ppapi_bad.html
fixes the issue.  Then LGTM.

[Nick Bray (chromium), 2014-06-19 22:27:53]

The CQ bit was checked by ncbray@chromium.org

[commit-bot: I haz the power, 2014-06-19 22:30:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ncbray@chromium.org/347803004/20001

[commit-bot: I haz the power, 2014-06-20 05:32:31]

Change committed as 278617