Mac: ensure crashes are forwarded to system crash reporter [crashpad/crashpad : main]
Leonard Grey (Gerrit)
Leonard Grey added 3 comments![]( "Open in Gerrit")
exc_to_reset & ExcMaskValid(),Should we drop this entirely? All of this logic is pre-10.10
CHECK(!set_state);Just encoding how this is actually called. I can't tell if there was an intention to have this vary at some point
Related details
- Mark Mentovai
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 1 comment![]( "Open in Gerrit")
Thanks! I’ll dig in next week.
Justin expressed interest in reading through this too.
Related details
- Justin Cohen
- Leonard Grey
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen added 1 comment![]( "Open in Gerrit")
Instead, it now listens to EXC_CORPSE_NOTIFY, which is raised postmortem iff an EXC_CRASH handler returns MACH_RCV_PORT_DIED. But we specifically were *not* doing that in order to avoid multiple reports (one from EXC_CRASH and one from EXC_CORPSE_NOTIFY) between 10.11 and 10.14.if
Related details
- Leonard Grey
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen voted and added 1 comment![]( "Open in Gerrit")
Votes added by Justin Cohen
| Code-Review | +1 |
1 comment
LG % Mark's review.
Related details
- Leonard Grey
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen added 1 comment![]( "Open in Gerrit")
Should `- Does assorted related cleanup` be put into a different CL?
Related details
- Leonard Grey
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leonard Grey (Gerrit)
Leonard Grey added 2 comments![]( "Open in Gerrit")
Should `- Does assorted related cleanup` be put into a different CL?
Done, will update with a dependent CL shortly
Instead, it now listens to EXC_CORPSE_NOTIFY, which is raised postmortem iff an EXC_CRASH handler returns MACH_RCV_PORT_DIED. But we specifically were *not* doing that in order to avoid multiple reports (one from EXC_CRASH and one from EXC_CORPSE_NOTIFY) between 10.11 and 10.14.Leonard Greyif
Actually meant iff for "if and only if". Spelled it out instead
Related details
- Justin Cohen
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 5 comments![]( "Open in Gerrit")
Historically, we have forwarded exceptions from Crashpad to Apple's system reporting tool explicitly. However, starting in 10.15, the
crash reporter stopped paying attention to EXC_CRASH type exceptions.
Instead, it now listens to EXC_CORPSE_NOTIFY, which is raised postmortem if and only if an EXC_CRASH handler returns MACH_RCV_PORT_DIED. But we specifically were *not* doing that in order to avoid multiple reports (one from EXC_CRASH and one from EXC_CORPSE_NOTIFY) between 10.11 and 10.14.
This change stops forwarding EXC_CRASH exceptions explicitly and *does* return MACH_RCV_PORT_DIED if they should be forwardedWrap to keep the whole thing within 72 columns.
// handler. See 10.9.4 `xnu-2422.110.17/osfmk/kern/exception.c`.
// `exception_deliver()` will only set a new thread state if the handler’sWouldn’t backtick the filename, but would update the versions for 15.2 xnu-11215.61.5 (the current version we have source for) provided that everything else is correct.
(backticking the function is OK, but avoid the parentheses.)
// if we do both, two crash reports are generated). Due to this, we
// previously returned `KERN_SUCCESS` to prevent corpse generation and
// manually forwarded. This flips that logic to always cause
// `EXC_CORPSE_NOTIFY` generation and never explicitly forward.This change’s commit message is a good place to write about the history and the flipping of the logic. Commit history is good to dig back and learn about what we used to do.
Code comments are a good place to talk about what the code does now, not what it used to do.
// Don’t forward simulated exceptions such as kMachExceptionSimulated to the
// system crash reporter. Only forward the types of exceptions that it wouldThis is a little out of place now.
// receive under normal conditions. Although the system crash reporter is
// able to deal with other exceptions including simulated ones, forwardingI’m not sure if this part is still true, but the rest of the sentence does apply: we don’t want ReportCrash UI to appear for anything other than situations where it would normally appear if we weren’t in the mix.
Related details
- Justin Cohen
- Leonard Grey
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leonard Grey (Gerrit)
Leonard Grey added 5 comments![]( "Open in Gerrit")
Historically, we have forwarded exceptions from Crashpad to Apple's system reporting tool explicitly. However, starting in 10.15, the
crash reporter stopped paying attention to EXC_CRASH type exceptions.
Instead, it now listens to EXC_CORPSE_NOTIFY, which is raised postmortem if and only if an EXC_CRASH handler returns MACH_RCV_PORT_DIED. But we specifically were *not* doing that in order to avoid multiple reports (one from EXC_CRASH and one from EXC_CORPSE_NOTIFY) between 10.11 and 10.14.
This change stops forwarding EXC_CRASH exceptions explicitly and *does* return MACH_RCV_PORT_DIED if they should be forwardedWrap to keep the whole thing within 72 columns.
Done
// handler. See 10.9.4 `xnu-2422.110.17/osfmk/kern/exception.c`.
// `exception_deliver()` will only set a new thread state if the handler’sWouldn’t backtick the filename, but would update the versions for 15.2 xnu-11215.61.5 (the current version we have source for) provided that everything else is correct.
(backticking the function is OK, but avoid the parentheses.)
This part is just the old comment moving over! Done, and yes the description still holds :)
// if we do both, two crash reports are generated). Due to this, we
// previously returned `KERN_SUCCESS` to prevent corpse generation and
// manually forwarded. This flips that logic to always cause
// `EXC_CORPSE_NOTIFY` generation and never explicitly forward.This change’s commit message is a good place to write about the history and the flipping of the logic. Commit history is good to dig back and learn about what we used to do.
Code comments are a good place to talk about what the code does now, not what it used to do.
Done
// Don’t forward simulated exceptions such as kMachExceptionSimulated to the
// system crash reporter. Only forward the types of exceptions that it wouldThis is a little out of place now.
Done
// receive under normal conditions. Although the system crash reporter is
// able to deal with other exceptions including simulated ones, forwardingI’m not sure if this part is still true, but the rest of the sentence does apply: we don’t want ReportCrash UI to appear for anything other than situations where it would normally appear if we weren’t in the mix.
- Justin Cohen
- Mark Mentovai
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 3 comments![]( "Open in Gerrit")
bool forward =
client_options.system_crash_reporter_forwarding != TriState::kDisabled;
if (forward && exception == EXC_CRASH) {Rather than testing `forwad` repeatedly (the next test is on line 223), I’d prefer
```
if (client_options.system_crash_reporter_forwarding != TriState::kDisabled) {
if (exception == EXC_CRASH) {
// explain
return MACH_RCV_PORT_DIED;
}
if (exception == EXC_RESOURCE || exception == EXC_GUARD) {
// explain
// …
}
}
```
as the more understandable, maintainable, and future-proof structure.
if (forward && (exception == EXC_RESOURCE || exception == EXC_GUARD)) {As you noted in the other change, ReportCrash isn’t registered for `EXC_GUARD` on recent OS versions. It must be picking those up as `EXC_CORPSE_NOTIFY`. We wouldn’t want to forward `EXC_GUARD` to it in those cases, right?
EXCEPTION_STATE_IDENTITY | MACH_EXCEPTION_CODES,It’s great if this works for now, but as I noted in your other change, this isn’t how ReportCrash is registered on current macOS, so we might want to look into changing this to match what the OS is doing more closely.
Can you confirm that you’ve tested `EXC_RESOURCE` and `EXC_GUARD`?
Related details
- Justin Cohen
- Leonard Grey
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthew Spelman
Leonard Grey
Leonard Grey (Gerrit)
Leonard Grey added 3 comments![]( "Open in Gerrit")
bool forward =
client_options.system_crash_reporter_forwarding != TriState::kDisabled;
if (forward && exception == EXC_CRASH) {Rather than testing `forwad` repeatedly (the next test is on line 223), I’d prefer
```
if (client_options.system_crash_reporter_forwarding != TriState::kDisabled) {
if (exception == EXC_CRASH) {
// explain
return MACH_RCV_PORT_DIED;
}
if (exception == EXC_RESOURCE || exception == EXC_GUARD) {
// explain
// …
}
}
```as the more understandable, maintainable, and future-proof structure.
Did it that way because one's an early return and the other's a side effect so the structure felt clearer, but sure :)
if (forward && (exception == EXC_RESOURCE || exception == EXC_GUARD)) {As you noted in the other change, ReportCrash isn’t registered for `EXC_GUARD` on recent OS versions. It must be picking those up as `EXC_CORPSE_NOTIFY`. We wouldn’t want to forward `EXC_GUARD` to it in those cases, right?
Done
It’s great if this works for now, but as I noted in your other change, this isn’t how ReportCrash is registered on current macOS, so we might want to look into changing this to match what the OS is doing more closely.
Can you confirm that you’ve tested `EXC_RESOURCE` and `EXC_GUARD`?
Pulling behavior from whatever's registered for CORPSE_NOTIFY now
Related details
- Justin Cohen
- Mark Mentovai
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 10 comments![]( "Open in Gerrit")
// Fetches the standard system exception handler (using whatever is registered
// for `EXC_CORPSE_NOTIFY` as a proxy). The system handler registering for
// `EXC_MASK_CORPSE_NOTIFY` is confirmed back through macOS 10.13.
std::optional<ExceptionPorts::ExceptionHandler> GetStandardHandler() {Let’s name this to convey what it’s actually doing, rather than naming it one thing and using something else as a proxy.
DCHECK_GT(handlers.size(), 0u);
if (handlers.size() > 0) {You’re testing the condition you just `DCHECK`ed? Can it be 0 or not?
A thing that you can safely `DCHECK` is that it’s ≤ 1. And if you decide that it can’t be 0, then you’d just `DCHECK` that it’s 1, exactly.
// For exception handlers that respond to state-carrying behaviors, whenGet a blank // in between these two lines, so that an auto-reflow doesn’t hoist this line up onto the previous one.
Or just get rid of “As background:”, which is kind of noise.
// exception handler. See 15.2 xnu-11215.1.10/osfmk/kern/exception.c.15.3 source is available now, xnu-11215.81.4. Please update comments to refer to what’s current as of the time that things get checked in.
(In fact, in the comment you had here, you name macOS 15.2 but the xnu version that shipped with 15.0.)
// Another side effect of returning `MACH_RCV_PORT_DIED` for `EXC_CRASH`Just an effect, nothing “side” about it.
// Another side effect of returning `MACH_RCV_PORT_DIED` for `EXC_CRASH`
// is that an `EXC_CORPSE_NOTIFY` exception is generated. Starting with
// macOS 10.15, for the system crash reporter to generate a report,
// `EXC_CORPSE_NOTIFY` *must* be generated and forwarding `EXC_CRASH` (as
// we do below with `EXC_RESOURCE` and pre-macOS 13 `EXC_GUARD`) is not
// sufficient. Between macOS 10.11 and macOS 10.14 (inclusive), both
// forwarding as below, and causing `EXC_CORPSE_NOTIFY` to be generated
// are sufficient (and in fact, if we do both, two crash reports are
// generated).This comment is the crux of what needs to be done. Thanks for writing it out so clearly!
return ExcServerSuccessfulReturnValue(exception, behavior, false);Are there non-test callers to this left?
Or are you saving further cleanup of this for a future change?
(MacOSVersionNumber() < 13'00'00 && exception == EXC_GUARD)) {1. I think this reads better if you flip the order of these two: test `exception == EXC_GUARD` before checking the OS version number.
2. If we’re going to get `EXC_GUARD` wrapped in `EXC_CRASH`, the right thing to do is to only register for `EXC_GUARD` on older OS versions where we know the OS won’t do the wrapping. Then, we wouldn’t need to do this version check here.
// Will likely not be used in practice, but we need a backup just inBreak things up vertically. If you find you need to write a comment to explain what follows, that’s also a good hint that it’s a viable spot to break things up with some whitespace.
// Will likely not be used in practice, but we need a backup just in
// case we can't find the default crash handler.
exception_behavior_t behavior =
EXCEPTION_STATE_IDENTITY | MACH_EXCEPTION_CODES;
auto handler = GetStandardHandler();
if (handler.has_value()) {
behavior = handler.value().behavior;
}I don’t really get what you’re doing here, or why, or what the comment is trying to convey.
Related details
- Justin Cohen
- Leonard Grey
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leonard Grey (Gerrit)
Leonard Grey added 9 comments![]( "Open in Gerrit")
// Fetches the standard system exception handler (using whatever is registered
// for `EXC_CORPSE_NOTIFY` as a proxy). The system handler registering for
// `EXC_MASK_CORPSE_NOTIFY` is confirmed back through macOS 10.13.
std::optional<ExceptionPorts::ExceptionHandler> GetStandardHandler() {Let’s name this to convey what it’s actually doing, rather than naming it one thing and using something else as a proxy.
Removed
DCHECK_GT(handlers.size(), 0u);
if (handlers.size() > 0) {You’re testing the condition you just `DCHECK`ed? Can it be 0 or not?
A thing that you can safely `DCHECK` is that it’s ≤ 1. And if you decide that it can’t be 0, then you’d just `DCHECK` that it’s 1, exactly.
Removed
// For exception handlers that respond to state-carrying behaviors, whenGet a blank // in between these two lines, so that an auto-reflow doesn’t hoist this line up onto the previous one.
Or just get rid of “As background:”, which is kind of noise.
Done
// exception handler. See 15.2 xnu-11215.1.10/osfmk/kern/exception.c.15.3 source is available now, xnu-11215.81.4. Please update comments to refer to what’s current as of the time that things get checked in.
(In fact, in the comment you had here, you name macOS 15.2 but the xnu version that shipped with 15.0.)
Done
// Another side effect of returning `MACH_RCV_PORT_DIED` for `EXC_CRASH`Just an effect, nothing “side” about it.
Done
return ExcServerSuccessfulReturnValue(exception, behavior, false);Are there non-test callers to this left?
Or are you saving further cleanup of this for a future change?
Future change
(MacOSVersionNumber() < 13'00'00 && exception == EXC_GUARD)) {1. I think this reads better if you flip the order of these two: test `exception == EXC_GUARD` before checking the OS version number.
2. If we’re going to get `EXC_GUARD` wrapped in `EXC_CRASH`, the right thing to do is to only register for `EXC_GUARD` on older OS versions where we know the OS won’t do the wrapping. Then, we wouldn’t need to do this version check here.
Done (2)
// Will likely not be used in practice, but we need a backup just inBreak things up vertically. If you find you need to write a comment to explain what follows, that’s also a good hint that it’s a viable spot to break things up with some whitespace.
Removed
// Will likely not be used in practice, but we need a backup just in
// case we can't find the default crash handler.
exception_behavior_t behavior =
EXCEPTION_STATE_IDENTITY | MACH_EXCEPTION_CODES;
auto handler = GetStandardHandler();
if (handler.has_value()) {
behavior = handler.value().behavior;
}I don’t really get what you’re doing here, or why, or what the comment is trying to convey.
Discussed offline. Hardcoded `EXCEPTION_STATE_IDENTITY | MACH_EXCEPTION_CODES` appears sufficient.
Related details
- Justin Cohen
- Mark Mentovai
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai voted and added 3 comments![]( "Open in Gerrit")
Votes added by Mark Mentovai
| Code-Review | +1 |
3 comments
exception_mask_t mask = EXC_MASK_CRASH | EXC_MASK_RESOURCE;Good place for a blank line.
return exception_ports.SetExceptionPort(Another good place for a blank line.
new_state, new_state + new_state_forward_count);Not if `!new_state_forward_count`.
Related details
- Justin Cohen
- Leonard Grey
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leonard Grey (Gerrit)
Leonard Grey added 3 comments![]( "Open in Gerrit")
exception_mask_t mask = EXC_MASK_CRASH | EXC_MASK_RESOURCE;Good place for a blank line.
Done
Another good place for a blank line.
Done
new_state, new_state + new_state_forward_count);Not if `!new_state_forward_count`.
Just moving this but `new_state_forward_count ? &new_state_forward[0] : nullptr` takes care of that below, no?
Related details
- Justin Cohen
- Mark Mentovai
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mark Mentovai
| Code-Review | +1 |
1 comment
new_state, new_state + new_state_forward_count);Leonard GreyNot if `!new_state_forward_count`.
Just moving this but `new_state_forward_count ? &new_state_forward[0] : nullptr` takes care of that below, no?
Just moving this but `new_state_forward_count ? &new_state_forward[0] : nullptr` takes care of that below, no?
- Justin Cohen
- Leonard Grey
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leonard Grey (Gerrit)
Leonard Grey added 1 comment![]( "Open in Gerrit")
new_state, new_state + new_state_forward_count);Leonard GreyNot if `!new_state_forward_count`.
Mark MentovaiJust moving this but `new_state_forward_count ? &new_state_forward[0] : nullptr` takes care of that below, no?
Just moving this but `new_state_forward_count ? &new_state_forward[0] : nullptr` takes care of that below, no?
You can’t do pointer arithmetic on a null pointer.
- Justin Cohen
- Mark Mentovai
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 1 comment![]( "Open in Gerrit")
new_state_forward.insert(new_state_forward.end(),Isn’t there an `assign` that you could use instead?
Related details
- Justin Cohen
- Leonard Grey
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leonard Grey (Gerrit)
Leonard Grey added 1 comment![]( "Open in Gerrit")
Isn’t there an `assign` that you could use instead?
- Justin Cohen
- Mark Mentovai
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai voted Code-Review+1![]( "Open in Gerrit")
- Justin Cohen
- Leonard Grey
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leonard Grey (Gerrit)
Leonard Grey voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Justin Cohen
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leonard Grey (Gerrit)
Leonard Grey added 1 comment![]( "Open in Gerrit")
Landing directly because the Fuschia thing is unrelated
Related details
- Justin Cohen
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leonard Grey (Gerrit)
Leonard Grey submitted the change![]( "Open in Gerrit")
Change information
Mac: ensure crashes are forwarded to system crash reporter
Historically, we have forwarded exceptions from Crashpad to Apple's
system reporting tool explicitly. However, starting in 10.15, the crash
reporter stopped paying attention to EXC_CRASH type exceptions.
Instead, it now listens to EXC_CORPSE_NOTIFY, which is raised postmortem
if and only if an EXC_CRASH handler returns MACH_RCV_PORT_DIED. But we
specifically were *not* doing that in order to avoid multiple reports
(one from EXC_CRASH and one from EXC_CORPSE_NOTIFY) between 10.11 and
10.14.
This change stops forwarding EXC_CRASH exceptions explicitly and *does*
return MACH_RCV_PORT_DIED if they should be forwarded
- M client/crashpad_client_mac.cc
- M handler/mac/crash_report_exception_handler.cc
- M tools/run_with_crashpad.md
- M util/mach/exception_types.cc
- M util/mach/exception_types_test.cc
Code-Review: +1 by Mark Mentovai
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |