[Mac] Remove base::LaunchSynchronize and rewrite content::MachBroker.

[Mac] Remove base::LaunchSynchronize and rewrite content::MachBroker.

This restructures the way MachBroker parent-child communication happens. Now,
the MachBroker lock will be held for the duration of LaunchProcess until the
PID is returned and inserted into the MachMap. Since the lock must also be
acquired on the broker thread to insert the received task port into the
MachMap, this ensures that the placeholder is always inserted before the task
port.

MachBroker has also been rewritten to use Mach IPC directly, rather than the C++
wrappers in base/mach_ipc_mac.h. The wrappers are not flexible enough to allow
the use of an audit trailer. This trailer is used to verify the PID of the
sender of the check in message in the parent. Previously, this was done by
another kernel trap, pid_for_task.

BUG=179923
Originally Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=193486
Reverted: https://src.chromium.org/viewvc/chrome?view=rev&revision=193511

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

[Robert Sesek, 2013-04-10 18:22:45]

Allays I put you a CL.

[Mark Mentovai, 2013-04-10 18:43:50]

OK

https://codereview.chromium.org/13845008/diff/1/content/browser/mach_broker_m...
File content/browser/mach_broker_mac.mm (right):

https://codereview.chromium.org/13845008/diff/1/content/browser/mach_broker_m...
content/browser/mach_broker_mac.mm:44: // Complement to the ChildSendMsg, this
is use din the parent for receiving
used in

https://codereview.chromium.org/13845008/diff/1/content/browser/mach_broker_m...
content/browser/mach_broker_mac.mm:54: MachListenerThreadDelegate(MachBroker*
broker)
explicit

https://codereview.chromium.org/13845008/diff/1/content/browser/mach_broker_m...
content/browser/mach_broker_mac.mm:61: mach_port_t port;
DCHECK that this isn’t called more than once (by checking that server_port_ is
MACH_PORT_NULL?)

https://codereview.chromium.org/13845008/diff/1/content/browser/mach_broker_m...
content/browser/mach_broker_mac.mm:62: kern_return_t kr =
mach_port_allocate(mach_task_self(),
Cache mach_task_self() since you use it twice in this function.

https://codereview.chromium.org/13845008/diff/1/content/browser/mach_broker_m...
content/browser/mach_broker_mac.mm:67: << MachErrorCode(kr);
Alignment.

https://codereview.chromium.org/13845008/diff/1/content/browser/mach_broker_m...
content/browser/mach_broker_mac.mm:88: name:name];
Any time you register stuff with the bootstrap server, you need to make sure
you’re not leaking your mappings. You can see the bootstrap mappings with
“launchctl bslist”. You should verify that all of your ports disappear after
your browser dies (including normal exit or fiery crash death). Have you done
this?

https://codereview.chromium.org/13845008/diff/1/content/browser/mach_broker_m...
content/browser/mach_broker_mac.mm:94: bzero(&msg, sizeof(msg));
It’s more usual in Chrome code to write msg = { 0 };

https://codereview.chromium.org/13845008/diff/1/content/browser/mach_broker_m...
content/browser/mach_broker_mac.mm:141: kern_return_t kr =
task_get_bootstrap_port(mach_task_self(), &bootstrap_port);
Cache mach_task_self in this function too.

https://codereview.chromium.org/13845008/diff/1/content/browser/mach_broker_m...
content/browser/mach_broker_mac.mm:155: // Create the check in message. This
will copy a send right on this' (the
this'?

https://codereview.chromium.org/13845008/diff/1/content/browser/mach_broker_m...
content/browser/mach_broker_mac.mm:157: MachBroker_ChildSendMsg msg;
= {0}

https://codereview.chromium.org/13845008/diff/1/content/browser/mach_broker_m...
content/browser/mach_broker_mac.mm:169: 0, MACH_PORT_NULL, 100 /*milliseconds*/,
MACH_PORT_NULL);
Why even bother timing out OR why 100ms? You’re going to get suicidal if it
times out.

[Robert Sesek, 2013-04-10 19:06:33]

https://codereview.chromium.org/13845008/diff/1/content/browser/mach_broker_m...
File content/browser/mach_broker_mac.mm (right):

https://codereview.chromium.org/13845008/diff/1/content/browser/mach_broker_m...
content/browser/mach_broker_mac.mm:44: // Complement to the ChildSendMsg, this
is use din the parent for receiving
On 2013/04/10 18:43:51, Mark Mentovai wrote:
> used in

Done.

https://codereview.chromium.org/13845008/diff/1/content/browser/mach_broker_m...
content/browser/mach_broker_mac.mm:54: MachListenerThreadDelegate(MachBroker*
broker)
On 2013/04/10 18:43:51, Mark Mentovai wrote:
> explicit

