Ensure that when a new navigation cancels an existing one, the old navigation's DidFinishNavigati...

Ensure that when a new navigation cancels an existing one, the old navigation's DidFinishNavigation callback has an net::ERR_ABORTED error code.

This fixes
UrlOverridingTest#testNavigationWithFallbackURL

BUG=699387
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2768873006
Cr-Commit-Position: refs/heads/master@{#459572}
Committed: https://chromium.googlesource.com/chromium/src/+/cd0b7b2029d532fd338ee0e5e7839987895971ab

[jam, 2017-03-24 18:29:25]

Description was changed from

==========
test with PlzNavigate

BUG=
==========

to

==========
test with PlzNavigate

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[jam, 2017-03-24 18:29:30]

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

[commit-bot: I haz the power, 2017-03-24 18:30:33]

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

[jam, 2017-03-24 18:31:33]

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

[commit-bot: I haz the power, 2017-03-24 18:33:05]

Dry run: 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-03-24 18:37:57]

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

[commit-bot: I haz the power, 2017-03-24 18:37:58]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jam, 2017-03-24 19:20:03]

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

[commit-bot: I haz the power, 2017-03-24 19:20:33]

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

[jam, 2017-03-24 19:21:41]

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

[commit-bot: I haz the power, 2017-03-24 19:22:06]

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

[jam, 2017-03-24 19:34:43]

Patchset #1 (id:1) has been deleted

[jam, 2017-03-24 19:34:48]

Patchset #1 (id:20001) has been deleted

[jam, 2017-03-24 20:00:03]

Description was changed from

==========
test with PlzNavigate

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Ensure that when a new navigation cancels an existing one, the old navigation's
DidFinishNavigation callback has an net::ERR_ABORTED error code.

This fixes
UrlOverridingTest#testNavigationWithFallbackURL

BUG=699387
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[jam, 2017-03-24 20:00:04]

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

[jam, 2017-03-24 20:01:15]

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

[commit-bot: I haz the power, 2017-03-24 20:01:39]

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-03-24 20:11:09]

LGTM, but see question about other ResetNavigationRequest call sites.

https://codereview.chromium.org/2768873006/diff/80001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/2768873006/diff/80001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:389:
navigation_request_->navigation_handle()->set_net_error_code(
I'm curious if this belongs in ResetNavigationRequest, since several other call
sites do this as well.  Looks like there's 16 call sites, so I wonder how many
of the other ones need it.

Looking closer, I suppose CommitNavigation is one that definitely doesn't need
it, and it might be unfortunate to add yet another bool parameter to
ResetNavigationRequest.  Still, I wouldn't be surprised if there are more places
that need this.

[commit-bot: I haz the power, 2017-03-24 20:46:48]

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

[commit-bot: I haz the power, 2017-03-24 20:46:49]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[jam, 2017-03-24 21:04:16]

The CQ bit was checked by jam@chromium.org

[commit-bot: I haz the power, 2017-03-24 21:04:59]

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

[jam, 2017-03-24 22:08:26]

https://codereview.chromium.org/2768873006/diff/80001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/2768873006/diff/80001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:389:
navigation_request_->navigation_handle()->set_net_error_code(
On 2017/03/24 20:11:09, Charlie Reis (OOO soon) wrote:
> I'm curious if this belongs in ResetNavigationRequest, since several other
call
> sites do this as well.  Looks like there's 16 call sites, so I wonder how many
> of the other ones need it.
> 
> Looking closer, I suppose CommitNavigation is one that definitely doesn't need
> it, and it might be unfortunate to add yet another bool parameter to
> ResetNavigationRequest.  Still, I wouldn't be surprised if there are more
places
> that need this.

That's a good question; I didn't see any obvious places in the other callsites,
but I didn't test with and without PlzNavigate to see what happens. Here are the
callsites that don't already have setters, and should probably be audited. I've
filed https://bugs.chromium.org/p/chromium/issues/detail?id=705119

NavigationRequest::OnRequestRedirected
-FilterURL fails

NavigationRequest::OnResponseStarted
-ShouldTransferNavigation returns false
  -this only happens for extensions

NavigatorImpl::OnBeginNavigation
-sends a stop IPC, which when it comes back should set the abort error code 

NavigatorImpl::CancelNavigation
RenderFrameHostImpl::DispatchBeforeUnload
RenderFrameHostManager::CancelPendingIfNecessary
-need to investigate what should happen

RenderFrameHostImpl::FailedNavigation
-should already be setting it today

[commit-bot: I haz the power, 2017-03-24 22:13:10]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490389456323200,
"parent_rev": "65f69d7473623280af7437e3be6836e062ab3984", "commit_rev":
"cd0b7b2029d532fd338ee0e5e7839987895971ab"}

[commit-bot: I haz the power, 2017-03-24 22:13:57]

Description was changed from

==========
Ensure that when a new navigation cancels an existing one, the old navigation's
DidFinishNavigation callback has an net::ERR_ABORTED error code.

This fixes
UrlOverridingTest#testNavigationWithFallbackURL

BUG=699387
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Ensure that when a new navigation cancels an existing one, the old navigation's
DidFinishNavigation callback has an net::ERR_ABORTED error code.

This fixes
UrlOverridingTest#testNavigationWithFallbackURL

BUG=699387
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

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

[commit-bot: I haz the power, 2017-03-24 22:13:59]

Committed patchset #3 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/cd0b7b2029d532fd338ee0e5e783...

[wychen, 2017-03-24 22:39:23]

wychen@chromium.org changed reviewers:
+ wychen@chromium.org

[wychen, 2017-03-24 22:39:23]

The bug ID should be
https://crbug.com/699388