Setup sandbox host in browser process early.

This is the precursor step before creating a thread to start the
ServiceManager before content_main_runner is running. It is because
no thread is expected to be when the zygote process is forked. In this
CL, we move the setup to ContentMainRunner::Initialize().

Bug: 740677,729596
Change-Id: I90e95cce8bc32be03c944aaa17f96c79d555e7cb
Reviewed-on: https://chromium-review.googlesource.com/999741Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: Xi Han <hanxi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550191}

content/app/content_main_runner.cc

@@ -61,6 +61,8 @@
 #include "services/service_manager/sandbox/sandbox_type.h"
 #include "ui/base/ui_base_paths.h"
 #include "ui/base/ui_base_switches.h"
+#include "ui/display/display_switches.h"
+#include "ui/gfx/switches.h"
 
 #if defined(OS_WIN)
 #include <malloc.h>
@@ -112,6 +114,15 @@
 #include "content/utility/in_process_utility_thread.h"
 #endif
 
+#if BUILDFLAG(USE_ZYGOTE_HANDLE)
+#include "content/browser/sandbox_host_linux.h"
+#include "content/browser/zygote_host/zygote_communication_linux.h"
+#include "content/browser/zygote_host/zygote_host_impl_linux.h"
+#include "content/public/common/common_sandbox_support_linux.h"
+#include "content/public/common/zygote_handle.h"
+#include "media/base/media_switches.h"
+#endif
+
 namespace content {
 extern int GpuMain(const content::MainFunctionParams&);
 #if BUILDFLAG(ENABLE_PLUGINS)
@@ -233,6 +244,67 @@ void InitializeV8IfNeeded(const base::CommandLine& command_line,
 #endif  // V8_USE_EXTERNAL_STARTUP_DATA
 }
 
+#if BUILDFLAG(USE_ZYGOTE_HANDLE)
+pid_t LaunchZygoteHelper(base::CommandLine* cmd_line,
+                         base::ScopedFD* control_fd) {
+  // Append any switches from the browser process that need to be forwarded on
+  // to the zygote/renderers.
+  static const char* const kForwardSwitches[] = {
+      switches::kAndroidFontsPath, switches::kClearKeyCdmPathForTesting,
+      switches::kEnableHeapProfiling,
+      switches::kEnableLogging,  // Support, e.g., --enable-logging=stderr.
+      // Need to tell the zygote that it is headless so that we don't try to use
+      // the wrong type of main delegate.
+      switches::kHeadless,
+      // Zygote process needs to know what resources to have loaded when it
+      // becomes a renderer process.
+      switches::kForceDeviceScaleFactor, switches::kLoggingLevel,
+      switches::kPpapiInProcess, switches::kRegisterPepperPlugins, switches::kV,
+      switches::kVModule,
+  };
+  cmd_line->CopySwitchesFrom(*base::CommandLine::ForCurrentProcess(),
+                             kForwardSwitches, arraysize(kForwardSwitches));
+
+  GetContentClient()->browser()->AppendExtraCommandLineSwitches(cmd_line, -1);
+
+  // Start up the sandbox host process and get the file descriptor for the
+  // sandboxed processes to talk to it.
+  base::FileHandleMappingVector additional_remapped_fds;
+  additional_remapped_fds.emplace_back(
+      SandboxHostLinux::GetInstance()->GetChildSocket(), GetSandboxFD());
+
+  return ZygoteHostImpl::GetInstance()->LaunchZygote(
+      cmd_line, control_fd, std::move(additional_remapped_fds));
+}
+
+// Initializes the Zygote sandbox host. No thread should be created before this
+// call, as InitializeZygoteSandboxForBrowserProcess() will end-up using fork().
+void InitializeZygoteSandboxForBrowserProcess(
+    const base::CommandLine& parsed_command_line) {
+  TRACE_EVENT0("startup", "SetupSandbox");
+  // SandboxHostLinux needs to be initialized even if the sandbox and
+  // zygote are both disabled. It initializes the sandboxed process socket.
+  SandboxHostLinux::GetInstance()->Init();
+
+  if (parsed_command_line.HasSwitch(switches::kNoZygote) &&
+      !parsed_command_line.HasSwitch(switches::kNoSandbox)) {
+    LOG(ERROR) << "--no-sandbox should be used together with --no--zygote";
+    exit(EXIT_FAILURE);
+  }
+
+  // Tickle the zygote host so it forks now.
+  ZygoteHostImpl::GetInstance()->Init(parsed_command_line);
+  ZygoteHandle generic_zygote =
+      CreateGenericZygote(base::BindOnce(LaunchZygoteHelper));
+
+  // TODO(kerrnel): Investigate doing this without the ZygoteHostImpl as a
+  // proxy. It is currently done this way due to concerns about race
+  // conditions.
+  ZygoteHostImpl::GetInstance()->SetRendererSandboxStatus(
+      generic_zygote->GetSandboxStatus());
+}
+#endif  // BUILDFLAG(USE_ZYGOTE_HANDLE)
+
 }  // namespace
 
 #if !defined(CHROME_MULTIPLE_DLL_CHILD)
