[Mac] Replace the existing browser-child mach ipc with a long-lived listener ...

[Mac] Replace the existing browser-child mach ipc with a long-lived listener on a well-known port.

Before this CL:
Before fork()ing a child, the browser process creates a mach receive port with a random name.  After the fork() but before exec(), the child uses mach ipc to transmit send rights to its task port.  The child has access to the random name because it inherits it from the browser process.  Unfortunately, some of the library functions involved in sending a mach message are not safe to call after fork().

After this CL:
Before forking the first child, the browser spins off a new thread that listens on a well-known port for mach ipc from any process.  This well-known port is "com.google.Chrome.<browserpid>".  When a child process starts up, it sends a mach message to its parent browser's well-known port.  On the browser side, we listen for said message, extract the pid of the sending process, and ignore any messages from processes we did not personally fork().  This check is necessary because any arbitrary process on the system could send mach ipc to that port.

BUG=35374
TEST=Browser should still start up.  The task manager should still show correct cpu/memory data.  There should be no perf regressions.  
TEST=Mac ui_tests and browser_tests should be less flaky.

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

[rohitrao (ping after 24h), 2010-09-14 22:39:17]

Ok, I think this is ready for a real look.  We're most likely leaking map
entries because we're getting bogus PIDs from the notifications, but that
problem is in the existing code too.  I vote we fix that separately =)

[Nico, 2010-09-15 00:00:38]

couple nits. didn't look closely yet, but looks good from a distance

http://codereview.chromium.org/3443002/diff/25001/23006
File chrome/app/chrome_dll_main.cc (right):

http://codereview.chromium.org/3443002/diff/25001/23006#newcode594
chrome/app/chrome_dll_main.cc:594: std::string mach_port_name =
MachBroker::MachPortNameForChildProcess();
this wants some kind of comment (e.g. "// Send mach_port_t to parent process
using mach IPC. See http://crbug.com/the bug from the cl description")

http://codereview.chromium.org/3443002/diff/25001/23008
File chrome/browser/mach_broker_mac.cc (right):

http://codereview.chromium.org/3443002/diff/25001/23008#newcode122
chrome/browser/mach_broker_mac.cc:122: mutable Lock lock_;
why mutable? do you use this form any const method?

http://codereview.chromium.org/3443002/diff/25001/23009
File chrome/browser/mach_broker_mac.h (right):

http://codereview.chromium.org/3443002/diff/25001/23009#newcode60
chrome/browser/mach_broker_mac.h:60: void PlaceholderForPid(base::ProcessHandle
pid);
AddPlaceholderForPid

[Mark Mentovai, 2010-09-15 17:02:17]

http://codereview.chromium.org/3443002/diff/25001/23003
File base/mach_ipc_mac.mm (right):

http://codereview.chromium.org/3443002/diff/25001/23003#newcode206
base/mach_ipc_mac.mm:206: options:NSMachPortDeallocateNone];
Align your colons.

http://codereview.chromium.org/3443002/diff/25001/23005
File base/process_util_posix.cc (left):

http://codereview.chromium.org/3443002/diff/25001/23005#oldcode303
base/process_util_posix.cc:303: 
This is certainly a nice simplification.

http://codereview.chromium.org/3443002/diff/25001/23006
File chrome/app/chrome_dll_main.cc (right):

http://codereview.chromium.org/3443002/diff/25001/23006#newcode84
chrome/app/chrome_dll_main.cc:84: #include
"chrome/common/chrome_paths_internal.h"
Sort (not your fault.)

