Clarify the process title of GPU broker process.

Clarify the process title of GPU broker process.

Currently, GPU broker process has the same process title to GPU process. To
distinguish GPU broker process, this CL updates the process title as follows:
"exec --type=gpu-broker".

In addition, callback name in BrokerProcess::Init() is change to
broker_process_init_callback in order to clarify the broker process calls it.

In addition, use base::Callback instead of function pointer.

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

[dshwang, 2014-02-14 18:55:56]

I refer to how zygote changes process title in Zygote::ReadArgsAndFork()

All process type switches are defined in
content/public/common/content_switches.h
However, I don't do that because "gpu-broker" type is used only in the CL.

[jln (very slow on Chromium), 2014-02-20 00:29:57]

This looks mostly ok, but let's re-write BrokerProcess::Init() to make it clear
that the callback is about more than enabling the sandbox.

Please, change the name and description in broker_process.* to make it a general
purpose, post-fork() child process callback.

(Maybe call it broker_process_init_callback).

If you want to make it a base::Callback as well, don't hesitate!

https://codereview.chromium.org/163433011/diff/1/content/common/sandbox_linux...
File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right):

https://codereview.chromium.org/163433011/diff/1/content/common/sandbox_linux...
content/common/sandbox_linux/bpf_gpu_policy_linux.cc:140: static void
UpdateProcessTitle() {
UpdateProcessTypeToGpuBroker() instead?

https://codereview.chromium.org/163433011/diff/1/content/common/sandbox_linux...
content/common/sandbox_linux/bpf_gpu_policy_linux.cc:154: bool
EnableGpuBrokerPolicyCallback() {
Let's rename to GpuBrokerChildInitCallback or something similar.

[Jorge Lucangeli Obes, 2014-02-20 00:33:05]

Apologies for the delay.

https://codereview.chromium.org/163433011/diff/1/content/common/sandbox_linux...
File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right):

https://codereview.chromium.org/163433011/diff/1/content/common/sandbox_linux...
content/common/sandbox_linux/bpf_gpu_policy_linux.cc:149: //
SetProcessTitleFromCommandLine in content_main_runner.cc, so we can pass
Is this comment still accurate?

[dshwang, 2014-02-20 20:36:30]

Thank you for review. I addressed your all suggestions.
Could you review again?

https://codereview.chromium.org/163433011/diff/1/content/common/sandbox_linux...
File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right):

https://codereview.chromium.org/163433011/diff/1/content/common/sandbox_linux...
content/common/sandbox_linux/bpf_gpu_policy_linux.cc:140: static void
UpdateProcessTitle() {
On 2014/02/20 00:29:57, jln wrote:
> UpdateProcessTypeToGpuBroker() instead?

Done

https://codereview.chromium.org/163433011/diff/1/content/common/sandbox_linux...
content/common/sandbox_linux/bpf_gpu_policy_linux.cc:149: //
SetProcessTitleFromCommandLine in content_main_runner.cc, so we can pass
On 2014/02/20 00:33:05, Jorge Lucangeli Obes wrote:
> Is this comment still accurate?

Yes. but there is some logic jump. Browser process cached argv in
SetProcessTitleFromCommandLine and then fork process to gpu process. So GPU
process also cached argv, and then fork to broker process. So broker process
also cached argv.
I copy this comment from zygote_linux.cc

https://codereview.chromium.org/163433011/diff/1/content/common/sandbox_linux...
content/common/sandbox_linux/bpf_gpu_policy_linux.cc:154: bool
EnableGpuBrokerPolicyCallback() {
On 2014/02/20 00:29:57, jln wrote:
> Let's rename to GpuBrokerChildInitCallback or something similar.

Done

[jln (very slow on Chromium), 2014-02-20 23:38:51]

A few more nits.

In particular let's rename things a bit to make it clear that:

- BrokerProcess::Init() takes a generic post-fork callback
- InitGpuBrokerProcess() takes a callback which is dedicated to sandboxing.

https://codereview.chromium.org/163433011/diff/160001/content/common/sandbox_...
File content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc (right):

https://codereview.chromium.org/163433011/diff/160001/content/common/sandbox_...
content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc:210:
read_whitelist_extra,
These are not Bind() arguments, they should be indented like base::Bind() is.

(FYI: src/third_party/clang_format/bin/linux/clang-format is now included in the
tree. "git cl format" is handy).

https://codereview.chromium.org/163433011/diff/160001/content/common/sandbox_...
File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right):

https://codereview.chromium.org/163433011/diff/160001/content/common/sandbox_...
content/common/sandbox_linux/bpf_gpu_policy_linux.cc:155: bool
GpuBrokerChildInitCallback(
Sorry for my earlier recommendation, but given how this now works, it's very
confusing to name it like this.

Let's do: UpdateProcessTypeAndEnableSandbox() or something similar, with the
argument being named broker_sandboxer_callback).

https://codereview.chromium.org/163433011/diff/160001/content/common/sandbox_...
content/common/sandbox_linux/bpf_gpu_policy_linux.cc:264: // Initialize the
broker process and give it a broker process init callback.
Let's add a clarification as this becomes convoluted:

"Initialize the broker process and give it an initialization callback."

"The initialization callback will perform generic initialization and then call
broker_sandboxer_callback" (let's keep this former name for this part of the
callback, which is dedicated to sandboxing).