Done.

https://codereview.chromium.org/13845008/diff/1/content/browser/mach_broker_m...
content/browser/mach_broker_mac.mm:61: mach_port_t port;
On 2013/04/10 18:43:51, Mark Mentovai wrote:
> DCHECK that this isn’t called more than once (by checking that server_port_ is
> MACH_PORT_NULL?)

Done.

https://codereview.chromium.org/13845008/diff/1/content/browser/mach_broker_m...
content/browser/mach_broker_mac.mm:62: kern_return_t kr =
mach_port_allocate(mach_task_self(),
On 2013/04/10 18:43:51, Mark Mentovai wrote:
> Cache mach_task_self() since you use it twice in this function.

But mach_task_self() is really just a macro to mach_task_self_, right?

https://codereview.chromium.org/13845008/diff/1/content/browser/mach_broker_m...
content/browser/mach_broker_mac.mm:67: << MachErrorCode(kr);
On 2013/04/10 18:43:51, Mark Mentovai wrote:
> Alignment.

Done.

https://codereview.chromium.org/13845008/diff/1/content/browser/mach_broker_m...
content/browser/mach_broker_mac.mm:88: name:name];
On 2013/04/10 18:43:51, Mark Mentovai wrote:
> Any time you register stuff with the bootstrap server, you need to make sure
> you’re not leaking your mappings. You can see the bootstrap mappings with
> “launchctl bslist”. You should verify that all of your ports disappear after
> your browser dies (including normal exit or fiery crash death). Have you done
> this?

Yes. This uses the same thing as mach_ipc_mac, so if there is an issue it would
be carryover. But there is not.

https://codereview.chromium.org/13845008/diff/1/content/browser/mach_broker_m...
content/browser/mach_broker_mac.mm:94: bzero(&msg, sizeof(msg));
On 2013/04/10 18:43:51, Mark Mentovai wrote:
> It’s more usual in Chrome code to write msg = { 0 };

The compiler says:

error: no matching constructor for initialization of
'content::<anonymous>::MachBroker_ParentRecvMsg'

https://codereview.chromium.org/13845008/diff/1/content/browser/mach_broker_m...
content/browser/mach_broker_mac.mm:141: kern_return_t kr =
task_get_bootstrap_port(mach_task_self(), &bootstrap_port);
On 2013/04/10 18:43:51, Mark Mentovai wrote:
> Cache mach_task_self in this function too.

Same as above.

https://codereview.chromium.org/13845008/diff/1/content/browser/mach_broker_m...
content/browser/mach_broker_mac.mm:155: // Create the check in message. This
will copy a send right on this' (the
On 2013/04/10 18:43:51, Mark Mentovai wrote:
> this'?

Done.

https://codereview.chromium.org/13845008/diff/1/content/browser/mach_broker_m...
content/browser/mach_broker_mac.mm:169: 0, MACH_PORT_NULL, 100 /*milliseconds*/,
MACH_PORT_NULL);
On 2013/04/10 18:43:51, Mark Mentovai wrote:
> Why even bother timing out OR why 100ms? You’re going to get suicidal if it
> times out.

This is what the old one did. I dunno. Ask Rohit.

[Mark Mentovai, 2013-04-10 19:09:43]

LGTM

https://codereview.chromium.org/13845008/diff/7001/content/browser/mach_broke...
File content/browser/mach_broker_mac.mm (right):

https://codereview.chromium.org/13845008/diff/7001/content/browser/mach_broke...
content/browser/mach_broker_mac.mm:156: // Create the check in message. This
will copy a send right on this (the
Meh…this should be “on this process’” or something else clearer.

[Robert Sesek, 2013-04-10 19:13:30]

Thanks!

+Avi for OWNERS

https://codereview.chromium.org/13845008/diff/7001/content/browser/mach_broke...
File content/browser/mach_broker_mac.mm (right):

https://codereview.chromium.org/13845008/diff/7001/content/browser/mach_broke...
content/browser/mach_broker_mac.mm:156: // Create the check in message. This
will copy a send right on this (the
On 2013/04/10 19:09:43, Mark Mentovai wrote:
> Meh…this should be “on this process’” or something else clearer.

Done.

[Avi (use Gerrit), 2013-04-10 20:54:34]

lgtm

stamp

[commit-bot: I haz the power, 2013-04-10 20:55:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsesek@chromium.org/13845008/12001

[commit-bot: I haz the power, 2013-04-10 22:57:01]

Change committed as 193486

[Robert Sesek, 2013-04-11 14:27:33]

Committed patchset #4 manually as r193638 (presubmit successful).