Reimplement GetAppOutput and friends on Mac with posix_spawn.

Mac no longer uses launch_posix.cc and all process creation goes through
posix_spawn now.

Bug: 179923
Change-Id: I5206cace69ea7863a72b58e1a59f2465492d4cbd
Reviewed-on: https://chromium-review.googlesource.com/c/1313334Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Robert Sesek <rsesek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604982}

base/BUILD.gn

@@ -1671,6 +1671,7 @@ jumbo_component("base") {
   # Desktop Mac.
   if (is_mac) {
     sources -= [
+      "process/launch_posix.cc",
       "profiler/native_stack_sampler_posix.cc",
       "sampling_heap_profiler/module_cache_posix.cc",
     ]

base/process/launch.h

@@ -297,10 +297,12 @@ BASE_EXPORT Process LaunchElevatedProcess(const CommandLine& cmdline,
 BASE_EXPORT Process LaunchProcess(const std::vector<std::string>& argv,
                                   const LaunchOptions& options);
 
+#if !defined(OS_MACOSX)
 // Close all file descriptors, except those which are a destination in the
 // given multimap. Only call this function in a child process where you know
 // that there aren't any other threads.
 BASE_EXPORT void CloseSuperfluousFds(const InjectiveMultimap& saved_map);
+#endif  // defined(OS_MACOSX)
 #endif  // defined(OS_WIN)
 
 #if defined(OS_WIN)
@@ -352,23 +354,6 @@ BASE_EXPORT bool GetAppOutputAndError(const std::vector<std::string>& argv,
 // the current process's scheduling priority to a high priority.
 BASE_EXPORT void RaiseProcessToHighPriority();
 
-#if defined(OS_MACOSX)
-// An implementation of LaunchProcess() that uses posix_spawn() instead of
-// fork()+exec(). This does not support the |pre_exec_delegate| and
-// |current_directory| options.
-Process LaunchProcessPosixSpawn(const std::vector<std::string>& argv,
-                                const LaunchOptions& options);
-
-// Restore the default exception handler, setting it to Apple Crash Reporter
-// (ReportCrash).  When forking and execing a new process, the child will
-// inherit the parent's exception ports, which may be set to the Breakpad
-// instance running inside the parent.  The parent's Breakpad instance should
-// not handle the child's exceptions.  Calling RestoreDefaultExceptionHandler
-// in the child after forking will restore the standard exception handler.
-// See http://crbug.com/20371/ for more details.
-void RestoreDefaultExceptionHandler();
-#endif  // defined(OS_MACOSX)
-
 // Creates a LaunchOptions object suitable for launching processes in a test
 // binary. This should not be called in production/released code.
 BASE_EXPORT LaunchOptions LaunchOptionsForTest();

base/process/launch_mac.cc

@@ -11,10 +11,14 @@
 #include <sys/syscall.h>
 #include <sys/wait.h>
 
+#include "base/command_line.h"
+#include "base/files/scoped_file.h"
 #include "base/logging.h"
 #include "base/mac/availability.h"
 #include "base/posix/eintr_wrapper.h"
 #include "base/threading/scoped_blocking_call.h"
+#include "base/threading/thread_restrictions.h"
+#include "base/trace_event/trace_event.h"
 
 // Changes the current thread's directory to a path or directory file
 // descriptor. libpthread only exposes a syscall wrapper starting in
@@ -27,6 +31,13 @@ int pthread_fchdir_np(int fd) API_AVAILABLE(macosx(10.12));
 
 namespace base {
 
+// Friend and derived class of ScopedAllowBaseSyncPrimitives which allows
+// GetAppOutputInternal() to join a process. GetAppOutputInternal() can't itself
+// be a friend of ScopedAllowBaseSyncPrimitives because it is in the anonymous
+// namespace.
+class GetAppOutputScopedAllowBaseSyncPrimitives
+    : public base::ScopedAllowBaseSyncPrimitives {};
+
 namespace {
 
 // DPSXCHECK is a Debug Posix Spawn Check macro. The posix_spawn* family of
@@ -100,27 +111,78 @@ int ResetCurrentThreadDirectory() {
   }
 }
 
+struct GetAppOutputOptions {
+  // Whether to pipe stderr to stdout in |output|.
+  bool include_stderr = false;
+  // Caller-supplied string poiter for the output.
+  std::string* output = nullptr;
+  // Result exit code of Process::Wait().
+  int exit_code = 0;
+};
+
+bool GetAppOutputInternal(const std::vector<std::string>& argv,
+                          GetAppOutputOptions* gao_options) {
+  ScopedFD read_fd, write_fd;
+  {
+    int pipefds[2];
+    if (pipe(pipefds) != 0) {
+      DPLOG(ERROR) << "pipe";
+      return false;
+    }
+    read_fd.reset(pipefds[0]);
+    write_fd.reset(pipefds[1]);
+  }
+
+  LaunchOptions launch_options;
+  launch_options.fds_to_remap.emplace_back(write_fd.get(), STDOUT_FILENO);
+  if (gao_options->include_stderr) {
+    launch_options.fds_to_remap.emplace_back(write_fd.get(), STDERR_FILENO);
+  }
+
+  Process process = LaunchProcess(argv, launch_options);
+
+  // Close the parent process' write descriptor, so that EOF is generated in
+  // read loop below.
+  write_fd.reset();
+
+  // Read the child's output before waiting for its exit, otherwise the pipe
+  // buffer may fill up if the process is producing a lot of output.
+  std::string* output = gao_options->output;
+  output->clear();
+
+  const size_t kBufferSize = 1024;
+  size_t total_bytes_read = 0;
+  ssize_t read_this_pass = 0;
+  do {
+    output->resize(output->size() + kBufferSize);
+    read_this_pass = HANDLE_EINTR(
+        read(read_fd.get(), &(*output)[total_bytes_read], kBufferSize));
+    if (read_this_pass >= 0) {
+      total_bytes_read += read_this_pass;
+      output->resize(total_bytes_read);
+    }
+  } while (read_this_pass > 0);
+
+  // Reap the child process.
+  GetAppOutputScopedAllowBaseSyncPrimitives allow_wait;
+  if (!process.WaitForExit(&gao_options->exit_code)) {
+    return false;
+  }
+
+  return read_this_pass == 0;
+}
+
 }  // namespace
 
-void RestoreDefaultExceptionHandler() {
-  // This function is tailored to remove the Breakpad exception handler.
-  // exception_mask matches s_exception_mask in
-  // third_party/breakpad/breakpad/src/client/mac/handler/exception_handler.cc
-  const exception_mask_t exception_mask = EXC_MASK_BAD_ACCESS |
-                                          EXC_MASK_BAD_INSTRUCTION |
-                                          EXC_MASK_ARITHMETIC |
-                                          EXC_MASK_BREAKPOINT;
-
-  // Setting the exception port to MACH_PORT_NULL may not be entirely
-  // kosher to restore the default exception handler, but in practice,
-  // it results in the exception port being set to Apple Crash Reporter,
-  // the desired behavior.
-  task_set_exception_ports(mach_task_self(), exception_mask, MACH_PORT_NULL,
-                           EXCEPTION_DEFAULT, THREAD_STATE_NONE);
+Process LaunchProcess(const CommandLine& cmdline,
+                      const LaunchOptions& options) {
+  return LaunchProcess(cmdline.argv(), options);
 }
 
-Process LaunchProcessPosixSpawn(const std::vector<std::string>& argv,
-                                const LaunchOptions& options) {
+Process LaunchProcess(const std::vector<std::string>& argv,
+                      const LaunchOptions& options) {
+  TRACE_EVENT0("base", "LaunchProcess");
+
   PosixSpawnAttr attr;
 
   short flags = POSIX_SPAWN_CLOEXEC_DEFAULT;
@@ -217,4 +279,38 @@ Process LaunchProcessPosixSpawn(const std::vector<std::string>& argv,
   return Process(pid);
 }
 
+bool GetAppOutput(const CommandLine& cl, std::string* output) {
+  return GetAppOutput(cl.argv(), output);
+}
+
+bool GetAppOutputAndError(const CommandLine& cl, std::string* output) {
+  return GetAppOutputAndError(cl.argv(), output);
+}
+
+bool GetAppOutputWithExitCode(const CommandLine& cl,
+                              std::string* output,
+                              int* exit_code) {
+  GetAppOutputOptions options;
+  options.output = output;
+  bool rv = GetAppOutputInternal(cl.argv(), &options);
+  *exit_code = options.exit_code;
+  return rv;
+}
+
+bool GetAppOutput(const std::vector<std::string>& argv, std::string* output) {
+  GetAppOutputOptions options;
+  options.output = output;
+  return GetAppOutputInternal(argv, &options) &&
+         options.exit_code == EXIT_SUCCESS;
+}
+
+bool GetAppOutputAndError(const std::vector<std::string>& argv,
+                          std::string* output) {
+  GetAppOutputOptions options;
+  options.include_stderr = true;
+  options.output = output;
+  return GetAppOutputInternal(argv, &options) &&
+         options.exit_code == EXIT_SUCCESS;
+}
+
 }  // namespace base

base/process/launch_posix.cc

@@ -60,13 +60,10 @@
 #endif
 
 #if defined(OS_MACOSX)
-#include <crt_externs.h>
-#include <sys/event.h>
+#error "macOS should use launch_mac.cc"
+#endif
 
-#include "base/feature_list.h"
-#else
 extern char** environ;
-#endif
 
 namespace base {
 
@@ -81,29 +78,16 @@ class GetAppOutputScopedAllowBaseSyncPrimitives
 
 namespace {
 
-#if defined(OS_MACOSX)
-const Feature kMacLaunchProcessPosixSpawn{"MacLaunchProcessPosixSpawn",
-                                          FEATURE_ENABLED_BY_DEFAULT};
-#endif
-
 // Get the process's "environment" (i.e. the thing that setenv/getenv
 // work with).
 char** GetEnvironment() {
-#if defined(OS_MACOSX)
-  return *_NSGetEnviron();
-#else
   return environ;
-#endif
 }
 
 // Set the process's "environment" (i.e. the thing that setenv/getenv
 // work with).
 void SetEnvironment(char** env) {
-#if defined(OS_MACOSX)
-  *_NSGetEnviron() = env;
-#else
   environ = env;
-#endif
 }
 
 // Set the calling thread's signal mask to new_sigmask and return
@@ -222,8 +206,6 @@ typedef std::unique_ptr<DIR, ScopedDIRClose> ScopedDIR;
 
 #if defined(OS_LINUX) || defined(OS_AIX)
 static const char kFDDir[] = "/proc/self/fd";
-#elif defined(OS_MACOSX)
-static const char kFDDir[] = "/dev/fd";
 #elif defined(OS_SOLARIS)
 static const char kFDDir[] = "/dev/fd";
 #elif defined(OS_FREEBSD)
@@ -302,12 +284,6 @@ Process LaunchProcess(const CommandLine& cmdline,
 Process LaunchProcess(const std::vector<std::string>& argv,
                       const LaunchOptions& options) {
   TRACE_EVENT0("base", "LaunchProcess");
-#if defined(OS_MACOSX)
-  // TODO(rsesek): Remove this feature. https://crbug.com/179923.
-  if (FeatureList::IsEnabled(kMacLaunchProcessPosixSpawn)) {
-    return LaunchProcessPosixSpawn(argv, options);
-  }
-#endif
 
   InjectiveMultimap fd_shuffle1;
   InjectiveMultimap fd_shuffle2;
@@ -427,10 +403,6 @@ Process LaunchProcess(const std::vector<std::string>& argv,
       }
     }
 
-#if defined(OS_MACOSX)
-    RestoreDefaultExceptionHandler();
-#endif  // defined(OS_MACOSX)
-
     ResetChildSignalHandlersToDefaults();
     SetSignalMask(orig_sigmask);
 
@@ -497,11 +469,9 @@ Process LaunchProcess(const std::vector<std::string>& argv,
       RAW_CHECK(chdir(current_directory) == 0);
     }
 
-#if !defined(OS_MACOSX)
     if (options.pre_exec_delegate != nullptr) {
       options.pre_exec_delegate->RunAsyncSafe();
     }
-#endif
 
     const char* executable_path = !options.real_path.empty() ?
         options.real_path.value().c_str() : argv_cstr[0];
@@ -583,10 +553,6 @@ static bool GetAppOutputInternal(
       // DANGER: no calls to malloc or locks are allowed from now on:
       // http://crbug.com/36678
 
-#if defined(OS_MACOSX)
-      RestoreDefaultExceptionHandler();
-#endif
-
       // Obscure fork() rule: in the child, if you don't end up doing exec*(),
       // you call _exit() instead of exit(). This is because _exit() does not
       // call any previously-registered (in the parent) exit handlers, which