Add UMA reports for Linux nacl_helper startup status

Add UMA reports for Linux nacl_helper startup status

This extends the Linux Zygote Fork request protocol so the Zygote process
can return a UMA histogram enumeration report to be made, along with the
PID.  In the Zygote process, the ZygoteForkDelegate decides what to report.
It gets to choose an initial report to make, which happens on the first
fork request that doesn't have its own report to make (as a generic fork
for a renderer won't).  It also gets to choose a report to make with each
individual fork request.

We then use this in the NaClForkDelegate to report status about the attempt
to start up the nacl_helper process.  We both make an initial report, so we
can collect this information from every Chrome instance, and make a report
repeating the same information on each NaCl process fork request, so that
we can correlate the nacl_helper startup success/failure rates with
sessions that actually make use of NaCl.

BUG= http://code.google.com/p/nativeclient/issues/detail?id=2361
TEST= looked at about:histograms/NaCl

R=agl@chromium.org,bradchen@google.com,ncbray@google.com

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

[agl, 2011-10-19 16:17:27]

LGTM

http://codereview.chromium.org/8342017/diff/1/chrome/nacl/nacl_fork_delegate_...
File chrome/nacl/nacl_fork_delegate_linux.cc (right):

http://codereview.chromium.org/8342017/diff/1/chrome/nacl/nacl_fork_delegate_...
chrome/nacl/nacl_fork_delegate_linux.cc:65: if
(base::LaunchProcess(cmd_line.argv(), options, NULL))
This looks so much like a bug that I strongly urge you to change it.

I think I would recommend removing |status_ = kNaClHelperLaunchFailed;| at 58
and then:

if (!base::LaunchProcess(cmd_line.argv(), options, NULL))
  status_ = kNaClHelperLaunchFailed;

then, at 71:
if (status_ == kNaClHelperUnused)

http://codereview.chromium.org/8342017/diff/1/content/browser/zygote_main_lin...
File content/browser/zygote_main_linux.cc (right):

http://codereview.chromium.org/8342017/diff/1/content/browser/zygote_main_lin...
content/browser/zygote_main_linux.cc:260: // the sandbox.  Optionally, it fills
in uma_name et al with a report
nit: only one space after a period (at least in this file by the looks of it).

http://codereview.chromium.org/8342017/diff/1/content/browser/zygote_main_lin...
content/browser/zygote_main_linux.cc:502: uma_name.swap(initial_uma_name_);
Would these initial values not be returned for any fork that doesn't return its
own values? I think they might need to be cleared after returning them once.

http://codereview.chromium.org/8342017/diff/1/content/common/zygote_fork_dele...
File content/common/zygote_fork_delegate_linux.h (right):

http://codereview.chromium.org/8342017/diff/1/content/common/zygote_fork_dele...
content/common/zygote_fork_delegate_linux.h:35: // would like to have done on
the first fork.
s/have done/supply/ ?

[Roland McGrath, 2011-10-19 18:34:15]

http://codereview.chromium.org/8342017/diff/1/chrome/nacl/nacl_fork_delegate_...
File chrome/nacl/nacl_fork_delegate_linux.cc (right):

http://codereview.chromium.org/8342017/diff/1/chrome/nacl/nacl_fork_delegate_...
chrome/nacl/nacl_fork_delegate_linux.cc:65: if
(base::LaunchProcess(cmd_line.argv(), options, NULL))
On 2011/10/19 16:17:27, agl wrote:
> This looks so much like a bug that I strongly urge you to change it.
> 
> I think I would recommend removing |status_ = kNaClHelperLaunchFailed;| at 58
> and then:
> 
> if (!base::LaunchProcess(cmd_line.argv(), options, NULL))
>   status_ = kNaClHelperLaunchFailed;
> 
> then, at 71:
> if (status_ == kNaClHelperUnused)

Done.

http://codereview.chromium.org/8342017/diff/1/content/browser/zygote_main_lin...
File content/browser/zygote_main_linux.cc (right):

http://codereview.chromium.org/8342017/diff/1/content/browser/zygote_main_lin...
content/browser/zygote_main_linux.cc:260: // the sandbox.  Optionally, it fills
in uma_name et al with a report
On 2011/10/19 16:17:27, agl wrote:
> nit: only one space after a period (at least in this file by the looks of it).

Done.

http://codereview.chromium.org/8342017/diff/1/content/browser/zygote_main_lin...
content/browser/zygote_main_linux.cc:502: uma_name.swap(initial_uma_name_);
On 2011/10/19 16:17:27, agl wrote:
> Would these initial values not be returned for any fork that doesn't return
its
> own values? I think they might need to be cleared after returning them once.

It is cleared, as the comment says.  Using swap is the efficient way to do this.
 I added a comment pointing out that subtlety.

http://codereview.chromium.org/8342017/diff/1/content/common/zygote_fork_dele...
File content/common/zygote_fork_delegate_linux.h (right):

http://codereview.chromium.org/8342017/diff/1/content/common/zygote_fork_dele...
content/common/zygote_fork_delegate_linux.h:35: // would like to have done on
the first fork.
On 2011/10/19 16:17:27, agl wrote:
> s/have done/supply/ ?

Done.

[Roland McGrath, 2011-10-19 19:53:04]

Oops, just noticed I need a content/{browser,common}/OWNERS member.
Brett, PTAL.

[brettw, 2011-10-20 06:07:09]

LGTM, I didn't look at the details. Some style nits:

http://codereview.chromium.org/8342017/diff/6001/content/browser/zygote_host_...
File content/browser/zygote_host_linux.cc (right):

http://codereview.chromium.org/8342017/diff/6001/content/browser/zygote_host_...
content/browser/zygote_host_linux.cc:296: if (!uma_histogram ||
uma_histogram->histogram_name() != uma_name)
Use {} for multi-line (even if it's just one stmt).

http://codereview.chromium.org/8342017/diff/6001/content/browser/zygote_main_...
File content/browser/zygote_main_linux.cc (right):

http://codereview.chromium.org/8342017/diff/6001/content/browser/zygote_main_...
content/browser/zygote_main_linux.cc:517: (ssize_t) reply_pickle.size())
Use static_cast instead of C-style casts.