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

content/browser/mach_broker_mac.mm

Index: content/browser/mach_broker_mac.mm
diff --git a/content/browser/mach_broker_mac.cc b/content/browser/mach_broker_mac.mm
similarity index 48%
rename from content/browser/mach_broker_mac.cc
rename to content/browser/mach_broker_mac.mm
index b25563586884c818f2dcbc0bd76d736d99a0325f..7f73bd3b7070cabb39a766a1790fb759e02fa57d 100644
--- a/content/browser/mach_broker_mac.cc
+++ b/content/browser/mach_broker_mac.mm
@@ -4,11 +4,15 @@
 
 #include "content/browser/mach_broker_mac.h"
 
+#include <bsm/libbsm.h>
+#include <mach/mach.h>
+
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/mac/foundation_util.h"
+#include "base/mac/scoped_mach_port.h"
 #include "base/mach_ipc_mac.h"
 #include "base/string_util.h"
 #include "base/stringprintf.h"
@@ -24,83 +28,161 @@
 namespace content {
 
 namespace {
+
 // Prints a string representation of a Mach error code.
 std::string MachErrorCode(kern_return_t err) {
   return base::StringPrintf("0x%x %s", err, mach_error_string(err));
 }
+
+// Mach message structure used in the child as a sending message.
+struct MachBroker_ChildSendMsg {
+  mach_msg_header_t header;
+  mach_msg_body_t body;
+  mach_msg_port_descriptor_t child_task_port;
+};
+
+// Complement to the ChildSendMsg, this is use din the parent for receiving

[Mark Mentovai, 2013/04/10 18:43:51]

used in

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

Done.

+// a message. Contains a message trailer with audit information.
+struct MachBroker_ParentRecvMsg : public MachBroker_ChildSendMsg {
+  mach_msg_audit_trailer_t trailer;
+};
+
 }  // namespace
 
 class MachListenerThreadDelegate : public base::PlatformThread::Delegate {
  public:
-  MachListenerThreadDelegate(MachBroker* broker) : broker_(broker) {
+  MachListenerThreadDelegate(MachBroker* broker)

[Mark Mentovai, 2013/04/10 18:43:51]

explicit

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

Done.

+      : broker_(broker),
+        server_port_(MACH_PORT_NULL) {
     DCHECK(broker_);
-    std::string port_name = MachBroker::GetMachPortName();
+  }
+
+  bool Init() {
+    mach_port_t port;

[Mark Mentovai, 2013/04/10 18:43:51]

DCHECK that this isn’t called more than once (by checking that server_port_ is
MACH_PORT_NULL?)

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

Done.

+    kern_return_t kr = mach_port_allocate(mach_task_self(),

[Mark Mentovai, 2013/04/10 18:43:51]

Cache mach_task_self() since you use it twice in this function.

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

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

+                                          MACH_PORT_RIGHT_RECEIVE,
+                                          &port);
+    if (kr != KERN_SUCCESS) {
+      LOG(ERROR) << "Failed to allocate MachBroker server port: "
+                  << MachErrorCode(kr);

[Mark Mentovai, 2013/04/10 18:43:51]

Alignment.

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

Done.

+      return false;
+    }
+
+    // Allocate a send right for the server port.
+    kr = mach_port_insert_right(
+        mach_task_self(), port, port, MACH_MSG_TYPE_MAKE_SEND);
+    if (kr != KERN_SUCCESS) {
+      LOG(ERROR) << "Failed to insert send right for MachBroker server port: "
+                 << MachErrorCode(kr);
+      return false;
+    }
 
-    // Create the receive port in the constructor, not in ThreadMain().  It is
-    // important to create and register the receive port before starting the
-    // thread so that child processes will always have someone who's listening.
-    receive_port_.reset(new base::ReceivePort(port_name.c_str()));
+    server_port_.reset(port);
+
+    // Register the port with the bootstrap server. Because bootstrap_register
+    // is deprecated, this has to be wraped in an ObjC interface.
+    NSPort* ns_port = [NSMachPort portWithMachPort:port
+                                           options:NSMachPortDeallocateNone];
+    NSString* name = base::SysUTF8ToNSString(broker_->GetMachPortName());
+    return [[NSMachBootstrapServer sharedInstance] registerPort:ns_port
+                                                           name:name];

[Mark Mentovai, 2013/04/10 18:43:51]

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?

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

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

   }
 
   // Implement |PlatformThread::Delegate|.
   virtual void ThreadMain() OVERRIDE {
-    base::MachReceiveMessage message;
-    kern_return_t err;
-    while ((err = receive_port_->WaitForMessage(&message,
-                                                MACH_MSG_TIMEOUT_NONE)) ==
-           KERN_SUCCESS) {
-      // 0 was the secret message id.  Reject any messages that don't have it.
-      if (message.GetMessageID() != 0) {
-        LOG(ERROR) << "Received message with incorrect id: "
-                   << message.GetMessageID();
-        continue;
-      }
-
-      const task_t child_task = message.GetTranslatedPort(0);
-      if (child_task == MACH_PORT_NULL) {
-        LOG(ERROR) << "parent GetTranslatedPort(0) failed.";
-        continue;
+    MachBroker_ParentRecvMsg msg;
+    bzero(&msg, sizeof(msg));

[Mark Mentovai, 2013/04/10 18:43:51]

It’s more usual in Chrome code to write msg = { 0 };

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

The compiler says:

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

+    msg.header.msgh_size = sizeof(msg);
+    msg.header.msgh_local_port = server_port_.get();
+
+    kern_return_t kr;
+    do {
+      // Use the kernel audit information to make sure this message is from
+      // a task that this process spawned. The kernel audit token contains the
+      // unspoofable pid of the task that sent the message.
+      mach_msg_option_t options = MACH_RCV_MSG |
+          MACH_RCV_TRAILER_TYPE(MACH_RCV_TRAILER_AUDIT) |
+          MACH_RCV_TRAILER_ELEMENTS(MACH_RCV_TRAILER_AUDIT);
+
+      kr = mach_msg(&msg.header, options, 0, sizeof(msg), server_port_,
+          MACH_MSG_TIMEOUT_NONE, MACH_PORT_NULL);
+      if (kr == KERN_SUCCESS) {
+        // TODO(rsesek): In the 10.7 SDK, there's audit_token_to_pid().
+        pid_t child_pid;
+        audit_token_to_au32(msg.trailer.msgh_audit,
+            NULL, NULL, NULL, NULL, NULL, &child_pid, NULL, NULL);
+
+        mach_port_t child_task_port = msg.child_task_port.name;
+
+        // Take the lock and update the broker information.
+        base::AutoLock lock(broker_->GetLock());
+        broker_->FinalizePid(child_pid, child_task_port);
       }
+    } while (kr == KERN_SUCCESS);
 
-      // It is possible for the child process to die after the call to
-      // |pid_for_task()| but before the call to |FinalizePid()|.  To prevent
-      // leaking MachBroker map entries in this case, lock around both these
-      // calls.  If the child dies, the death notification will be processed
-      // after the call to FinalizePid(), ensuring proper cleanup.
-      base::AutoLock lock(broker_->GetLock());
-
-      int pid;
-      err = pid_for_task(child_task, &pid);
-      if (err == KERN_SUCCESS) {
-        broker_->FinalizePid(pid,
-                             MachBroker::MachInfo().SetTask(child_task));
-      } else {
-        LOG(ERROR) << "Error getting pid for task " << child_task
-                   << ": " << MachErrorCode(err);
-      }
-    }
-
-    LOG(ERROR) << "Mach listener thread exiting; "
-               << "parent WaitForMessage() likely failed: "
-               << MachErrorCode(err);
+    LOG(ERROR) << "MachBroker thread exiting; mach_msg() likely failed: "
+               << MachErrorCode(kr);
   }
 
  private:
-  // The Mach port to listen on.  Created on thread startup.
-  scoped_ptr<base::ReceivePort> receive_port_;
-
   // The MachBroker to use when new child task rights are received.  Can be
   // NULL.
   MachBroker* broker_;  // weak
 
+  base::mac::ScopedMachPort server_port_;
+
   DISALLOW_COPY_AND_ASSIGN(MachListenerThreadDelegate);
 };
 
-// Returns the global MachBroker.
+bool MachBroker::ChildSendTaskPortToParent() {
+  // Look up the named MachBroker port that's been registered with the
+  // bootstrap server.
+  mach_port_t bootstrap_port;
+  kern_return_t kr = task_get_bootstrap_port(mach_task_self(), &bootstrap_port);

[Mark Mentovai, 2013/04/10 18:43:51]

Cache mach_task_self in this function too.

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

Same as above.

+  if (kr != KERN_SUCCESS) {
+    LOG(ERROR) << "Failed to look up bootstrap port: " << MachErrorCode(kr);
+    return false;
+  }
+
+  mach_port_t parent_port;
+  kr = bootstrap_look_up(bootstrap_port,
+      const_cast<char*>(GetMachPortName().c_str()), &parent_port);
+  if (kr != KERN_SUCCESS) {
+    LOG(ERROR) << "Failed to look up named parent port: " << MachErrorCode(kr);
+    return false;
+  }
+
+  // Create the check in message. This will copy a send right on this' (the

[Mark Mentovai, 2013/04/10 18:43:51]

this'?

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

Done.

+  // child's) task port and send it to the parent.
+  MachBroker_ChildSendMsg msg;

[Mark Mentovai, 2013/04/10 18:43:51]

= {0}

+  bzero(&msg, sizeof(msg));
+  msg.header.msgh_bits = MACH_MSGH_BITS_REMOTE(MACH_MSG_TYPE_COPY_SEND) |
+                         MACH_MSGH_BITS_COMPLEX;
+  msg.header.msgh_remote_port = parent_port;
+  msg.header.msgh_size = sizeof(msg);
+  msg.body.msgh_descriptor_count = 1;
+  msg.child_task_port.name = mach_task_self();
+  msg.child_task_port.disposition = MACH_MSG_TYPE_PORT_SEND;
+  msg.child_task_port.type = MACH_MSG_PORT_DESCRIPTOR;
+
+  kr = mach_msg(&msg.header, MACH_SEND_MSG | MACH_SEND_TIMEOUT, sizeof(msg),
+      0, MACH_PORT_NULL, 100 /*milliseconds*/, MACH_PORT_NULL);

[Mark Mentovai, 2013/04/10 18:43:51]

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]

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

+  if (kr != KERN_SUCCESS) {
+    LOG(ERROR) << "Failed to send task port to parent: " << MachErrorCode(kr);
+    return false;
+  }
+
+  return true;
+}
+
 MachBroker* MachBroker::GetInstance() {
   return Singleton<MachBroker, LeakySingletonTraits<MachBroker> >::get();
 }
 
+base::Lock& MachBroker::GetLock() {
+  return lock_;
+}
+
 void MachBroker::EnsureRunning() {
   lock_.AssertAcquired();
 
@@ -112,64 +194,28 @@ void MachBroker::EnsureRunning() {
         base::Bind(&MachBroker::RegisterNotifications, base::Unretained(this)));
 
     // Intentional leak.  This thread is never joined or reaped.
-    base::PlatformThread::CreateNonJoinable(
-        0, new MachListenerThreadDelegate(this));
+    MachListenerThreadDelegate* thread = new MachListenerThreadDelegate(this);
+    if (thread->Init()) {
+      base::PlatformThread::CreateNonJoinable(0, thread);
+    } else {
+      LOG(ERROR) << "Failed to initialize the MachListenerThreadDelegate";
+    }
   }
 }
 
-// Adds a placeholder to the map for the given pid with MACH_PORT_NULL.
 void MachBroker::AddPlaceholderForPid(base::ProcessHandle pid) {
   lock_.AssertAcquired();
 
-  MachInfo mach_info;
   DCHECK_EQ(0u, mach_map_.count(pid));
-  mach_map_[pid] = mach_info;
+  mach_map_[pid] = MACH_PORT_NULL;
 }
 
-// Updates the mapping for |pid| to include the given |mach_info|.
-void MachBroker::FinalizePid(base::ProcessHandle pid,
-                             const MachInfo& mach_info) {
-  lock_.AssertAcquired();
-
-  const int count = mach_map_.count(pid);
-  if (count == 0) {
-    // Do nothing for unknown pids.
-    LOG(ERROR) << "Unknown process " << pid << " is sending Mach IPC messages!";
-    return;
-  }
-
-  DCHECK_EQ(1, count);
-  DCHECK(mach_map_[pid].mach_task_ == MACH_PORT_NULL);
-  if (mach_map_[pid].mach_task_ == MACH_PORT_NULL)
-    mach_map_[pid] = mach_info;
-}
-
-// Removes all mappings belonging to |pid| from the broker.
-void MachBroker::InvalidatePid(base::ProcessHandle pid) {
-  base::AutoLock lock(lock_);
-  MachBroker::MachMap::iterator it = mach_map_.find(pid);
-  if (it == mach_map_.end())
-    return;
-
-  kern_return_t kr = mach_port_deallocate(mach_task_self(),
-                                          it->second.mach_task_);
-  LOG_IF(WARNING, kr != KERN_SUCCESS)
-     << "Failed to mach_port_deallocate mach task " << it->second.mach_task_
-     << ", error " << MachErrorCode(kr);
-  mach_map_.erase(it);
-}
-
-base::Lock& MachBroker::GetLock() {
-  return lock_;
-}
-
-// Returns the mach task belonging to |pid|.
 mach_port_t MachBroker::TaskForPid(base::ProcessHandle pid) const {
   base::AutoLock lock(lock_);
   MachBroker::MachMap::const_iterator it = mach_map_.find(pid);
   if (it == mach_map_.end())
     return MACH_PORT_NULL;
-  return it->second.mach_task_;
+  return it->second;
 }
 
 void MachBroker::BrowserChildProcessHostDisconnected(
@@ -203,6 +249,41 @@ void MachBroker::Observe(int type,
   InvalidatePid(handle);
 }
 
+MachBroker::MachBroker() : listener_thread_started_(false) {
+}
+
+MachBroker::~MachBroker() {}
+
+void MachBroker::FinalizePid(base::ProcessHandle pid,
+                             mach_port_t task_port) {
+  lock_.AssertAcquired();
+
+  MachMap::iterator it = mach_map_.find(pid);
+  if (it == mach_map_.end()) {
+    // Do nothing for unknown pids.
+    LOG(ERROR) << "Unknown process " << pid << " is sending Mach IPC messages!";
+    return;
+  }
+
+  DCHECK(it->second == MACH_PORT_NULL);
+  if (it->second == MACH_PORT_NULL)
+    it->second = task_port;
+}
+
+void MachBroker::InvalidatePid(base::ProcessHandle pid) {
+  base::AutoLock lock(lock_);
+  MachBroker::MachMap::iterator it = mach_map_.find(pid);
+  if (it == mach_map_.end())
+    return;
+
+  kern_return_t kr = mach_port_deallocate(mach_task_self(),
+                                          it->second);
+  LOG_IF(WARNING, kr != KERN_SUCCESS)
+     << "Failed to mach_port_deallocate mach task " << it->second
+     << ", error " << MachErrorCode(kr);
+  mach_map_.erase(it);
+}
+
 // static
 std::string MachBroker::GetMachPortName() {
   const CommandLine* command_line = CommandLine::ForCurrentProcess();
@@ -213,11 +294,6 @@ std::string MachBroker::GetMachPortName() {
   return base::StringPrintf("%s.rohitfork.%d", base::mac::BaseBundleID(), pid);
 }
 
-MachBroker::MachBroker() : listener_thread_started_(false) {
-}
-
-MachBroker::~MachBroker() {}
-
 void MachBroker::RegisterNotifications() {
   registrar_.Add(this, NOTIFICATION_RENDERER_PROCESS_CLOSED,
                  NotificationService::AllBrowserContextsAndSources());