Mac: Other approach for IPCing child task_ts.

Mac: Other approach for IPCing child task_ts.

Also move mach_ipc_mac to base, where it's now used.

Based on http://www.foldr.org/~michaelw/log/2009/03/13/ , but uses a named connection instead. Do the IPC right after fork-time, so that the sandbox is not yet in effect.

See the codereview comments for a benchmark that proves that this shouldn't be expensive, and for pros and cons for using a named connection vs temporarily switching out the bootstrap port.

Works for worker processes too and seems more reliable in general.

Measured perf impact in http://src.chromium.org/viewvc/chrome?view=rev&revision=35888 , it's negligible.

BUG=13156
TEST=(requires that one enables the task manager in browser.cc)
1.) Open one tab that plays a youtube video 
2.) Open a second and visit http://www.whatwg.org/demos/workers/primes/page.html 
3.) Install e.g. the gmail checker extension 
4.) Open the task manager 
It should report metrics for 
* one browser process 
* two renderer processes 
* one plugin process 
* one worker process 
* one extension process 
Check that %cpu etc more or less match what Activity Monitor displays if you filter for "Chromium". 
Also choose "Open all bookmarks" on the bookmarks bar with the task manager open and check that metrics for all tabs appear immediately.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=35977

[Nico, 2010-01-10 06:09:01]

My former approach ( http://codereview.chromium.org/460126 ) didn't work with
worker and utility processes.

Reason 1 for that is the sandbox. MachPortSender's call to bootstrap_look_up()
returned  0x10000003 / MACH_SEND_INVALID_DEST for worker and utility processes,
the reason being that their sandboxes are more restrictive than the renderer
sandbox (took me a while to figure this out, 'cause I believed that the flag to
disable the sandbox is --disable-sandbox and not --no-sandbox :-P).

When the sandbox is disabled, bootstrap_look_up()  fails with 1100 /
BOOTSTRAP_NOT_PRIVILEGED instead for worker and utility processes. I didn't
track down why.

Poking a hole in the sandbox just for this seems bad, so I'm trying a different
approach, it's in this CL. As suggested by some blog post I found on the
interwebs, I'm exchanging task_ts at fork() time. For this, I need to
temporarily change the browser process's bootstrap port, fork, exchange ports,
and then re-set the bootstrap port to its old value.

Amazingly, this does not negatively affect startup perf:
http://build.chromium.org/buildbot/perf/dashboard/perf.html?builder=Chromium%...
I had this checked in temporarily as r35888 (it even helped v8 pref :-P).

This again is not the general approach that Jeremy suggested, but doing this in
general while keeping the sandbox intact would mean moving a lot of stuff
around, so I'd rather not do that now.

A downside is that the sampling_fork() part has to be protected by a mutex. An
upside is that this works for _every_ process that LaunchApp() launches, not
just for our own chrome processes that use ChildProcess etc.

I think I like this. It should work for all child processes and is pretty
simple. If you agree, I'll clean it up a bit (add timeouts, remove the global
variables, add a "task_t* out" parameter to LaunchApp() on OS X and add a mutex)
and ask for a real review. But since this might be controversial, I figured I
ask you guys first. What do you think?

[Nico, 2010-01-10 21:06:51]

FFFFFFFFFUUUUU…wrote a long reply, then codereview told me "invalid xsrf token",
and now it's gone.

In short: This CL is not that different from my previous CL. The only
differences are:
1.) I do the port exchange right after fork, before the sandbox is on or
execve() is even done
2.) I temporarily change the bootstrap port to establish the parent-child
connection

I wrote a small benchmark that compares this approach with using a named
connection instead of temporarily changing the bootstrap_port (which would
require a mutex) ( http://codereview.appspot.com/186060/diff/1/2 ). Results:
Fork takes 0.112ms, mutexed_samplingfork takes 0.158ms, and samplingfork2 takes
0.19ms. samplingfork2 sometimes prints error messages related to
bootstrap_register. bootstrap_register is deprecated too and it's function
comment says

 * 2) If the code was registering a dynamically generated string and passing
 *    the string to other applications, please rewrite the code to send a Mach
 *    send-right directly.

i.e. "do what sampling_fork() does" (this could be worked around by calling
NSMachBootstrapServer's -registerPort:name:, but that feels like using a
deprecation hole).

