Track renderer crash detected on WebState via GlobalWebStateObserver

Track renderer crash detected on WebState via GlobalWebStateObserver

Use RenderProcessGone() method in GlobalWebStateObserver added
in https://codereview.chromium.org/2666953003/
By using WebState observer, it is now possible to pass the WebState
to the handler.

BUG=685649
Review-Url: https://codereview.chromium.org/2670743002
Cr-Commit-Position: refs/heads/master@{#464816}
Committed: https://chromium.googlesource.com/chromium/src/+/c425b642cf0da2336925cfcd05b9883a45f146a2

[pkl (ping after 24h if needed), 2017-03-23 23:42:36]

Description was changed from

==========
Logs URL of tab when renderer crash detected on WebState.

BUG=685649
==========

to

==========
Track renderer crash detected on WebState via GlobalWebStateObserver

Use RenderProcessGone() method in GlobalWebStateObserver added
in https://codereview.chromium.org/2666953003/
By using WebState observer, it is now possible to pass the WebState
to the handler.

BUG=685649
==========

[pkl (ping after 24h if needed), 2017-03-23 23:50:49]

pkl@chromium.org changed reviewers:
+ eugenebut@chromium.org, rohitrao@chromium.org

[pkl (ping after 24h if needed), 2017-03-23 23:50:49]

This is a follow up to having URL of a crashed tab available for crash logging.

[Eugene But (OOO till 7-30), 2017-03-24 15:37:50]

lgtm!

https://codereview.chromium.org/2670743002/diff/40001/ios/chrome/browser/metr...
File ios/chrome/browser/metrics/ios_chrome_stability_metrics_provider.mm
(right):

https://codereview.chromium.org/2670743002/diff/40001/ios/chrome/browser/metr...
ios/chrome/browser/metrics/ios_chrome_stability_metrics_provider.mm:58: //
TODO(crbug.com/685649): Logs web_state->GetLastCommittedURL().
Really nit: s/Logs/Log

[rohitrao (ping after 24h), 2017-03-26 23:26:23]

lgtm

Is there any chance that another codepath is still calling
OnRendererProcessCrash(), causing us to double-count crashes?

https://codereview.chromium.org/2670743002/diff/40001/ios/chrome/browser/tabs...
File ios/chrome/browser/tabs/tab.mm (left):

https://codereview.chromium.org/2670743002/diff/40001/ios/chrome/browser/tabs...
ios/chrome/browser/tabs/tab.mm:1910: ->OnRendererProcessCrash();
Is anyone else calling this OnRendererProcessCrash method?  Can we delete it?

[pkl (ping after 24h if needed), 2017-03-27 15:32:47]

https://codereview.chromium.org/2670743002/diff/40001/ios/chrome/browser/metr...
File ios/chrome/browser/metrics/ios_chrome_stability_metrics_provider.mm
(right):

https://codereview.chromium.org/2670743002/diff/40001/ios/chrome/browser/metr...
ios/chrome/browser/metrics/ios_chrome_stability_metrics_provider.mm:58: //
TODO(crbug.com/685649): Logs web_state->GetLastCommittedURL().
On 2017/03/24 15:37:50, Eugene But wrote:
> Really nit: s/Logs/Log

Expanded the comment rendering this irrelevant.

https://codereview.chromium.org/2670743002/diff/40001/ios/chrome/browser/tabs...
File ios/chrome/browser/tabs/tab.mm (left):

https://codereview.chromium.org/2670743002/diff/40001/ios/chrome/browser/tabs...
ios/chrome/browser/tabs/tab.mm:1910: ->OnRendererProcessCrash();
On 2017/03/26 23:26:23, rohitrao (OOO until 3-31) wrote:
> Is anyone else calling this OnRendererProcessCrash method?  Can we delete it?

I think it can be removed. Since it involves metrics_service_client.h, I'm going
to do it as a separate CL.

[pkl (ping after 24h if needed), 2017-03-27 15:32:57]

The CQ bit was checked by pkl@chromium.org

[pkl (ping after 24h if needed), 2017-03-27 15:32:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
eugenebut@chromium.org, rohitrao@chromium.org
Link to the patchset: https://codereview.chromium.org/2670743002/#ps60001
(title: "comment")

[commit-bot: I haz the power, 2017-03-27 15:33:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eugene But (OOO till 7-30), 2017-03-27 16:56:27]

https://codereview.chromium.org/2670743002/diff/60001/ios/chrome/browser/metr...
File ios/chrome/browser/metrics/ios_chrome_stability_metrics_provider.mm
(right):

https://codereview.chromium.org/2670743002/diff/60001/ios/chrome/browser/metr...
ios/chrome/browser/metrics/ios_chrome_stability_metrics_provider.mm:53: void
IOSChromeStabilityMetricsProvider::RenderProcessGone(
I gave some more thoughts about the problem, so sorry for commenting after CL
was landed. Do we even think that GlobalWebStateObserver is a good pattern to
use? GlobalWebStateObserver is not much different from NotificationCenter which
makes code tightly coupled and hard to refactor. Should we reconsider the
ownership model for IOSChromeMetricsServiceClient instead of relying on
"notification center"?

[commit-bot: I haz the power, 2017-03-27 17:29:00]

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

[commit-bot: I haz the power, 2017-03-27 17:29:01]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[pkl (ping after 24h if needed), 2017-04-13 22:29:40]

This CL hasn't landed yet, so let's discuss in person.

https://codereview.chromium.org/2670743002/diff/60001/ios/chrome/browser/metr...
File ios/chrome/browser/metrics/ios_chrome_stability_metrics_provider.mm
(right):

https://codereview.chromium.org/2670743002/diff/60001/ios/chrome/browser/metr...
ios/chrome/browser/metrics/ios_chrome_stability_metrics_provider.mm:53: void
IOSChromeStabilityMetricsProvider::RenderProcessGone(
On 2017/03/27 16:56:26, Eugene But wrote:
> I gave some more thoughts about the problem, so sorry for commenting after CL
> was landed. Do we even think that GlobalWebStateObserver is a good pattern to
> use? GlobalWebStateObserver is not much different from NotificationCenter
which
> makes code tightly coupled and hard to refactor. Should we reconsider the
> ownership model for IOSChromeMetricsServiceClient instead of relying on
> "notification center"? 

Let's discuss this in person.

[pkl (ping after 24h if needed), 2017-04-14 21:38:53]

https://codereview.chromium.org/2670743002/diff/60001/ios/chrome/browser/metr...
File ios/chrome/browser/metrics/ios_chrome_stability_metrics_provider.mm
(right):

https://codereview.chromium.org/2670743002/diff/60001/ios/chrome/browser/metr...
ios/chrome/browser/metrics/ios_chrome_stability_metrics_provider.mm:53: void
IOSChromeStabilityMetricsProvider::RenderProcessGone(
On 2017/04/13 22:29:40, pklpkl wrote:
> On 2017/03/27 16:56:26, Eugene But wrote:
> > I gave some more thoughts about the problem, so sorry for commenting after
CL
> > was landed. Do we even think that GlobalWebStateObserver is a good pattern
to
> > use? GlobalWebStateObserver is not much different from NotificationCenter
> which
> > makes code tightly coupled and hard to refactor. Should we reconsider the
> > ownership model for IOSChromeMetricsServiceClient instead of relying on
> > "notification center"? 
> 
> Let's discuss this in person.

Discussed offline with eugenebut@. This CL should land first so the refactoring
of crash reporting can be completed. This is one step closer to an
implementation that does not involve Tab. Further discussion around using
GlobalWebStateObserver vs. notifications may still be necessary.

[pkl (ping after 24h if needed), 2017-04-14 21:51:18]

The CQ bit was checked by pkl@chromium.org

[commit-bot: I haz the power, 2017-04-14 21:51:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-14 23:09:55]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492206678656570,
"parent_rev": "8811aed463ee87b0302ff8a80a730ebd86dee515", "commit_rev":
"c425b642cf0da2336925cfcd05b9883a45f146a2"}

[commit-bot: I haz the power, 2017-04-14 23:10:50]

Description was changed from

==========
Track renderer crash detected on WebState via GlobalWebStateObserver

Use RenderProcessGone() method in GlobalWebStateObserver added
in https://codereview.chromium.org/2666953003/
By using WebState observer, it is now possible to pass the WebState
to the handler.

BUG=685649
==========

to

==========
Track renderer crash detected on WebState via GlobalWebStateObserver

Use RenderProcessGone() method in GlobalWebStateObserver added
in https://codereview.chromium.org/2666953003/
By using WebState observer, it is now possible to pass the WebState
to the handler.

BUG=685649

Review-Url: https://codereview.chromium.org/2670743002
Cr-Commit-Position: refs/heads/master@{#464816}
Committed:
https://chromium.googlesource.com/chromium/src/+/c425b642cf0da2336925cfcd05b9...
==========

[commit-bot: I haz the power, 2017-04-14 23:10:51]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/c425b642cf0da2336925cfcd05b9...