http://codereview.chromium.org/3443002/diff/25001/23006#newcode388
chrome/app/chrome_dll_main.cc:388: if (process_type.empty() || process_type ==
switches::kServiceProcess) {
I guess the printery and remotey processes are “service?”

http://codereview.chromium.org/3443002/diff/25001/23006#newcode595
chrome/app/chrome_dll_main.cc:595: kern_return_t err;
Don’t declare this until you use it, several lines below.

http://codereview.chromium.org/3443002/diff/25001/23006#newcode596
chrome/app/chrome_dll_main.cc:596: base::MachSendMessage child_message(/* id=
*/0);
Should we maybe have some sort of constant for this message? (Even if we decide
to make the number be 0?)

http://codereview.chromium.org/3443002/diff/25001/23006#newcode602
chrome/app/chrome_dll_main.cc:602: err = child_sender.SendMessage(child_message,
1000);
What’s 1000, the timeout? Something that should have a symbolic name?

http://codereview.chromium.org/3443002/diff/25001/23007
File chrome/browser/child_process_launcher.cc (right):

http://codereview.chromium.org/3443002/diff/25001/23007#newcode174
chrome/browser/child_process_launcher.cc:174:
MachBroker::instance()->AcquireLock();
Is there a scoper that can hold the lock?

http://codereview.chromium.org/3443002/diff/25001/23007#newcode174
chrome/browser/child_process_launcher.cc:174:
MachBroker::instance()->AcquireLock();
Does this mean that someone registers an atexit-type destructor for MachBrokern?
We probably don’t need that. Letting it live forever (until exit) is probably
easier and would clean up some of the mechanics you’ve added with the thread
connection.

http://codereview.chromium.org/3443002/diff/25001/23008
File chrome/browser/mach_broker_mac.cc (right):

http://codereview.chromium.org/3443002/diff/25001/23008#newcode169
chrome/browser/mach_broker_mac.cc:169: mach_map_[pid] = mach_info;
I think that the above DCHECK should just be a conditional guarding this line.

(In addition to having a DCHECK?)

http://codereview.chromium.org/3443002/diff/25001/23008#newcode234
chrome/browser/mach_broker_mac.cc:234: return
StringPrintf("com.google.Chrome.rohitfork.%d", getpid());
I will let you call this rohitfork, but I don’t want to call it
com.google.Chrome unless we’re actually com.google.Chrome. Otherwise, it should
reflect Chromium. You can look up the bundle ID or just use a compile-time
constant switching on Chrome branding.

http://codereview.chromium.org/3443002/diff/25001/23009
File chrome/browser/mach_broker_mac.h (right):

http://codereview.chromium.org/3443002/diff/25001/23009#newcode64
chrome/browser/mach_broker_mac.h:64: // |pid|.  Callers MUST called
AcquireLock() and ReleaseLock() before and
This sounds Romanian. “Callers MUST called AcquireLock()…” Allays.

http://codereview.chromium.org/3443002/diff/25001/23009#newcode67
chrome/browser/mach_broker_mac.h:67: 
I was expecting to see something in here that cleans stale entries out of the
map. No?

http://codereview.chromium.org/3443002/diff/25001/23009#newcode108
chrome/browser/mach_broker_mac.h:108: // Thread that listens for mach ipc
messages from children.  This pointer is
Mach IPC, with the shift key.

[rohitrao (ping after 24h), 2010-09-15 19:23:08]

New diffs up.  The comments below include a few questions here and there.

http://codereview.chromium.org/3443002/diff/25001/23003
File base/mach_ipc_mac.mm (right):

http://codereview.chromium.org/3443002/diff/25001/23003#newcode206
base/mach_ipc_mac.mm:206: options:NSMachPortDeallocateNone];
On 2010/09/15 17:02:17, Mark Mentovai wrote:
> Align your colons.

Done.

http://codereview.chromium.org/3443002/diff/25001/23006
File chrome/app/chrome_dll_main.cc (right):

http://codereview.chromium.org/3443002/diff/25001/23006#newcode84
chrome/app/chrome_dll_main.cc:84: #include
"chrome/common/chrome_paths_internal.h"
On 2010/09/15 17:02:17, Mark Mentovai wrote:
> Sort (not your fault.)

Done.

