AdjustRendererOOMScore isn't execute in linux.

AdjustRendererOOMScore isn't execute in linux.

This is a regression issue what happened because
PR_SET_NO_NEW_PRIVS is set by default when launching some process.
It must be execute normally. (when using sandbox)

BUG=358713, 366056
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266768

[zino, 2014-04-23 10:10:11]

Please take a look.

[jln (very slow on Chromium), 2014-04-24 00:17:59]

The OOM adjuster part looks legit to me, but I don't understand why this would
be needed for the GPU process.

Also changing the child process launcher generally is too heavy a hammer and
we'd loose the benefits of NNP.

Could you please try to figure out what's happening with the GPU process?

Thanks!

https://chromiumcodereview.appspot.com/246653008/diff/20001/content/browser/c...
File content/browser/child_process_launcher.cc (right):

https://chromiumcodereview.appspot.com/246653008/diff/20001/content/browser/c...
content/browser/child_process_launcher.cc:271: options.allow_new_privs =
ZygoteHostImpl::GetInstance()->
You are doing this for the GPU process aren't you?

The GPU process is not going through the setuid sandbox, so this is incorrect in
any case.

Moreover, I don't see why the GPU process could ever need this.

https://chromiumcodereview.appspot.com/246653008/diff/20001/content/browser/z...
File content/browser/zygote_host/zygote_host_impl_linux.cc (right):

https://chromiumcodereview.appspot.com/246653008/diff/20001/content/browser/z...
content/browser/zygote_host/zygote_host_impl_linux.cc:437:
options.allow_new_privs = true;
Please add a comment along the lines of "sandbox_helper_process is a setuid
binary".

[zino, 2014-04-24 09:21:03]

On 2014/04/24 00:17:59, jln wrote:
> The OOM adjuster part looks legit to me, but I don't understand why this would
> be needed for the GPU process.
> 
> Also changing the child process launcher generally is too heavy a hammer and
> we'd loose the benefits of NNP.
> 
> Could you please try to figure out what's happening with the GPU process?
> 
> Thanks!
> 
>
https://chromiumcodereview.appspot.com/246653008/diff/20001/content/browser/c...
> File content/browser/child_process_launcher.cc (right):
> 
>
https://chromiumcodereview.appspot.com/246653008/diff/20001/content/browser/c...
> content/browser/child_process_launcher.cc:271: options.allow_new_privs =
> ZygoteHostImpl::GetInstance()->
> You are doing this for the GPU process aren't you?
> 
> The GPU process is not going through the setuid sandbox, so this is incorrect
in
> any case.
> 
> Moreover, I don't see why the GPU process could ever need this.
> 
>
https://chromiumcodereview.appspot.com/246653008/diff/20001/content/browser/z...
> File content/browser/zygote_host/zygote_host_impl_linux.cc (right):
> 
>
https://chromiumcodereview.appspot.com/246653008/diff/20001/content/browser/z...
> content/browser/zygote_host/zygote_host_impl_linux.cc:437:
> options.allow_new_privs = true;
> Please add a comment along the lines of "sandbox_helper_process is a setuid
> binary".

Thank you for review :)

I think it is better to separate another CL regarding GPU from this CL.
(https://codereview.chromium.org/257493003/)
I uploaded the patch set 2 and modified description.

Could you please review again?

[jln (very slow on Chromium), 2014-04-24 17:48:16]

lgtm

https://chromiumcodereview.appspot.com/246653008/diff/40001/content/browser/z...
File content/browser/zygote_host/zygote_host_impl_linux.cc (right):

https://chromiumcodereview.appspot.com/246653008/diff/40001/content/browser/z...
content/browser/zygote_host/zygote_host_impl_linux.cc:432: //
sandbox_helper_process is a setuid binary.
Please, move this comment above "options.allow_new_privs = true;"

[zino, 2014-04-28 13:40:48]

On 2014/04/24 17:48:16, jln wrote:
> lgtm
> 
>
https://chromiumcodereview.appspot.com/246653008/diff/40001/content/browser/z...
> File content/browser/zygote_host/zygote_host_impl_linux.cc (right):
> 
>
https://chromiumcodereview.appspot.com/246653008/diff/40001/content/browser/z...
> content/browser/zygote_host/zygote_host_impl_linux.cc:432: //
> sandbox_helper_process is a setuid binary.
> Please, move this comment above "options.allow_new_privs = true;"

Done.

Can I commit and close this?
Please take a look again.

Thank you!

[jln (very slow on Chromium), 2014-04-28 21:05:19]

Yes, lgtm (you can commit this)

[zino, 2014-04-28 21:17:28]

The CQ bit was checked by jinho.bang@samsung.com

[commit-bot: I haz the power, 2014-04-28 21:18:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jinho.bang@samsung.com/246653008/60001

[commit-bot: I haz the power, 2014-04-28 22:21:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-28 22:21:44]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[zino, 2014-04-28 23:48:07]

The CQ bit was checked by jinho.bang@samsung.com

[commit-bot: I haz the power, 2014-04-28 23:49:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jinho.bang@samsung.com/246653008/60001

[commit-bot: I haz the power, 2014-04-29 07:31:47]

Change committed as 266768