Remove mention of the nacl process in content.

Refactor sandbox_policy.cc so that it doesn't contain the sandbox policies for all processes. Instead have whoever creates a sandboxed process set this data. This allows us to clean a few NaCl related changes in content:
-remove NaCl sandbox rules from content
-remove the hack for ifdef'ing out the GPU policy since it didn't link for nacl64.exe
-remove the 1GB memory reservation for the NaCl loader process out of content

Other cleanup:
-renamed sandbox_policy.* to sandbox_win.* to match the other platform-specific sandbox files
-moved BrokerGetFileHandleForProcess to internal content files since it's not called from outside
-remove AddGpuDllEvictionPolicy since it was redundant (the one dll it removed was already listed in the generic list)

There's still more cleanup to be done in the sandbox code (i.e. remove chrome frame switch, nacl process type switch etc). I will do that in future changes.

BUG=191682
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=189175

[cpu_(ooo_6.6-7.5), 2013-03-15 17:58:53]

https://codereview.chromium.org/12805004/diff/10001/content/common/sandbox_po...
File content/common/sandbox_policy.cc (right):

https://codereview.chromium.org/12805004/diff/10001/content/common/sandbox_po...
content/common/sandbox_policy.cc:816: GetContentClient()->AddPolicy(cmd_line,
policy);
how about not having this #if here and have it in the other side of the call?

[jam, 2013-03-15 18:06:33]

https://codereview.chromium.org/12805004/diff/10001/content/common/sandbox_po...
File content/common/sandbox_policy.cc (right):

https://codereview.chromium.org/12805004/diff/10001/content/common/sandbox_po...
content/common/sandbox_policy.cc:816: GetContentClient()->AddPolicy(cmd_line,
policy);
On 2013/03/15 17:58:53, cpu wrote:
> how about not having this #if here and have it in the other side of the call?

the problem was that the nacl win64 binary doesn't link with ContentClient.

btw i'm still trying to find a clean way of doing this :) (that's why I didn't
send it out for review yet)

[jam, 2013-03-15 22:33:08]

ok Carlos, this is now ready to look at.

https://codereview.chromium.org/12805004/diff/43001/content/common/sandbox_in...
File content/common/sandbox_init_win.cc (left):