Loosing 0.046ms (!) every time LaunchApp() is called sounds perfectly reasonably
to me. I will move forward with this CL.

[Nico, 2010-01-11 02:33:25]

This is ready for review. I'm pretty happy with it.

[Nico, 2010-01-11 15:50:25]

On 2010/01/11 02:33:25, Nico wrote:
> This is ready for review. I'm pretty happy with it.

If I simulate IPC failure in the child process (by adding a "true &&" to one of
the ifs that checks for error in sampling_fork()), the child processes come up
in a completely unusable state. That's not good.

[Nico, 2010-01-11 16:12:24]

http://codereview.chromium.org/549002/diff/9024/6018
File base/process_util_posix.cc (right):

http://codereview.chromium.org/549002/diff/9024/6018#newcode307
base/process_util_posix.cc:307: AutoLock lock(*Singleton<Lock>::get());
fail

[Nico, 2010-01-11 17:50:39]

I switched this to a name-based connection scheme. This another 0.032 ms slower
(for a total slowdown of 0.078ms, at least on dual-core systems (i.e. nearly all
intel macs there are)), but if the IPC fails the child lives on as if nothing
bad happened because it never has a "bad" bootstrap port.

[Mark Mentovai, 2010-01-11 21:11:30]

Hope I got the right one this time.

I didn't have a chance to use “turdlet” at all in this review.

http://codereview.chromium.org/549002/diff/6023/9028
File base/process_util.h (right):

http://codereview.chromium.org/549002/diff/6023/9028#newcode168
base/process_util.h:168: // Similar to above, but also returns the new process's
task_t if |task_handle|
nit: Similar to the above.

http://codereview.chromium.org/549002/diff/6023/9028#newcode175
base/process_util.h:175: #endif
// defined(OS_MACOSX)

http://codereview.chromium.org/549002/diff/6023/9029
File base/process_util_posix.cc (right):

http://codereview.chromium.org/549002/diff/6023/9029#newcode32
base/process_util_posix.cc:32: #include "base/singleton.h"
Don't put this in the Mac-only section.

You didn't put rand_util in the Mac-only section.

http://codereview.chromium.org/549002/diff/6023/9029#newcode290
base/process_util_posix.cc:290: // Forks the current process and returns the
child's |task_t| in the parent
This belongs in an anonymous namespace.  Or it should be static.