@@ -676,6 +748,16 @@ class ContentMainRunnerImpl : public ContentMainRunner {
     if (delegate_)
       delegate_->SandboxInitialized(process_type);
 
+#if BUILDFLAG(USE_ZYGOTE_HANDLE)
+    if (process_type.empty()) {
+      // The sandbox host needs to be initialized before forking a thread to
+      // start the ServiceManager, and after setting up the sandbox and invoking
+      // SandboxInitialized().
+      InitializeZygoteSandboxForBrowserProcess(
+          *base::CommandLine::ForCurrentProcess());
+    }
+#endif  // BUILDFLAG(USE_ZYGOTE_HANDLE)
+
     // Return -1 to indicate no early termination.
     return -1;
   }

content/browser/browser_main_loop.cc

@@ -20,7 +20,6 @@
 #include "base/feature_list.h"
 #include "base/location.h"
 #include "base/logging.h"
-#include "base/macros.h"
 #include "base/memory/memory_coordinator_proxy.h"
 #include "base/memory/memory_pressure_monitor.h"
 #include "base/message_loop/message_loop.h"
@@ -145,12 +144,6 @@
 #include "ui/aura/env.h"
 #endif
 
-#if BUILDFLAG(USE_ZYGOTE_HANDLE)
-#include "content/public/common/common_sandbox_support_linux.h"
-#include "content/public/common/zygote_handle.h"
-#include "media/base/media_switches.h"
-#endif
-
 #if defined(OS_ANDROID)
 #include "base/android/jni_android.h"
 #include "components/tracing/common/graphics_memory_dump_provider_android.h"
@@ -214,13 +207,7 @@
 
 #if defined(OS_POSIX) && !defined(OS_MACOSX)
 #include "content/browser/sandbox_host_linux.h"
-#include "content/browser/zygote_host/zygote_host_impl_linux.h"
-
-#if !defined(OS_ANDROID)
-#include "content/browser/zygote_host/zygote_communication_linux.h"
-#endif  // !defined(OS_ANDROID)
-#endif  // defined(OS_POSIX) && !defined(OS_MACOSX)
-
+#endif
 
 #if BUILDFLAG(ENABLE_PLUGINS)
 #include "content/browser/plugin_service_impl.h"
