Don't trigger EXC_CORPSE_NOTIFY on OS X 10.11 (issue 1305893010 by mark@chromium.org)

[ma...@chromium.org]

Reviewers: Greg Kerr,


https://codereview.chromium.org/1305893010/diff/100001/compat/mac/mach/mach.h
File compat/mac/mach/mach.h (left):

https://codereview.chromium.org/1305893010/diff/100001/compat/mac/mach/mach.h#oldcode47
compat/mac/mach/mach.h:47: #undef EXC_MASK_VALID
The SDK never exposed this, it was always KERNEL_PRIVATE. But it did
provide the inspiration for the ExcMaskValid() function.

Description:
Don't trigger EXC_CORPSE_NOTIFY on OS X 10.11

CrashReportExceptionHandler::CatchMachException() must always set a
valid new_state. Failing to do so appears to trigger corpse generation
on OS X 10.11. This is addressed by calling ExcServerCopyState().
Previously, this was not done for exceptions forwarded to the user
ReportCrash, under the apparent mistaken assumption that ReportCrash
would do it. However, ReportCrash is given copies of out-parameters like
new_state to explicitly prevent it from influencing Crashpad's returned
state.

ExcServerSuccessfulReturnValue() must not return MACH_RCV_PORT_DIED for
an EXC_CRASH handler on OS X 10.11. This appears to trigger corpse
generation. This is addressed by always returning KERN_SUCCESS from
EXC_CRASH handlers on OS X 10.11.

This also adds generic EXC_CORPSE_NOTIFY support throughout Crashpad.
The crashpad_handler does not listen for this exception type, but it is
now possible to work with this exception type using tools like
exception_port_tool and catch_exception_tool.

BUG=crashpad:48
TEST=Crashes handled by crashpad_handler do not result in the generation
of reports in the root /Library/Logs/DiagnosticReports.

Please review this at https://codereview.chromium.org/1305893010/

Base URL: https://chromium.googlesource.com/crashpad/crashpad@master

Affected files (+282, -86 lines):
M client/crashpad_client_mac.cc
M client/simulate_crash_mac_test.cc
M compat/mac/AvailabilityMacros.h
M compat/mac/mach/mach.h
M compat/non_mac/mach/mach.h
M handler/mac/crash_report_exception_handler.cc
M snapshot/mac/mach_o_image_annotations_reader_test.cc
M tools/mac/catch_exception_tool.cc
M tools/mac/exception_port_tool.ad
M tools/mac/exception_port_tool.cc
M util/mach/exc_server_variants.h
M util/mach/exc_server_variants.cc
M util/mach/exc_server_variants_test.cc
M util/mach/exception_ports_test.cc
M util/mach/mach_extensions.h
M util/mach/mach_extensions.cc
M util/mach/mach_extensions_test.cc
M util/mach/symbolic_constants_mach.cc

[ma...@chromium.org]

Thanks, Greg. I'd like Robert to have a look also.

https://codereview.chromium.org/1305893010/

[rse...@chromium.org]

LGTM w/ nits


https://codereview.chromium.org/1305893010/diff/100001/util/mach/exc_server_variants.h
File util/mach/exc_server_variants.h (right):

https://codereview.chromium.org/1305893010/diff/100001/util/mach/exc_server_variants.h#newcode158
util/mach/exc_server_variants.h:158: //! handler is valid, because it
will be passed to `thread_set_status()`.
Do you need the `` here since thread_set_status has the () ?

https://codereview.chromium.org/1305893010/diff/100001/util/mach/exc_server_variants_test.cc
File util/mach/exc_server_variants_test.cc (right):

https://codereview.chromium.org/1305893010/diff/100001/util/mach/exc_server_variants_test.cc#newcode1221
util/mach/exc_server_variants_test.cc:1221:
Extra blank line?

https://codereview.chromium.org/1305893010/diff/100001/util/mach/mach_extensions.cc
File util/mach/mach_extensions.cc (right):

https://codereview.chromium.org/1305893010/diff/100001/util/mach/mach_extensions.cc#newcode93
util/mach/mach_extensions.cc:93: const exception_mask_t
kExcMaskValid_10_11 =
Add a comment "10.11 added EXC_MASK_CORPSE_NOTIFY..." like above.

https://codereview.chromium.org/1305893010/

[ma...@chromium.org]

https://codereview.chromium.org/1305893010/diff/100001/util/mach/exc_server_variants.h
File util/mach/exc_server_variants.h (right):

https://codereview.chromium.org/1305893010/diff/100001/util/mach/exc_server_variants.h#newcode158
util/mach/exc_server_variants.h:158: //! handler is valid, because it
will be passed to `thread_set_status()`.

Robert Sesek wrote:
> Do you need the `` here since thread_set_status has the () ?

thread_set_status isn't special to Doxygen because it's in a system
header that it never sees and doesn't have documentation for. The ()
doesn't make Doxygen render it any differently, but the `` does.

https://codereview.chromium.org/1305893010/