Remove unneeded code in NavigatorImpl::OnBeginNavigation.

Remove unneeded code in NavigatorImpl::OnBeginNavigation.

This got added in r451507, but I don't see it being hit for the layout test referenced in it. I also don't see that code being reached in any browser or layout test (see https://codereview.chromium.org/2884383002). I can confirm the change in the original cl to NavigatorImpl::RequestNavigation is what fixes the layout test.

BUG=691168, 705119
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

[jam, 2017-05-16 20:20:54]

Description was changed from

==========
Remove unneeded code in NavigatorImpl::OnBeginNavigation.

This got added in r451507, but I don't see it being hit for the layout test
referenced in it. I also don't see that code being reached in any browser or
layout test (see https://codereview.chromium.org/2884383002). I can confirm the
change in the original cl to NavigatorImpl::RequestNavigation is what fixes the
layout test.

BUG=691168,705119
==========

to

==========
Remove unneeded code in NavigatorImpl::OnBeginNavigation.

This got added in r451507, but I don't see it being hit for the layout test
referenced in it. I also don't see that code being reached in any browser or
layout test (see https://codereview.chromium.org/2884383002). I can confirm the
change in the original cl to NavigatorImpl::RequestNavigation is what fixes the
layout test.

BUG=691168,705119
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[jam, 2017-05-16 20:22:01]

jam@chromium.org changed reviewers:
+ ananta@chromium.org, creis@chromium.org

[jam, 2017-05-16 20:22:01]

Am I missing something?

[Charlie Reis, 2017-05-16 20:58:05]

The CQ bit was checked by creis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-16 20:59:17]

Dry run: CQ is trying da patch.

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

[Charlie Reis, 2017-05-16 20:59:42]

On 2017/05/16 20:22:01, jam wrote:
> Am I missing something?

That's disturbing.  I remember there being a real issue here that we were
fixing, but I'll need to take a closer look to see if we misunderstood it or if
something else is making the tests pass now.

[ananta, 2017-05-16 21:31:10]

On 2017/05/16 20:59:42, Charlie Reis wrote:
> On 2017/05/16 20:22:01, jam wrote:
> > Am I missing something?
> 
> That's disturbing.  I remember there being a real issue here that we were
> fixing, but I'll need to take a closer look to see if we misunderstood it or
if
> something else is making the tests pass now.

The snippet in question was required for the test to pass when I landed this
patch IIRC.

Perhaps it got fixed by some of the work which japhet did in blink to get rid of
the provisional document
loader and the issues with the way we tracked browser side navigations?

[ananta, 2017-05-16 21:31:41]

Description was changed from

==========
Remove unneeded code in NavigatorImpl::OnBeginNavigation.

This got added in r451507, but I don't see it being hit for the layout test
referenced in it. I also don't see that code being reached in any browser or
layout test (see https://codereview.chromium.org/2884383002). I can confirm the
change in the original cl to NavigatorImpl::RequestNavigation is what fixes the
layout test.

BUG=691168,705119
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove unneeded code in NavigatorImpl::OnBeginNavigation.

This got added in r451507, but I don't see it being hit for the layout test
referenced in it. I also don't see that code being reached in any browser or
layout test (see https://codereview.chromium.org/2884383002). I can confirm the
change in the original cl to NavigatorImpl::RequestNavigation is what fixes the
layout test.

BUG=691168,705119
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[ananta, 2017-05-16 21:31:41]

ananta@chromium.org changed reviewers:
+ japhet@chromium.org

[ananta, 2017-05-16 21:32:01]

+japhet

[jam, 2017-05-17 00:04:38]

FYI I synced to r451507, and I can confirm that deleting this code also didn't
affect the layout test in question. Only the latter bit of r451507 (not deleted
here) was needed.

[jam, 2017-05-17 00:35:48]

On 2017/05/17 00:04:38, jam wrote:
> FYI I synced to r451507, and I can confirm that deleting this code also didn't
> affect the layout test in question. Only the latter bit of r451507 (not
deleted
> here) was needed.

Ananta pointed out in person that either of the two different additions in
r451507 fix the issue by themselves. He mentioned that perhaps Charlie asked for
both because a race condition could trigger the first one?

[commit-bot: I haz the power, 2017-05-17 04:01:41]

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

[commit-bot: I haz the power, 2017-05-17 04:01:43]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2017-05-18 21:20:52]

[+nasko, clamy to CC]

On 2017/05/17 00:35:48, jam wrote:
> On 2017/05/17 00:04:38, jam wrote:
> > FYI I synced to r451507, and I can confirm that deleting this code also
didn't
> > affect the layout test in question. Only the latter bit of r451507 (not
> deleted
> > here) was needed.
> 
> Ananta pointed out in person that either of the two different additions in
> r451507 fix the issue by themselves. He mentioned that perhaps Charlie asked
for
> both because a race condition could trigger the first one?

Ok, I've confirmed that this code cannot be removed.  There's a race that will
cause the test to fail in some runs, and I've observed it in practice (when
visiting the test page in a real Chrome build, not in the layout test).  Doing
the wrong thing here means ads will break when you go back, which would be a
regression of https://crbug.com/657896 (P0).

There are two changes in Ananta's CL, and in most cases the test passes if
either one is present.  Only the OnBeginNavigation change is necessary, but the
RequestNavigation change (to skip beforeunload) often makes the test pass in
kind of an unexpected way.  In more detail:

1) The OnBeginNavigation change ensures that we don't ignore an incoming JS
navigation in a subframe while it's in the middle of a history navigation.  We
*want* the JS navigation to win, or some pages (e.g., ads) will break.

2) Meanwhile, the beforeunload change was an attempt to simplify things because
it isn't necessary in the is_history_navigation_in_new_child case.  It had the
effect of speeding up the history navigation, such that it often gets to
PlzNavigate's CommitNavigation stage *before* the JS navigation from the
renderer arrives in OnBeginNavigation.  CommitNavigation destroys the ongoing
NavigationRequest, which means we won't hit the ResetNavigationRequest line
Ananta added to OnBeginNavigation.  Interestingly, the test passes when this
happens anyway, because we have a "interrupted_by_client_redirect" check in
RenderFrameImpl::NavigateInternal which catches this case and ignores the
CommitNavigation IPC.  As a result, the renderer never requests the stream for
the history navigation, and instead the new JS navigation succeeds.

However, the test fails in the case that we *don't* get to the history
navigation's CommitNavigation stage before OnBeginNavigation.  Then we do have
an ongoing NavigationRequest, and we incorrectly ignore the JS navigation and
let the history navigation win.

Due to that, patch set 1 is not LGTM.

The fact that the test usually passes even without the OnBeginNavigation fix
means it's not a perfect test for this bug.  It's probably hard to get a good
test for it given the raciness, but I bet we could find a way to pull it off
with a content_browsertest and TestNavigationManager that stalls certain
requests.

I bet there's also a leak of the stream URL in PlzNavigate in this case, where
it doesn't expect the renderer process to ignore a CommitNavigation IPC.  I'll
file a bug so that we can investigate it.

Given that we need this code, is there a way to update it for your goals in
https://crbug.com/705119?

[Charlie Reis, 2017-05-18 21:32:54]

Filed https://crbug.com/724271 for the test and https://crbug.com/724275 for the
leak.  Thanks for bringing it to our attention!