@@ -261,64 +248,6 @@
 namespace content {
 namespace {
 
-#if BUILDFLAG(USE_ZYGOTE_HANDLE)
-pid_t LaunchZygoteHelper(base::CommandLine* cmd_line,
-                         base::ScopedFD* control_fd) {
-  // Append any switches from the browser process that need to be forwarded on
-  // to the zygote/renderers.
-  static const char* const kForwardSwitches[] = {
-      switches::kAndroidFontsPath, switches::kClearKeyCdmPathForTesting,
-      switches::kEnableHeapProfiling,
-      switches::kEnableLogging,  // Support, e.g., --enable-logging=stderr.
-      // Need to tell the zygote that it is headless so that we don't try to use
-      // the wrong type of main delegate.
-      switches::kHeadless,
-      // Zygote process needs to know what resources to have loaded when it
-      // becomes a renderer process.
-      switches::kForceDeviceScaleFactor, switches::kLoggingLevel,
-      switches::kPpapiInProcess, switches::kRegisterPepperPlugins, switches::kV,
-      switches::kVModule,
-  };
-  cmd_line->CopySwitchesFrom(*base::CommandLine::ForCurrentProcess(),
-                             kForwardSwitches, arraysize(kForwardSwitches));
-
-  GetContentClient()->browser()->AppendExtraCommandLineSwitches(cmd_line, -1);
-
-  // Start up the sandbox host process and get the file descriptor for the
-  // sandboxed processes to talk to it.
-  base::FileHandleMappingVector additional_remapped_fds;
-  additional_remapped_fds.emplace_back(
-      SandboxHostLinux::GetInstance()->GetChildSocket(), GetSandboxFD());
-
-  return ZygoteHostImpl::GetInstance()->LaunchZygote(
-      cmd_line, control_fd, std::move(additional_remapped_fds));
-}
-
-void SetupSandbox(const base::CommandLine& parsed_command_line) {
-  TRACE_EVENT0("startup", "SetupSandbox");
-  // SandboxHostLinux needs to be initialized even if the sandbox and
-  // zygote are both disabled. It initializes the sandboxed process socket.
-  SandboxHostLinux::GetInstance()->Init();
-
-  if (parsed_command_line.HasSwitch(switches::kNoZygote) &&
-      !parsed_command_line.HasSwitch(switches::kNoSandbox)) {
-    LOG(ERROR) << "--no-sandbox should be used together with --no--zygote";
-    exit(EXIT_FAILURE);
-  }
-
-  // Tickle the zygote host so it forks now.
-  ZygoteHostImpl::GetInstance()->Init(parsed_command_line);
-  ZygoteHandle generic_zygote =
-      CreateGenericZygote(base::BindOnce(LaunchZygoteHelper));
-
-  // TODO(kerrnel): Investigate doing this without the ZygoteHostImpl as a
-  // proxy. It is currently done this way due to concerns about race
-  // conditions.
-  ZygoteHostImpl::GetInstance()->SetRendererSandboxStatus(
-      generic_zygote->GetSandboxStatus());
-}
-#endif  // BUILDFLAG(USE_ZYGOTE_HANDLE)
-
 #if defined(USE_GLIB)
 static void GLibLogHandler(const gchar* log_domain,
                            GLogLevelFlags log_level,
@@ -614,9 +543,11 @@ int BrowserMainLoop::EarlyInitialization() {
   TRACE_EVENT0("startup", "BrowserMainLoop::EarlyInitialization");
 
 #if BUILDFLAG(USE_ZYGOTE_HANDLE)
-  // No thread should be created before this call, as SetupSandbox()
-  // will end-up using fork().
-  SetupSandbox(parsed_command_line_);
+  // The initialization of the sandbox host ends up with forking the Zygote
+  // process and requires no thread been forked. The initialization has happened
+  // by now since a thread to start the ServiceManager has been created
+  // before the browser main loop starts.
+  DCHECK(SandboxHostLinux::GetInstance()->IsInitialized());
 #endif
 
 #if defined(USE_X11)

content/browser/sandbox_host_linux.h

@@ -38,6 +38,8 @@ class CONTENT_EXPORT SandboxHostLinux {
   }
   void Init();
 
+  bool IsInitialized() const { return initialized_; }
+
  private:
   friend struct base::DefaultSingletonTraits<SandboxHostLinux>;
   // This object must be constructed on the main thread.