https://codereview.chromium.org/163433011/diff/160001/content/common/sandbox_...
File content/common/sandbox_linux/bpf_gpu_policy_linux.h (right):

https://codereview.chromium.org/163433011/diff/160001/content/common/sandbox_...
content/common/sandbox_linux/bpf_gpu_policy_linux.h:33: // sandbox for the
broker process itself.
Let's call it clearly broker_sandboxer_callback since that's still what
InitGpuBrokerProcess() expects as an argument: something that enables a sandbox.

https://codereview.chromium.org/163433011/diff/160001/sandbox/linux/seccomp-b...
File sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc (right):

https://codereview.chromium.org/163433011/diff/160001/sandbox/linux/seccomp-b...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:675: bool NoOpCallback() {
The comment becomes a little confusing here. Let's move it to the top of the
anonymous namespace, near the constants?

https://codereview.chromium.org/163433011/diff/160001/sandbox/linux/services/...
File sandbox/linux/services/broker_process.cc (right):

https://codereview.chromium.org/163433011/diff/160001/sandbox/linux/services/...
sandbox/linux/services/broker_process.cc:178: // Enable the sandbox if provided.
Change the comment (or just remove it).

https://codereview.chromium.org/163433011/diff/160001/sandbox/linux/services/...
File sandbox/linux/services/broker_process.h (right):

https://codereview.chromium.org/163433011/diff/160001/sandbox/linux/services/...
sandbox/linux/services/broker_process.h:46: // broker_process_init_callback is a
function that should be called to enable
s/function/Callback/

Or simply:
"broker_process_init_callback will be called in the new broker process, after
fork() returns."

[dshwang, 2014-02-21 07:03:13]

Thank you for kind review. I address all your comments.
Could you review again?

https://codereview.chromium.org/163433011/diff/160001/content/common/sandbox_...
File content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc (right):

https://codereview.chromium.org/163433011/diff/160001/content/common/sandbox_...
content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc:210:
read_whitelist_extra,
On 2014/02/20 23:38:52, jln wrote:
> These are not Bind() arguments, they should be indented like base::Bind() is.
> 
> (FYI: src/third_party/clang_format/bin/linux/clang-format is now included in
the
> tree. "git cl format" is handy).

'git cl format' is super cool. thx.

https://codereview.chromium.org/163433011/diff/160001/content/common/sandbox_...
File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right):

