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..9c3d924f9bc8c904ec268fcd7a86476ce51dd67a 100644
--- a/content/gpu/gpu_main.cc
+++ b/content/gpu/gpu_main.cc
@@ -54,7 +54,10 @@ const int kGpuTimeout = 10000;
 
 namespace content {
 namespace {
-void WarmUpSandbox(const GPUInfo&, bool);
+void WarmUpSandboxInitial();
+void WarmUpSandboxComplete(const GPUInfo&, bool);
+void DoInitializeSandbox(scoped_refptr<GpuWatchdogThread>, GPUInfo&);
+void DoLowerToken(GPUInfo&);
 }
 
 // Main function for starting the Gpu process.
@@ -170,12 +173,20 @@ int GpuMain(const MainFunctionParams& parameters) {
       command_line.GetSwitchValueASCII(switches::kGpuDriverVersion);
   GetContentClient()->SetGpuInfo(gpu_info);
 
-  // We need to track that information for the WarmUpSandbox function.
+  WarmUpSandboxInitial();
+
+  bool initialized_sandbox = false;
+#if defined(OS_CHROMEOS) && defined(ARCH_CPU_ARMEL)

[jln (very slow on Chromium), 2013/04/16 19:45:54]

Are these ifdef really the best way ? Can we have any information about the
driver at this stage instead?

Also lacks a comments: "In blahblah configuration, we can initialize the sandbox
early".

[Jorge Lucangeli Obes, 2013/04/16 21:26:29]

The only way I see to have driver information at this point is passing it on the
cmdline. Any attempt to populate GPUInfo seems to create a GL context and
therefore threads.

However, maybe what we need on ARM is a better way to get some of the fields in
GPUInfo without creating a GL context.

+  DoInitializeSandbox(watchdog_thread, gpu_info);
+  initialized_sandbox = true;
+#endif
+
+  // We need to track this information for the WarmUpSandbox 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 +233,18 @@ 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);
-  }
+  const bool should_initialize_gl_context = !initialized_gl_context &&
+                                            !dead_on_arrival;
+  // Finish warming up the current process.
+  WarmUpSandboxComplete(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();
-    }
-  }
+  if (!initialized_sandbox)

[jln (very slow on Chromium), 2013/04/16 19:45:54]

Wouldn't it make sense to also move "WarmUpSandboxComplete()" inside this if ?
It's semantically invariant, but I think it'd make things more clear.

If you do that also move the #ifdef outside of WarmUpSandboxComplete() at the
definition site.

[Jorge Lucangeli Obes, 2013/04/16 21:26:29]

Done.

+    DoInitializeSandbox(watchdog_thread, gpu_info);
 #endif
 
 #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;
-    }
-  }
+  DoLowerToken(gpu_info);

[jln (very slow on Chromium), 2013/04/16 19:45:54]

Maybe rename to StartWindowsSandbox() ? It's quite unfortunate that
"LowerToken()" was kept as a name, not a bad idea to use the opportunity to
isolate gpu_main.cc from that detail.

[Jorge Lucangeli Obes, 2013/04/16 21:26:29]

Done.

 #endif
 
   GpuProcess gpu_process;
@@ -315,8 +297,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,17 +323,6 @@ void WarmUpSandbox(const GPUInfo& gpu_info,
   VaapiVideoDecodeAccelerator::PreSandboxInitialization();
 #endif
 
-#if defined(OS_LINUX)
-  // 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") ||
-                            gpu_info.optimus;
-  if (uses_nvidia_driver && should_initialize_gl_context) {
-    // We need this on Nvidia to pre-open /dev/nvidiactl and /dev/nvidia0.
-    CreateDummyGlContext();
-  }
-#endif
-
 #if defined(OS_WIN)
   {
     TRACE_EVENT0("gpu", "Preload setupapi.dll");
@@ -368,6 +338,48 @@ void WarmUpSandbox(const GPUInfo& gpu_info,
 #endif
 }
 
+void WarmUpSandboxComplete(const GPUInfo& gpu_info,

[jln (very slow on Chromium), 2013/04/16 19:45:54]

Nit: for some reason I have the impression that WarmUpSandboxFinish() would be a
better name. Up to you.

+                           bool should_initialize_gl_context) {
+#if defined(OS_LINUX)
+  // 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") ||
+                            gpu_info.optimus;
+  if (uses_nvidia_driver && should_initialize_gl_context) {
+    // We need this on Nvidia to pre-open /dev/nvidiactl and /dev/nvidia0.
+    CreateDummyGlContext();
+  }
+#endif
+}
+
+#if defined(OS_LINUX)
+void DoInitializeSandbox(scoped_refptr<GpuWatchdogThread> watchdog_thread,

[jln (very slow on Chromium), 2013/04/16 19:45:54]

I think a raw pointer to the watchdog_thread is better (use .get()).
Your caller clearly owns a pointer to it, this creates a new scoped_refptr for
no benefit.

[Jorge Lucangeli Obes, 2013/04/16 21:26:29]

Done.

+                         GPUInfo& gpu_info) {

[jln (very slow on Chromium), 2013/04/16 19:45:54]

Non const references are evil (and forbidden). Just return true or false and set
 gpu_info.sandboxed in the caller!

[Jorge Lucangeli Obes, 2013/04/16 21:26:29]

Done.

+  TRACE_EVENT0("gpu", "Initialize sandbox");
+
+  if (watchdog_thread.get())
+    watchdog_thread->Stop();

[jln (very slow on Chromium), 2013/04/16 19:45:54]

Do you want to take this opportunity to add a comment here that
InitializeSandbox() should always be called mono threaded?

[Jorge Lucangeli Obes, 2013/04/16 21:26:29]

Done.

+  gpu_info.sandboxed = LinuxSandbox::InitializeSandbox();
+  if (watchdog_thread.get())
+    watchdog_thread->Start();
+}
+#endif
+
+#if defined(OS_WIN)
+bool DoLowerToken(GPUInfo& gpu_info) {

[jln (very slow on Chromium), 2013/04/16 19:45:54]

Same remark regarding non const references.

[Jorge Lucangeli Obes, 2013/04/16 21:26:29]

Done.

+  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;
+  }
+}
+#endif
+
 }  // namespace.
 
 }  // namespace content