[Mac] Use Crashpad in the AlertNotificationService.xpc.

[Mac] Use Crashpad in the AlertNotificationService.xpc.

Launchd does not transfer the service owner's exception port to the service,
nor can an exception handler be specified as an appliction domain service name
in a service's Info.plist (that can only target a higher-level domain service).

Since Crashpad is not registered as a system or user domain service, its handler
port is instead sent to the XPC service, where the service can configure
Crashpad itself.

BUG=699607
Review-Url: https://codereview.chromium.org/2748903003
Cr-Commit-Position: refs/heads/master@{#457244}
Committed: https://chromium.googlesource.com/chromium/src/+/25df70294a0c70bb8b39a30ea69ef0be072c4097

[Robert Sesek, 2017-03-14 22:04:06]

rsesek@chromium.org changed reviewers:
+ mark@chromium.org, miguelg@chromium.org

[Robert Sesek, 2017-03-14 22:04:07]

The XPC part of this is pretty much set. But we may need to make some larger
changes to the way the CrashpadClient is exposed, since none of the logic done
in InitializeCrashpadImpl() is being performed in the service. Namely, any
annotations that we'd want to set in the service (e.g. process type) aren't. I
wanted to get feedback on that, but perhaps that can come later (since we
currently get no crash reports from XPC).

[Robert Sesek, 2017-03-14 22:14:54]

https://codereview.chromium.org/2748903003/diff/1/components/crash/content/ap...
File components/crash/content/app/crashpad_mac.mm (right):

https://codereview.chromium.org/2748903003/diff/1/components/crash/content/ap...
components/crash/content/app/crashpad_mac.mm:37: LAZY_INSTANCE_INITIALIZER;
Also worth noting that this is how crashpad_win.cc does things too. Should we
combine into crashpad.cc?

[Mark Mentovai, 2017-03-14 23:24:52]

LG mostly

https://codereview.chromium.org/2748903003/diff/1/chrome/browser/notification...
File chrome/browser/notifications/notification_platform_bridge_mac.mm (right):

https://codereview.chromium.org/2748903003/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_platform_bridge_mac.mm:1: // Copyright
2015 The Chromium Authors. All rights reserved.
> Since Crashpad is not registered as a domain service, its handler port is

…and it’s probably not reasonable for us to expose an XPC service from Crashpad,
right? We’d still need to push the handler port through XPC with that mechanism,
and it’s probably a bunch of extra plumbing and plists.

https://codereview.chromium.org/2748903003/diff/1/chrome/browser/ui/cocoa/not...
File chrome/browser/ui/cocoa/notifications/alert_notification_service.mm
(right):

https://codereview.chromium.org/2748903003/diff/1/chrome/browser/ui/cocoa/not...
chrome/browser/ui/cocoa/notifications/alert_notification_service.mm:39:
didSetExceptionPort_ = client.SetHandlerMachPort(std::move(sendRight));
How much stuff runs in the service before we’re able to do this?

https://codereview.chromium.org/2748903003/diff/1/chrome/browser/ui/cocoa/not...
File chrome/browser/ui/cocoa/notifications/xpc_mach_port.h (right):

https://codereview.chromium.org/2748903003/diff/1/chrome/browser/ui/cocoa/not...
chrome/browser/ui/cocoa/notifications/xpc_mach_port.h:22: // Instead, this class
encodes Mach port OOL data using the private
Seems like we might want to have a test for at least this portion.

https://codereview.chromium.org/2748903003/diff/1/components/crash/content/ap...
File components/crash/content/app/crashpad.h (right):

https://codereview.chromium.org/2748903003/diff/1/components/crash/content/ap...
components/crash/content/app/crashpad.h:16: #if defined(OS_MACOSX)
#include "build/build_config.h" first (this file should have had it already).

https://codereview.chromium.org/2748903003/diff/1/components/crash/content/ap...
File components/crash/content/app/crashpad_mac.mm (right):

https://codereview.chromium.org/2748903003/diff/1/components/crash/content/ap...
components/crash/content/app/crashpad_mac.mm:37: LAZY_INSTANCE_INITIALIZER;
Robert Sesek wrote:
> Also worth noting that this is how crashpad_win.cc does things too. Should we
> combine into crashpad.cc?

Yeah, but let’s also adopt the standard and more modern

crashpad::CrashpadClient& GetCrashpadClient() {
  static crashpad::CrashpadClient* const crashpad_client = new
crashpad::CrashpadClient();
  return *crashpad_client;
}

https://codereview.chromium.org/2748903003/diff/1/components/crash/content/ap...
components/crash/content/app/crashpad_mac.mm:106: bool result =
g_crashpad_client.Get().StartHandler(
clang-format’s worse, and I guess I’m not supposed to say anything, but I can
still groan. :(

[Robert Sesek, 2017-03-15 19:21:25]

Description was changed from

==========
[Mac] Use Crashpad in the AlertNotificationService.xpc.

Launchd does not transfer the service owner's exception port to the service,
nor can an exception handler be specified except by a domain service name in
a service's Info.plist.

Since Crashpad is not registered as a domain service, its handler port is
instead sent to the XPC service, where the service can configure Crashpad
itself.

BUG=699607
==========

to

==========
[Mac] Use Crashpad in the AlertNotificationService.xpc.

Launchd does not transfer the service owner's exception port to the service,
nor can an exception handler be specified as an appliction domain service name
in a service's Info.plist (that can only target a higher-level domain service).

Since Crashpad is not registered as a system or user domain service, its handler
port is instead sent to the XPC service, where the service can configure
Crashpad itself.

BUG=699607
==========

[Robert Sesek, 2017-03-15 19:22:10]

https://codereview.chromium.org/2748903003/diff/1/chrome/browser/ui/cocoa/not...
File chrome/browser/ui/cocoa/notifications/alert_notification_service.mm
(right):

https://codereview.chromium.org/2748903003/diff/1/chrome/browser/ui/cocoa/not...
chrome/browser/ui/cocoa/notifications/alert_notification_service.mm:39:
didSetExceptionPort_ = client.SetHandlerMachPort(std::move(sendRight));
On 2017/03/14 23:24:51, Mark Mentovai wrote:
> How much stuff runs in the service before we’re able to do this?

A significant amount of framework stuff, but very little code that we author.

https://codereview.chromium.org/2748903003/diff/1/chrome/browser/ui/cocoa/not...
File chrome/browser/ui/cocoa/notifications/xpc_mach_port.h (right):

https://codereview.chromium.org/2748903003/diff/1/chrome/browser/ui/cocoa/not...
chrome/browser/ui/cocoa/notifications/xpc_mach_port.h:22: // Instead, this class
encodes Mach port OOL data using the private
On 2017/03/14 23:24:52, Mark Mentovai wrote:
> Seems like we might want to have a test for at least this portion.

Agreed, but the "how" of that is not-trivial. While we can create an
NSXPCEncoder/NSXPCDecoder, I'm not sure how to extract the resulting data so as
to test round-tripping the encode through the decode. It's really not possible
to set up an entire XPCService in unit_tests for this, either.

https://codereview.chromium.org/2748903003/diff/1/components/crash/content/ap...
File components/crash/content/app/crashpad.h (right):

https://codereview.chromium.org/2748903003/diff/1/components/crash/content/ap...
components/crash/content/app/crashpad.h:16: #if defined(OS_MACOSX)
On 2017/03/14 23:24:52, Mark Mentovai wrote:
> #include "build/build_config.h" first (this file should have had it already).

Done.

https://codereview.chromium.org/2748903003/diff/1/components/crash/content/ap...
File components/crash/content/app/crashpad_mac.mm (right):

https://codereview.chromium.org/2748903003/diff/1/components/crash/content/ap...
components/crash/content/app/crashpad_mac.mm:37: LAZY_INSTANCE_INITIALIZER;
On 2017/03/14 23:24:52, Mark Mentovai wrote:
> Robert Sesek wrote:
> > Also worth noting that this is how crashpad_win.cc does things too. Should
we
> > combine into crashpad.cc?
> 
> Yeah, but let’s also adopt the standard and more modern
> 
> crashpad::CrashpadClient& GetCrashpadClient() {
>   static crashpad::CrashpadClient* const crashpad_client = new
> crashpad::CrashpadClient();
>   return *crashpad_client;
> }

Done. (Assuming you really did want a non-const-ref return value).

https://codereview.chromium.org/2748903003/diff/1/components/crash/content/ap...
components/crash/content/app/crashpad_mac.mm:106: bool result =
g_crashpad_client.Get().StartHandler(
On 2017/03/14 23:24:52, Mark Mentovai wrote:
> clang-format’s worse, and I guess I’m not supposed to say anything, but I can
> still groan. :(

https://bugs.chromium.org/p/chromium/issues/detail?id=690114

[Mark Mentovai, 2017-03-15 19:38:43]

LGTM

https://codereview.chromium.org/2748903003/diff/20001/components/crash/conten...
File components/crash/content/app/crashpad.h (right):

https://codereview.chromium.org/2748903003/diff/20001/components/crash/conten...
components/crash/content/app/crashpad.h:69: crashpad::CrashpadClient&
GetCrashpadClient();
Non-const ref or pointer, either one is fine by me.

[Robert Sesek, 2017-03-15 21:19:26]

The CQ bit was checked by rsesek@chromium.org

[commit-bot: I haz the power, 2017-03-15 21:20:07]

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, 2017-03-15 22:42:54]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1489612766043160,
"parent_rev": "3e212c72e769086678bdaeab2a6814b9639ea69a", "commit_rev":
"25df70294a0c70bb8b39a30ea69ef0be072c4097"}

[commit-bot: I haz the power, 2017-03-15 22:43:36]

Description was changed from

==========
[Mac] Use Crashpad in the AlertNotificationService.xpc.

Launchd does not transfer the service owner's exception port to the service,
nor can an exception handler be specified as an appliction domain service name
in a service's Info.plist (that can only target a higher-level domain service).

Since Crashpad is not registered as a system or user domain service, its handler
port is instead sent to the XPC service, where the service can configure
Crashpad itself.

BUG=699607
==========

to

==========
[Mac] Use Crashpad in the AlertNotificationService.xpc.

Launchd does not transfer the service owner's exception port to the service,
nor can an exception handler be specified as an appliction domain service name
in a service's Info.plist (that can only target a higher-level domain service).

Since Crashpad is not registered as a system or user domain service, its handler
port is instead sent to the XPC service, where the service can configure
Crashpad itself.

BUG=699607

Review-Url: https://codereview.chromium.org/2748903003
Cr-Commit-Position: refs/heads/master@{#457244}
Committed:
https://chromium.googlesource.com/chromium/src/+/25df70294a0c70bb8b39a30ea69e...
==========

[commit-bot: I haz the power, 2017-03-15 22:43:37]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/25df70294a0c70bb8b39a30ea69e...