Refactor GPU init to allow ARM sandboxing.

content/gpu/gpu_main.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <stdlib.h>
 
 #if defined(OS_WIN)
 #include <windows.h>
 #endif
 
@@ skipping 36 matching lines @@
 #endif
 
 #if defined(OS_LINUX)
 #include "content/public/common/sandbox_init.h"
 #endif
 
 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.
 int GpuMain(const MainFunctionParams& parameters) {
   TRACE_EVENT0("gpu", "GpuMain");
 
   base::Time start_time = base::Time::Now();
 
   const CommandLine& command_line = parameters.command_line;
   if (command_line.HasSwitch(switches::kGpuStartupDialog)) {
@@ skipping 95 matching lines @@
   success = base::HexStringToInt(
       command_line.GetSwitchValueASCII(switches::kGpuDeviceID),
       reinterpret_cast<int*>(&(gpu_info.gpu.device_id)));
   DCHECK(success);
   gpu_info.driver_vendor =
       command_line.GetSwitchValueASCII(switches::kGpuDriverVendor);
   gpu_info.driver_version =
       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,
     // because the basic GPU information is passed down from browser process
     // and we already registered them through SetGpuInfo() above.
 #if !defined(OS_MACOSX)
     if (!gpu_info_collector::CollectContextGraphicsInfo(&gpu_info))
       VLOG(1) << "gpu_info_collector::CollectGraphicsInfo failed";
     GetContentClient()->SetGpuInfo(gpu_info);
 
@@ skipping 26 matching lines @@
 
   // OSMesa is expected to run very slowly, so disable the watchdog in that
   // case.
   if (enable_watchdog &&
       gfx::GetGLImplementation() == gfx::kGLImplementationOSMesaGL) {
     watchdog_thread->Stop();
 
     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;
 
   GpuChildThread* child_thread = new GpuChildThread(watchdog_thread.get(),
                                                     dead_on_arrival, gpu_info);
 
   child_thread->Init(start_time);
 
   gpu_process.set_main_thread(child_thread);
@@ skipping 32 matching lines @@
 
   // Similarly, this is needed for /dev/nvidia0.
   if (context->MakeCurrent(surface)) {
     context->ReleaseCurrent(surface.get());
   } else {
     VLOG(1)  << "gfx::GLContext::MakeCurrent failed";
   }
 }
 #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
     // platforms.
     (void) base::RandUint64();
   }
   {
     TRACE_EVENT0("gpu", "Warm up HMAC");
     // Warm up the crypto subsystem, which needs to done pre-sandbox on all
     // platforms.
     crypto::HMAC hmac(crypto::HMAC::SHA256);
     unsigned char key = '\0';
     bool ret = hmac.Init(&key, sizeof(key));
     (void) ret;
   }
 
 #if defined(OS_CHROMEOS) && defined(ARCH_CPU_ARMEL) && defined(USE_X11)
   if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kUseExynosVda))
     ExynosVideoDecodeAccelerator::PreSandboxInitialization();
   else
     OmxVideoDecodeAccelerator::PreSandboxInitialization();
 #elif defined(OS_CHROMEOS) && defined(ARCH_CPU_X86_FAMILY) && defined(USE_X11)
   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");
     // 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
 }
 
+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