Add new termination status for failed launch

Add new termination status for failed child process launch.

Treat renderer startup failure as a renderer crash for metrics.

This makes situations where sandbox failures are happening en-masse
without crash reports being produced easier to spot on the stability
dashboard.

BUG=526198, 467920
Committed: https://crrev.com/0d9532a1e73db2bc36a865655b0727d74dcf13a6
Cr-Commit-Position: refs/heads/master@{#347060}

[Will Harris, 2015-08-28 16:38:29]

wfh@chromium.org changed reviewers:
+ asvitkine@chromium.org, jam@chromium.org

[Will Harris, 2015-08-28 16:41:11]

PTAL

asvitkine -> */metrics/*
jam -> chrome/common

amineer -> this will increase renderer crash rates seen in UMA stability.

https://codereview.chromium.org/1320153002/diff/1/chrome/browser/metrics/chro...
File chrome/browser/metrics/chrome_stability_metrics_provider.cc (right):

https://codereview.chromium.org/1320153002/diff/1/chrome/browser/metrics/chro...
chrome/browser/metrics/chrome_stability_metrics_provider.cc:286: exit_code ==
-1)) {
reviewer note: this error code comes from

RenderProcessHostImpl::OnProcessLaunchFailed()

[Alexei Svitkine (slow), 2015-08-28 16:45:50]

https://codereview.chromium.org/1320153002/diff/1/chrome/browser/metrics/chro...
File chrome/browser/metrics/chrome_stability_metrics_provider.cc (right):

https://codereview.chromium.org/1320153002/diff/1/chrome/browser/metrics/chro...
chrome/browser/metrics/chrome_stability_metrics_provider.cc:285: (status ==
base::TERMINATION_STATUS_PROCESS_WAS_KILLED &&
Probably worth adding a unit test for - e.g. call LogRendererCrash() with given
params and check that a crash is recorded.

https://codereview.chromium.org/1320153002/diff/1/chrome/browser/metrics/chro...
chrome/browser/metrics/chrome_stability_metrics_provider.cc:286: exit_code ==
-1)) {
On 2015/08/28 16:41:11, Will Harris wrote:
> reviewer note: this error code comes from
> 
> RenderProcessHostImpl::OnProcessLaunchFailed()

Can this be made a constant that both places can refer to?

Is that what you meant to do with RESULT_CODE_RENDERER_PROCESS_START_FAILURE?

[Will Harris, 2015-08-28 16:52:14]

https://codereview.chromium.org/1320153002/diff/1/chrome/browser/metrics/chro...
File chrome/browser/metrics/chrome_stability_metrics_provider.cc (right):

https://codereview.chromium.org/1320153002/diff/1/chrome/browser/metrics/chro...
chrome/browser/metrics/chrome_stability_metrics_provider.cc:286: exit_code ==
-1)) {
render_process_host_impl.cc is in content so I'd have to make a new content
result code to return this value. These are content/public/common/result_codes.h
and there's not many of them.

I chose to put the result code in chrome/ as it's a "psuedo" resultcode i.e. the
process doesn't actually have a real exit code, it just represents the failure
to start so we can see it on chrome histograms.

tbh not entirely sure what is best here perhaps jam has an opinion as well.

[jam, 2015-08-28 17:17:08]

lgtm

[jam, 2015-08-28 17:18:35]

https://codereview.chromium.org/1320153002/diff/1/chrome/browser/metrics/chro...
File chrome/browser/metrics/chrome_stability_metrics_provider.cc (right):

https://codereview.chromium.org/1320153002/diff/1/chrome/browser/metrics/chro...
chrome/browser/metrics/chrome_stability_metrics_provider.cc:286: exit_code ==
-1)) {
On 2015/08/28 16:52:13, Will Harris wrote:
> render_process_host_impl.cc is in content so I'd have to make a new content
> result code to return this value. These are
content/public/common/result_codes.h
> and there's not many of them.
> 
> I chose to put the result code in chrome/ as it's a "psuedo" resultcode i.e.
the
> process doesn't actually have a real exit code, it just represents the failure
> to start so we can see it on chrome histograms.

It seems a pseudo code can be in both modules. Since content already knows about
it, i agree better to have one place for the constant instead of duplicating it.
> 
> tbh not entirely sure what is best here perhaps jam has an opinion as well.

[Will Harris, 2015-08-28 22:52:36]

wfh@chromium.org changed reviewers:
+ thestig@chromium.org

[Will Harris, 2015-08-29 01:42:33]

Here is a new approach that adds a base::TerminationStatus for a failure to
start a process at all. This means that the error code from the launcher can be
passed along with the notification. In addition, adding an error code in
content/public/common/result_codes.h seemed unwise as it would have shifted all
of the exit codes and had knockon effects on the existing histograms. I have
sketched out a design for this new way in crbug.com/526198

I will add tests in another CL because right now ChromeStabilityMetricsProvider
is not structured to support running tests (uses g_browser_process) and I don't
want to clutter up this CL, which I will loop back to after the test framework
is running.

Comments appreciated but I will loop back to this after
ChromeStabilityMetricsProvider has tests.

[Alexei Svitkine (slow), 2015-08-31 19:06:14]

https://codereview.chromium.org/1320153002/diff/60001/chrome/browser/metrics/...
File chrome/browser/metrics/chrome_stability_metrics_provider.cc (left):

https://codereview.chromium.org/1320153002/diff/60001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_stability_metrics_provider.cc:69: 
Nit: Remove spurious change.

https://codereview.chromium.org/1320153002/diff/60001/chrome/browser/metrics/...
File chrome/browser/metrics/chrome_stability_metrics_provider.cc (right):

https://codereview.chromium.org/1320153002/diff/60001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_stability_metrics_provider.cc:315:
was_extension_process ? 2 : 1);
Hmm, this macro shouldn't be using UMA_HISTOGRAM_PERCENTAGE - it doesn't seem to
be logging a percent. It should likely be UMA_HISTOGRAM_ENUMERATION().

The one above should be fixed up too - as well as the ones in
cast_stability_metrics_provider.cc.

https://codereview.chromium.org/1320153002/diff/60001/tools/metrics/actions/a...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/1320153002/diff/60001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:1893: <action
name="BrowserPlugin.Guest.LaunchFailed">
Not for this CL, but I wonder if we should just convert these metrics to
histograms instead of actions.

[Will Harris, 2015-09-01 01:15:38]

All done PTAL.

jam -> code has changed since your last lgtm so please have another look at
content/ and chrome/
thestig -> please look at base/

https://codereview.chromium.org/1320153002/diff/1/chrome/browser/metrics/chro...
File chrome/browser/metrics/chrome_stability_metrics_provider.cc (right):

https://codereview.chromium.org/1320153002/diff/1/chrome/browser/metrics/chro...
chrome/browser/metrics/chrome_stability_metrics_provider.cc:285: (status ==
base::TERMINATION_STATUS_PROCESS_WAS_KILLED &&
On 2015/08/28 16:45:50, Alexei Svitkine wrote:
> Probably worth adding a unit test for - e.g. call LogRendererCrash() with
given
> params and check that a crash is recorded.

Done.

https://codereview.chromium.org/1320153002/diff/60001/chrome/browser/metrics/...
File chrome/browser/metrics/chrome_stability_metrics_provider.cc (left):

https://codereview.chromium.org/1320153002/diff/60001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_stability_metrics_provider.cc:69: 
On 2015/08/31 19:06:14, Alexei Svitkine wrote:
> Nit: Remove spurious change.

Done.

https://codereview.chromium.org/1320153002/diff/60001/chrome/browser/metrics/...
File chrome/browser/metrics/chrome_stability_metrics_provider.cc (right):

https://codereview.chromium.org/1320153002/diff/60001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_stability_metrics_provider.cc:315:
was_extension_process ? 2 : 1);
On 2015/08/31 19:06:14, Alexei Svitkine wrote:
> Hmm, this macro shouldn't be using UMA_HISTOGRAM_PERCENTAGE - it doesn't seem
to
> be logging a percent. It should likely be UMA_HISTOGRAM_ENUMERATION().
> 
> The one above should be fixed up too - as well as the ones in
> cast_stability_metrics_provider.cc.

Done.

https://codereview.chromium.org/1320153002/diff/60001/tools/metrics/actions/a...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/1320153002/diff/60001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:1893: <action
name="BrowserPlugin.Guest.LaunchFailed">
On 2015/08/31 19:06:14, Alexei Svitkine wrote:
> Not for this CL, but I wonder if we should just convert these metrics to
> histograms instead of actions.

Acknowledged.

[Lei Zhang, 2015-09-01 02:32:59]

On 2015/09/01 01:15:38, Will Harris wrote:
> thestig -> please look at base/

lgtm stamp, but really jam can review the whole thing.

[Alexei Svitkine (slow), 2015-09-01 20:50:42]

lgtm % the issue with the test from your other cl

[jabdelmalek, 2015-09-02 00:39:31]

jabdelmalek@google.com changed reviewers:
+ jabdelmalek@google.com

[jabdelmalek, 2015-09-02 00:39:31]

slgtm with one comment

https://codereview.chromium.org/1320153002/diff/120001/chromecast/browser/met...
File chromecast/browser/metrics/cast_stability_metrics_provider.cc (right):

https://codereview.chromium.org/1320153002/diff/120001/chromecast/browser/met...
chromecast/browser/metrics/cast_stability_metrics_provider.cc:32:
RENDERER_TYPE_EXTENSION,
? i don't see this used, and also this is iin chromecast which doesn't have
extensions afaik

(ah now i see this was copy & paste from some other chrome file, please remove)

[Will Harris, 2015-09-02 19:59:42]

On 2015/09/02 00:39:31, jabdelmalek wrote:
> (ah now i see this was copy & paste from some other chrome file, please
remove)

I left this in since I need COUNT for the UMA metric, and hardcoding "1" and "3"
seemed inelegant, but I added a comment that EXTENSION is not used.

[Will Harris, 2015-09-02 20:00:21]

Thanks for reviews.

https://codereview.chromium.org/1320153002/diff/120001/chromecast/browser/met...
File chromecast/browser/metrics/cast_stability_metrics_provider.cc (right):

https://codereview.chromium.org/1320153002/diff/120001/chromecast/browser/met...
chromecast/browser/metrics/cast_stability_metrics_provider.cc:32:
RENDERER_TYPE_EXTENSION,
On 2015/09/02 00:39:31, jabdelmalek wrote:
> ? i don't see this used, and also this is iin chromecast which doesn't have
> extensions afaik
> 
> (ah now i see this was copy & paste from some other chrome file, please
remove)

Done.

[Will Harris, 2015-09-02 20:00:25]

The CQ bit was checked by wfh@chromium.org

[Will Harris, 2015-09-02 20:00:25]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
thestig@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1320153002/#ps140001
(title: "rebase and a nit.")

[commit-bot: I haz the power, 2015-09-02 20:01:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1320153002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1320153002/140001

[commit-bot: I haz the power, 2015-09-02 23:19:08]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-09-02 23:20:30]

Patchset 8 (id:??) landed as
https://crrev.com/0d9532a1e73db2bc36a865655b0727d74dcf13a6
Cr-Commit-Position: refs/heads/master@{#347060}