Attempt to fix a renderer crash in NetErrorHelperCore.

Attempt to fix a renderer crash in NetErrorHelperCore.

The code was not distinguishing between same page navigations, which do
not have a provision load start event (But do have a provisional commit)
from other navigations, which have both events.

The old code would cause a crash in the case of a same page navigation
on the error page.  This could be causing a crash we're seeing, but it's
unclear why the error page would be making same page navigations in the
first place.

BUG=557541
Committed: https://crrev.com/96f8988dba625765d815537c562e7aff2dd7edc0
Cr-Commit-Position: refs/heads/master@{#364388}

[mmenke, 2015-12-09 18:36:12]

Patchset #1 (id:1) has been deleted

[mmenke, 2015-12-09 20:18:32]

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

[mmenke, 2015-12-09 20:18:33]

https://codereview.chromium.org/1516623002/diff/40001/chrome/browser/errorpag...
File chrome/browser/errorpage_browsertest.cc (right):

https://codereview.chromium.org/1516623002/diff/40001/chrome/browser/errorpag...
chrome/browser/errorpage_browsertest.cc:1117:
IN_PROC_BROWSER_TEST_F(ErrorPageAutoReloadTest, IgnoresSamePageNavigation) {
I'm a big concerned that this may timeout on OSX official builders, like the
first reload test (Which also goes through 2 auto reloads).  We may want to look
into workaround at some point.

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

LGTM.

https://codereview.chromium.org/1516623002/diff/40001/chrome/browser/errorpag...
File chrome/browser/errorpage_browsertest.cc (right):

https://codereview.chromium.org/1516623002/diff/40001/chrome/browser/errorpag...
chrome/browser/errorpage_browsertest.cc:770: // No new requests should have been
issues.
nit:issued.

[mmenke, 2015-12-10 15:52:24]

Thanks!

https://codereview.chromium.org/1516623002/diff/40001/chrome/browser/errorpag...
File chrome/browser/errorpage_browsertest.cc (right):

https://codereview.chromium.org/1516623002/diff/40001/chrome/browser/errorpag...
chrome/browser/errorpage_browsertest.cc:770: // No new requests should have been
issues.
On 2015/12/09 21:23:46, rdsmith wrote:
> nit:issued.

Done.

[mmenke, 2015-12-10 15:58:15]

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

[mmenke, 2015-12-10 15:58:16]

[+thakis]:  Please review chrome/browser/errorpage_browsertest.cc.

Wonder if we should just move it into chrome/browser/net/.  It largely tests
chrome/renderer/net/net_error_helper, components/error_page.  Also test
chrome/common/localized_error, and how things are wired up in the
RenderFrameImpl, though.

[Randy Smith (Not in Mondays), 2015-12-10 16:01:47]

On 2015/12/10 15:58:16, mmenke wrote:
> [+thakis]:  Please review chrome/browser/errorpage_browsertest.cc.
> 
> Wonder if we should just move it into chrome/browser/net/.  It largely tests
> chrome/renderer/net/net_error_helper, components/error_page.  Also test
> chrome/common/localized_error, and how things are wired up in the
> RenderFrameImpl, though.

What is localized_error used for besides the net error page?

[Nico, 2015-12-10 16:05:10]

test lgtm

https://codereview.chromium.org/1516623002/diff/60001/chrome/browser/errorpag...
File chrome/browser/errorpage_browsertest.cc (right):

https://codereview.chromium.org/1516623002/diff/60001/chrome/browser/errorpag...
chrome/browser/errorpage_browsertest.cc:784: // There should have two more
requests to the correction service:  One for the
"should have been" or "should be"

[mmenke, 2015-12-10 16:06:11]

On 2015/12/10 16:01:47, rdsmith wrote:
> On 2015/12/10 15:58:16, mmenke wrote:
> > [+thakis]:  Please review chrome/browser/errorpage_browsertest.cc.
> > 
> > Wonder if we should just move it into chrome/browser/net/.  It largely tests
> > chrome/renderer/net/net_error_helper, components/error_page.  Also test
> > chrome/common/localized_error, and how things are wired up in the
> > RenderFrameImpl, though.
> 
> What is localized_error used for besides the net error page?

Nothing, though ChromeContentRendererClient has some calls directly into it.

These tests also test neterror.html, but that's moving into
components/error_page as well.  So more and more of this stuff is migrating into
places the net stack team owns (Or at least I own).  We could move
localized_error over to components/error_page as well, I suppose.  Nothing there
currently users it, of course, but if neterror.html is moving there, as I
believe is the plan, seems to make sense.

[mmenke, 2015-12-10 16:09:13]

Thanks!

https://codereview.chromium.org/1516623002/diff/60001/chrome/browser/errorpag...
File chrome/browser/errorpage_browsertest.cc (right):

https://codereview.chromium.org/1516623002/diff/60001/chrome/browser/errorpag...
chrome/browser/errorpage_browsertest.cc:784: // There should have two more
requests to the correction service:  One for the
On 2015/12/10 16:05:09, Nico wrote:
> "should have been" or "should be"

Done!  Copied that from another test.  Fixed both comments.

[mmenke, 2015-12-10 16:10:48]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2015-12-10 16:10:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdsmith@chromium.org, thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1516623002/#ps80001
(title: "Fix comment")

[commit-bot: I haz the power, 2015-12-10 16:11:41]

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

[commit-bot: I haz the power, 2015-12-10 17:27:27]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-12-10 17:28:27]

Description was changed from

==========
Attempt to fix a renderer crash in NetErrorHelperCore.

The code was not distinguishing between same page navigations, which do
not have a provision load start event (But do have a provisional commit)
from other navigations, which have both events.

The old code would cause a crash in the case of a same page navigation
on the error page.  This could be causing a crash we're seeing, but it's
unclear why the error page would be making same page navigations in the
first place.

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

to

==========
Attempt to fix a renderer crash in NetErrorHelperCore.

The code was not distinguishing between same page navigations, which do
not have a provision load start event (But do have a provisional commit)
from other navigations, which have both events.

The old code would cause a crash in the case of a same page navigation
on the error page.  This could be causing a crash we're seeing, but it's
unclear why the error page would be making same page navigations in the
first place.

BUG=557541

Committed: https://crrev.com/96f8988dba625765d815537c562e7aff2dd7edc0
Cr-Commit-Position: refs/heads/master@{#364388}
==========

[commit-bot: I haz the power, 2015-12-10 17:28:27]

Patchset 4 (id:??) landed as
https://crrev.com/96f8988dba625765d815537c562e7aff2dd7edc0
Cr-Commit-Position: refs/heads/master@{#364388}