Disable the seccomp filter GPU process sandbox by default on Chrome OS.

Disable the seccomp filter GPU process sandbox by default on Chrome OS.

We don't want the GPU process sandbox on by default on Chrome OS.
Since we control Chrome's command line, we can make sure it's enabled
for users, but disabled for developers.

Add an '--enable-chromeos-gpu-sandbox' flag, and remove the #ifdef that
disabled the sandbox on Chrome OS. Until we manually add the flag to
Chrome OS, the sandbox will continue to be disabled.

Moreover, even with the flag on, we only enable the sandbox on 64 bits and
for Intel GPUs.

BUG=127664
TEST=unit tests, and WebGL test websites on Lumpy.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=141683

[Jorge Lucangeli Obes, 2012-06-07 20:16:28]

Still building to test on Chrome OS, but wanted to do an early sanity check.
Probably need to add OWNERS for the GPU bit.

[Chris Evans, 2012-06-07 23:11:34]

Some initial thoughts.

https://chromiumcodereview.appspot.com/10534049/diff/1/content/common/sandbox...
File content/common/sandbox_init_linux.cc (right):

https://chromiumcodereview.appspot.com/10534049/diff/1/content/common/sandbox...
content/common/sandbox_init_linux.cc:25: #include
"content/gpu/gpu_info_collector.h"
I'm not 100% sure, but isn't it considered a layering violation to include
content/gpu from content/common ?

Generally, I think I'd prefer the logic to go in gpu_main

https://chromiumcodereview.appspot.com/10534049/diff/1/content/public/common/...
File content/public/common/content_switches.cc (right):

https://chromiumcodereview.appspot.com/10534049/diff/1/content/public/common/...
content/public/common/content_switches.cc:187: const char
kEnableChromeOSGPUSandbox[]      = "enable-chromeos-gpu-sandbox";
I think this flag should be more generic, i.e. "enable-gpu-sandbox". In the
future, there might be more cases it is useful to cover (e.g. if we end up
disabling GPU sandbox for Nvidia Linux binary driver by default)

It'd also be nice if this forced on the GPU sandbox for non-Intel chipsets on
ChromeOS. We could perhaps predicate such behaviour as ifdef DEBUG to begin
with?

[Jorge Lucangeli Obes, 2012-06-07 23:21:15]

On 2012/06/07 23:11:34, Chris Evans wrote:
> Some initial thoughts.
> 
>
https://chromiumcodereview.appspot.com/10534049/diff/1/content/common/sandbox...
> File content/common/sandbox_init_linux.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10534049/diff/1/content/common/sandbox...
> content/common/sandbox_init_linux.cc:25: #include
> "content/gpu/gpu_info_collector.h"
> I'm not 100% sure, but isn't it considered a layering violation to include
> content/gpu from content/common ?
> 
> Generally, I think I'd prefer the logic to go in gpu_main

Yeah, the trybots failed because of that. Already fixed locally.
 
>
https://chromiumcodereview.appspot.com/10534049/diff/1/content/public/common/...
> File content/public/common/content_switches.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10534049/diff/1/content/public/common/...
> content/public/common/content_switches.cc:187: const char
> kEnableChromeOSGPUSandbox[]      = "enable-chromeos-gpu-sandbox";
> I think this flag should be more generic, i.e. "enable-gpu-sandbox". In the
> future, there might be more cases it is useful to cover (e.g. if we end up
> disabling GPU sandbox for Nvidia Linux binary driver by default)
> 
> It'd also be nice if this forced on the GPU sandbox for non-Intel chipsets on
> ChromeOS. We could perhaps predicate such behaviour as ifdef DEBUG to begin
> with?

So the current plan was to enable the GPU sandbox on Chrome OS *only* on Intel
GPUs (hardcoded, no flag to override), and only when the flag was on.

If we change the flag then Linux users won't have the sandbox on by default.
That might be good, but wasn't the objective.

Since the flag will be off by default, we can make it force every chipset when
it's on. I'm not very fond of that approach since the point of this CL is to
enable the GPU sandbox only for production systems.

