[Mac] Modify the ObjC exception preprocessor to make some exceptions fatal.

[Mac] Modify the ObjC exception preprocessor to make some exceptions fatal.

Various Cocoa routines in the event loop obscure exception stack traces
by catching-and-rethrowing exceptions, so that the original trace from
throwing the exception is lost.

To combat this, the ObjC exception preprocessor will now unwind the stack to
search for an exception handler. If it finds a stack frame with a handler, it
will check that the function's name is not on the sinkhole list. If it is on the
list, the preprocessor will make the exception fatal to produce a stack trace
from the point of throw. If it is not on the list, then the preprocessor will
not take action and will let the normal exception handling mechanism run.

The preprocessor works in conjunction with the base::mac::CallWithEHFrame
system. The preprocessor is only called for exceptions raised with objc_throw,
which C++ exceptions are not. Furthermore, some of the system routines that do
the catch-and-rethrow are higher on the stack than CallWithEHFrame, so it
cannot force those exceptions to be fatal at the point of throw.

This change also wraps -[BrowserCrApplication sendAction:to:from:] in
CallWithEHFrame().

BUG=637270
R=mark@chromium.org

Committed: https://crrev.com/1e145dcb71a3bedf9fde398045cb8e54e244c3e3
Cr-Commit-Position: refs/heads/master@{#437033}

[Robert Sesek, 2016-12-01 02:20:08]

Description was changed from

==========
[Mac] Modify the ObjC exception preprocessor to make some exceptions fatal.

Various Cocoa routines in the event loop obscure exception stack traces
by catching-and-rethrowing exceptions, so that the original trace from
throwing the exception is lost.

To combat this, the ObjC exception preprocessor will now unwind the stack to
search for an exception handler. If it finds a stack frame with a handler, it
will check that the function's name is not on the sinkhole list. If it is on the
list, the preprocessor will make the exception fatal to produce a stack trace
from the point of throw. If it is not on the list, then the preprocessor will
not take action and will let the normal exception handling mechanism run.

The preprocessor works in conjunction with the base::mac::CallWithEHFrame
system. The preprocessor is only called for exceptions raised with objc_throw,
which C++ exceptions are not. Furthermore, some of the system routines that do
the catch-and-rethrow are higher on the stack than CallWithEHFrame, so it
cannot force those exceptions to be fatal at the point of throw.

This change also wraps -[BrowserCrApplication sendAction:to:from:] in
CallWithEHFrame().

BUG=520706,637270,
R=mark@chromium.org
==========

to

==========
[Mac] Modify the ObjC exception preprocessor to make some exceptions fatal.

Various Cocoa routines in the event loop obscure exception stack traces
by catching-and-rethrowing exceptions, so that the original trace from
throwing the exception is lost.

To combat this, the ObjC exception preprocessor will now unwind the stack to
search for an exception handler. If it finds a stack frame with a handler, it
will check that the function's name is not on the sinkhole list. If it is on the
list, the preprocessor will make the exception fatal to produce a stack trace
from the point of throw. If it is not on the list, then the preprocessor will
not take action and will let the normal exception handling mechanism run.

The preprocessor works in conjunction with the base::mac::CallWithEHFrame
system. The preprocessor is only called for exceptions raised with objc_throw,
which C++ exceptions are not. Furthermore, some of the system routines that do
the catch-and-rethrow are higher on the stack than CallWithEHFrame, so it
cannot force those exceptions to be fatal at the point of throw.

This change also wraps -[BrowserCrApplication sendAction:to:from:] in
CallWithEHFrame().

BUG=520706,637270
R=mark@chromium.org
==========

[Robert Sesek, 2016-12-01 02:21:50]

Description was changed from

==========
[Mac] Modify the ObjC exception preprocessor to make some exceptions fatal.

Various Cocoa routines in the event loop obscure exception stack traces
by catching-and-rethrowing exceptions, so that the original trace from
throwing the exception is lost.

To combat this, the ObjC exception preprocessor will now unwind the stack to
search for an exception handler. If it finds a stack frame with a handler, it
will check that the function's name is not on the sinkhole list. If it is on the
list, the preprocessor will make the exception fatal to produce a stack trace
from the point of throw. If it is not on the list, then the preprocessor will
not take action and will let the normal exception handling mechanism run.

The preprocessor works in conjunction with the base::mac::CallWithEHFrame
system. The preprocessor is only called for exceptions raised with objc_throw,
which C++ exceptions are not. Furthermore, some of the system routines that do
the catch-and-rethrow are higher on the stack than CallWithEHFrame, so it
cannot force those exceptions to be fatal at the point of throw.

This change also wraps -[BrowserCrApplication sendAction:to:from:] in
CallWithEHFrame().

BUG=520706,637270
R=mark@chromium.org
==========

to

==========
[Mac] Modify the ObjC exception preprocessor to make some exceptions fatal.

Various Cocoa routines in the event loop obscure exception stack traces
by catching-and-rethrowing exceptions, so that the original trace from
throwing the exception is lost.

To combat this, the ObjC exception preprocessor will now unwind the stack to
search for an exception handler. If it finds a stack frame with a handler, it
will check that the function's name is not on the sinkhole list. If it is on the
list, the preprocessor will make the exception fatal to produce a stack trace
from the point of throw. If it is not on the list, then the preprocessor will
not take action and will let the normal exception handling mechanism run.

The preprocessor works in conjunction with the base::mac::CallWithEHFrame
system. The preprocessor is only called for exceptions raised with objc_throw,
which C++ exceptions are not. Furthermore, some of the system routines that do
the catch-and-rethrow are higher on the stack than CallWithEHFrame, so it
cannot force those exceptions to be fatal at the point of throw.

This change also wraps -[BrowserCrApplication sendAction:to:from:] in
CallWithEHFrame().

BUG=637270
R=mark@chromium.org
==========

[Mark Mentovai, 2016-12-01 19:02:33]

Cool! Mostly minor comments.

https://codereview.chromium.org/2543813003/diff/1/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_application_mac.mm (right):

https://codereview.chromium.org/2543813003/diff/1/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_application_mac.mm:241: rv = [super
sendAction:anAction to:aTarget from:sender];
I’ve got half a mind to suggest making the block return void* and propagate that
from CallWithEHFrame(), but it’s OK either way.