http://codereview.chromium.org/549002/diff/6023/9029#newcode292
base/process_util_posix.cc:292: pid_t sampling_fork(task_t* child_task) {
I know that it's called sampling_fork in the thing that you ripped this off of-I
MEAN-copied from-I MEAN-used as an inspiration...but is it the best name?

I mean, what's "sampling" about anyway?  That might not be why someone calls
this to get a task_t.  How about fork_and_get_task?

http://codereview.chromium.org/549002/diff/6023/9029#newcode296
base/process_util_posix.cc:296: // But a random number into the channel name, so
that a compromised renderer
But -> Put

http://codereview.chromium.org/549002/diff/6023/9029#newcode297
base/process_util_posix.cc:297: // can't fool us into believing he's our child.
us/our are bad.

You have no way of knowing at this point in the program that your child is a
boy.

http://codereview.chromium.org/549002/diff/6023/9029#newcode299
base/process_util_posix.cc:299: sprintf(buff,
"com.google.Chrome.samplingfork.%p.%d",
I'd be more comfortable if you built this up in something safe like std::string
land and then took the c_str instead of working with a fixed-size buffer and
sprintf (not even snprintf!)

http://codereview.chromium.org/549002/diff/6023/9029#newcode308
base/process_util_posix.cc:308: switch (pid = fork()) {
I'd find this ever so slightly more readable if you wrote:

  pid_t pid = fork();
  switch (pid) {

http://codereview.chromium.org/549002/diff/6023/9029#newcode316
base/process_util_posix.cc:316: PLOG(ERROR) << "child
AddDescriptor(mach_task_self()) failed.";
Is PLOG right in this function?  Except for fork(), you're not interacting
directly with system calls here.

And you didn't even PLOG for the fork failure (|case -1| above), which I assume
is intentional.

http://codereview.chromium.org/549002/diff/6023/9029#newcode319
base/process_util_posix.cc:319: if
(!child_message.AddDescriptor(child_recv_port.GetPort())) {
Maybe you want to pull child_recv_port.GetPort() out into its own variable so
you don't have to call it twice when this fails.

http://codereview.chromium.org/549002/diff/6023/9029#newcode328
base/process_util_posix.cc:328: PLOG(ERROR) << "child SendMessage() failed:" <<
err
Maybe you should have a small helper function to return a std::string with err +
" " + mach_error_string(err) instead of having to do it yourself each time you
want it.

http://codereview.chromium.org/549002/diff/6023/9029#newcode342
base/process_util_posix.cc:342: PLOG(ERROR) << "child GetTranslatedPort(0)
failed:";
"failed:" but nothing follows the colon - I think you wanted a period.

http://codereview.chromium.org/549002/diff/6023/9029#newcode354
base/process_util_posix.cc:354: default:  // parent
This |default:| is a long block.  Can you give me a pair of {braces} so that I
have something to bounce between in vi?

http://codereview.chromium.org/549002/diff/6023/9029#newcode398
base/process_util_posix.cc:398: #endif
Gimme a // defined(OS_MACOSX) here.

Or get rid of this #endif and the #if on line 400, and put the comment on the
#endif on line 407.

http://codereview.chromium.org/549002/diff/6023/9029#newcode405
base/process_util_posix.cc:405: return LaunchApp(argv, environ, fds_to_remap,
wait, NULL, process_handle);
This makes me say "meh" and then makes me say "can you rename the extra-argument
version of LaunchApp when OS_MACOSX to avoid the overloading thing?"

http://codereview.chromium.org/549002/diff/6023/9029#newcode423
base/process_util_posix.cc:423: // the function task_for_pid() requires
priviledges a normal user doesn't
Check spelling, privileges has no d!

http://codereview.chromium.org/549002/diff/6023/9030
File chrome/browser/child_process_launcher.cc (right):

http://codereview.chromium.org/549002/diff/6023/9030#newcode173
chrome/browser/child_process_launcher.cc:173: &child_task,
This is ugly as sin.  I think it's almost worth doing

#if defined(OS_MACOSX)
task_t task;
if (base::LaunchApp(crap, &task, more_crap) {
  if (task != MACH_PORT_NULL) {
  }
} else {
#else
if (!base::LaunchApp(crap, more_crap) {
#endif
  handle = base::kNullProcessHandle;
}

but that's kind of ugly too, and difficult to follow in new and exciting ways
(flipping the if/else thing).

http://codereview.chromium.org/549002/diff/6023/9031
File chrome/browser/mach_broker_mac.cc (right):

http://codereview.chromium.org/549002/diff/6023/9031#newcode8
chrome/browser/mach_broker_mac.cc:8: #include
"chrome/browser/extensions/extension_host.h"
I probably had some suggestions about this file in the wrong CL that I read. 
Some of those may still be valid.

http://codereview.chromium.org/549002/diff/6023/9031#newcode106
chrome/browser/mach_broker_mac.cc:106: Invalidate(handle);
I remember commenting on this, for example.

But you don't need to take that suggestion if you don't like it.

[Nico, 2010-01-11 22:32:26]

Thank you, good sir.

http://codereview.chromium.org/549002/diff/6023/9028
File base/process_util.h (right):

http://codereview.chromium.org/549002/diff/6023/9028#newcode168
base/process_util.h:168: // Similar to above, but also returns the new process's
task_t if |task_handle|
On 2010/01/11 21:11:30, Mark Mentovai wrote:
> nit: Similar to the above.

Done.

http://codereview.chromium.org/549002/diff/6023/9028#newcode175
base/process_util.h:175: #endif
On 2010/01/11 21:11:30, Mark Mentovai wrote:
> // defined(OS_MACOSX)

Done.

http://codereview.chromium.org/549002/diff/6023/9029
File base/process_util_posix.cc (right):

http://codereview.chromium.org/549002/diff/6023/9029#newcode32
base/process_util_posix.cc:32: #include "base/singleton.h"
On 2010/01/11 21:11:30, Mark Mentovai wrote:
> Don't put this in the Mac-only section.
> 
> You didn't put rand_util in the Mac-only section.

Removed it, I don't need it any more.

http://codereview.chromium.org/549002/diff/6023/9029#newcode290
base/process_util_posix.cc:290: // Forks the current process and returns the
child's |task_t| in the parent
On 2010/01/11 21:11:30, Mark Mentovai wrote:
> This belongs in an anonymous namespace.  Or it should be static.

Made it static.

http://codereview.chromium.org/549002/diff/6023/9029#newcode292
base/process_util_posix.cc:292: pid_t sampling_fork(task_t* child_task) {
On 2010/01/11 21:11:30, Mark Mentovai wrote:
> I know that it's called sampling_fork in the thing that you ripped this off
of-I
> MEAN-copied from-I MEAN-used as an inspiration

:-) By now it's pretty different.

>...but is it the best name?
> I mean, what's "sampling" about anyway?  That might not be why someone calls
> this to get a task_t.  How about fork_and_get_task?

That is a _great_ idea.

http://codereview.chromium.org/549002/diff/6023/9029#newcode296
base/process_util_posix.cc:296: // But a random number into the channel name, so
that a compromised renderer
On 2010/01/11 21:11:30, Mark Mentovai wrote:
> But -> Put

Done.

http://codereview.chromium.org/549002/diff/6023/9029#newcode297
base/process_util_posix.cc:297: // can't fool us into believing he's our child.
On 2010/01/11 21:11:30, Mark Mentovai wrote:
> us/our are bad.
> 
> You have no way of knowing at this point in the program that your child is a
> boy.

Done?

http://codereview.chromium.org/549002/diff/6023/9029#newcode299
base/process_util_posix.cc:299: sprintf(buff,
"com.google.Chrome.samplingfork.%p.%d",
On 2010/01/11 21:11:30, Mark Mentovai wrote:
> I'd be more comfortable if you built this up in something safe like
std::string
> land and then took the c_str instead of working with a fixed-size buffer and
> sprintf (not even snprintf!)

:-(

Done.

http://codereview.chromium.org/549002/diff/6023/9029#newcode308
base/process_util_posix.cc:308: switch (pid = fork()) {
On 2010/01/11 21:11:30, Mark Mentovai wrote:
> I'd find this ever so slightly more readable if you wrote:
> 
>   pid_t pid = fork();
>   switch (pid) {

Done.

http://codereview.chromium.org/549002/diff/6023/9029#newcode316
base/process_util_posix.cc:316: PLOG(ERROR) << "child
AddDescriptor(mach_task_self()) failed.";
On 2010/01/11 21:11:30, Mark Mentovai wrote:
> Is PLOG right in this function?  Except for fork(), you're not interacting
> directly with system calls here.
> 
> And you didn't even PLOG for the fork failure (|case -1| above), which I
assume
> is intentional.

Switched to LOG.

The old code didn't log for fork failure either. I expect the function assumes
the caller wants to do that.

http://codereview.chromium.org/549002/diff/6023/9029#newcode319
base/process_util_posix.cc:319: if
(!child_message.AddDescriptor(child_recv_port.GetPort())) {
On 2010/01/11 21:11:30, Mark Mentovai wrote:
> Maybe you want to pull child_recv_port.GetPort() out into its own variable so
> you don't have to call it twice when this fails.

Really?

Done. (but…really?)

http://codereview.chromium.org/549002/diff/6023/9029#newcode328
base/process_util_posix.cc:328: PLOG(ERROR) << "child SendMessage() failed:" <<
err
On 2010/01/11 21:11:30, Mark Mentovai wrote:
> Maybe you should have a small helper function to return a std::string with err
+
> " " + mach_error_string(err) instead of having to do it yourself each time you
> want it.

Done.

http://codereview.chromium.org/549002/diff/6023/9029#newcode342
base/process_util_posix.cc:342: PLOG(ERROR) << "child GetTranslatedPort(0)
failed:";
On 2010/01/11 21:11:30, Mark Mentovai wrote:
> "failed:" but nothing follows the colon - I think you wanted a period.

Done.

http://codereview.chromium.org/549002/diff/6023/9029#newcode354
base/process_util_posix.cc:354: default:  // parent
On 2010/01/11 21:11:30, Mark Mentovai wrote:
> This |default:| is a long block.  Can you give me a pair of {braces} so that I
> have something to bounce between in vi?

Everything for a fellow vi user.

http://codereview.chromium.org/549002/diff/6023/9029#newcode405
base/process_util_posix.cc:405: return LaunchApp(argv, environ, fds_to_remap,
wait, NULL, process_handle);
On 2010/01/11 21:11:30, Mark Mentovai wrote:
> This makes me say "meh" and then makes me say "can you rename the
extra-argument
> version of LaunchApp when OS_MACOSX to avoid the overloading thing?"

Done. Moved the #ifdefs around a bit and didn't find anything great.

http://codereview.chromium.org/549002/diff/6023/9029#newcode423
base/process_util_posix.cc:423: // the function task_for_pid() requires
priviledges a normal user doesn't
On 2010/01/11 21:11:30, Mark Mentovai wrote:
> Check spelling, privileges has no d!

I know. Done.

http://codereview.chromium.org/549002/diff/6023/9030
File chrome/browser/child_process_launcher.cc (right):

http://codereview.chromium.org/549002/diff/6023/9030#newcode173
chrome/browser/child_process_launcher.cc:173: &child_task,
On 2010/01/11 21:11:30, Mark Mentovai wrote:
> This is ugly as sin.  I think it's almost worth doing
> 
> #if defined(OS_MACOSX)
> task_t task;
> if (base::LaunchApp(crap, &task, more_crap) {
>   if (task != MACH_PORT_NULL) {
>   }
> } else {
> #else
> if (!base::LaunchApp(crap, more_crap) {
> #endif
>   handle = base::kNullProcessHandle;
> }
> 
> but that's kind of ugly too, and difficult to follow in new and exciting ways
> (flipping the if/else thing).

Came up with something better, I think.

http://codereview.chromium.org/549002/diff/6023/9031
File chrome/browser/mach_broker_mac.cc (right):

http://codereview.chromium.org/549002/diff/6023/9031#newcode106
chrome/browser/mach_broker_mac.cc:106: Invalidate(handle);
On 2010/01/11 21:11:30, Mark Mentovai wrote:
> I remember commenting on this, for example.
> 
> But you don't need to take that suggestion if you don't like it.

Then I won't. One other suggestion still applied (use LOG_IF), which I did.

[Mark Mentovai, 2010-01-11 22:43:30]

This will do.  LGTM if you look into whatever I ask about below.

http://codereview.chromium.org/549002/diff/9033/9037
File base/process_util.h (right):

http://codereview.chromium.org/549002/diff/9033/9037#newcode170
base/process_util.h:170: // responsible for calling |vm_deallocate()| on the
returned handle.
Where's the vm_deallocate actually happen?

http://codereview.chromium.org/549002/diff/9033/9039
File chrome/browser/child_process_launcher.cc (right):

http://codereview.chromium.org/549002/diff/9033/9039#newcode185
chrome/browser/child_process_launcher.cc:185: #endif // else defined(OS_POSIX)
At least two spaces before a //.

[Nico, 2010-01-12 00:38:04]

http://codereview.chromium.org/549002/diff/9033/9037
File base/process_util.h (right):

http://codereview.chromium.org/549002/diff/9033/9037#newcode170
base/process_util.h:170: // responsible for calling |vm_deallocate()| on the
returned handle.
On 2010/01/11 22:43:30, Mark Mentovai wrote:
> Where's the vm_deallocate actually happen?

That's a good question. I meant to write "for calling |mach_port_deallocate()|",
which is done by mach_broker_mac.cc in the notification listener.

I think that's correct, but I asked someone familiar with the matter to confirm
that.

Since I'm reasonably confident that this is correct as-is, I will check this in
(with an updated comment in this file).

http://codereview.chromium.org/549002/diff/9033/9039
File chrome/browser/child_process_launcher.cc (right):

http://codereview.chromium.org/549002/diff/9033/9039#newcode185
chrome/browser/child_process_launcher.cc:185: #endif // else defined(OS_POSIX)
On 2010/01/11 22:43:30, Mark Mentovai wrote:
> At least two spaces before a //.

Done.

[Mark Mentovai, 2010-01-12 00:50:59]

Thanks, Nico.  That sounds fine to me.

[Nico, 2010-01-12 21:36:04]

The expert says:

"""
looks like we are talking about an inline port right (MACH_MSG_PORT_DESCRIPTOR)
and not anything out-of-line (like a port array) ...
if it's a single port right you are exchanging, mach_port_deallocate() is fine
"""