However, if the flag ends up controlling the GPU sandbox on Linux and Chrome OS
(which I'm not sure we want), then it probably makes sense to have it enable
everything.

The other thing is that we clearly don't want to do this for Windows, so we need
a way to break the tie between --disable-gpu-sandbox and --enable-gpu-sandbox.

[Jorge Lucangeli Obes, 2012-06-08 00:15:24]

https://chromiumcodereview.appspot.com/10534049/diff/1/content/common/sandbox...
File content/common/sandbox_init_linux.cc (right):

https://chromiumcodereview.appspot.com/10534049/diff/1/content/common/sandbox...
content/common/sandbox_init_linux.cc:25: #include
"content/gpu/gpu_info_collector.h"
On 2012/06/07 23:11:34, Chris Evans wrote:
> I'm not 100% sure, but isn't it considered a layering violation to include
> content/gpu from content/common ?
> 
> Generally, I think I'd prefer the logic to go in gpu_main

GPU info logic moved to gpu_main.cc, but cmdline flag logic kept here. Was that
what you meant?

https://chromiumcodereview.appspot.com/10534049/diff/1/content/public/common/...
File content/public/common/content_switches.cc (right):

https://chromiumcodereview.appspot.com/10534049/diff/1/content/public/common/...
content/public/common/content_switches.cc:187: const char
kEnableChromeOSGPUSandbox[]      = "enable-chromeos-gpu-sandbox";
On 2012/06/07 23:11:34, Chris Evans wrote:
> I think this flag should be more generic, i.e. "enable-gpu-sandbox". In the
> future, there might be more cases it is useful to cover (e.g. if we end up
> disabling GPU sandbox for Nvidia Linux binary driver by default)
> 
> It'd also be nice if this forced on the GPU sandbox for non-Intel chipsets on
> ChromeOS. We could perhaps predicate such behaviour as ifdef DEBUG to begin
> with?

Changed flag name, but I'm wary about using the flag to enable on non-Intel
chipsets as well. We really have no way to preemptively test the devices that
might come along, and people might run a regular Chrome OS image (with the flag)
on them.

[Chris Evans, 2012-06-08 07:13:57]

https://chromiumcodereview.appspot.com/10534049/diff/8001/content/common/sand...
File content/common/sandbox_init_linux.cc (right):

https://chromiumcodereview.appspot.com/10534049/diff/8001/content/common/sand...
content/common/sandbox_init_linux.cc:396: return true;
return !command_line.HasSwitch(kDisable) -- see below.

https://chromiumcodereview.appspot.com/10534049/diff/8001/content/common/sand...
content/common/sandbox_init_linux.cc:413:
command_line.HasSwitch(switches::kDisableGpuSandbox))
Do it here (see below :P )
Keep your separate function, as the logic for enable vs. disable GPU sandbox is
no longer trivial. Move the check for kDisableGpuSandbox into your new function.

