Issue 5491001: Mac: Sandbox GPU process.

Issue 5491001: Mac: Sandbox GPU process. (Closed)

Created:
10 years ago by Nico

Modified:
9 years, 7 months ago

Reviewers:
vtl, jeremy

CC:
chromium-reviews, pam+watch_chromium.org, apatrick_chromium, ben+cc_chromium.org, darin-cc_chromium.org, brettw-cc_chromium.org, Paweł Hajdan Jr.

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

Description

Mac: Scaffolding for sandboxing GPU process. The sandbox config allows everything for now; I will put in restrictions in a follow-up CL (which should be small). This CL should have no visible effect (other than changing a few LOG(WARNING) to LOG(ERROR)). BUG=48607 TEST=GPU process still works Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=67939

Patch Set 1 #

Patch Set 2 : sandbox is active and successfully stops the gpu process from doing anything #

Patch Set 3 : make gpu process sandbox disablable, LOG(ERROR) if either gpu or renderer sandbox are disabled #

Patch Set 4 : fix unintentional revert #

Patch Set 5 : let ChildProcess:WaitForStartup log to ERROR, which is visible in release builds too #

Patch Set 6 : let sandbox allow everything for now #

Patch Set 7 : '' #

Total comments: 1

Patch Set 8 : comments #

Patch Set 9 : '' #

Patch Set 10 : make tests work for now #

Total comments: 5

Created: 10 years ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+81 lines, -15 lines)			Patch
M	chrome/app/chrome_main.cc	View	1	1 chunk	+4 lines, -2 lines	0 comments	Download
A	chrome/browser/gpu.sb	View	1 2 3 4 5 6 7	1 chunk	+10 lines, -0 lines	0 comments	Download
M	chrome/browser/gpu_process_host.cc	View	1 2 3 4 5 6 7 8	1 chunk	+5 lines, -0 lines	0 comments	Download
M	chrome/chrome_browser.gypi	View	1	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/common/child_process.cc	View	1 2 3 4	1 chunk	+4 lines, -4 lines	1 comment	Download
M	chrome/common/chrome_switches.h	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/common/chrome_switches.cc	View	1 2 3 4 5 6 7	2 chunks	+4 lines, -1 line	1 comment	Download
M	chrome/common/sandbox_init_wrapper.h	View		1 chunk	+3 lines, -2 lines	0 comments	Download
M	chrome/common/sandbox_init_wrapper_mac.cc	View	3	1 chunk	+2 lines, -1 line	0 comments	Download
M	chrome/common/sandbox_mac.h	View		1 chunk	+3 lines, -0 lines	0 comments	Download
M	chrome/common/sandbox_mac.mm	View	1 2 3 4	2 chunks	+4 lines, -1 line	0 comments	Download
M	chrome/common/sandbox_mac_unittest_helper.mm	View	1 2 3 4 5 6 7 8 9	1 chunk	+9 lines, -2 lines	1 comment	Download
M	chrome/gpu/gpu_main.cc	View	1 2 3 4 5 6 7	4 chunks	+29 lines, -2 lines	1 comment	Download
M	chrome/renderer/renderer_main.cc	View	1 2	1 chunk	+2 lines, -0 lines	1 comment	Download

Messages

Total messages: 5 (0 generated)

Expand Messages | Collapse Messages

vtl

LGTM, except for your inconsistent capitalization of "gpu"/"GPU". http://codereview.chromium.org/5491001/diff/15001/chrome/renderer/renderer_main.cc File chrome/renderer/renderer_main.cc (right): http://codereview.chromium.org/5491001/diff/15001/chrome/renderer/renderer_main.cc#newcode282 chrome/renderer/renderer_main.cc:282: LOG(ERROR) ...

10 years ago (2010-12-01 22:52:11 UTC) #2

jeremy

http://codereview.chromium.org/5491001/diff/24001/chrome/common/child_process.cc File chrome/common/child_process.cc (right): http://codereview.chromium.org/5491001/diff/24001/chrome/common/child_process.cc#newcode100 chrome/common/child_process.cc:100: LOG(ERROR) << label Why did you change this to ...

10 years ago (2010-12-02 08:33:55 UTC) #4

Nico

10 years ago (2010-12-02 18:17:52 UTC) #5

On Thu, Dec 2, 2010 at 12:33 AM,  <jeremy@chromium.org> wrote:
>
>
http://codereview.chromium.org/5491001/diff/24001/chrome/common/child_process.cc
> File chrome/common/child_process.cc (right):
>
>
http://codereview.chromium.org/5491001/diff/24001/chrome/common/child_process...
> chrome/common/child_process.cc:100: LOG(ERROR) << label
> Why did you change this to error?

WebGL is too slow to be usable in Debug mode, so I do most of my
development in Release + symbols builds. LOG(WARNING) isn't shown in
release builds by default.

>
http://codereview.chromium.org/5491001/diff/24001/chrome/common/chrome_switch...
> File chrome/common/chrome_switches.cc (right):
>
>
http://codereview.chromium.org/5491001/diff/24001/chrome/common/chrome_switch...
> chrome/common/chrome_switches.cc:825: // Runs just the GPU process
> outside the sandbox.
> How about:
> Don't Sandbox the GPU process, does not affect other sandboxed
> processes.

Done.

>
http://codereview.chromium.org/5491001/diff/24001/chrome/common/sandbox_mac_u...
> File chrome/common/sandbox_mac_unittest_helper.mm (right):
>
>
http://codereview.chromium.org/5491001/diff/24001/chrome/common/sandbox_mac_u...
> chrome/common/sandbox_mac_unittest_helper.mm:63: //
> http://crbug.com/48607
> Could you please add a note to the bug to remove this? So we don't
> forget it...

I intend to have a real sandbox config in this week, and I have
removed this in my local build already so I don't forget about it.

> http://codereview.chromium.org/5491001/diff/24001/chrome/gpu/gpu_main.cc
> File chrome/gpu/gpu_main.cc (right):
>
>
http://codereview.chromium.org/5491001/diff/24001/chrome/gpu/gpu_main.cc#newc...
> chrome/gpu/gpu_main.cc:53: return
> sandbox_wrapper.InitializeSandbox(*parsed_command_line,
> Does this call Warmup first?

Yes.

>
http://codereview.chromium.org/5491001/diff/24001/chrome/renderer/renderer_ma...
> File chrome/renderer/renderer_main.cc (right):
>
>
http://codereview.chromium.org/5491001/diff/24001/chrome/renderer/renderer_ma...
> chrome/renderer/renderer_main.cc:282: LOG(ERROR) << "Running without
> renderer sandbox";
> How about:
> Renderer Sandbox disabled

Meh :-)

>
> http://codereview.chromium.org/5491001/
>

Expand Messages | Collapse Messages