https://codereview.chromium.org/2543813003/diff/1/chrome/browser/mac/exceptio...
File chrome/browser/mac/exception_processor.h (right):

https://codereview.chromium.org/2543813003/diff/1/chrome/browser/mac/exceptio...
chrome/browser/mac/exception_processor.h:25: 
Extra blank line?

https://codereview.chromium.org/2543813003/diff/1/chrome/browser/mac/exceptio...
chrome/browser/mac/exception_processor.h:26: // Bin for unknown exceptions.
Exposed for testing purposes.
“Exposed for testing purposes” on all of these isn’t necessary, because you’ve
put them into the “exposed only for testing” section titled above.

https://codereview.chromium.org/2543813003/diff/1/chrome/browser/mac/exceptio...
chrome/browser/mac/exception_processor.h:27: extern const size_t
kUnknownNSException;
Can be constexpr and get its value right here?

https://codereview.chromium.org/2543813003/diff/1/chrome/browser/mac/exceptio...
File chrome/browser/mac/exception_processor.mm (right):

https://codereview.chromium.org/2543813003/diff/1/chrome/browser/mac/exceptio...
chrome/browser/mac/exception_processor.mm:26: const size_t
kKnownNSExceptionCount = 25;
Can constexpr these two also.

https://codereview.chromium.org/2543813003/diff/1/chrome/browser/mac/exceptio...
chrome/browser/mac/exception_processor.mm:34: static NSString* const
kKnownNSExceptionNames[] = {
static doesn’t contribute to anything here.

https://codereview.chromium.org/2543813003/diff/1/chrome/browser/mac/exceptio...
chrome/browser/mac/exception_processor.mm:55: nil
Instead of a nil-terminated list, just use arraysize in the loop below. Remember
to use _LT or < for the check that follows, too.

https://codereview.chromium.org/2543813003/diff/1/chrome/browser/mac/exceptio...
chrome/browser/mac/exception_processor.mm:59:
DCHECK_LE(arraysize(kKnownNSExceptionNames), kKnownNSExceptionCount);
constexpr benefit: static_cast instead of DCHECK!

https://codereview.chromium.org/2543813003/diff/1/chrome/browser/mac/exceptio...
chrome/browser/mac/exception_processor.mm:77: static const char*
kExceptionSinkholes[] = {
const the pointers too.

https://codereview.chromium.org/2543813003/diff/1/chrome/browser/mac/exceptio...
chrome/browser/mac/exception_processor.mm:82: id ObjcExceptionPreprocessor(id
exception) {
Should be static.

Or introduce an unnamed namespace and group all of the other static and
non-extern const things into it.

https://codereview.chromium.org/2543813003/diff/1/chrome/browser/mac/exceptio...
chrome/browser/mac/exception_processor.mm:107: // Unwind the stack looking for
any exception handlers. If an exception
Oh good, new stuff!

https://codereview.chromium.org/2543813003/diff/1/chrome/browser/mac/exceptio...
chrome/browser/mac/exception_processor.mm:120: int result = unw_step(&cursor);
Why not fold this into the loop condition?: while (unw_step(&cursor) > 0)

https://codereview.chromium.org/2543813003/diff/1/chrome/browser/mac/exceptio...
chrome/browser/mac/exception_processor.mm:133: unw_word_t name_len;
This is not name_len, it’s the offset of unw_get_reg(cursor, UNW_REG_IP)
relative to the address of the symbol whose name is returned.

https://codereview.chromium.org/2543813003/diff/1/chrome/browser/mac/exceptio...
chrome/browser/mac/exception_processor.mm:141: // by crashing at the point of
throw.
by crashing here at the point of throw

https://codereview.chromium.org/2543813003/diff/1/chrome/browser/mac/exceptio...
chrome/browser/mac/exception_processor.mm:144: LOG(FATAL) << "Terminating from
Objective-C exception: "
It might be neat if we could do something to get the actual exception thrower’s
context to appear at the top of the stack instead of this preprocessor function.

Barring that, what do you think about calling a function named for what’s
actually happening, like TerminateOnObjectiveCException(), and have death happen
there? You know, Apple-style. That’d at least make the top of the stack in
symbolized crash reports look like the Objective-C exception was the problem and
less like ObjcExceptionPreprocessor() has a crash bug. Because you know
someone’s gonna think that…

https://codereview.chromium.org/2543813003/diff/1/chrome/browser/mac/exceptio...
chrome/browser/mac/exception_processor.mm:170: if (!g_next_preprocessor)
If there was no previous preprocessor, this won’t restore the no-preprocessor
state. I expect that no previous preprocessor is legitimate because you test for
it on line 155.

[Robert Sesek, 2016-12-06 23:17:39]

The CQ bit was checked by rsesek@chromium.org to run a CQ dry run

[Robert Sesek, 2016-12-06 23:18:09]

https://codereview.chromium.org/2543813003/diff/1/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_application_mac.mm (right):

https://codereview.chromium.org/2543813003/diff/1/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_application_mac.mm:241: rv = [super
sendAction:anAction to:aTarget from:sender];
On 2016/12/01 19:02:32, Mark Mentovai wrote:
> I’ve got half a mind to suggest making the block return void* and propagate
that
> from CallWithEHFrame(), but it’s OK either way.

I prefer this, since it's strongly typed.

https://codereview.chromium.org/2543813003/diff/1/chrome/browser/mac/exceptio...
File chrome/browser/mac/exception_processor.h (right):

https://codereview.chromium.org/2543813003/diff/1/chrome/browser/mac/exceptio...
chrome/browser/mac/exception_processor.h:25: 
On 2016/12/01 19:02:32, Mark Mentovai wrote:
> Extra blank line?

Done.

https://codereview.chromium.org/2543813003/diff/1/chrome/browser/mac/exceptio...
chrome/browser/mac/exception_processor.h:26: // Bin for unknown exceptions.
Exposed for testing purposes.
On 2016/12/01 19:02:32, Mark Mentovai wrote:
> “Exposed for testing purposes” on all of these isn’t necessary, because you’ve
> put them into the “exposed only for testing” section titled above.

Done.

https://codereview.chromium.org/2543813003/diff/1/chrome/browser/mac/exceptio...
chrome/browser/mac/exception_processor.h:27: extern const size_t
kUnknownNSException;
On 2016/12/01 19:02:32, Mark Mentovai wrote:
> Can be constexpr and get its value right here?

Its value is kKnownNSExceptionCount, which isn't part of the header.

https://codereview.chromium.org/2543813003/diff/1/chrome/browser/mac/exceptio...
File chrome/browser/mac/exception_processor.mm (right):

https://codereview.chromium.org/2543813003/diff/1/chrome/browser/mac/exceptio...
chrome/browser/mac/exception_processor.mm:26: const size_t
kKnownNSExceptionCount = 25;
On 2016/12/01 19:02:33, Mark Mentovai wrote:
> Can constexpr these two also.

Did this one, but not the latter (per comment in other file).

https://codereview.chromium.org/2543813003/diff/1/chrome/browser/mac/exceptio...
chrome/browser/mac/exception_processor.mm:34: static NSString* const
kKnownNSExceptionNames[] = {
On 2016/12/01 19:02:33, Mark Mentovai wrote:
> static doesn’t contribute to anything here.

Done.

https://codereview.chromium.org/2543813003/diff/1/chrome/browser/mac/exceptio...
chrome/browser/mac/exception_processor.mm:55: nil
On 2016/12/01 19:02:33, Mark Mentovai wrote:
> Instead of a nil-terminated list, just use arraysize in the loop below.
Remember
> to use _LT or < for the check that follows, too.

Done.

https://codereview.chromium.org/2543813003/diff/1/chrome/browser/mac/exceptio...
chrome/browser/mac/exception_processor.mm:59:
DCHECK_LE(arraysize(kKnownNSExceptionNames), kKnownNSExceptionCount);
On 2016/12/01 19:02:33, Mark Mentovai wrote:
> constexpr benefit: static_cast instead of DCHECK!

Done.

https://codereview.chromium.org/2543813003/diff/1/chrome/browser/mac/exceptio...
chrome/browser/mac/exception_processor.mm:77: static const char*
kExceptionSinkholes[] = {
On 2016/12/01 19:02:33, Mark Mentovai wrote:
> const the pointers too.

Done.

https://codereview.chromium.org/2543813003/diff/1/chrome/browser/mac/exceptio...
chrome/browser/mac/exception_processor.mm:82: id ObjcExceptionPreprocessor(id
exception) {
On 2016/12/01 19:02:32, Mark Mentovai wrote:
> Should be static.
> 
> Or introduce an unnamed namespace and group all of the other static and
> non-extern const things into it.

Made static because the anon namespace adds extra junk to the crash signatures.

https://codereview.chromium.org/2543813003/diff/1/chrome/browser/mac/exceptio...
chrome/browser/mac/exception_processor.mm:120: int result = unw_step(&cursor);
On 2016/12/01 19:02:33, Mark Mentovai wrote:
> Why not fold this into the loop condition?: while (unw_step(&cursor) > 0)

Done.

https://codereview.chromium.org/2543813003/diff/1/chrome/browser/mac/exceptio...
chrome/browser/mac/exception_processor.mm:133: unw_word_t name_len;
On 2016/12/01 19:02:33, Mark Mentovai wrote:
> This is not name_len, it’s the offset of unw_get_reg(cursor, UNW_REG_IP)
> relative to the address of the symbol whose name is returned.

Oops, done.

https://codereview.chromium.org/2543813003/diff/1/chrome/browser/mac/exceptio...
chrome/browser/mac/exception_processor.mm:141: // by crashing at the point of
throw.
On 2016/12/01 19:02:33, Mark Mentovai wrote:
> by crashing here at the point of throw

Done.

https://codereview.chromium.org/2543813003/diff/1/chrome/browser/mac/exceptio...
chrome/browser/mac/exception_processor.mm:144: LOG(FATAL) << "Terminating from
Objective-C exception: "
On 2016/12/01 19:02:33, Mark Mentovai wrote:
> It might be neat if we could do something to get the actual exception
thrower’s
> context to appear at the top of the stack instead of this preprocessor
function.

I considered this but it may make things less clear, and it's more complicated
to do correctly.

> Barring that, what do you think about calling a function named for what’s
> actually happening, like TerminateOnObjectiveCException(), and have death
happen
> there? You know, Apple-style. That’d at least make the top of the stack in
> symbolized crash reports look like the Objective-C exception was the problem
and
> less like ObjcExceptionPreprocessor() has a crash bug. Because you know
> someone’s gonna think that…

Done.

https://codereview.chromium.org/2543813003/diff/1/chrome/browser/mac/exceptio...
chrome/browser/mac/exception_processor.mm:170: if (!g_next_preprocessor)
On 2016/12/01 19:02:33, Mark Mentovai wrote:
> If there was no previous preprocessor, this won’t restore the no-preprocessor
> state. I expect that no previous preprocessor is legitimate because you test
for
> it on line 155.

Done.

[commit-bot: I haz the power, 2016-12-06 23:18:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-06 23:30:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-06 23:30:12]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[Mark Mentovai, 2016-12-07 18:50:59]

LGTM

[Robert Sesek, 2016-12-07 18:57:43]

The CQ bit was checked by rsesek@chromium.org

[commit-bot: I haz the power, 2016-12-07 18:58:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-07 19:40:33]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1481137063021220,
"parent_rev": "3ac783fb2d100dc24c102da98c3daf782343af23", "commit_rev":
"1b7379f4523e8db2917e6c372ecb9fe8a777cd7e"}

[commit-bot: I haz the power, 2016-12-07 19:40:58]

Description was changed from

==========
[Mac] Modify the ObjC exception preprocessor to make some exceptions fatal.

Various Cocoa routines in the event loop obscure exception stack traces
by catching-and-rethrowing exceptions, so that the original trace from
throwing the exception is lost.

To combat this, the ObjC exception preprocessor will now unwind the stack to
search for an exception handler. If it finds a stack frame with a handler, it
will check that the function's name is not on the sinkhole list. If it is on the
list, the preprocessor will make the exception fatal to produce a stack trace
from the point of throw. If it is not on the list, then the preprocessor will
not take action and will let the normal exception handling mechanism run.

The preprocessor works in conjunction with the base::mac::CallWithEHFrame
system. The preprocessor is only called for exceptions raised with objc_throw,
which C++ exceptions are not. Furthermore, some of the system routines that do
the catch-and-rethrow are higher on the stack than CallWithEHFrame, so it
cannot force those exceptions to be fatal at the point of throw.

This change also wraps -[BrowserCrApplication sendAction:to:from:] in
CallWithEHFrame().

BUG=637270
R=mark@chromium.org
==========

to

==========
[Mac] Modify the ObjC exception preprocessor to make some exceptions fatal.

Various Cocoa routines in the event loop obscure exception stack traces
by catching-and-rethrowing exceptions, so that the original trace from
throwing the exception is lost.

To combat this, the ObjC exception preprocessor will now unwind the stack to
search for an exception handler. If it finds a stack frame with a handler, it
will check that the function's name is not on the sinkhole list. If it is on the
list, the preprocessor will make the exception fatal to produce a stack trace
from the point of throw. If it is not on the list, then the preprocessor will
not take action and will let the normal exception handling mechanism run.

The preprocessor works in conjunction with the base::mac::CallWithEHFrame
system. The preprocessor is only called for exceptions raised with objc_throw,
which C++ exceptions are not. Furthermore, some of the system routines that do
the catch-and-rethrow are higher on the stack than CallWithEHFrame, so it
cannot force those exceptions to be fatal at the point of throw.

This change also wraps -[BrowserCrApplication sendAction:to:from:] in
CallWithEHFrame().

BUG=637270
R=mark@chromium.org
==========

[commit-bot: I haz the power, 2016-12-07 19:40:59]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-12-07 19:42:28]

Description was changed from

==========
[Mac] Modify the ObjC exception preprocessor to make some exceptions fatal.

Various Cocoa routines in the event loop obscure exception stack traces
by catching-and-rethrowing exceptions, so that the original trace from
throwing the exception is lost.

To combat this, the ObjC exception preprocessor will now unwind the stack to
search for an exception handler. If it finds a stack frame with a handler, it
will check that the function's name is not on the sinkhole list. If it is on the
list, the preprocessor will make the exception fatal to produce a stack trace
from the point of throw. If it is not on the list, then the preprocessor will
not take action and will let the normal exception handling mechanism run.

The preprocessor works in conjunction with the base::mac::CallWithEHFrame
system. The preprocessor is only called for exceptions raised with objc_throw,
which C++ exceptions are not. Furthermore, some of the system routines that do
the catch-and-rethrow are higher on the stack than CallWithEHFrame, so it
cannot force those exceptions to be fatal at the point of throw.

This change also wraps -[BrowserCrApplication sendAction:to:from:] in
CallWithEHFrame().

BUG=637270
R=mark@chromium.org
==========

to

==========
[Mac] Modify the ObjC exception preprocessor to make some exceptions fatal.

Various Cocoa routines in the event loop obscure exception stack traces
by catching-and-rethrowing exceptions, so that the original trace from
throwing the exception is lost.

To combat this, the ObjC exception preprocessor will now unwind the stack to
search for an exception handler. If it finds a stack frame with a handler, it
will check that the function's name is not on the sinkhole list. If it is on the
list, the preprocessor will make the exception fatal to produce a stack trace
from the point of throw. If it is not on the list, then the preprocessor will
not take action and will let the normal exception handling mechanism run.

The preprocessor works in conjunction with the base::mac::CallWithEHFrame
system. The preprocessor is only called for exceptions raised with objc_throw,
which C++ exceptions are not. Furthermore, some of the system routines that do
the catch-and-rethrow are higher on the stack than CallWithEHFrame, so it
cannot force those exceptions to be fatal at the point of throw.

This change also wraps -[BrowserCrApplication sendAction:to:from:] in
CallWithEHFrame().

BUG=637270
R=mark@chromium.org

Committed: https://crrev.com/1e145dcb71a3bedf9fde398045cb8e54e244c3e3
Cr-Commit-Position: refs/heads/master@{#437033}
==========

[commit-bot: I haz the power, 2016-12-07 19:42:30]

Patchset 2 (id:??) landed as
https://crrev.com/1e145dcb71a3bedf9fde398045cb8e54e244c3e3
Cr-Commit-Position: refs/heads/master@{#437033}