https://chromiumcodereview.appspot.com/10534049/diff/8001/content/common/sand...
content/common/sandbox_init_linux.cc:425: ShouldEnableGPUSandbox()) {
You want to do it above, otherwise you'll crash into NOTREACHED I think.

https://chromiumcodereview.appspot.com/10534049/diff/8001/content/gpu/gpu_mai...
File content/gpu/gpu_main.cc (right):

https://chromiumcodereview.appspot.com/10534049/diff/8001/content/gpu/gpu_mai...
content/gpu/gpu_main.cc:147: #if defined(OS_CHROMEOS)
I think we can have the best of two worlds:
- never accidentally turn it on for non-intel in production
- enable debug builds to play with turning on sandbox for non-intel

This could be achieved by adding && !defined(DEBUG) to your conditional I think?

[Jorge Lucangeli Obes, 2012-06-11 20:23:04]

https://chromiumcodereview.appspot.com/10534049/diff/8001/content/common/sand...
File content/common/sandbox_init_linux.cc (right):

https://chromiumcodereview.appspot.com/10534049/diff/8001/content/common/sand...
content/common/sandbox_init_linux.cc:396: return true;
On 2012/06/08 07:13:57, Chris Evans wrote:
> return !command_line.HasSwitch(kDisable) -- see below.

Done.

https://chromiumcodereview.appspot.com/10534049/diff/8001/content/common/sand...
content/common/sandbox_init_linux.cc:425: ShouldEnableGPUSandbox()) {
On 2012/06/08 07:13:57, Chris Evans wrote:
> You want to do it above, otherwise you'll crash into NOTREACHED I think.

Done.

https://chromiumcodereview.appspot.com/10534049/diff/8001/content/gpu/gpu_mai...
File content/gpu/gpu_main.cc (right):

https://chromiumcodereview.appspot.com/10534049/diff/8001/content/gpu/gpu_mai...
content/gpu/gpu_main.cc:147: #if defined(OS_CHROMEOS)
On 2012/06/08 07:13:57, Chris Evans wrote:
> I think we can have the best of two worlds:
> - never accidentally turn it on for non-intel in production
> - enable debug builds to play with turning on sandbox for non-intel
> 
> This could be achieved by adding && !defined(DEBUG) to your conditional I
think?

Done.

[Chris Evans, 2012-06-11 20:52:23]

https://chromiumcodereview.appspot.com/10534049/diff/5005/content/common/sand...
File content/common/sandbox_init_linux.cc (right):

https://chromiumcodereview.appspot.com/10534049/diff/5005/content/common/sand...
content/common/sandbox_init_linux.cc:396: #if defined(OS_CHROMEOS)
Can you get rid of this ifdef? If we have to pull the sandbox from e.g. binary
Nvidia driver on desktop Linux, I want this flag to work by default.

In terms of conflict, "disable" should win.

[Jorge Lucangeli Obes, 2012-06-11 22:56:46]

https://chromiumcodereview.appspot.com/10534049/diff/5005/content/common/sand...
File content/common/sandbox_init_linux.cc (right):

https://chromiumcodereview.appspot.com/10534049/diff/5005/content/common/sand...
content/common/sandbox_init_linux.cc:396: #if defined(OS_CHROMEOS)
On 2012/06/11 20:52:23, Chris Evans wrote:
> Can you get rid of this ifdef? If we have to pull the sandbox from e.g. binary
> Nvidia driver on desktop Linux, I want this flag to work by default.
> 
> In terms of conflict, "disable" should win.

Done.

[Chris Evans, 2012-06-11 23:09:00]

LGTM

You may need to go hunting for other owners ;-)

[Jorge Lucangeli Obes, 2012-06-11 23:39:30]

On 2012/06/11 23:09:00, Chris Evans wrote:
> LGTM
> 
> You may need to go hunting for other owners ;-)

brettw, apatrick, OWNERS LGTM?

brettw: cmdline switch.
apatrick: gpu_main.cc changes.

Thanks!

[apatrick_chromium, 2012-06-11 23:40:53]

gpu_main.cc LGTM

[brettw, 2012-06-12 01:03:54]

lgtm

https://chromiumcodereview.appspot.com/10534049/diff/12006/content/public/com...
File content/public/common/content_switches.cc (right):

https://chromiumcodereview.appspot.com/10534049/diff/12006/content/public/com...
content/public/common/content_switches.cc:134: const char kEnableGpuSandbox[]   
  = "enable-gpu-sandbox";
Please aligh the = like the other ones (looks like you just did it randomly?).

[Jorge Lucangeli Obes, 2012-06-12 01:27:34]

https://chromiumcodereview.appspot.com/10534049/diff/12006/content/public/com...
File content/public/common/content_switches.cc (right):

https://chromiumcodereview.appspot.com/10534049/diff/12006/content/public/com...
content/public/common/content_switches.cc:134: const char kEnableGpuSandbox[]   
  = "enable-gpu-sandbox";
On 2012/06/12 01:03:54, brettw wrote:
> Please aligh the = like the other ones (looks like you just did it randomly?).

Done.

[commit-bot: I haz the power, 2012-06-12 16:31:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jorgelo@chromium.org/10534049/11008

[commit-bot: I haz the power, 2012-06-12 17:55:02]

Change committed as 141683