https://codereview.chromium.org/163433011/diff/160001/content/common/sandbox_...
content/common/sandbox_linux/bpf_gpu_policy_linux.cc:155: bool
GpuBrokerChildInitCallback(
On 2014/02/20 23:38:52, jln wrote:
> Sorry for my earlier recommendation, but given how this now works, it's very
> confusing to name it like this.
> 
> Let's do: UpdateProcessTypeAndEnableSandbox() or something similar, with the
> argument being named broker_sandboxer_callback).

done.

https://codereview.chromium.org/163433011/diff/160001/content/common/sandbox_...
content/common/sandbox_linux/bpf_gpu_policy_linux.cc:264: // Initialize the
broker process and give it a broker process init callback.
On 2014/02/20 23:38:52, jln wrote:
> Let's add a clarification as this becomes convoluted:
> 
> "Initialize the broker process and give it an initialization callback."
> 
> "The initialization callback will perform generic initialization and then call
> broker_sandboxer_callback" (let's keep this former name for this part of the
> callback, which is dedicated to sandboxing).

thank you for good sentence. this part is the most difficult part for non native
:)

https://codereview.chromium.org/163433011/diff/160001/content/common/sandbox_...
File content/common/sandbox_linux/bpf_gpu_policy_linux.h (right):

https://codereview.chromium.org/163433011/diff/160001/content/common/sandbox_...
content/common/sandbox_linux/bpf_gpu_policy_linux.h:33: // sandbox for the
broker process itself.
On 2014/02/20 23:38:52, jln wrote:
> Let's call it clearly broker_sandboxer_callback since that's still what
> InitGpuBrokerProcess() expects as an argument: something that enables a
sandbox.

yep, done.

https://codereview.chromium.org/163433011/diff/160001/sandbox/linux/seccomp-b...
File sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc (right):

https://codereview.chromium.org/163433011/diff/160001/sandbox/linux/seccomp-b...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:675: bool NoOpCallback() {
On 2014/02/20 23:38:52, jln wrote:
> The comment becomes a little confusing here. Let's move it to the top of the
> anonymous namespace, near the constants?

I move this function few lines above, not the top. It's because all code in this
file are inside anonymous namespace.

https://codereview.chromium.org/163433011/diff/160001/sandbox/linux/services/...
File sandbox/linux/services/broker_process.cc (right):

https://codereview.chromium.org/163433011/diff/160001/sandbox/linux/services/...
sandbox/linux/services/broker_process.cc:178: // Enable the sandbox if provided.
On 2014/02/20 23:38:52, jln wrote:
> Change the comment (or just remove it).

it's mistake. done.

https://codereview.chromium.org/163433011/diff/160001/sandbox/linux/services/...
File sandbox/linux/services/broker_process.h (right):

https://codereview.chromium.org/163433011/diff/160001/sandbox/linux/services/...
sandbox/linux/services/broker_process.h:46: // broker_process_init_callback is a
function that should be called to enable
On 2014/02/20 23:38:52, jln wrote:
> s/function/Callback/
> 
> Or simply:
> "broker_process_init_callback will be called in the new broker process, after
> fork() returns."

yep, done.

[Jorge Lucangeli Obes, 2014-02-21 15:58:30]

lgtm but wait for jln@ before landing.

[dshwang, 2014-02-21 16:37:24]

On 2014/02/21 15:58:30, Jorge Lucangeli Obes wrote:
> lgtm but wait for jln@ before landing.

yes, thank you for lgtm.

[jln (very slow on Chromium), 2014-02-21 21:58:12]

lgtm, thanks!

[dshwang, 2014-02-22 00:30:38]

On 2014/02/21 21:58:12, jln wrote:
> lgtm, thanks!

thank you for kind review!

[dshwang, 2014-02-22 00:30:45]

The CQ bit was checked by dongseong.hwang@intel.com

[commit-bot: I haz the power, 2014-02-22 00:32:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/163433011/29...

[commit-bot: I haz the power, 2014-02-22 01:33:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/163433011/29...

[commit-bot: I haz the power, 2014-02-22 08:26:31]

Change committed as 252781