Refactor GPU init to allow ARM sandboxing.

content/gpu/gpu_main.cc

Index: content/gpu/gpu_main.cc
diff --git a/content/gpu/gpu_main.cc b/content/gpu/gpu_main.cc
index 270a52b7508a06f82c8cc97cd3fff72c2937315b..41479c593055afb17c779668697dbddf319058bc 100644
--- a/content/gpu/gpu_main.cc
+++ b/content/gpu/gpu_main.cc
@@ -54,7 +54,13 @@ const int kGpuTimeout = 10000;
 
 namespace content {
 namespace {
-void WarmUpSandbox(const GPUInfo&, bool);
+void WarmUpSandboxInitial();
+void WarmUpSandboxComplete(const GPUInfo&, bool);

[sheu, 2013/04/16 21:47:39]

WarmUpSandboxComplete() is only used (and only defined) on the OS_LINUX path,
but it's declared here unconditionally.  Move the declaration inside the
OS_LINUX block?

+#if defined(OS_LINUX)
+bool StartSandboxLinux(GpuWatchdogThread*);
+#elif defined(OS_WIN)
+bool StartSandboxWindows();
+#endif
 }
 
 // Main function for starting the Gpu process.
@@ -170,12 +176,23 @@ int GpuMain(const MainFunctionParams& parameters) {
       command_line.GetSwitchValueASCII(switches::kGpuDriverVersion);
   GetContentClient()->SetGpuInfo(gpu_info);
 
-  // We need to track that information for the WarmUpSandbox function.
+  // Warm up resources that don't need access to GPUInfo.
+  WarmUpSandboxInitial();
+
+  bool initialized_sandbox = false;
+#if defined(OS_CHROMEOS) && defined(ARCH_CPU_ARMEL)

[jln (very slow on Chromium), 2013/04/16 21:32:22]

Maybe owners can suggest something better to do here to detect that
configuration, I'll leave this up to them.

+  // On Chrome OS ARM, GPU driver userspace creates threads when initializing
+  // a GL context, so start the sandbox early.
+  gpu_info.sandboxed = StartSandboxLinux(watchdog_thread.get());
+  initialized_sandbox = true;
+#endif
+
+  // We need to track this information for the WarmUpSandboxComplete function.
   bool initialized_gl_context = false;
   // Load and initialize the GL implementation and locate the GL entry points.
   if (gfx::GLSurface::InitializeOneOff()) {
     // We need to collect GL strings (VENDOR, RENDERER) for blacklisting
-    // purpose. However, on Mac we don't actually use them. As documented in
+    // purposes. However, on Mac we don't actually use them. As documented in
     // crbug.com/222934, due to some driver issues, glGetString could take
     // multiple seconds to finish, which in turn cause the GPU process to crash.
     // By skipping the following code on Mac, we don't really lose anything,
@@ -222,47 +239,17 @@ int GpuMain(const MainFunctionParams& parameters) {
     watchdog_thread = NULL;
   }
 
-  {
-    const bool should_initialize_gl_context = !initialized_gl_context &&
-                                              !dead_on_arrival;
-    // Warm up the current process before enabling the sandbox.
-    WarmUpSandbox(gpu_info, should_initialize_gl_context);
-  }
-
 #if defined(OS_LINUX)
-  {
-    TRACE_EVENT0("gpu", "Initialize sandbox");
-    bool do_init_sandbox = true;
-
-#if defined(OS_CHROMEOS) && defined(NDEBUG)
-    // On Chrome OS and when not on a debug build, initialize
-    // the GPU process' sandbox only for Intel GPUs.
-    do_init_sandbox = gpu_info.gpu.vendor_id == 0x8086;   // Intel GPU.
-#endif
-
-    if (do_init_sandbox) {
-      if (watchdog_thread.get())
-        watchdog_thread->Stop();
-      gpu_info.sandboxed = LinuxSandbox::InitializeSandbox();
-      if (watchdog_thread.get())
-        watchdog_thread->Start();
-    }
-  }
-#endif
+  const bool should_initialize_gl_context = !initialized_gl_context &&
+                                            !dead_on_arrival;
 
-#if defined(OS_WIN)
-  {
-    TRACE_EVENT0("gpu", "Lower token");
-    // For windows, if the target_services interface is not zero, the process
-    // is sandboxed and we must call LowerToken() before rendering untrusted
-    // content.
-    sandbox::TargetServices* target_services =
-        parameters.sandbox_info->target_services;
-    if (target_services) {
-      target_services->LowerToken();
-      gpu_info.sandboxed = true;
-    }
+  if (!initialized_sandbox) {
+    // Finish warming up the current process.
+    WarmUpSandboxComplete(gpu_info, should_initialize_gl_context);
+    gpu_info.sandboxed = StartSandboxLinux(watchdog_thread);
   }
+#elif defined(OS_WIN)
+  gpu_info.sandboxed = StartSandboxWindows();
 #endif
 
   GpuProcess gpu_process;
@@ -315,8 +302,7 @@ void CreateDummyGlContext() {
 }
 #endif
 
-void WarmUpSandbox(const GPUInfo& gpu_info,
-                   bool should_initialize_gl_context) {
+void WarmUpSandboxInitial() {
   {
     TRACE_EVENT0("gpu", "Warm up rand");
     // Warm up the random subsystem, which needs to be done pre-sandbox on all
@@ -342,7 +328,24 @@ void WarmUpSandbox(const GPUInfo& gpu_info,
   VaapiVideoDecodeAccelerator::PreSandboxInitialization();
 #endif
 
+#if defined(OS_WIN)
+  {
+    TRACE_EVENT0("gpu", "Preload setupapi.dll");
+    // Preload this DLL because the sandbox prevents it from loading.
+    LoadLibrary(L"setupapi.dll");
+  }
+
+  {
+    TRACE_EVENT0("gpu", "Initialize DXVA");
+    // Initialize H/W video decoding stuff which fails in the sandbox.
+    DXVAVideoDecodeAccelerator::PreSandboxInitialization();
+  }
+#endif
+}
+
 #if defined(OS_LINUX)
+void WarmUpSandboxComplete(const GPUInfo& gpu_info,
+                           bool should_initialize_gl_context) {

[sheu, 2013/04/16 21:47:39]

WarmUpSandboxComplete() seems to be NVIDIA-specific logic, so calling
WarmUpSandboxInitial() and then not calling WarmUpSandboxComplete() on
non-NVIDIA hardware is not an error.  By the way the functions are named though,
the names seem to imply that calling the -Initial() without calling a
-Complete() would be an error.

Could you rename this function, or otherwise refactor it such that you don't
need to break it up?

[Jorge Lucangeli Obes, 2013/04/16 22:39:57]

Before making it Linux-only due to its code, the point was splitting the
original WarmUpSandbox into code that needed GPUInfo and code that didn't. Turns
out that the only code needing GPUInfo was NVIDIA-related, so this can be an
NVIDIA-specific sandbox warmup.

   // We special case Optimus since the vendor_id we see may not be Nvidia.
   bool uses_nvidia_driver = (gpu_info.gpu.vendor_id == 0x10de &&  // NVIDIA.
                              gpu_info.driver_vendor == "NVIDIA") ||
@@ -351,22 +354,42 @@ void WarmUpSandbox(const GPUInfo& gpu_info,
     // We need this on Nvidia to pre-open /dev/nvidiactl and /dev/nvidia0.
     CreateDummyGlContext();
   }
+}
+
+bool StartSandboxLinux(GpuWatchdogThread* watchdog_thread) {
+  TRACE_EVENT0("gpu", "Initialize sandbox");
+
+  bool res = false;
+
+  if (watchdog_thread)
+    watchdog_thread->Stop();
+  // LinuxSandbox::InitializeSandbox() must always be called
+  // with only one thread.
+  res = LinuxSandbox::InitializeSandbox();
+  if (watchdog_thread)
+    watchdog_thread->Start();
+
+  return res;
+}
 #endif

[jln (very slow on Chromium), 2013/04/16 21:32:22]

style: add  // defined(OS_LINUX)

[Jorge Lucangeli Obes, 2013/04/16 21:40:31]

Done.

 
 #if defined(OS_WIN)
-  {
-    TRACE_EVENT0("gpu", "Preload setupapi.dll");
-    // Preload this DLL because the sandbox prevents it from loading.
-    LoadLibrary(L"setupapi.dll");
+bool StartSandboxWindows() {
+  TRACE_EVENT0("gpu", "Lower token");
+
+  // For Windows, if the target_services interface is not zero, the process
+  // is sandboxed and we must call LowerToken() before rendering untrusted
+  // content.
+  sandbox::TargetServices* target_services =
+      parameters.sandbox_info->target_services;

[jln (very slow on Chromium), 2013/04/16 21:33:52]

Looks like you'll need to take this as a parameter.

[Jorge Lucangeli Obes, 2013/04/16 21:40:31]

Done.

+  if (target_services) {
+    target_services->LowerToken();
+    return true:
   }
 
-  {
-    TRACE_EVENT0("gpu", "Initialize DXVA");
-    // Initialize H/W video decoding stuff which fails in the sandbox.
-    DXVAVideoDecodeAccelerator::PreSandboxInitialization();
-  }
-#endif
+  return false;
 }
+#endif

[jln (very slow on Chromium), 2013/04/16 21:32:22]

style: add // defined(OS_WIN)

[Jorge Lucangeli Obes, 2013/04/16 21:40:31]

Done.

 
 }  // namespace.