http://codereview.chromium.org/3443002/diff/25001/23006#newcode388
chrome/app/chrome_dll_main.cc:388: if (process_type.empty() || process_type ==
switches::kServiceProcess) {
On 2010/09/15 17:02:17, Mark Mentovai wrote:
> I guess the printery and remotey processes are “service?”

That's what I gathered in IRC yesterday.  At worst, we'll send some Mach ipc
when we're not supposed to and ignore the message on the browser end.  If we see
any error messages, we can take care of it.

http://codereview.chromium.org/3443002/diff/25001/23006#newcode594
chrome/app/chrome_dll_main.cc:594: std::string mach_port_name =
MachBroker::MachPortNameForChildProcess();
On 2010/09/15 00:00:38, Nico wrote:
> this wants some kind of comment (e.g. "// Send mach_port_t to parent process
> using mach IPC. See http://crbug.com/the bug from the cl description")

I moved all this code to a separate function and added a function-level comment.

http://codereview.chromium.org/3443002/diff/25001/23006#newcode595
chrome/app/chrome_dll_main.cc:595: kern_return_t err;
On 2010/09/15 17:02:17, Mark Mentovai wrote:
> Don’t declare this until you use it, several lines below.

Done.

http://codereview.chromium.org/3443002/diff/25001/23006#newcode596
chrome/app/chrome_dll_main.cc:596: base::MachSendMessage child_message(/* id=
*/0);
On 2010/09/15 17:02:17, Mark Mentovai wrote:
> Should we maybe have some sort of constant for this message? (Even if we
decide
> to make the number be 0?)

Done.

http://codereview.chromium.org/3443002/diff/25001/23006#newcode602
chrome/app/chrome_dll_main.cc:602: err = child_sender.SendMessage(child_message,
1000);
On 2010/09/15 17:02:17, Mark Mentovai wrote:
> What’s 1000, the timeout? Something that should have a symbolic name?

Yeah.  Also, it should be 100ms, and maybe it can go even lower than that.  I'll
check with Nico.

http://codereview.chromium.org/3443002/diff/25001/23007
File chrome/browser/child_process_launcher.cc (right):

http://codereview.chromium.org/3443002/diff/25001/23007#newcode174
chrome/browser/child_process_launcher.cc:174:
MachBroker::instance()->AcquireLock();
On 2010/09/15 17:02:17, Mark Mentovai wrote:
> Does this mean that someone registers an atexit-type destructor for
MachBrokern?
> We probably don’t need that. Letting it live forever (until exit) is probably
> easier and would clean up some of the mechanics you’ve added with the thread
> connection.

Going by the logging I added, the singleton MachBroker object is destroyed on
exit.  I don't know how or why that is happening, but given that it is, thread
cleanup/disconnection seemed appropriate.  If you know how to keep the
MachBroker around forever, I'm happy to do that instead.

Your question seemed unrelated to this particular chunk of code, so I'm
wondering if my comment was worded poorly.

http://codereview.chromium.org/3443002/diff/25001/23007#newcode174
chrome/browser/child_process_launcher.cc:174:
MachBroker::instance()->AcquireLock();
On 2010/09/15 17:02:17, Mark Mentovai wrote:
> Is there a scoper that can hold the lock?

We can either use AutoLock or roll our own, but given the ifdefs here, I don't
think adding a scoper will actually make the code any cleaner.

http://codereview.chromium.org/3443002/diff/25001/23008
File chrome/browser/mach_broker_mac.cc (right):

http://codereview.chromium.org/3443002/diff/25001/23008#newcode122
chrome/browser/mach_broker_mac.cc:122: mutable Lock lock_;
On 2010/09/15 00:00:38, Nico wrote:
> why mutable? do you use this form any const method?

Nope, just copy/pasted from somewhere else.

http://codereview.chromium.org/3443002/diff/25001/23008#newcode169
chrome/browser/mach_broker_mac.cc:169: mach_map_[pid] = mach_info;
On 2010/09/15 17:02:17, Mark Mentovai wrote:
> I think that the above DCHECK should just be a conditional guarding this line.
> 
> (In addition to having a DCHECK?)

Added the conditional, kept the DCHECK.

http://codereview.chromium.org/3443002/diff/25001/23008#newcode234
chrome/browser/mach_broker_mac.cc:234: return
StringPrintf("com.google.Chrome.rohitfork.%d", getpid());
On 2010/09/15 17:02:17, Mark Mentovai wrote:
> I will let you call this rohitfork, but I don’t want to call it
> com.google.Chrome unless we’re actually com.google.Chrome. Otherwise, it
should
> reflect Chromium. You can look up the bundle ID or just use a compile-time
> constant switching on Chrome branding.

Used #if defined(GOOGLE_CHROME) because that was easier than converting this to
a .mm file.  Is there a way to get the bundle ID without using NSBundle?

http://codereview.chromium.org/3443002/diff/25001/23009
File chrome/browser/mach_broker_mac.h (right):

http://codereview.chromium.org/3443002/diff/25001/23009#newcode60
chrome/browser/mach_broker_mac.h:60: void PlaceholderForPid(base::ProcessHandle
pid);
On 2010/09/15 00:00:38, Nico wrote:
> AddPlaceholderForPid

Done.

http://codereview.chromium.org/3443002/diff/25001/23009#newcode64
chrome/browser/mach_broker_mac.h:64: // |pid|.  Callers MUST called
AcquireLock() and ReleaseLock() before and
On 2010/09/15 17:02:17, Mark Mentovai wrote:
> This sounds Romanian. “Callers MUST called AcquireLock()…” Allays.

Done.

http://codereview.chromium.org/3443002/diff/25001/23009#newcode67
chrome/browser/mach_broker_mac.h:67: 
On 2010/09/15 17:02:17, Mark Mentovai wrote:
> I was expecting to see something in here that cleans stale entries out of the
> map. No?

Invalidate() does that.  I've moved its declaration up above the locking
functions.  Also, I'm renaming it to InvalidatePid(), for great justice and
symmetry.

http://codereview.chromium.org/3443002/diff/25001/23009#newcode108
chrome/browser/mach_broker_mac.h:108: // Thread that listens for mach ipc
messages from children.  This pointer is
On 2010/09/15 17:02:17, Mark Mentovai wrote:
> Mach IPC, with the shift key.

Do I need to s/mach/Mach/ everywhere else?  (Is it mach port name or Mach port
name?)

[Mark Mentovai, 2010-09-16 16:56:36]

Allays I been reviewd

http://codereview.chromium.org/3443002/diff/25001/23007
File chrome/browser/child_process_launcher.cc (right):

http://codereview.chromium.org/3443002/diff/25001/23007#newcode174
chrome/browser/child_process_launcher.cc:174:
MachBroker::instance()->AcquireLock();
rohitrao wrote:
> Your question seemed unrelated to this particular chunk of code, so I'm
> wondering if my comment was worded poorly.

Nah, it was just as good a place as any to put my question.

http://codereview.chromium.org/3443002/diff/25001/23007#newcode174
chrome/browser/child_process_launcher.cc:174:
MachBroker::instance()->AcquireLock();
rohitrao wrote:
> We can either use AutoLock or roll our own, but given the ifdefs here, I don't
> think adding a scoper will actually make the code any cleaner.

#if defined(OS_MACOSX)
{  // new scope!
// some mac-specific code
#endif

// shared code

#if defined(OS_MACOSX)
// maybe some more mac-specific code
}  // end scope!
#endif

Sure, it’s not the cleanest. Your call. I’m always afraid of early returns when
I see locks like this.

http://codereview.chromium.org/3443002/diff/25001/23008
File chrome/browser/mach_broker_mac.cc (right):

http://codereview.chromium.org/3443002/diff/25001/23008#newcode234
chrome/browser/mach_broker_mac.cc:234: return
StringPrintf("com.google.Chrome.rohitfork.%d", getpid());
rohitrao wrote:
> Used #if defined(GOOGLE_CHROME) because that was easier than converting this
to
> a .mm file.  Is there a way to get the bundle ID without using NSBundle?

You can use CFBundle calls, but you’ll get the helper’s bundle ID in the helper
process (if the sandbox doesn’t interfere, that is) meaning you’d need to
manipulate the string. The #ifdef is fine.

http://codereview.chromium.org/3443002/diff/7004/32006
File chrome/app/chrome_dll_main.cc (right):

http://codereview.chromium.org/3443002/diff/7004/32006#newcode433
chrome/app/chrome_dll_main.cc:433: LOG(ERROR) << StringPrintf("child
SendMessage() failed: 0x%x %s", err,
There’s a way to make the stream do the formatting and everything for you
without having to use StringPrintf, but considering that you’d then want to
reset the stream to how you found it, it’s probably not worth it.

http://codereview.chromium.org/3443002/diff/7004/32006#newcode583
chrome/app/chrome_dll_main.cc:583: // Always ignore SIGPIPE.  We check the
return value of write().
Allays.

http://codereview.chromium.org/3443002/diff/7004/32007
File chrome/browser/child_process_launcher.cc (right):

http://codereview.chromium.org/3443002/diff/7004/32007#newcode169
chrome/browser/child_process_launcher.cc:169: // AddPlaceholderForPid(),
enabling proper cleanup.
Did you actually test this?

What happens in the browser if the only thing the child ever does in ChromeMain
is exit(0)?

http://codereview.chromium.org/3443002/diff/7004/32007#newcode173
chrome/browser/child_process_launcher.cc:173: // listening for mach ipc before
LaunchApp() is called.
Mach IPC.

Mach is a proper name.

IPC is a capitalized acronym.

Also, somewhere else in the change, you wrote cpu, but you should be writing
CPU.

http://codereview.chromium.org/3443002/diff/7004/32008
File chrome/browser/mach_broker_mac.cc (right):

http://codereview.chromium.org/3443002/diff/7004/32008#newcode143
chrome/browser/mach_broker_mac.cc:143: return Singleton<MachBroker>::get();
You had asked “why is this getting deleted with something like atexit? How can I
make it stop?” You didn’t use those exact words, though.

See base/singleton.h.

When using a singleton like this, you get something whose destructor will be
called atexit. This is “DefaultSingeltonTraits.”

In order to get the non-destructive and leaky behavior, you can use
“LeakySingletonTraits.” You would write this line as:

  return Singleton<MachBroker, LeakySingletonTraits>::get()

http://codereview.chromium.org/3443002/diff/7004/32008#newcode155
chrome/browser/mach_broker_mac.cc:155: // Updates the mapping for |pid| to
include the given |mach_info|.
What’s with the funky indentation?

http://codereview.chromium.org/3443002/diff/7004/32008#newcode157
chrome/browser/mach_broker_mac.cc:157: base::ProcessHandle pid, const MachInfo&
mach_info) {
I prefer

void MachBroker::FinalizePid(base::ProcessHandle pid,
                             const MachInfo& mach_info) {

Don’t you?

It’s OK if you don’t, because you have to format it how I want you to anyway.

http://codereview.chromium.org/3443002/diff/7004/32008#newcode163
chrome/browser/mach_broker_mac.cc:163: LOG(ERROR) << "Unknown process " << pid
<< " is sending mach ipc messages!";
Mach IPC

http://codereview.chromium.org/3443002/diff/7004/32008#newcode167
chrome/browser/mach_broker_mac.cc:167: DCHECK_EQ(1u, mach_map_.count(pid));
You already pulled count into its own variable. You should use it here.

(Otherwise there really isn’t any reason to have count in its own variable,
y’know?)

http://codereview.chromium.org/3443002/diff/7004/32008#newcode168
chrome/browser/mach_broker_mac.cc:168: DCHECK(MACH_PORT_NULL ==
mach_map_[pid].mach_task_);
Was DCHECK_EQ no good here? You used it on the line above.

http://codereview.chromium.org/3443002/diff/7004/32008#newcode169
chrome/browser/mach_broker_mac.cc:169: if (mach_map_[pid].mach_task_ ==
MACH_PORT_NULL)
Why’d you flip the order of your condition between this and the preceding
DCHECK?

I think that humans usually prefer how you wrote it here. Some bots seem to
prefer the former under the assumption that you’ll write = instead of ==. gtest
macros adhere to this bot-loving convention, but we’re not looking at gtest
macros here.

http://codereview.chromium.org/3443002/diff/7004/32008#newcode235
chrome/browser/mach_broker_mac.cc:235: #if defined(GOOGLE_CHROME)
But that’s not the right macro. It’s GOOGLE_CHROME_BUILD.

http://codereview.chromium.org/3443002/diff/7004/32008#newcode236
chrome/browser/mach_broker_mac.cc:236: std::string base = "com.google.Chrome";
1. You’re paying for a std::string when all you need is a static const char[].

2. You’re paying sprintf to do a stupid string operation when you can just write
the string in line.

  static const char kFormatString[] =
#if defined(GOOGLE_CHROME_BUILD)
      "com.google.Chrome"
#else
      "org.chromium.Chromium"
#endif
      ".rohitfork.%d";

http://codereview.chromium.org/3443002/diff/7004/32008#newcode245
chrome/browser/mach_broker_mac.cc:245: return MachPortNameForPid(getpid());
Assert that you’re in a browser process?

http://codereview.chromium.org/3443002/diff/7004/32008#newcode251
chrome/browser/mach_broker_mac.cc:251: return MachPortNameForPid(getppid());
Assert that you’re in a not-so-browser process?

Maybe given that you might have these sorts of asserts, you can actually just
simplify these two into a single function that does the right thing based on
process type. APIs that are easier to not abuse are better for everyone.

http://codereview.chromium.org/3443002/diff/7004/32009
File chrome/browser/mach_broker_mac.h (right):

http://codereview.chromium.org/3443002/diff/7004/32009#newcode96
chrome/browser/mach_broker_mac.h:96: static std::string MachPortNameForPid(pid_t
pid);
“private static” functions that don’t need to access any private static class
fields are almost always better off not even being class members at all. This
one is no exception. It’d be better written as a nonmember function in the .cc
file, either marked static or placed within an anonymous namespace. There’s no
reason for stupid implementation details like this to clutter up the header
unless you can’t get by without it (say, for data access.)

[rohitrao (ping after 24h), 2010-09-16 18:35:06]

http://codereview.chromium.org/3443002/diff/25001/23007
File chrome/browser/child_process_launcher.cc (right):

http://codereview.chromium.org/3443002/diff/25001/23007#newcode174
chrome/browser/child_process_launcher.cc:174:
MachBroker::instance()->AcquireLock();
On 2010/09/16 16:56:36, Mark Mentovai wrote:
> Sure, it’s not the cleanest. Your call. I’m always afraid of early returns
when
> I see locks like this.

Meh, I'm equally afraid of people not realizing what we've done (hidden braces
in mac-only code), although one would be a compile-time failure and the other
would be a run-time failure.  Ok, I'll change it.

http://codereview.chromium.org/3443002/diff/7004/32007#newcode169
chrome/browser/child_process_launcher.cc:169: // PlaceholderForPid(), enabling
proper cleanup.
On 2010/09/16 16:56:36, Mark Mentovai wrote:
> Did you actually test this?

Kinda.  If you put a sleep in the parent after fork() but before
AddPlaceholder() then kill the child while the parent is sleeping, we do the
right thing.  Due to the locking, the "child died" notification is not processed
by the MachBroker until after the placeholder is set.

> What happens in the browser if the only thing the child ever does in
ChromeMain
> is exit(0)?

In this case, no "child died" notification is ever sent, so we end up with a
leaked map entry.  Well, that's not entirely true.  We do get a notification
when you try to close the dead tab, but that notification has the wrong pid in
it.  (Bugs are filed for that.)

http://codereview.chromium.org/3443002/diff/7004/32007#newcode173
chrome/browser/child_process_launcher.cc:173: // listening for mach ipc before
LaunchApp() is called.
On 2010/09/16 16:56:36, Mark Mentovai wrote:
> Mach IPC.
> 
> Mach is a proper name.
> 
> IPC is a capitalized acronym.
> 
> Also, somewhere else in the change, you wrote cpu, but you should be writing
> CPU.

Fixed in as many places as I could find.

http://codereview.chromium.org/3443002/diff/7004/32008
File chrome/browser/mach_broker_mac.cc (right):

http://codereview.chromium.org/3443002/diff/7004/32008#newcode143
chrome/browser/mach_broker_mac.cc:143: return Singleton<MachBroker>::get();
On 2010/09/16 16:56:36, Mark Mentovai wrote:
>   return Singleton<MachBroker, LeakySingletonTraits>::get()

Aah, ok.  This will let me get rid of the thread Disconnect() and the extra
locking.

http://codereview.chromium.org/3443002/diff/7004/32008#newcode155
chrome/browser/mach_broker_mac.cc:155: // Updates the mapping for |pid| to
include the given |mach_info|.
On 2010/09/16 16:56:36, Mark Mentovai wrote:
> What’s with the funky indentation?

You know, just thought I'd mix things up a bit.

http://codereview.chromium.org/3443002/diff/7004/32008#newcode157
chrome/browser/mach_broker_mac.cc:157: base::ProcessHandle pid, const MachInfo&
mach_info) {
On 2010/09/16 16:56:36, Mark Mentovai wrote:
> I prefer
> 
> void MachBroker::FinalizePid(base::ProcessHandle pid,
>                              const MachInfo& mach_info) {
> 
> Don’t you?
> 
> It’s OK if you don’t, because you have to format it how I want you to anyway.

Done.

http://codereview.chromium.org/3443002/diff/7004/32008#newcode163
chrome/browser/mach_broker_mac.cc:163: LOG(ERROR) << "Unknown process " << pid
<< " is sending mach ipc messages!";
On 2010/09/16 16:56:36, Mark Mentovai wrote:
> Mach IPC

Done.

http://codereview.chromium.org/3443002/diff/7004/32008#newcode163
chrome/browser/mach_broker_mac.cc:163: LOG(ERROR) << "Unknown process " << pid
<< " is sending mach ipc messages!";
On 2010/09/16 16:56:36, Mark Mentovai wrote:
> Mach IPC

Done.

http://codereview.chromium.org/3443002/diff/7004/32008#newcode167
chrome/browser/mach_broker_mac.cc:167: DCHECK_EQ(1u, mach_map_.count(pid));
On 2010/09/16 16:56:36, Mark Mentovai wrote:
> You already pulled count into its own variable. You should use it here.

Hah, I bet I did it cause I saw the same map access twice, then forgot to change
one of them.  Fixed.

http://codereview.chromium.org/3443002/diff/7004/32008#newcode168
chrome/browser/mach_broker_mac.cc:168: DCHECK(MACH_PORT_NULL ==
mach_map_[pid].mach_task_);
On 2010/09/16 16:56:36, Mark Mentovai wrote:
> Was DCHECK_EQ no good here? You used it on the line above.

DCHECK_EQ was complaining about signed-ness.

http://codereview.chromium.org/3443002/diff/7004/32008#newcode169
chrome/browser/mach_broker_mac.cc:169: if (mach_map_[pid].mach_task_ ==
MACH_PORT_NULL)
On 2010/09/16 16:56:36, Mark Mentovai wrote:
> Why’d you flip the order of your condition between this and the preceding
> DCHECK?
> 
> I think that humans usually prefer how you wrote it here. Some bots seem to
> prefer the former under the assumption that you’ll write = instead of ==.
gtest
> macros adhere to this bot-loving convention, but we’re not looking at gtest
> macros here.

Changed the line above to match.

http://codereview.chromium.org/3443002/diff/7004/32008#newcode235
chrome/browser/mach_broker_mac.cc:235: #if defined(GOOGLE_CHROME)
On 2010/09/16 16:56:36, Mark Mentovai wrote:
> But that’s not the right macro. It’s GOOGLE_CHROME_BUILD.

Oops.

http://codereview.chromium.org/3443002/diff/7004/32008#newcode236
chrome/browser/mach_broker_mac.cc:236: std::string base = "com.google.Chrome";
On 2010/09/16 16:56:36, Mark Mentovai wrote:
> 1. You’re paying for a std::string when all you need is a static const char[].
> 
> 2. You’re paying sprintf to do a stupid string operation when you can just
write
> the string in line.
> 
>   static const char kFormatString[] =
> #if defined(GOOGLE_CHROME_BUILD)
>       "com.google.Chrome"
> #else
>       "org.chromium.Chromium"
> #endif
>       ".rohitfork.%d";

Done.

http://codereview.chromium.org/3443002/diff/7004/32008#newcode251
chrome/browser/mach_broker_mac.cc:251: return MachPortNameForPid(getppid());
On 2010/09/16 16:56:36, Mark Mentovai wrote:
> Assert that you’re in a not-so-browser process?
> 
> Maybe given that you might have these sorts of asserts, you can actually just
> simplify these two into a single function that does the right thing based on
> process type. APIs that are easier to not abuse are better for everyone.

Done.  I'm checking for the presence of a --type switch (any value will do). 
Should I do something stricter?

http://codereview.chromium.org/3443002/diff/7004/32009
File chrome/browser/mach_broker_mac.h (right):

http://codereview.chromium.org/3443002/diff/7004/32009#newcode96
chrome/browser/mach_broker_mac.h:96: static std::string MachPortNameForPid(pid_t
pid);
On 2010/09/16 16:56:36, Mark Mentovai wrote:
> “private static” functions that don’t need to access any private static class
> fields are almost always better off not even being class members at all. This
> one is no exception. It’d be better written as a nonmember function in the .cc
> file, either marked static or placed within an anonymous namespace. There’s no
> reason for stupid implementation details like this to clutter up the header
> unless you can’t get by without it (say, for data access.)

Done.

[Mark Mentovai, 2010-09-16 18:56:28]

The race sucks. The rest is just anal polish.

http://codereview.chromium.org/3443002/diff/4003/42007
File chrome/browser/child_process_launcher.cc (right):

http://codereview.chromium.org/3443002/diff/4003/42007#newcode180
chrome/browser/child_process_launcher.cc:180: launched =
base::LaunchApp(cmd_line->argv(), env, fds_to_map,
There’s a subtle race here between the Mach send in the child process*1 and the
port creation (and bootstrap service name mapping registration)*2. Do you see
it? It’s fairly easy to fix.

*1 happens inside the execed child forked within LaunchApp called on this line.
*2 happens the first time this code is executed on the previous line, when
MachBroker::instance() is called.

http://codereview.chromium.org/3443002/diff/4003/42008
File chrome/browser/mach_broker_mac.cc (right):

http://codereview.chromium.org/3443002/diff/4003/42008#newcode53
chrome/browser/mach_broker_mac.cc:53: : broker_(broker) {
This will fit on the preceding line, put it there.

http://codereview.chromium.org/3443002/diff/4003/42008#newcode72
chrome/browser/mach_broker_mac.cc:72: // TODO(rohitrao): Should we just kill the
thread here?
Make up your mind.

I say “yes.” Here’s why: if WaitForMessage fails once, and we’re not checking on
why it failed, it might fail again. If the port is totally invalid, for example,
this won’t ever start succeeding or even blocking. It’ll spin rapidly on
mach_msg inside WaitForMessage.

For that reason, you can probably restructure this loop by transforming |while
(true)| into |while ((err = ...->WaitForMessage()) != KERN_SUCCESS)|.

http://codereview.chromium.org/3443002/diff/4003/42008#newcode86
chrome/browser/mach_broker_mac.cc:86: // after the call to FinalizePid(),
enabling proper cleanup.
enabling -> ensuring.

http://codereview.chromium.org/3443002/diff/4003/42008#newcode163
chrome/browser/mach_broker_mac.cc:163: << ", error " << kr;
mach_error_string?

http://codereview.chromium.org/3443002/diff/4003/42008#newcode221
chrome/browser/mach_broker_mac.cc:221: const pid_t pid = (is_child ? getppid() :
getpid());
1. Extra ().

2. Nice use of const for things that are const. Why didn’t you also apply it to
line 218? Or the count on line 139? Or the task on line 76?

http://codereview.chromium.org/3443002/diff/4003/42009
File chrome/browser/mach_broker_mac.h (right):

http://codereview.chromium.org/3443002/diff/4003/42009#newcode73
chrome/browser/mach_broker_mac.h:73: // Does the Right Thing in the browser and
in children processes.
“child processes.” Don’t take fake Romanian too far.

http://codereview.chromium.org/3443002/diff/4003/42009#newcode98
chrome/browser/mach_broker_mac.h:98: friend struct
DefaultSingletonTraits<MachBroker>;
Needed?

[rohitrao (ping after 24h), 2010-09-16 19:28:47]

http://codereview.chromium.org/3443002/diff/4003/42007
File chrome/browser/child_process_launcher.cc (right):

http://codereview.chromium.org/3443002/diff/4003/42007#newcode180
chrome/browser/child_process_launcher.cc:180: launched =
base::LaunchApp(cmd_line->argv(), env, fds_to_map,
On 2010/09/16 18:56:28, Mark Mentovai wrote:
> There’s a subtle race here between the Mach send in the child process*1 and
the
> port creation (and bootstrap service name mapping registration)*2. Do you see
> it? It’s fairly easy to fix.
> 
> *1 happens inside the execed child forked within LaunchApp called on this
line.
> *2 happens the first time this code is executed on the previous line, when
> MachBroker::instance() is called.

The receive port is created before the thread is started.  Added a comment
saying so.

http://codereview.chromium.org/3443002/diff/4003/42008
File chrome/browser/mach_broker_mac.cc (right):

http://codereview.chromium.org/3443002/diff/4003/42008#newcode53
chrome/browser/mach_broker_mac.cc:53: : broker_(broker) {
On 2010/09/16 18:56:28, Mark Mentovai wrote:
> This will fit on the preceding line, put it there.

Done.

http://codereview.chromium.org/3443002/diff/4003/42008#newcode72
chrome/browser/mach_broker_mac.cc:72: // TODO(rohitrao): Should we just kill the
thread here?
On 2010/09/16 18:56:28, Mark Mentovai wrote:
> Make up your mind.
> 
> I say “yes.” Here’s why: if WaitForMessage fails once, and we’re not checking
on
> why it failed, it might fail again. If the port is totally invalid, for
example,
> this won’t ever start succeeding or even blocking. It’ll spin rapidly on
> mach_msg inside WaitForMessage.
> 
> For that reason, you can probably restructure this loop by transforming |while
> (true)| into |while ((err = ...->WaitForMessage()) != KERN_SUCCESS)|.

Done.  Indentation here is a bit wonky.

http://codereview.chromium.org/3443002/diff/4003/42008#newcode86
chrome/browser/mach_broker_mac.cc:86: // after the call to FinalizePid(),
enabling proper cleanup.
On 2010/09/16 18:56:28, Mark Mentovai wrote:
> enabling -> ensuring.

Done.

http://codereview.chromium.org/3443002/diff/4003/42008#newcode163
chrome/browser/mach_broker_mac.cc:163: << ", error " << kr;
On 2010/09/16 18:56:28, Mark Mentovai wrote:
> mach_error_string?

MachErrorCode(), even.

http://codereview.chromium.org/3443002/diff/4003/42008#newcode221
chrome/browser/mach_broker_mac.cc:221: const pid_t pid = (is_child ? getppid() :
getpid());
On 2010/09/16 18:56:28, Mark Mentovai wrote:
> 1. Extra ().
> 
> 2. Nice use of const for things that are const. Why didn’t you also apply it
to
> line 218? Or the count on line 139? Or the task on line 76?

Done.

http://codereview.chromium.org/3443002/diff/4003/42008#newcode221
chrome/browser/mach_broker_mac.cc:221: const pid_t pid = (is_child ? getppid() :
getpid());
On 2010/09/16 18:56:28, Mark Mentovai wrote:
> 1. Extra ().
> 
> 2. Nice use of const for things that are const. Why didn’t you also apply it
to
> line 218? Or the count on line 139? Or the task on line 76?

Done.

http://codereview.chromium.org/3443002/diff/4003/42009
File chrome/browser/mach_broker_mac.h (right):

http://codereview.chromium.org/3443002/diff/4003/42009#newcode73
chrome/browser/mach_broker_mac.h:73: // Does the Right Thing in the browser and
in children processes.
On 2010/09/16 18:56:28, Mark Mentovai wrote:
> “child processes.” Don’t take fake Romanian too far.

Done.

http://codereview.chromium.org/3443002/diff/4003/42009#newcode98
chrome/browser/mach_broker_mac.h:98: friend struct
DefaultSingletonTraits<MachBroker>;
On 2010/09/16 18:56:28, Mark Mentovai wrote:
> Needed?

// If you want to ensure that your class can only exist as a singleton, make
// its constructors private, and make DefaultSingletonTraits<> a friend:

[Mark Mentovai, 2010-09-16 19:40:28]

Stop asking me to review this, because I’ll just keep finding stupid things to
ask you to change. This is fine now. LGTM.

http://codereview.chromium.org/3443002/diff/4003/42009
File chrome/browser/mach_broker_mac.h (right):

http://codereview.chromium.org/3443002/diff/4003/42009#newcode98
chrome/browser/mach_broker_mac.h:98: friend struct
DefaultSingletonTraits<MachBroker>;
rohitrao wrote:
> // If you want to ensure that your class can only exist as a singleton, make
> // its constructors private, and make DefaultSingletonTraits<> a friend:

I know, but I didn’t realize LeakySingletonTraits<x> inherited from
DefaultSingletonTraits<x>. (It does.)

You can tighten this up a tiny bit by making it Leaky instead of Default here
too.

http://codereview.chromium.org/3443002/diff/45002/66006
File chrome/browser/child_process_launcher.cc (right):

http://codereview.chromium.org/3443002/diff/45002/66006#newcode181
chrome/browser/child_process_launcher.cc:181: /* wait= */false, &handle);
Also, the indent on this line got stupid.

http://codereview.chromium.org/3443002/diff/45002/66007
File chrome/browser/mach_broker_mac.cc (right):

http://codereview.chromium.org/3443002/diff/45002/66007#newcode75
chrome/browser/mach_broker_mac.cc:75: == KERN_SUCCESS) {
I like == on the preceding line.

http://codereview.chromium.org/3443002/diff/45002/66007#newcode76
chrome/browser/mach_broker_mac.cc:76: const task_t child_task =
message.GetTranslatedPort(0);
Wanna check the ID of the message to make sure it’s that magic 0 you stuck into
it in the other file?

http://codereview.chromium.org/3443002/diff/45002/66008
File chrome/browser/mach_broker_mac.h (right):

http://codereview.chromium.org/3443002/diff/45002/66008#newcode17
chrome/browser/mach_broker_mac.h:17: #include "base/scoped_ptr.h"
For what?

[rohitrao (ping after 24h), 2010-09-16 20:57:30]

http://codereview.chromium.org/3443002/diff/45002/66006
File chrome/browser/child_process_launcher.cc (right):

http://codereview.chromium.org/3443002/diff/45002/66006#newcode181
chrome/browser/child_process_launcher.cc:181: /* wait= */false, &handle);
On 2010/09/16 19:40:29, Mark Mentovai wrote:
> Also, the indent on this line got stupid.

I blame emacs.

http://codereview.chromium.org/3443002/diff/45002/66007
File chrome/browser/mach_broker_mac.cc (right):

http://codereview.chromium.org/3443002/diff/45002/66007#newcode75
chrome/browser/mach_broker_mac.cc:75: == KERN_SUCCESS) {
On 2010/09/16 19:40:29, Mark Mentovai wrote:
> I like == on the preceding line.

Done.

http://codereview.chromium.org/3443002/diff/45002/66007#newcode76
chrome/browser/mach_broker_mac.cc:76: const task_t child_task =
message.GetTranslatedPort(0);
On 2010/09/16 19:40:29, Mark Mentovai wrote:
> Wanna check the ID of the message to make sure it’s that magic 0 you stuck
into
> it in the other file?

Rewrote the code to skip any messages with a non-zero id.

http://codereview.chromium.org/3443002/diff/45002/66008
File chrome/browser/mach_broker_mac.h (right):

http://codereview.chromium.org/3443002/diff/45002/66008#newcode17
chrome/browser/mach_broker_mac.h:17: #include "base/scoped_ptr.h"
On 2010/09/16 19:40:29, Mark Mentovai wrote:
> For what?

That thread.  It's gone now.

[Mark Mentovai, 2010-09-16 21:57:35]

LGTM

http://codereview.chromium.org/3443002/diff/51003/18019
File chrome/browser/mach_broker_mac.cc (right):

http://codereview.chromium.org/3443002/diff/51003/18019#newcode25
chrome/browser/mach_broker_mac.cc:25: }  // end namespace
}  // namespace

http://codereview.chromium.org/3443002/diff/51003/18020
File chrome/browser/mach_broker_mac.h (right):

http://codereview.chromium.org/3443002/diff/51003/18020#newcode90
chrome/browser/mach_broker_mac.h:90: // true if the listener thread has been
started.
Capitalize.

[Mark Mentovai, 2010-09-16 21:57:57]

unsubscribe