https://codereview.chromium.org/12805004/diff/43001/content/common/sandbox_in...
content/common/sandbox_init_win.cc:47: if ((process_type ==
switches::kRendererProcess) ||
when we get to this point, target_services is non-NULL by definition (since if
it was NULL, then broker_services would be non-NULL)

[cpu_(ooo_6.6-7.5), 2013-03-18 19:46:26]

https://codereview.chromium.org/12805004/diff/55002/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main_win.cc (right):

https://codereview.chromium.org/12805004/diff/55002/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main_win.cc:90: 
this function looks really ugly here, isn't a better place to put this code,
like src\chrome\browser\nacl_host or other place?

You know the directories better than me.

https://codereview.chromium.org/12805004/diff/55002/content/common/sandbox_in...
File content/common/sandbox_init_win.cc (left):

https://codereview.chromium.org/12805004/diff/55002/content/common/sandbox_in...
content/common/sandbox_init_win.cc:24: if (process_type.empty() || process_type
== switches::kNaClBrokerProcess) {
we should leave the if (process_type.empty())

The reason being that CreateAlternateDesktop() is heavyweight specially in Vista
and XP boxes and here we would be doing it for the service process and maybe
others.

https://codereview.chromium.org/12805004/diff/55002/content/common/sandbox_po...
File content/common/sandbox_policy.cc (right):

https://codereview.chromium.org/12805004/diff/55002/content/common/sandbox_po...
content/common/sandbox_policy.cc:836: // For Native Client sel_ldr processes on
32-bit Windows, reserve 1 GB of
you want to add this as another callback after process started as well?

[jam, 2013-03-18 19:50:19]

https://codereview.chromium.org/12805004/diff/55002/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main_win.cc (right):

https://codereview.chromium.org/12805004/diff/55002/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main_win.cc:90: 
On 2013/03/18 19:46:26, cpu wrote:
> this function looks really ugly here, isn't a better place to put this code,
> like src\chrome\browser\nacl_host or other place?

I agree this is a bit of an odd function. I think it'll look odd anywhere.

The issue of putting it in nacl_host (which I think is also as odd to put it as
it's here) is that then we'd have to put this in a header.

> 
> You know the directories better than me.

https://codereview.chromium.org/12805004/diff/55002/content/common/sandbox_in...
File content/common/sandbox_init_win.cc (left):

https://codereview.chromium.org/12805004/diff/55002/content/common/sandbox_in...
content/common/sandbox_init_win.cc:24: if (process_type.empty() || process_type
== switches::kNaClBrokerProcess) {
On 2013/03/18 19:46:26, cpu wrote:
> we should leave the if (process_type.empty())
> 
> The reason being that CreateAlternateDesktop() is heavyweight specially in
Vista
> and XP boxes and here we would be doing it for the service process and maybe
> others.

I would much rather have this code be generic. The service process starting up
is not a common thing, and isn't user-visible anyways. If this is good enough to
block chrome starting, then I'm personally fine with it delaying service process
startup

https://codereview.chromium.org/12805004/diff/55002/content/common/sandbox_po...
File content/common/sandbox_policy.cc (right):

https://codereview.chromium.org/12805004/diff/55002/content/common/sandbox_po...
content/common/sandbox_policy.cc:836: // For Native Client sel_ldr processes on
32-bit Windows, reserve 1 GB of
On 2013/03/18 19:46:26, cpu wrote:
> you want to add this as another callback after process started as well?

yeah, this and other nacl stuff in this file will be done in future changes

[jam, 2013-03-19 16:06:27]

Carlos/Justin: please take a look, these are the changes we talked about
yesterday

Robert: small change in chrome_frame/test/net/fake_external_tab.cc. I could have
null-checked in the sandbox code but I prefer it this way so that we do get a
crash in production if someone doesn't initialize the sandbox.

https://codereview.chromium.org/12805004/diff/80001/content/browser/gpu/gpu_p...
File content/browser/gpu/gpu_process_host.cc (left):

https://codereview.chromium.org/12805004/diff/80001/content/browser/gpu/gpu_p...
content/browser/gpu/gpu_process_host.cc:1069:
cmd_line->AppendSwitchASCII("ignored", " --type=renderer ");
btw I too this out because I made the code in sandbox_policy.cc which did this
for all processes other than gpu just do it for all

https://codereview.chromium.org/12805004/diff/80001/content/common/sandbox_wi...
File content/common/sandbox_win.cc (right):

https://codereview.chromium.org/12805004/diff/80001/content/common/sandbox_wi...
content/common/sandbox_win.cc:285: result =
policy->AddRule(sandbox::TargetPolicy::SUBSYS_HANDLES,
this same code was in AddPolicyForGPU and AddPolicyForRenderer (which was called
for all non-gpu processes), so I just moved it here to a common place.

https://codereview.chromium.org/12805004/diff/80001/content/common/sandbox_wi...
content/common/sandbox_win.cc:335: bool
AddPolicyForSandboxedProcess(sandbox::TargetPolicy* policy) {
(I renamed this to something more generic since it was called for all non-gpu
processes, not just renderers)

[robertshield, 2013-03-19 16:36:15]

On 2013/03/19 16:06:27, jam wrote:
> Carlos/Justin: please take a look, these are the changes we talked about
> yesterday
> 
> Robert: small change in chrome_frame/test/net/fake_external_tab.cc. I could
have
> null-checked in the sandbox code but I prefer it this way so that we do get a
> crash in production if someone doesn't initialize the sandbox.
> 
>
https://codereview.chromium.org/12805004/diff/80001/content/browser/gpu/gpu_p...
> File content/browser/gpu/gpu_process_host.cc (left):
> 
>
https://codereview.chromium.org/12805004/diff/80001/content/browser/gpu/gpu_p...
> content/browser/gpu/gpu_process_host.cc:1069:
> cmd_line->AppendSwitchASCII("ignored", " --type=renderer ");
> btw I too this out because I made the code in sandbox_policy.cc which did this
> for all processes other than gpu just do it for all
> 
>
https://codereview.chromium.org/12805004/diff/80001/content/common/sandbox_wi...
> File content/common/sandbox_win.cc (right):
> 
>
https://codereview.chromium.org/12805004/diff/80001/content/common/sandbox_wi...
> content/common/sandbox_win.cc:285: result =
> policy->AddRule(sandbox::TargetPolicy::SUBSYS_HANDLES,
> this same code was in AddPolicyForGPU and AddPolicyForRenderer (which was
called
> for all non-gpu processes), so I just moved it here to a common place.
> 
>
https://codereview.chromium.org/12805004/diff/80001/content/common/sandbox_wi...
> content/common/sandbox_win.cc:335: bool
> AddPolicyForSandboxedProcess(sandbox::TargetPolicy* policy) {
> (I renamed this to something more generic since it was called for all non-gpu
> processes, not just renderers)

Tiny cf change lgtm

[cpu_(ooo_6.6-7.5), 2013-03-19 21:22:54]

https://codereview.chromium.org/12805004/diff/94001/chrome/nacl/nacl_broker_l...
File chrome/nacl/nacl_broker_listener.cc (right):

https://codereview.chromium.org/12805004/diff/94001/chrome/nacl/nacl_broker_l...
chrome/nacl/nacl_broker_listener.cc:52: // This code is duplicated in
chrome_browser_main_win.cc.
is the comment of line 52 correct?

https://codereview.chromium.org/12805004/diff/94001/content/browser/gpu/gpu_p...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/12805004/diff/94001/content/browser/gpu/gpu_p...
content/browser/gpu/gpu_process_host.cc:188: // TODO(cpu): Lock down the sandbox
more if possible.
please remove comment of line 188.

https://codereview.chromium.org/12805004/diff/98003/content/common/sandbox_wi...
File content/common/sandbox_win.cc (right):

https://codereview.chromium.org/12805004/diff/98003/content/common/sandbox_wi...
content/common/sandbox_win.cc:538: } else if (type_str ==
switches::kWorkerProcess) {
Most of this 'else if' section was only needed to call the functions that you
moved, take for example the nacl loader, that should go away.

https://codereview.chromium.org/12805004/diff/98003/content/common/sandbox_wi...
content/common/sandbox_win.cc:562: bool in_sandbox =
same here, this decision can be pushed to each host. If you remove this you can
also remove references to the nacl_broker above.

[cpu_(ooo_6.6-7.5), 2013-03-19 21:25:48]

https://codereview.chromium.org/12805004/diff/98003/chrome/nacl/nacl_broker_l...
File chrome/nacl/nacl_broker_listener.h (right):

https://codereview.chromium.org/12805004/diff/98003/chrome/nacl/nacl_broker_l...
chrome/nacl/nacl_broker_listener.h:30: bool* success);
OVERRIDE

[cpu_(ooo_6.6-7.5), 2013-03-19 21:27:42]

https://codereview.chromium.org/12805004/diff/98003/content/browser/gpu/gpu_p...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/12805004/diff/98003/content/browser/gpu/gpu_p...
content/browser/gpu/gpu_process_host.cc:183: }
OVERRIDE

same below

[jam, 2013-03-19 22:57:29]

https://codereview.chromium.org/12805004/diff/94001/chrome/nacl/nacl_broker_l...
File chrome/nacl/nacl_broker_listener.cc (right):

https://codereview.chromium.org/12805004/diff/94001/chrome/nacl/nacl_broker_l...
chrome/nacl/nacl_broker_listener.cc:52: // This code is duplicated in
chrome_browser_main_win.cc.
On 2013/03/19 21:22:55, cpu wrote:
> is the comment of line 52 correct?

updated now,thanks

https://codereview.chromium.org/12805004/diff/94001/content/browser/gpu/gpu_p...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/12805004/diff/94001/content/browser/gpu/gpu_p...
content/browser/gpu/gpu_process_host.cc:188: // TODO(cpu): Lock down the sandbox
more if possible.
On 2013/03/19 21:22:55, cpu wrote:
> please remove comment of line 188.

Done.

https://codereview.chromium.org/12805004/diff/98003/content/common/sandbox_wi...
File content/common/sandbox_win.cc (right):

https://codereview.chromium.org/12805004/diff/98003/content/common/sandbox_wi...
content/common/sandbox_win.cc:538: } else if (type_str ==
switches::kWorkerProcess) {
On 2013/03/19 21:22:55, cpu wrote:
> Most of this 'else if' section was only needed to call the functions that you
> moved, take for example the nacl loader, that should go away.

yep, I'm planning on removing this whole section in the next cl

https://codereview.chromium.org/12805004/diff/98003/content/common/sandbox_wi...
content/common/sandbox_win.cc:562: bool in_sandbox =
On 2013/03/19 21:22:55, cpu wrote:
> same here, this decision can be pushed to each host. If you remove this you
can
> also remove references to the nacl_broker above.

ditto

[jschuh, 2013-03-19 23:24:35]

I don't see anything Carlos didn't catch, so lgtm.

[cpu_(ooo_6.6-7.5), 2013-03-20 00:03:48]

lgtm