Get ipc_perftests to pass on Mac.

The MachPortRelay and MachPortBroker were not set up in all the
circumstances in which they were required.

This also changes the base::MachPortBroker to log more descriptive
failure messages.

Change-Id: Id711b7a30acb31c2c3366d31f4d0b0bfb1a2815d
Reviewed-on: https://chromium-review.googlesource.com/c/1371932Reviewed-by: Ken Rockot <rockot@google.com>
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Robert Sesek <rsesek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615634}

base/mac/mach_port_broker.mm

@@ -39,14 +39,15 @@ struct MachPortBroker_ParentRecvMsg : public MachPortBroker_ChildSendMsg {
 bool MachPortBroker::ChildSendTaskPortToParent(const std::string& name) {
   // Look up the named MachPortBroker port that's been registered with the
   // bootstrap server.
-  mach_port_t parent_port;
-  kern_return_t kr = bootstrap_look_up(bootstrap_port,
-      const_cast<char*>(GetMachPortName(name, true).c_str()), &parent_port);
+  mac::ScopedMachSendRight parent_port;
+  std::string bootstrap_name = GetMachPortName(name, true);
+  kern_return_t kr = bootstrap_look_up(
+      bootstrap_port, const_cast<char*>(bootstrap_name.c_str()),
+      mac::ScopedMachSendRight::Receiver(parent_port).get());
   if (kr != KERN_SUCCESS) {
-    BOOTSTRAP_LOG(ERROR, kr) << "bootstrap_look_up";
+    BOOTSTRAP_LOG(ERROR, kr) << "bootstrap_look_up " << bootstrap_name;
     return false;
   }
-  base::mac::ScopedMachSendRight scoped_right(parent_port);
 
   // Create the check in message. This will copy a send right on this process'
   // (the child's) task port and send it to the parent.
@@ -54,7 +55,7 @@ bool MachPortBroker::ChildSendTaskPortToParent(const std::string& name) {
   msg.header.msgh_bits = MACH_MSGH_BITS_REMOTE(MACH_MSG_TYPE_COPY_SEND) |
                          MACH_MSGH_BITS_COMPLEX;
   msg.header.msgh_size = sizeof(msg);
-  msg.header.msgh_remote_port = parent_port;
+  msg.header.msgh_remote_port = parent_port.get();
   msg.header.msgh_id = kTaskPortMessageId;
   msg.body.msgh_descriptor_count = 1;
   msg.child_task_port.name = mach_task_self();
@@ -96,14 +97,14 @@ bool MachPortBroker::Init() {
   DCHECK(server_port_.get() == MACH_PORT_NULL);
 
   // Check in with launchd and publish the service name.
-  mach_port_t port;
+  std::string bootstrap_name = GetMachPortName(name_, false);
   kern_return_t kr = bootstrap_check_in(
-      bootstrap_port, GetMachPortName(name_, false).c_str(), &port);
+      bootstrap_port, bootstrap_name.c_str(),
+      mac::ScopedMachReceiveRight::Receiver(server_port_).get());
   if (kr != KERN_SUCCESS) {
-    BOOTSTRAP_LOG(ERROR, kr) << "bootstrap_check_in";
+    BOOTSTRAP_LOG(ERROR, kr) << "bootstrap_check_in " << bootstrap_name;
     return false;
   }
-  server_port_.reset(port);
 
   // Start the dispatch source.
   std::string queue_name =

ipc/run_all_perftests.cc

@@ -7,10 +7,15 @@
 #include "base/command_line.h"
 #include "base/test/perf_test_suite.h"
 #include "base/test/test_io_thread.h"
+#include "build/build_config.h"
 #include "mojo/core/embedder/embedder.h"
 #include "mojo/core/embedder/scoped_ipc_support.h"
 #include "mojo/core/test/test_support_impl.h"
 
+#if defined(OS_MACOSX) && !defined(OS_IOS)
+#include "mojo/core/embedder/default_mach_broker.h"
+#endif
+
 int main(int argc, char** argv) {
   base::PerfTestSuite test(argc, argv);
 
@@ -20,6 +25,10 @@ int main(int argc, char** argv) {
       test_io_thread.task_runner(),
       mojo::core::ScopedIPCSupport::ShutdownPolicy::CLEAN);
   mojo::test::TestSupport::Init(new mojo::core::test::TestSupportImpl());
+#if defined(OS_MACOSX) && !defined(OS_IOS)
+  mojo::core::SetMachPortProvider(
+      mojo::core::DefaultMachBroker::Get()->port_provider());
+#endif
 
   return test.Run();
 }

mojo/core/test/mojo_test_base.cc

@@ -19,11 +19,6 @@
 #include "mojo/public/cpp/system/wait.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-#if defined(OS_MACOSX) && !defined(OS_IOS)
-#include "base/mac/mach_port_broker.h"
-#include "mojo/core/embedder/default_mach_broker.h"
-#endif
-
 namespace mojo {
 namespace core {
 namespace test {
@@ -43,20 +38,7 @@ MojoTestBase::ClientController::ClientController(const std::string& client_name,
                                                  MojoTestBase* test,
                                                  LaunchType launch_type) {
 #if !defined(OS_IOS)
-#if defined(OS_MACOSX)
-  // This lock needs to be held while launching the child because the Mach port
-  // broker only allows task ports to be received from known child processes.
-  // However, it can only know the child process's pid after the child has
-  // launched. To prevent a race where the child process sends its task port
-  // before the pid has been registered, the lock needs to be held over both
-  // launch and child pid registration.
-  auto* broker = DefaultMachBroker::Get();
-  base::AutoLock lock(broker->GetLock());
-#endif
   pipe_ = helper_.StartChild(client_name, launch_type);
-#if defined(OS_MACOSX)
-  broker->ExpectPid(helper_.test_child().Handle());
-#endif
 #endif
 }
 
@@ -69,11 +51,6 @@ int MojoTestBase::ClientController::WaitForShutdown() {
   was_shutdown_ = true;
 #if !defined(OS_IOS)
   int retval = helper_.WaitForChildShutdown();
-#if defined(OS_MACOSX)
-  auto* broker = DefaultMachBroker::Get();
-  base::AutoLock lock(broker->GetLock());
-  broker->RemovePid(helper_.test_child().Handle());
-#endif
   return retval;
 #else
   NOTREACHED();

mojo/core/test/multiprocess_test_helper.cc

@@ -202,8 +202,26 @@ ScopedMessagePipeHandle MultiprocessTestHelper::StartChildWithExtraSwitch(
       break;
   }
 
+#if defined(OS_MACOSX) && !defined(OS_IOS)
+  // This lock needs to be held while launching the child because the Mach port
+  // broker only allows task ports to be received from known child processes.
+  // However, it can only know the child process's pid after the child has
+  // launched. To prevent a race where the child process sends its task port
+  // before the pid has been registered, the lock needs to be held over both
+  // launch and child pid registration.
+  auto* mach_broker = mojo::core::DefaultMachBroker::Get();
+  mach_broker->GetLock().Acquire();
+#endif
+
   test_child_ =
       base::SpawnMultiProcessTestChild(test_child_main, command_line, options);
+
+#if defined(OS_MACOSX) && !defined(OS_IOS)
+  if (test_child_.IsValid())
+    mach_broker->ExpectPid(test_child_.Pid());
+  mach_broker->GetLock().Release();
+#endif
+
   if (launch_type == LaunchType::CHILD || launch_type == LaunchType::PEER)
     channel.RemoteProcessLaunchAttempted();
 
@@ -232,6 +250,13 @@ int MultiprocessTestHelper::WaitForChildShutdown() {
   int rv = -1;
   WaitForMultiprocessTestChildExit(test_child_, TestTimeouts::action_timeout(),
                                    &rv);
+
+#if defined(OS_MACOSX) && !defined(OS_IOS)
+  auto* mach_broker = mojo::core::DefaultMachBroker::Get();
+  base::AutoLock lock(mach_broker->GetLock());
+  mach_broker->RemovePid(test_child_.Pid());
+#endif
+
   test_child_.Close();
   return rv;
 }