Add code to network error page to figure out a renderer crash.

Add code to network error page to figure out a renderer crash.

It looks like NetErrorHelperCore may be seeing events in an
unexpected order, so this CL logs events as they occur, and
when in a situation that is causing the crash, makes sure the
log is in the crash dump.

BUG=557541
Committed: https://crrev.com/5b69f5a09b685b2d6d8a846a5122fccecac5a5ad
Cr-Commit-Position: refs/heads/master@{#362542}

[mmenke, 2015-12-01 19:22:19]

Patchset #2 (id:20001) has been deleted

[mmenke, 2015-12-01 19:25:04]

mmenke@chromium.org changed reviewers:
+ rdsmith@chromium.org

[mmenke, 2015-12-01 19:25:04]

Randy:  I considered just trying to add a workaround, but decided I couldn't
really write tests unless I knew just what is happening.  I suspect we'll see
two commits in a row without any intervening start even, but we should be sure
of that.

I may have gone a bit overboard in logging events, but don't want to do too many
rounds of this.

Open to other ideas.

[Randy Smith (Not in Mondays), 2015-12-01 20:52:34]

No, I'm with you on this.  It's better to have root cause than just workaround,
and if we don't know what's going on, data collection with a sledgehammer is a
good thing.  LGTM, with suggestions below you can take or not as you wish.

https://codereview.chromium.org/1482363005/diff/40001/components/error_page/r...
File components/error_page/renderer/net_error_helper_core.cc (right):

https://codereview.chromium.org/1482363005/diff/40001/components/error_page/r...
components/error_page/renderer/net_error_helper_core.cc:611:
RecordHistoryDebugEvent(HistoryDebugEvent::ON_FINISH_LOAD_END);
Hmmm.  Up to you--I don't have anything like the understanding of the state
machine in this space that I'd like (which I know is part of the problem) but
I'd naively think we'd want to have three different events for the three
different ways the OnFinishLoad can end.

https://codereview.chromium.org/1482363005/diff/40001/components/error_page/r...
components/error_page/renderer/net_error_helper_core.cc:946: // This seems to
actually crash the renderer.
DumpWithoutCrashing() crashes the renderer?  Should we file a bug?

https://codereview.chromium.org/1482363005/diff/40001/components/error_page/r...
components/error_page/renderer/net_error_helper_core.cc:1106:
history_debug_events_.erase(history_debug_events_.begin());
Hmmm.  You're going to a lot of trouble to hit this data gathering with a large
sledgehammer, but including a lot of variation in how much data you actually
collect.  Might it be worth the I think fairly minor extra effort to make
history_debug_events_ a circular buffer, so you always have the last N events? 
Up to you; I'm just surprised to see this.

(Though I don't remember whether NetErrorHelperCore is per-tab, or per-page; if
the latter, this code is I think so unlikely to be triggered that it doesn't
matter much.)

[Randy Smith (Not in Mondays), 2015-12-01 20:53:06]

On 2015/12/01 20:52:34, rdsmith wrote:
> No, I'm with you on this.  It's better to have root cause than just
workaround,
> and if we don't know what's going on, data collection with a sledgehammer is a
> good thing.  LGTM, with suggestions below you can take or not as you wish.
> 
>
https://codereview.chromium.org/1482363005/diff/40001/components/error_page/r...
> File components/error_page/renderer/net_error_helper_core.cc (right):
> 
>
https://codereview.chromium.org/1482363005/diff/40001/components/error_page/r...
> components/error_page/renderer/net_error_helper_core.cc:611:
> RecordHistoryDebugEvent(HistoryDebugEvent::ON_FINISH_LOAD_END);
> Hmmm.  Up to you--I don't have anything like the understanding of the state
> machine in this space that I'd like (which I know is part of the problem) but
> I'd naively think we'd want to have three different events for the three
> different ways the OnFinishLoad can end.
> 
>
https://codereview.chromium.org/1482363005/diff/40001/components/error_page/r...
> components/error_page/renderer/net_error_helper_core.cc:946: // This seems to
> actually crash the renderer.
> DumpWithoutCrashing() crashes the renderer?  Should we file a bug?
> 
>
https://codereview.chromium.org/1482363005/diff/40001/components/error_page/r...
> components/error_page/renderer/net_error_helper_core.cc:1106:
> history_debug_events_.erase(history_debug_events_.begin());
> Hmmm.  You're going to a lot of trouble to hit this data gathering with a
large
> sledgehammer, but including a lot of variation in how much data you actually
> collect.  Might it be worth the I think fairly minor extra effort to make
> history_debug_events_ a circular buffer, so you always have the last N events?

> Up to you; I'm just surprised to see this.

Though I'd suggest another round of review if you follow this suggestion;
circular buffers can sometimes be tricky.

> 
> (Though I don't remember whether NetErrorHelperCore is per-tab, or per-page;
if
> the latter, this code is I think so unlikely to be triggered that it doesn't
> matter much.)

[Randy Smith (Not in Mondays), 2015-12-01 21:09:04]

https://codereview.chromium.org/1482363005/diff/40001/components/error_page/r...
File components/error_page/renderer/net_error_helper_core.cc (right):

https://codereview.chromium.org/1482363005/diff/40001/components/error_page/r...
components/error_page/renderer/net_error_helper_core.cc:1106:
history_debug_events_.erase(history_debug_events_.begin());
On 2015/12/01 20:52:34, rdsmith wrote:
> Hmmm.  You're going to a lot of trouble to hit this data gathering with a
large
> sledgehammer, but including a lot of variation in how much data you actually
> collect.  Might it be worth the I think fairly minor extra effort to make
> history_debug_events_ a circular buffer, so you always have the last N events?

> Up to you; I'm just surprised to see this.
> 
> (Though I don't remember whether NetErrorHelperCore is per-tab, or per-page;
if
> the latter, this code is I think so unlikely to be triggered that it doesn't
> matter much.)

As you pointed out offline, this code has exactly the effect I was suggesting
because it just erases the first element.  So please ignore this comment (I
suspect it's not very good performance, but I think that's the least of either
of our worries).

[mmenke, 2015-12-01 21:40:09]

Patchset #3 (id:60001) has been deleted

[mmenke, 2015-12-01 21:45:54]

Thanks for the comments!

https://codereview.chromium.org/1482363005/diff/40001/components/error_page/r...
File components/error_page/renderer/net_error_helper_core.cc (right):

https://codereview.chromium.org/1482363005/diff/40001/components/error_page/r...
components/error_page/renderer/net_error_helper_core.cc:611:
RecordHistoryDebugEvent(HistoryDebugEvent::ON_FINISH_LOAD_END);
On 2015/12/01 20:52:34, rdsmith wrote:
> Hmmm.  Up to you--I don't have anything like the understanding of the state
> machine in this space that I'd like (which I know is part of the problem) but
> I'd naively think we'd want to have three different events for the three
> different ways the OnFinishLoad can end.

You're right, done.

You're also right that lack of understanding is part of the problem - the state
transitions are unclear, and not really documented - Blink docs say when these
methods are called, but not how they actually relate to each other.

https://codereview.chromium.org/1482363005/diff/40001/components/error_page/r...
components/error_page/renderer/net_error_helper_core.cc:946: // This seems to
actually crash the renderer.
On 2015/12/01 20:52:34, rdsmith wrote:
> DumpWithoutCrashing() crashes the renderer?  Should we file a bug?

I'm not sure.  I'm not actually completely sure we crash (We don't in Chromium
debug builds, certainly), but some "crashes" with the only currently existing
call of the method in renderer process code looked like we were really crashing.

I've removed the comment (Problem solved, clearly!)

https://codereview.chromium.org/1482363005/diff/40001/components/error_page/r...
components/error_page/renderer/net_error_helper_core.cc:1106:
history_debug_events_.erase(history_debug_events_.begin());
On 2015/12/01 20:52:34, rdsmith wrote:
> Hmmm.  You're going to a lot of trouble to hit this data gathering with a
large
> sledgehammer, but including a lot of variation in how much data you actually
> collect.  Might it be worth the I think fairly minor extra effort to make
> history_debug_events_ a circular buffer, so you always have the last N events?

> Up to you; I'm just surprised to see this.
> 
> (Though I don't remember whether NetErrorHelperCore is per-tab, or per-page;
if
> the latter, this code is I think so unlikely to be triggered that it doesn't
> matter much.)

Doesn't this just erase the first element, giving us effectively a circular
buffer (Though a rather inefficient one)?

"vector::erase:

iterator erase (iterator position);
iterator erase (iterator first, iterator last);

Removes from the vector either a single element (position) or a range of
elements ([first,last))."

https://codereview.chromium.org/1482363005/diff/40001/components/error_page/r...
components/error_page/renderer/net_error_helper_core.cc:1106:
history_debug_events_.erase(history_debug_events_.begin());
On 2015/12/01 21:09:04, rdsmith wrote:
> On 2015/12/01 20:52:34, rdsmith wrote:
> > Hmmm.  You're going to a lot of trouble to hit this data gathering with a
> large
> > sledgehammer, but including a lot of variation in how much data you actually
> > collect.  Might it be worth the I think fairly minor extra effort to make
> > history_debug_events_ a circular buffer, so you always have the last N
events?
> 
> > Up to you; I'm just surprised to see this.
> > 
> > (Though I don't remember whether NetErrorHelperCore is per-tab, or per-page;
> if
> > the latter, this code is I think so unlikely to be triggered that it doesn't
> > matter much.)
> 
> As you pointed out offline, this code has exactly the effect I was suggesting
> because it just erases the first element.  So please ignore this comment (I
> suspect it's not very good performance, but I think that's the least of either
> of our worries).  

Yea, performance is irrelevant.  I don't plan for this ever to reach stable.

[mmenke, 2015-12-01 21:46:23]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2015-12-01 21:46:24]

The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org
Link to the patchset: https://codereview.chromium.org/1482363005/#ps80001
(title: "Response to comments")

[commit-bot: I haz the power, 2015-12-01 21:48:45]

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

[commit-bot: I haz the power, 2015-12-01 23:21:08]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2015-12-01 23:22:05]

Description was changed from

==========
Add code to network error page to figure out a renderer crash.

It looks like NetErrorHelperCore may be seeing events in an
unexpected order, so this CL logs events as they occur, and
when in a situation that is causing the crash, makes sure the
log is in the crash dump.

BUG=557541
==========

to

==========
Add code to network error page to figure out a renderer crash.

It looks like NetErrorHelperCore may be seeing events in an
unexpected order, so this CL logs events as they occur, and
when in a situation that is causing the crash, makes sure the
log is in the crash dump.

BUG=557541

Committed: https://crrev.com/5b69f5a09b685b2d6d8a846a5122fccecac5a5ad
Cr-Commit-Position: refs/heads/master@{#362542}
==========

[commit-bot: I haz the power, 2015-12-01 23:22:06]

Patchset 3 (id:??) landed as
https://crrev.com/5b69f5a09b685b2d6d8a846a5122fccecac5a5ad
Cr-Commit-Position: refs/heads/master@{#362542}