Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14271005:
Refactor GPU init to allow ARM sandboxing. (Closed)
|
Created:
7 years, 8 months ago by Jorge Lucangeli Obes Modified:
7 years, 8 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, apatrick_chromium Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionRefactor GPU init to allow ARM sandboxing.
BUG=232068
TEST=daisy loads and GPU acceleration works together with a policy change.
TEST=desktop Chrome works with NVIDIA graphics.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194793
Patch Set 1 #
Total comments: 15
Patch Set 2 : Address jln's comments. #
Total comments: 10
Patch Set 3 : Fix Windows compile, address comments/nits. #
Total comments: 8
Patch Set 4 : Address sheu's and piman's comments. #Patch Set 5 : Fix compile. #Patch Set 6 : Fix typo. #
Total comments: 2
Patch Set 7 : Remove "defined(NDEBUG)". #Patch Set 8 : Rebase. #Messages
Total messages: 23 (0 generated)
RFC Hey guys, this is what I need on the GPU side to be able to enable the seccomp-bpf sandbox for the GPU process on ARM.
https://chromiumcodereview.appspot.com/14271005/diff/1/content/gpu/gpu_main.cc File content/gpu/gpu_main.cc (right): https://chromiumcodereview.appspot.com/14271005/diff/1/content/gpu/gpu_main.c... content/gpu/gpu_main.cc:179: #if defined(OS_CHROMEOS) && defined(ARCH_CPU_ARMEL) 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". https://chromiumcodereview.appspot.com/14271005/diff/1/content/gpu/gpu_main.c... content/gpu/gpu_main.cc:242: if (!initialized_sandbox) 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. https://chromiumcodereview.appspot.com/14271005/diff/1/content/gpu/gpu_main.c... content/gpu/gpu_main.cc:247: DoLowerToken(gpu_info); 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. https://chromiumcodereview.appspot.com/14271005/diff/1/content/gpu/gpu_main.c... content/gpu/gpu_main.cc:341: void WarmUpSandboxComplete(const GPUInfo& gpu_info, Nit: for some reason I have the impression that WarmUpSandboxFinish() would be a better name. Up to you. https://chromiumcodereview.appspot.com/14271005/diff/1/content/gpu/gpu_main.c... content/gpu/gpu_main.cc:356: void DoInitializeSandbox(scoped_refptr<GpuWatchdogThread> watchdog_thread, 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. https://chromiumcodereview.appspot.com/14271005/diff/1/content/gpu/gpu_main.c... content/gpu/gpu_main.cc:357: GPUInfo& gpu_info) { Non const references are evil (and forbidden). Just return true or false and set gpu_info.sandboxed in the caller! https://chromiumcodereview.appspot.com/14271005/diff/1/content/gpu/gpu_main.c... content/gpu/gpu_main.cc:361: watchdog_thread->Stop(); Do you want to take this opportunity to add a comment here that InitializeSandbox() should always be called mono threaded? https://chromiumcodereview.appspot.com/14271005/diff/1/content/gpu/gpu_main.c... content/gpu/gpu_main.cc:369: bool DoLowerToken(GPUInfo& gpu_info) { Same remark regarding non const references.
On 2013/04/16 19:45:54, Julien Tinnes wrote: > https://chromiumcodereview.appspot.com/14271005/diff/1/content/gpu/gpu_main.cc > File content/gpu/gpu_main.cc (right): > > https://chromiumcodereview.appspot.com/14271005/diff/1/content/gpu/gpu_main.c... > content/gpu/gpu_main.cc:179: #if defined(OS_CHROMEOS) && defined(ARCH_CPU_ARMEL) > 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". > > https://chromiumcodereview.appspot.com/14271005/diff/1/content/gpu/gpu_main.c... > content/gpu/gpu_main.cc:242: if (!initialized_sandbox) > 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. > > https://chromiumcodereview.appspot.com/14271005/diff/1/content/gpu/gpu_main.c... > content/gpu/gpu_main.cc:247: DoLowerToken(gpu_info); > 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. > > https://chromiumcodereview.appspot.com/14271005/diff/1/content/gpu/gpu_main.c... > content/gpu/gpu_main.cc:341: void WarmUpSandboxComplete(const GPUInfo& gpu_info, > Nit: for some reason I have the impression that WarmUpSandboxFinish() would be a > better name. Up to you. > > https://chromiumcodereview.appspot.com/14271005/diff/1/content/gpu/gpu_main.c... > content/gpu/gpu_main.cc:356: void > DoInitializeSandbox(scoped_refptr<GpuWatchdogThread> watchdog_thread, > 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. > > https://chromiumcodereview.appspot.com/14271005/diff/1/content/gpu/gpu_main.c... > content/gpu/gpu_main.cc:357: GPUInfo& gpu_info) { > Non const references are evil (and forbidden). Just return true or false and set > gpu_info.sandboxed in the caller! > > https://chromiumcodereview.appspot.com/14271005/diff/1/content/gpu/gpu_main.c... > content/gpu/gpu_main.cc:361: watchdog_thread->Stop(); > Do you want to take this opportunity to add a comment here that > InitializeSandbox() should always be called mono threaded? > > https://chromiumcodereview.appspot.com/14271005/diff/1/content/gpu/gpu_main.c... > content/gpu/gpu_main.cc:369: bool DoLowerToken(GPUInfo& gpu_info) { > Same remark regarding non const references. PTAL
https://chromiumcodereview.appspot.com/14271005/diff/1/content/gpu/gpu_main.cc File content/gpu/gpu_main.cc (right): https://chromiumcodereview.appspot.com/14271005/diff/1/content/gpu/gpu_main.c... content/gpu/gpu_main.cc:179: #if defined(OS_CHROMEOS) && defined(ARCH_CPU_ARMEL) On 2013/04/16 19:45:54, Julien Tinnes wrote: > 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". 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. https://chromiumcodereview.appspot.com/14271005/diff/1/content/gpu/gpu_main.c... content/gpu/gpu_main.cc:242: if (!initialized_sandbox) On 2013/04/16 19:45:54, Julien Tinnes wrote: > 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. Done. https://chromiumcodereview.appspot.com/14271005/diff/1/content/gpu/gpu_main.c... content/gpu/gpu_main.cc:247: DoLowerToken(gpu_info); On 2013/04/16 19:45:54, Julien Tinnes wrote: > 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. Done. https://chromiumcodereview.appspot.com/14271005/diff/1/content/gpu/gpu_main.c... content/gpu/gpu_main.cc:356: void DoInitializeSandbox(scoped_refptr<GpuWatchdogThread> watchdog_thread, On 2013/04/16 19:45:54, Julien Tinnes wrote: > 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. Done. https://chromiumcodereview.appspot.com/14271005/diff/1/content/gpu/gpu_main.c... content/gpu/gpu_main.cc:357: GPUInfo& gpu_info) { On 2013/04/16 19:45:54, Julien Tinnes wrote: > Non const references are evil (and forbidden). Just return true or false and set > gpu_info.sandboxed in the caller! Done. https://chromiumcodereview.appspot.com/14271005/diff/1/content/gpu/gpu_main.c... content/gpu/gpu_main.cc:361: watchdog_thread->Stop(); On 2013/04/16 19:45:54, Julien Tinnes wrote: > Do you want to take this opportunity to add a comment here that > InitializeSandbox() should always be called mono threaded? Done. https://chromiumcodereview.appspot.com/14271005/diff/1/content/gpu/gpu_main.c... content/gpu/gpu_main.cc:369: bool DoLowerToken(GPUInfo& gpu_info) { On 2013/04/16 19:45:54, Julien Tinnes wrote: > Same remark regarding non const references. Done.
lgtm piman@ is probably a good choice of owner for this as he has reviewed code in that area before. https://chromiumcodereview.appspot.com/14271005/diff/4001/content/gpu/gpu_mai... File content/gpu/gpu_main.cc (right): https://chromiumcodereview.appspot.com/14271005/diff/4001/content/gpu/gpu_mai... content/gpu/gpu_main.cc:183: #if defined(OS_CHROMEOS) && defined(ARCH_CPU_ARMEL) Maybe owners can suggest something better to do here to detect that configuration, I'll leave this up to them. https://chromiumcodereview.appspot.com/14271005/diff/4001/content/gpu/gpu_mai... content/gpu/gpu_main.cc:374: #endif style: add // defined(OS_LINUX) https://chromiumcodereview.appspot.com/14271005/diff/4001/content/gpu/gpu_mai... content/gpu/gpu_main.cc:392: #endif style: add // defined(OS_WIN)
https://chromiumcodereview.appspot.com/14271005/diff/4001/content/gpu/gpu_mai... File content/gpu/gpu_main.cc (right): https://chromiumcodereview.appspot.com/14271005/diff/4001/content/gpu/gpu_mai... content/gpu/gpu_main.cc:384: parameters.sandbox_info->target_services; Looks like you'll need to take this as a parameter.
+piman@ for OWNERS. https://chromiumcodereview.appspot.com/14271005/diff/4001/content/gpu/gpu_mai... File content/gpu/gpu_main.cc (right): https://chromiumcodereview.appspot.com/14271005/diff/4001/content/gpu/gpu_mai... content/gpu/gpu_main.cc:374: #endif On 2013/04/16 21:32:22, Julien Tinnes wrote: > style: add // defined(OS_LINUX) Done. https://chromiumcodereview.appspot.com/14271005/diff/4001/content/gpu/gpu_mai... content/gpu/gpu_main.cc:384: parameters.sandbox_info->target_services; On 2013/04/16 21:33:52, Julien Tinnes wrote: > Looks like you'll need to take this as a parameter. Done. https://chromiumcodereview.appspot.com/14271005/diff/4001/content/gpu/gpu_mai... content/gpu/gpu_main.cc:392: #endif On 2013/04/16 21:32:22, Julien Tinnes wrote: > style: add // defined(OS_WIN) Done.
https://chromiumcodereview.appspot.com/14271005/diff/3002/content/gpu/gpu_mai... File content/gpu/gpu_main.cc (right): https://chromiumcodereview.appspot.com/14271005/diff/3002/content/gpu/gpu_mai... content/gpu/gpu_main.cc:377: bool StartSandboxWindows(const MainFunctionParams& parameters) { Might be better to directly pass parameters.sandbox_info as a const reference?
https://chromiumcodereview.appspot.com/14271005/diff/4001/content/gpu/gpu_mai... File content/gpu/gpu_main.cc (right): https://chromiumcodereview.appspot.com/14271005/diff/4001/content/gpu/gpu_mai... content/gpu/gpu_main.cc:58: void WarmUpSandboxComplete(const GPUInfo&, bool); 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? https://chromiumcodereview.appspot.com/14271005/diff/4001/content/gpu/gpu_mai... content/gpu/gpu_main.cc:348: bool should_initialize_gl_context) { 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?
https://chromiumcodereview.appspot.com/14271005/diff/4001/content/gpu/gpu_mai... File content/gpu/gpu_main.cc (right): https://chromiumcodereview.appspot.com/14271005/diff/4001/content/gpu/gpu_mai... content/gpu/gpu_main.cc:348: bool should_initialize_gl_context) { On 2013/04/16 21:47:39, sheu wrote: > 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? 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.
https://chromiumcodereview.appspot.com/14271005/diff/3002/content/gpu/gpu_mai... File content/gpu/gpu_main.cc (right): https://chromiumcodereview.appspot.com/14271005/diff/3002/content/gpu/gpu_mai... content/gpu/gpu_main.cc:58: void WarmUpSandboxComplete(const GPUInfo&, bool); This should be inside the #ifdef OS_LINUX, because it's only implemented and called when OS_LINUX is set. Alternatively, leave the WarmUpSandboxComplete implmentation out of the #ifdef and call it always, but either way, let's be consistent :) https://chromiumcodereview.appspot.com/14271005/diff/3002/content/gpu/gpu_mai... content/gpu/gpu_main.cc:186: gpu_info.sandboxed = StartSandboxLinux(watchdog_thread.get()); I'm confused - gfx::GLSurface::InitializeOneOff is what loads (dlopen) the gl library. How does it work when the sandbox is on? https://chromiumcodereview.appspot.com/14271005/diff/3002/content/gpu/gpu_mai... content/gpu/gpu_main.cc:248: WarmUpSandboxComplete(gpu_info, should_initialize_gl_context); This step is rather confusing, because we don't do it in all cases. There's a combination of compile-time and run-time checks, plus built-in assumption about what it does, which isn't reflected in the name. Would it make more sense to always call WarmUpSandboxComplete before starting the sandbox, possibly directly from StartSandboxLinux?
https://chromiumcodereview.appspot.com/14271005/diff/3002/content/gpu/gpu_mai... File content/gpu/gpu_main.cc (right): https://chromiumcodereview.appspot.com/14271005/diff/3002/content/gpu/gpu_mai... content/gpu/gpu_main.cc:186: gpu_info.sandboxed = StartSandboxLinux(watchdog_thread.get()); On 2013/04/16 23:06:45, piman wrote: > I'm confused - gfx::GLSurface::InitializeOneOff is what loads (dlopen) the gl > library. How does it work when the sandbox is on? This CL does not enable the sandbox by itself, it only gives us a hook before the GL library is dlopen'd. The seccomp-bpf sandbox has functionality (for processes that don't go through the zygote) to allow open() for a list of allowed files. I have a follow-up CL that enables the sandbox and uses this feature to whitelist the GL libraries for the ARM GPU userspace.
On 2013/04/16 23:11:38, Jorge Lucangeli Obes wrote: > https://chromiumcodereview.appspot.com/14271005/diff/3002/content/gpu/gpu_mai... > File content/gpu/gpu_main.cc (right): > > https://chromiumcodereview.appspot.com/14271005/diff/3002/content/gpu/gpu_mai... > content/gpu/gpu_main.cc:186: gpu_info.sandboxed = > StartSandboxLinux(watchdog_thread.get()); > On 2013/04/16 23:06:45, piman wrote: > > I'm confused - gfx::GLSurface::InitializeOneOff is what loads (dlopen) the gl > > library. How does it work when the sandbox is on? > > This CL does not enable the sandbox by itself, it only gives us a hook before > the GL library is dlopen'd. > > The seccomp-bpf sandbox has functionality (for processes that don't go through > the zygote) to allow open() for a list of allowed files. I have a follow-up CL > that enables the sandbox and uses this feature to whitelist the GL libraries for > the ARM GPU userspace. Is there a reason why we can't do that on x86 too then?
On Tue, Apr 16, 2013 at 4:12 PM, <piman@chromium.org> wrote: > On 2013/04/16 23:11:38, Jorge Lucangeli Obes wrote: > > https://chromiumcodereview.**appspot.com/14271005/diff/** > 3002/content/gpu/gpu_main.cc<https://chromiumcodereview.appspot.com/14271005/diff/3002/content/gpu/gpu_main.cc> > >> File content/gpu/gpu_main.cc (right): >> > > > https://chromiumcodereview.**appspot.com/14271005/diff/** > 3002/content/gpu/gpu_main.cc#**newcode186<https://chromiumcodereview.appspot.com/14271005/diff/3002/content/gpu/gpu_main.cc#newcode186> > >> content/gpu/gpu_main.cc:186: gpu_info.sandboxed = >> StartSandboxLinux(watchdog_**thread.get()); >> On 2013/04/16 23:06:45, piman wrote: >> > I'm confused - gfx::GLSurface::**InitializeOneOff is what loads >> (dlopen) the >> > gl > >> > library. How does it work when the sandbox is on? >> > > This CL does not enable the sandbox by itself, it only gives us a hook >> before >> the GL library is dlopen'd. >> > > The seccomp-bpf sandbox has functionality (for processes that don't go >> through >> the zygote) to allow open() for a list of allowed files. I have a >> follow-up CL >> that enables the sandbox and uses this feature to whitelist the GL >> libraries >> > for > >> the ARM GPU userspace. >> > > Is there a reason why we can't do that on x86 too then? > Basically, because we don't need to. On x86 we can create a GL context and populate GPUInfo without creating threads, so there's no need to enable the sandbox before populating GPUInfo. That way, we just let the driver do the warm-up itself. On ARM, we have to do it ourselves. > > https://chromiumcodereview.**appspot.com/14271005/<https://chromiumcodereview... >
On Tue, Apr 16, 2013 at 4:19 PM, Jorge Lucangeli Obes <jorgelo@chromium.org>wrote: > On Tue, Apr 16, 2013 at 4:12 PM, <piman@chromium.org> wrote: > >> On 2013/04/16 23:11:38, Jorge Lucangeli Obes wrote: >> >> https://chromiumcodereview.**appspot.com/14271005/diff/** >> 3002/content/gpu/gpu_main.cc<https://chromiumcodereview.appspot.com/14271005/diff/3002/content/gpu/gpu_main.cc> >> >>> File content/gpu/gpu_main.cc (right): >>> >> >> >> https://chromiumcodereview.**appspot.com/14271005/diff/** >> 3002/content/gpu/gpu_main.cc#**newcode186<https://chromiumcodereview.appspot.com/14271005/diff/3002/content/gpu/gpu_main.cc#newcode186> >> >>> content/gpu/gpu_main.cc:186: gpu_info.sandboxed = >>> StartSandboxLinux(watchdog_**thread.get()); >>> On 2013/04/16 23:06:45, piman wrote: >>> > I'm confused - gfx::GLSurface::**InitializeOneOff is what loads >>> (dlopen) the >>> >> gl >> >>> > library. How does it work when the sandbox is on? >>> >> >> This CL does not enable the sandbox by itself, it only gives us a hook >>> before >>> the GL library is dlopen'd. >>> >> >> The seccomp-bpf sandbox has functionality (for processes that don't go >>> through >>> the zygote) to allow open() for a list of allowed files. I have a >>> follow-up CL >>> that enables the sandbox and uses this feature to whitelist the GL >>> libraries >>> >> for >> >>> the ARM GPU userspace. >>> >> >> Is there a reason why we can't do that on x86 too then? >> > > Basically, because we don't need to. On x86 we can create a GL context and > populate GPUInfo without creating threads, so there's no need to enable the > sandbox before populating GPUInfo. That way, we just let the driver do the > warm-up itself. On ARM, we have to do it ourselves. > Doesn't the driver on Alex create threads? Anyway, I understand, though fast-forwarding into the future, I foresee some more logic here to bring up the next ARM driver whatever that is, it would be nice to have comments about why we do all this. Right now, everything seems completely arbitrary from the outsider's pov (me). It would be nice to have a flag that says "driver bringup creates threads" and make decisions based on that, so that we can separate out the hard-coded logic to decide whether that is true or not. > >> >> https://chromiumcodereview.**appspot.com/14271005/<https://chromiumcodereview... >> > >
PTAL. I'm happier with the code now, it's definitely clearer. Antoine: Alex does *not* show the threading issue, checked this morning. If you still think it's worth to add the flag I'll do it. Alternatively, I'll land this to do the major refactoring and then add the flag to change the early sandboxing logic. https://chromiumcodereview.appspot.com/14271005/diff/3002/content/gpu/gpu_mai... File content/gpu/gpu_main.cc (right): https://chromiumcodereview.appspot.com/14271005/diff/3002/content/gpu/gpu_mai... content/gpu/gpu_main.cc:58: void WarmUpSandboxComplete(const GPUInfo&, bool); On 2013/04/16 23:06:45, piman wrote: > This should be inside the #ifdef OS_LINUX, because it's only implemented and > called when OS_LINUX is set. > > Alternatively, leave the WarmUpSandboxComplete implmentation out of the #ifdef > and call it always, but either way, let's be consistent :) Done. https://chromiumcodereview.appspot.com/14271005/diff/3002/content/gpu/gpu_mai... content/gpu/gpu_main.cc:248: WarmUpSandboxComplete(gpu_info, should_initialize_gl_context); On 2013/04/16 23:06:45, piman wrote: > This step is rather confusing, because we don't do it in all cases. There's a > combination of compile-time and run-time checks, plus built-in assumption about > what it does, which isn't reflected in the name. > > Would it make more sense to always call WarmUpSandboxComplete before starting > the sandbox, possibly directly from StartSandboxLinux? Done. https://chromiumcodereview.appspot.com/14271005/diff/3002/content/gpu/gpu_mai... content/gpu/gpu_main.cc:377: bool StartSandboxWindows(const MainFunctionParams& parameters) { On 2013/04/16 21:42:24, Julien Tinnes wrote: > Might be better to directly pass parameters.sandbox_info as a const reference? Done.
https://chromiumcodereview.appspot.com/14271005/diff/25001/content/gpu/gpu_ma... File content/gpu/gpu_main.cc (right): https://chromiumcodereview.appspot.com/14271005/diff/25001/content/gpu/gpu_ma... content/gpu/gpu_main.cc:185: #if defined(OS_CHROMEOS) && defined(ARCH_CPU_ARMEL) && defined(NDEBUG) I don't think we should make this depend on NDEBUG - if there's an interaction between the initialization and the sandbox, it will be impossible to debug, if turning on debugging removes that interaction.
https://chromiumcodereview.appspot.com/14271005/diff/25001/content/gpu/gpu_ma... File content/gpu/gpu_main.cc (right): https://chromiumcodereview.appspot.com/14271005/diff/25001/content/gpu/gpu_ma... content/gpu/gpu_main.cc:185: #if defined(OS_CHROMEOS) && defined(ARCH_CPU_ARMEL) && defined(NDEBUG) On 2013/04/17 17:45:41, piman wrote: > I don't think we should make this depend on NDEBUG - if there's an interaction > between the initialization and the sandbox, it will be impossible to debug, if > turning on debugging removes that interaction. Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jorgelo@chromium.org/14271005/29001
Failed to apply patch for content/gpu/gpu_main.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file content/gpu/gpu_main.cc
Hunk #5 FAILED at 241.
1 out of 8 hunks FAILED -- saving rejects to file content/gpu/gpu_main.cc.rej
Patch: 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..efab5cf6b398b4fdf8c393b6e5ddee1fd8dd7095
100644
--- a/content/gpu/gpu_main.cc
+++ b/content/gpu/gpu_main.cc
@@ -54,7 +54,12 @@ const int kGpuTimeout = 10000;
namespace content {
namespace {
-void WarmUpSandbox(const GPUInfo&, bool);
+void WarmUpSandbox();
+#if defined(OS_LINUX)
+bool StartSandboxLinux(const GPUInfo&, GpuWatchdogThread*, bool);
+#elif defined(OS_WIN)
+bool StartSandboxWindows(const sandbox::SandboxInterfaceInfo*);
+#endif
}
// Main function for starting the Gpu process.
@@ -170,12 +175,26 @@ 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.
+ WarmUpSandbox();
+
+#if defined(OS_LINUX)
+ bool initialized_sandbox = false;
bool initialized_gl_context = false;
+ bool should_initialize_gl_context = false;
+#if defined(OS_CHROMEOS) && defined(ARCH_CPU_ARMEL)
+ // On Chrome OS ARM, GPU driver userspace creates threads when initializing
+ // a GL context, so start the sandbox early.
+ gpu_info.sandboxed = StartSandboxLinux(gpu_info, watchdog_thread.get(),
+ should_initialize_gl_context);
+ initialized_sandbox = true;
+#endif
+#endif // defined(OS_LINUX)
+
// 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,
@@ -186,10 +205,9 @@ int GpuMain(const MainFunctionParams& parameters) {
VLOG(1) << "gpu_info_collector::CollectGraphicsInfo failed";
GetContentClient()->SetGpuInfo(gpu_info);
- // We know that CollectGraphicsInfo will initialize a GLContext.
+#if defined(OS_LINUX)
initialized_gl_context = true;
-
-#if defined(OS_LINUX) && !defined(OS_CHROMEOS)
+#if !defined(OS_CHROMEOS)
if (gpu_info.gpu.vendor_id == 0x10de && // NVIDIA
gpu_info.driver_vendor == "NVIDIA") {
base::ThreadRestrictions::AssertIOAllowed();
@@ -199,8 +217,9 @@ int GpuMain(const MainFunctionParams& parameters) {
dead_on_arrival = true;
}
}
-#endif // OS_CHROMEOS
-#endif // OS_MACOSX
+#endif // !defined(OS_CHROMEOS)
+#endif // defined(OS_LINUX)
+#endif // !defined(OS_MACOSX)
} else {
VLOG(1) << "gfx::GLSurface::InitializeOneOff failed";
gpu_info.gpu_accessible = false;
@@ -222,47 +241,15 @@ 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
-
-#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;
- }
- }
+ should_initialize_gl_context = !initialized_gl_context &&
+ !dead_on_arrival;
+
+ if (!initialized_sandbox)
+ gpu_info.sandboxed = StartSandboxLinux(gpu_info, watchdog_thread.get(),
+ should_initialize_gl_context);
+#elif defined(OS_WIN)
+ gpu_info.sandboxed = StartSandboxWindows(parameters.sandbox_info);
#endif
GpuProcess gpu_process;
@@ -315,8 +302,7 @@ void CreateDummyGlContext() {
}
#endif
-void WarmUpSandbox(const GPUInfo& gpu_info,
- bool should_initialize_gl_context) {
+void WarmUpSandbox() {
{
TRACE_EVENT0("gpu", "Warm up rand");
// Warm up the random subsystem, which needs to be done pre-sandbox on all
@@ -342,17 +328,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 +343,57 @@ void WarmUpSandbox(const GPUInfo& gpu_info,
#endif
}
+#if defined(OS_LINUX)
+void WarmUpSandboxNvidia(const GPUInfo& gpu_info,
+ bool should_initialize_gl_context) {
+ // 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();
+ }
+}
+
+bool StartSandboxLinux(const GPUInfo& gpu_info,
+ GpuWatchdogThread* watchdog_thread,
+ bool should_initialize_gl_context) {
+ TRACE_EVENT0("gpu", "Initialize sandbox");
+
+ bool res = false;
+
+ WarmUpSandboxNvidia(gpu_info, should_initialize_gl_context);
+
+ 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 // defined(OS_LINUX)
+
+#if defined(OS_WIN)
+bool StartSandboxWindows(const sandbox::SandboxInterfaceInfo* sandbox_info) {
+ 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 = sandbox_info->target_services;
+ if (target_services) {
+ target_services->LowerToken();
+ return true;
+ }
+
+ return false;
+}
+#endif // defined(OS_WIN)
+
} // namespace.
} // namespace content
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jorgelo@chromium.org/14271005/30002
Message was sent while issue was closed.
Change committed as 194793 |
