Add extra parameter to BrowserChildProcessCrashed to pass the exit_code at time of crash/termination

Add extra parameter to BrowserChildProcessCrashed to pass the exit_code at time of crash/termination.

Wire this into all the existing callers.

Add a new child process watcher in chrome to be notified on child process crashes. This will confirm whether users with RESULT_CODE_INVALID_SANDBOX_STATE are crashing on background start (--no-startup-window) or not. This also adds required framework to paint a sad tab UI from this class in the future.

BUG=472324, 453541
TEST=Apply patch in http://pastebin.com/uEDxngBa and verify histogram ChildProcess.InvalidSandboxStateCrash.NoStartupWindow is recorded.

Committed: https://crrev.com/22e2f4a23530e0b22c8f37dd91e4e803a0a6e355
Cr-Commit-Position: refs/heads/master@{#327380}

[Will Harris, 2015-04-15 23:27:27]

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

[Will Harris, 2015-04-15 23:27:54]

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

[Will Harris, 2015-04-15 23:28:14]

PTAL

jam -> content/
thestig -> chrome/
asvitkine -> histograms.xml

[jam, 2015-04-16 04:14:57]

On 2015/04/15 23:28:14, Will Harris wrote:
> PTAL
> 
> jam -> content/
> thestig -> chrome/
> asvitkine -> histograms.xml

why not do this in content, which already histograms some exit codes (i.e. in
browser_child_process_host_impl.cc)? that way you don't need to change all that
code and add an observer in chrome?

[Will Harris, 2015-04-16 04:25:19]

On 2015/04/16 04:14:57, jam wrote:
> On 2015/04/15 23:28:14, Will Harris wrote:
> > PTAL
> > 
> > jam -> content/
> > thestig -> chrome/
> > asvitkine -> histograms.xml
> 
> why not do this in content, which already histograms some exit codes (i.e. in
> browser_child_process_host_impl.cc)? that way you don't need to change all
that
> code and add an observer in chrome?

The reason why this is in chrome/ is because this will eventually be calling
into chrome::SadTab::Create which lives in chrome/browser/ui so I have to bridge
between content and chrome already.  Does this sound okay?

[jam, 2015-04-16 14:43:05]

On 2015/04/16 04:25:19, Will Harris wrote:
> On 2015/04/16 04:14:57, jam wrote:
> > On 2015/04/15 23:28:14, Will Harris wrote:
> > > PTAL
> > > 
> > > jam -> content/
> > > thestig -> chrome/
> > > asvitkine -> histograms.xml
> > 
> > why not do this in content, which already histograms some exit codes (i.e.
in
> > browser_child_process_host_impl.cc)? that way you don't need to change all
> that
> > code and add an observer in chrome?
> 
> The reason why this is in chrome/ is because this will eventually be calling
> into chrome::SadTab::Create which lives in chrome/browser/ui so I have to
bridge
> between content and chrome already.  Does this sound okay?

right, but when you need that functionality, we can look into either using one
of the embedder callbacks (adding extra parameter if necessary) or adding a new
callback.

[Will Harris, 2015-04-16 16:24:34]

On 2015/04/16 14:43:05, jam wrote:
> On 2015/04/16 04:25:19, Will Harris wrote:
> > On 2015/04/16 04:14:57, jam wrote:
> > > On 2015/04/15 23:28:14, Will Harris wrote:
> > > > PTAL
> > > > 
> > > > jam -> content/
> > > > thestig -> chrome/
> > > > asvitkine -> histograms.xml
> > > 
> > > why not do this in content, which already histograms some exit codes (i.e.
> in
> > > browser_child_process_host_impl.cc)? that way you don't need to change all
> > that
> > > code and add an observer in chrome?
> > 
> > The reason why this is in chrome/ is because this will eventually be calling
> > into chrome::SadTab::Create which lives in chrome/browser/ui so I have to
> bridge
> > between content and chrome already.  Does this sound okay?
> 
> right, but when you need that functionality, we can look into either using one
> of the embedder callbacks (adding extra parameter if necessary) or adding a
new
> callback.

the callback I'm most interested in (the one I get from an early exit of a child
process before the IPC is up and running) is BrowserChildProcessCrashed and I
notice that the only part of chrome/ that listens for this is
ChromeStabilityMetricsProvider which I didn't really want to hijack for this use
(since it's about metrics). What other embedder callback are you meaning here,
am I missing something?

[Will Harris, 2015-04-17 01:55:24]

wfh@chromium.org changed reviewers:
- jam@chromium.org

[Will Harris, 2015-04-17 01:55:36]

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

[Will Harris, 2015-04-17 01:56:42]

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

[Will Harris, 2015-04-17 01:57:51]

PTAL

Adding that extra parameter touched a lot of code but all the changes are
benign.

jam - content/
asvitkine - tools/metrics and chrome/browser/metrics
thestig - chrome/

[jam, 2015-04-23 21:40:13]

On 2015/04/17 01:57:51, Will Harris wrote:
> PTAL
> 
> Adding that extra parameter touched a lot of code but all the changes are
> benign.
> 
> jam - content/
> asvitkine - tools/metrics and chrome/browser/metrics
> thestig - chrome/

lgtm, i'm sorry for the delay I just saw this

[Alexei Svitkine (slow), 2015-04-27 19:26:18]

lgtm

[Lei Zhang, 2015-04-27 21:47:35]

chrome/ lgtm with some nits

https://codereview.chromium.org/1067733006/diff/80001/chrome/browser/chrome_c...
File chrome/browser/chrome_child_process_watcher.cc (right):

https://codereview.chromium.org/1067733006/diff/80001/chrome/browser/chrome_c...
chrome/browser/chrome_child_process_watcher.cc:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
nit: 2015

https://codereview.chromium.org/1067733006/diff/80001/chrome/browser/chrome_c...
File chrome/browser/chrome_child_process_watcher.h (right):

https://codereview.chromium.org/1067733006/diff/80001/chrome/browser/chrome_c...
chrome/browser/chrome_child_process_watcher.h:8: #include <set>
nit: not used, most of the #includes and the RenderProcessHost fwd-decl are also
not needed.

[Will Harris, 2015-04-28 21:15:45]

https://codereview.chromium.org/1067733006/diff/80001/chrome/browser/chrome_c...
File chrome/browser/chrome_child_process_watcher.cc (right):

https://codereview.chromium.org/1067733006/diff/80001/chrome/browser/chrome_c...
chrome/browser/chrome_child_process_watcher.cc:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
On 2015/04/27 21:47:35, Lei Zhang wrote:
> nit: 2015

Done.

https://codereview.chromium.org/1067733006/diff/80001/chrome/browser/chrome_c...
File chrome/browser/chrome_child_process_watcher.h (right):

https://codereview.chromium.org/1067733006/diff/80001/chrome/browser/chrome_c...
chrome/browser/chrome_child_process_watcher.h:8: #include <set>
On 2015/04/27 21:47:35, Lei Zhang wrote:
> nit: not used, most of the #includes and the RenderProcessHost fwd-decl are
also
> not needed.

Done. These were accidental leftovers from a previous CL.

[Will Harris, 2015-04-28 21:17:21]

The CQ bit was checked by wfh@chromium.org

[Will Harris, 2015-04-28 21:17:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, asvitkine@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1067733006/#ps120001
(title: "code review comments")

[commit-bot: I haz the power, 2015-04-28 21:19:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1067733006/120001

[commit-bot: I haz the power, 2015-04-28 22:39:46]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-04-28 22:41:15]

Patchset 7 (id:??) landed as
https://crrev.com/22e2f4a23530e0b22c8f37dd91e4e803a0a6e355
Cr-Commit-Position: refs/heads/master@{#327380}