Ensure that all code paths which call FrameTreeNode::ResetNavigationRequest set NavigationHandle::G…

Ensure that all code paths which call FrameTreeNode::ResetNavigationRequest set NavigationHandle::GetNetErrorCode().

The places were

1) NavigationRequest::OnRequestRedirected (redirected URL fails FilterURL check)
The non-PlzNavigate code was setting this in ResourceLoader::OnReceivedRedirect. Added NavigationHandleImplBrowserTest.ErrorCodeOnRedirect to test this.

2) NavigationRequest::OnResponseStarted (cross-process commit cancelled by embedder)
Neither PlzNavigate or old code path were setting this. Added this check to existing test CrossSiteTransferTest.NoLeakOnCrossSiteCancel. Fixed PlzNavigate in the above method, and non-Navigate in RenderFrameHostManager::OnCrossSiteResponse.

3) NavigatorImpl::CancelNavigation (navigation aborted case, i.e. beforeunload cancels it)
This codepath is only hit for PlzNavigate, since non-PlzNavigate doesn't create a NavigationHandle until after beforeunload succeeds. Added test in NavigationHandleImplBrowserTest.ErrorCodeOnAbortedNavigation.

4) RenderFrameHostImpl::DispatchBeforeUnload
This is a PlzNavigate-only code path. Added checks to existing test BrowserSideNavigationBrowserTest.UnloadDuringNavigation.

5) RenderFrameHostManager::CancelPendingIfNecessary
Fixed in the above method for PlzNavigate, and in RenderFrameHostImpl::DispatchBeforeUnload for old code path. Added checks to existing test SitePerProcessBrowserTest.RenderViewHostKeepsSwappedOutStateIfPendingRFHDies.

6) RenderFrameHostImpl::FailedNavigation is only called by NavigationRequest::OnRequestFailed which already sets the error code. I've audited all code path leading to this to check that they set an error code, and added a DCHECK to confirm.

7) NavigatorImpl::OnBeginNavigation
This looks like dead code, I've sent https://codereview.chromium.org/2890613002/ to remove it.

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

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

[jam, 2017-05-16 18:09:45]

Description was changed from

==========
Ensure that all code paths which call FrameTreeNode::ResetNavigationRequest set
NavigationHandle::GetNetErrorCode().

The places were

1) NavigationRequest::OnRequestRedirected (redirected URL fails FilterURL check)
The non-PlzNavigate code was setting this in ResourceLoader::OnReceivedRedirect.
Added NavigationHandleImplBrowserTest.ErrorCodeOnRedirect to test this.

2) NavigationRequest::OnResponseStarted (cross-process commit cancelled by
embedder)
Neither PlzNavigate or old code path were setting this. Added this check to
existing test CrossSiteTransferTest.NoLeakOnCrossSiteCancel. Fixed PlzNavigate
in the above method, and non-Navigate in
RenderFrameHostManager::OnCrossSiteResponse.

3) NavigatorImpl::CancelNavigation (navigation aborted case, i.e. beforeunload
cancels it)
This codepath is only hit for PlzNavigate, since non-PlzNavigate doesn't create
a NavigationHandle until after beforeunload succeeds. Added test in
NavigationHandleImplBrowserTest.ErrorCodeOnAbortedNavigation.

4) RenderFrameHostImpl::DispatchBeforeUnload
This is a PlzNavigate-only code path. Added checks to existing test
BrowserSideNavigationBrowserTest.UnloadDuringNavigation.

5) RenderFrameHostManager::CancelPendingIfNecessary
Fixed in the above method for PlzNavigate, and in
RenderFrameHostImpl::DispatchBeforeUnload for old code path. Added checks to
existing test
SitePerProcessBrowserTest.RenderViewHostKeepsSwappedOutStateIfPendingRFHDies.

6) RenderFrameHostImpl::FailedNavigation is only called by
NavigationRequest::OnRequestFailed which already sets the error code. I've
audited all code path leading to this to check that they set an error code, and
added a DCHECK to confirm.

7) NavigatorImpl::OnBeginNavigation
I can't actually get this code path to be hit, even with the layout test that
this code was supposed to fix. I still added a call to set the error code.

BUG=705119

review comments
==========

to

==========
Ensure that all code paths which call FrameTreeNode::ResetNavigationRequest set
NavigationHandle::GetNetErrorCode().

The places were

1) NavigationRequest::OnRequestRedirected (redirected URL fails FilterURL check)
The non-PlzNavigate code was setting this in ResourceLoader::OnReceivedRedirect.
Added NavigationHandleImplBrowserTest.ErrorCodeOnRedirect to test this.

2) NavigationRequest::OnResponseStarted (cross-process commit cancelled by
embedder)
Neither PlzNavigate or old code path were setting this. Added this check to
existing test CrossSiteTransferTest.NoLeakOnCrossSiteCancel. Fixed PlzNavigate
in the above method, and non-Navigate in
RenderFrameHostManager::OnCrossSiteResponse.

3) NavigatorImpl::CancelNavigation (navigation aborted case, i.e. beforeunload
cancels it)
This codepath is only hit for PlzNavigate, since non-PlzNavigate doesn't create
a NavigationHandle until after beforeunload succeeds. Added test in
NavigationHandleImplBrowserTest.ErrorCodeOnAbortedNavigation.

4) RenderFrameHostImpl::DispatchBeforeUnload
This is a PlzNavigate-only code path. Added checks to existing test
BrowserSideNavigationBrowserTest.UnloadDuringNavigation.

5) RenderFrameHostManager::CancelPendingIfNecessary
Fixed in the above method for PlzNavigate, and in
RenderFrameHostImpl::DispatchBeforeUnload for old code path. Added checks to
existing test
SitePerProcessBrowserTest.RenderViewHostKeepsSwappedOutStateIfPendingRFHDies.

6) RenderFrameHostImpl::FailedNavigation is only called by
NavigationRequest::OnRequestFailed which already sets the error code. I've
audited all code path leading to this to check that they set an error code, and
added a DCHECK to confirm.

7) NavigatorImpl::OnBeginNavigation
I can't actually get this code path to be hit, even with the layout test that
this code was supposed to fix. I still added a call to set the error code.

BUG=705119

review comments
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[jam, 2017-05-16 18:10:53]

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

[commit-bot: I haz the power, 2017-05-16 18:11:41]

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-05-16 18:18:12]

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

[commit-bot: I haz the power, 2017-05-16 18:18:13]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[jam, 2017-05-16 18:19:06]

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

[jam, 2017-05-16 18:19:15]

Patchset #1 (id:1) has been deleted

[jam, 2017-05-16 18:19:35]

Description was changed from

==========
Ensure that all code paths which call FrameTreeNode::ResetNavigationRequest set
NavigationHandle::GetNetErrorCode().

The places were

1) NavigationRequest::OnRequestRedirected (redirected URL fails FilterURL check)
The non-PlzNavigate code was setting this in ResourceLoader::OnReceivedRedirect.
Added NavigationHandleImplBrowserTest.ErrorCodeOnRedirect to test this.

2) NavigationRequest::OnResponseStarted (cross-process commit cancelled by
embedder)
Neither PlzNavigate or old code path were setting this. Added this check to
existing test CrossSiteTransferTest.NoLeakOnCrossSiteCancel. Fixed PlzNavigate
in the above method, and non-Navigate in
RenderFrameHostManager::OnCrossSiteResponse.

3) NavigatorImpl::CancelNavigation (navigation aborted case, i.e. beforeunload
cancels it)
This codepath is only hit for PlzNavigate, since non-PlzNavigate doesn't create
a NavigationHandle until after beforeunload succeeds. Added test in
NavigationHandleImplBrowserTest.ErrorCodeOnAbortedNavigation.

4) RenderFrameHostImpl::DispatchBeforeUnload
This is a PlzNavigate-only code path. Added checks to existing test
BrowserSideNavigationBrowserTest.UnloadDuringNavigation.

5) RenderFrameHostManager::CancelPendingIfNecessary
Fixed in the above method for PlzNavigate, and in
RenderFrameHostImpl::DispatchBeforeUnload for old code path. Added checks to
existing test
SitePerProcessBrowserTest.RenderViewHostKeepsSwappedOutStateIfPendingRFHDies.

6) RenderFrameHostImpl::FailedNavigation is only called by
NavigationRequest::OnRequestFailed which already sets the error code. I've
audited all code path leading to this to check that they set an error code, and
added a DCHECK to confirm.

7) NavigatorImpl::OnBeginNavigation
I can't actually get this code path to be hit, even with the layout test that
this code was supposed to fix. I still added a call to set the error code.

BUG=705119

review comments
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Ensure that all code paths which call FrameTreeNode::ResetNavigationRequest set
NavigationHandle::GetNetErrorCode().

The places were

1) NavigationRequest::OnRequestRedirected (redirected URL fails FilterURL check)
The non-PlzNavigate code was setting this in ResourceLoader::OnReceivedRedirect.
Added NavigationHandleImplBrowserTest.ErrorCodeOnRedirect to test this.

2) NavigationRequest::OnResponseStarted (cross-process commit cancelled by
embedder)
Neither PlzNavigate or old code path were setting this. Added this check to
existing test CrossSiteTransferTest.NoLeakOnCrossSiteCancel. Fixed PlzNavigate
in the above method, and non-Navigate in
RenderFrameHostManager::OnCrossSiteResponse.

3) NavigatorImpl::CancelNavigation (navigation aborted case, i.e. beforeunload
cancels it)
This codepath is only hit for PlzNavigate, since non-PlzNavigate doesn't create
a NavigationHandle until after beforeunload succeeds. Added test in
NavigationHandleImplBrowserTest.ErrorCodeOnAbortedNavigation.

4) RenderFrameHostImpl::DispatchBeforeUnload
This is a PlzNavigate-only code path. Added checks to existing test
BrowserSideNavigationBrowserTest.UnloadDuringNavigation.

5) RenderFrameHostManager::CancelPendingIfNecessary
Fixed in the above method for PlzNavigate, and in
RenderFrameHostImpl::DispatchBeforeUnload for old code path. Added checks to
existing test
SitePerProcessBrowserTest.RenderViewHostKeepsSwappedOutStateIfPendingRFHDies.

6) RenderFrameHostImpl::FailedNavigation is only called by
NavigationRequest::OnRequestFailed which already sets the error code. I've
audited all code path leading to this to check that they set an error code, and
added a DCHECK to confirm.

7) NavigatorImpl::OnBeginNavigation
I can't actually get this code path to be hit, even with the layout test that
this code was supposed to fix. I still added a call to set the error code.

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

[commit-bot: I haz the power, 2017-05-16 18:20:04]

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-05-16 19:28:21]

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

[commit-bot: I haz the power, 2017-05-16 19:28:21]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

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

Description was changed from

==========
Ensure that all code paths which call FrameTreeNode::ResetNavigationRequest set
NavigationHandle::GetNetErrorCode().

The places were

1) NavigationRequest::OnRequestRedirected (redirected URL fails FilterURL check)
The non-PlzNavigate code was setting this in ResourceLoader::OnReceivedRedirect.
Added NavigationHandleImplBrowserTest.ErrorCodeOnRedirect to test this.

2) NavigationRequest::OnResponseStarted (cross-process commit cancelled by
embedder)
Neither PlzNavigate or old code path were setting this. Added this check to
existing test CrossSiteTransferTest.NoLeakOnCrossSiteCancel. Fixed PlzNavigate
in the above method, and non-Navigate in
RenderFrameHostManager::OnCrossSiteResponse.

3) NavigatorImpl::CancelNavigation (navigation aborted case, i.e. beforeunload
cancels it)
This codepath is only hit for PlzNavigate, since non-PlzNavigate doesn't create
a NavigationHandle until after beforeunload succeeds. Added test in
NavigationHandleImplBrowserTest.ErrorCodeOnAbortedNavigation.

4) RenderFrameHostImpl::DispatchBeforeUnload
This is a PlzNavigate-only code path. Added checks to existing test
BrowserSideNavigationBrowserTest.UnloadDuringNavigation.

5) RenderFrameHostManager::CancelPendingIfNecessary
Fixed in the above method for PlzNavigate, and in
RenderFrameHostImpl::DispatchBeforeUnload for old code path. Added checks to
existing test
SitePerProcessBrowserTest.RenderViewHostKeepsSwappedOutStateIfPendingRFHDies.

6) RenderFrameHostImpl::FailedNavigation is only called by
NavigationRequest::OnRequestFailed which already sets the error code. I've
audited all code path leading to this to check that they set an error code, and
added a DCHECK to confirm.

7) NavigatorImpl::OnBeginNavigation
I can't actually get this code path to be hit, even with the layout test that
this code was supposed to fix. I still added a call to set the error code.

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

to

==========
Ensure that all code paths which call FrameTreeNode::ResetNavigationRequest set
NavigationHandle::GetNetErrorCode().

The places were

1) NavigationRequest::OnRequestRedirected (redirected URL fails FilterURL check)
The non-PlzNavigate code was setting this in ResourceLoader::OnReceivedRedirect.
Added NavigationHandleImplBrowserTest.ErrorCodeOnRedirect to test this.

2) NavigationRequest::OnResponseStarted (cross-process commit cancelled by
embedder)
Neither PlzNavigate or old code path were setting this. Added this check to
existing test CrossSiteTransferTest.NoLeakOnCrossSiteCancel. Fixed PlzNavigate
in the above method, and non-Navigate in
RenderFrameHostManager::OnCrossSiteResponse.

3) NavigatorImpl::CancelNavigation (navigation aborted case, i.e. beforeunload
cancels it)
This codepath is only hit for PlzNavigate, since non-PlzNavigate doesn't create
a NavigationHandle until after beforeunload succeeds. Added test in
NavigationHandleImplBrowserTest.ErrorCodeOnAbortedNavigation.

4) RenderFrameHostImpl::DispatchBeforeUnload
This is a PlzNavigate-only code path. Added checks to existing test
BrowserSideNavigationBrowserTest.UnloadDuringNavigation.

5) RenderFrameHostManager::CancelPendingIfNecessary
Fixed in the above method for PlzNavigate, and in
RenderFrameHostImpl::DispatchBeforeUnload for old code path. Added checks to
existing test
SitePerProcessBrowserTest.RenderViewHostKeepsSwappedOutStateIfPendingRFHDies.

6) RenderFrameHostImpl::FailedNavigation is only called by
NavigationRequest::OnRequestFailed which already sets the error code. I've
audited all code path leading to this to check that they set an error code, and
added a DCHECK to confirm.

7) NavigatorImpl::OnBeginNavigation
I can't actually get this code path to be hit, even with the layout test that
this code was supposed to fix (it's also not triggered through any test or
browser test, see https://codereview.chromium.org/2884383002). I still added a
call to set the error code.

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

[jam, 2017-05-16 20:12:07]

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

[commit-bot: I haz the power, 2017-05-16 20:13:35]

Dry run: CQ is trying da patch.

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

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

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

[commit-bot: I haz the power, 2017-05-16 20:22:04]

Dry run: CQ is trying da patch.

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

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

Description was changed from

==========
Ensure that all code paths which call FrameTreeNode::ResetNavigationRequest set
NavigationHandle::GetNetErrorCode().

The places were

1) NavigationRequest::OnRequestRedirected (redirected URL fails FilterURL check)
The non-PlzNavigate code was setting this in ResourceLoader::OnReceivedRedirect.
Added NavigationHandleImplBrowserTest.ErrorCodeOnRedirect to test this.

2) NavigationRequest::OnResponseStarted (cross-process commit cancelled by
embedder)
Neither PlzNavigate or old code path were setting this. Added this check to
existing test CrossSiteTransferTest.NoLeakOnCrossSiteCancel. Fixed PlzNavigate
in the above method, and non-Navigate in
RenderFrameHostManager::OnCrossSiteResponse.

3) NavigatorImpl::CancelNavigation (navigation aborted case, i.e. beforeunload
cancels it)
This codepath is only hit for PlzNavigate, since non-PlzNavigate doesn't create
a NavigationHandle until after beforeunload succeeds. Added test in
NavigationHandleImplBrowserTest.ErrorCodeOnAbortedNavigation.

4) RenderFrameHostImpl::DispatchBeforeUnload
This is a PlzNavigate-only code path. Added checks to existing test
BrowserSideNavigationBrowserTest.UnloadDuringNavigation.

5) RenderFrameHostManager::CancelPendingIfNecessary
Fixed in the above method for PlzNavigate, and in
RenderFrameHostImpl::DispatchBeforeUnload for old code path. Added checks to
existing test
SitePerProcessBrowserTest.RenderViewHostKeepsSwappedOutStateIfPendingRFHDies.

6) RenderFrameHostImpl::FailedNavigation is only called by
NavigationRequest::OnRequestFailed which already sets the error code. I've
audited all code path leading to this to check that they set an error code, and
added a DCHECK to confirm.

7) NavigatorImpl::OnBeginNavigation
I can't actually get this code path to be hit, even with the layout test that
this code was supposed to fix (it's also not triggered through any test or
browser test, see https://codereview.chromium.org/2884383002). I still added a
call to set the error code.

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

to

==========
Ensure that all code paths which call FrameTreeNode::ResetNavigationRequest set
NavigationHandle::GetNetErrorCode().

The places were

1) NavigationRequest::OnRequestRedirected (redirected URL fails FilterURL check)
The non-PlzNavigate code was setting this in ResourceLoader::OnReceivedRedirect.
Added NavigationHandleImplBrowserTest.ErrorCodeOnRedirect to test this.

2) NavigationRequest::OnResponseStarted (cross-process commit cancelled by
embedder)
Neither PlzNavigate or old code path were setting this. Added this check to
existing test CrossSiteTransferTest.NoLeakOnCrossSiteCancel. Fixed PlzNavigate
in the above method, and non-Navigate in
RenderFrameHostManager::OnCrossSiteResponse.

3) NavigatorImpl::CancelNavigation (navigation aborted case, i.e. beforeunload
cancels it)
This codepath is only hit for PlzNavigate, since non-PlzNavigate doesn't create
a NavigationHandle until after beforeunload succeeds. Added test in
NavigationHandleImplBrowserTest.ErrorCodeOnAbortedNavigation.

4) RenderFrameHostImpl::DispatchBeforeUnload
This is a PlzNavigate-only code path. Added checks to existing test
BrowserSideNavigationBrowserTest.UnloadDuringNavigation.

5) RenderFrameHostManager::CancelPendingIfNecessary
Fixed in the above method for PlzNavigate, and in
RenderFrameHostImpl::DispatchBeforeUnload for old code path. Added checks to
existing test
SitePerProcessBrowserTest.RenderViewHostKeepsSwappedOutStateIfPendingRFHDies.

6) RenderFrameHostImpl::FailedNavigation is only called by
NavigationRequest::OnRequestFailed which already sets the error code. I've
audited all code path leading to this to check that they set an error code, and
added a DCHECK to confirm.

7) NavigatorImpl::OnBeginNavigation
This looks like dead code, I've sent https://codereview.chromium.org/2890613002/
to remove it.

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

[commit-bot: I haz the power, 2017-05-16 22:06:23]

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

[commit-bot: I haz the power, 2017-05-16 22:06:24]

Dry run: Try jobs failed on following builders:
  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-05-16 22:39:43]

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

[jam, 2017-05-17 00:34:47]

Patchset #2 (id:40001) has been deleted

[nasko, 2017-05-18 05:14:15]

LGTM with a few nits.

https://codereview.chromium.org/2889703002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right):

https://codereview.chromium.org/2889703002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:1053: //
renderer can't access, the right error code is set on the navigation handle.
nit: s/navigation handle/NavigationHandle/

https://codereview.chromium.org/2889703002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:1059:
url.host(),
nit: url.host() is redundant here.

https://codereview.chromium.org/2889703002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:1071: // right
error code is set on the navigation handle.
s/navigation handle/NavigationHandle/

https://codereview.chromium.org/2889703002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:1076: if
(!IsBrowserSideNavigationEnabled())
Hmm, I wonder if this behavior could be explaining the slowness in back/forward
navigations. Just thinking aloud, nothing for the CL :).

https://codereview.chromium.org/2889703002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:1082: GURL
new_url("about:blank");
Does this need to be about:blank? It is a special URL with a bunch of corner
cases, so if we can use another URL  ("/title1.html") it will be better.

[jam, 2017-05-18 16:00:33]

https://codereview.chromium.org/2889703002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right):

https://codereview.chromium.org/2889703002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:1053: //
renderer can't access, the right error code is set on the navigation handle.
On 2017/05/18 05:14:15, nasko (out) wrote:
> nit: s/navigation handle/NavigationHandle/

Done.

https://codereview.chromium.org/2889703002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:1059:
url.host(),
On 2017/05/18 05:14:15, nasko (out) wrote:
> nit: url.host() is redundant here.

Done.

https://codereview.chromium.org/2889703002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:1071: // right
error code is set on the navigation handle.
On 2017/05/18 05:14:14, nasko (out) wrote:
> s/navigation handle/NavigationHandle/

Done.

https://codereview.chromium.org/2889703002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:1076: if
(!IsBrowserSideNavigationEnabled())
On 2017/05/18 05:14:14, nasko (out) wrote:
> Hmm, I wonder if this behavior could be explaining the slowness in
back/forward
> navigations. Just thinking aloud, nothing for the CL :).

by behavior you mean that we create a NH? Curious what you mean would be the
slow part?

[jam, 2017-05-18 16:01:31]

The CQ bit was checked by jam@chromium.org

[jam, 2017-05-18 16:01:32]

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

[commit-bot: I haz the power, 2017-05-18 16:02:29]

CQ is trying da patch.

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

[nasko, 2017-05-18 16:04:25]

On 2017/05/18 16:00:33, jam wrote:
>
https://codereview.chromium.org/2889703002/diff/60001/content/browser/frame_h...
> content/browser/frame_host/navigation_handle_impl_browsertest.cc:1076: if
> (!IsBrowserSideNavigationEnabled())
> On 2017/05/18 05:14:14, nasko (out) wrote:
> > Hmm, I wonder if this behavior could be explaining the slowness in
> back/forward
> > navigations. Just thinking aloud, nothing for the CL :).
> 
> by behavior you mean that we create a NH? Curious what you mean would be the
> slow part?

clamy@ and I have been chatting about whether the navigation start timing is
correctly set in PlzNavigate. Poking around that code it looks like we create
the navigation timing parameters before creating the handle and asking for the
beforeunload handler to be run. This is not the case without PlzNavigate, so the
next step should be looking at whether we use the same points in time for the
timing data, which is what the metrics are based on. If we are capturing the
start time for browser initiated navigations (which history ones are) before we
execute the beforeunload, but we capture it afterwards without PlzNavigate, this
would explain why metrics say back/forward is slower (takes more time due to the
longer measurement), while in everyday usage it doesn't seem that it is
preceivable.

[jam, 2017-05-18 16:13:42]

On 2017/05/18 16:04:25, nasko wrote:
> On 2017/05/18 16:00:33, jam wrote:
> >
>
https://codereview.chromium.org/2889703002/diff/60001/content/browser/frame_h...
> > content/browser/frame_host/navigation_handle_impl_browsertest.cc:1076: if
> > (!IsBrowserSideNavigationEnabled())
> > On 2017/05/18 05:14:14, nasko (out) wrote:
> > > Hmm, I wonder if this behavior could be explaining the slowness in
> > back/forward
> > > navigations. Just thinking aloud, nothing for the CL :).
> > 
> > by behavior you mean that we create a NH? Curious what you mean would be the
> > slow part?
> 
> clamy@ and I have been chatting about whether the navigation start timing is
> correctly set in PlzNavigate. Poking around that code it looks like we create
> the navigation timing parameters before creating the handle and asking for the
> beforeunload handler to be run. This is not the case without PlzNavigate, so
the
> next step should be looking at whether we use the same points in time for the
> timing data, which is what the metrics are based on. If we are capturing the
> start time for browser initiated navigations (which history ones are) before
we
> execute the beforeunload, but we capture it afterwards without PlzNavigate,
this
> would explain why metrics say back/forward is slower (takes more time due to
the
> longer measurement), while in everyday usage it doesn't seem that it is
> preceivable.

ah, ok thanks for the explanation. since we have UMA for how long it takes to
execute beforeunload handler, seems like we can see if that is roughly the delta
between plznav and non-plznav forward/backward navigations?

[nasko, 2017-05-18 16:33:06]

On 2017/05/18 16:13:42, jam wrote:
> On 2017/05/18 16:04:25, nasko wrote:
> > On 2017/05/18 16:00:33, jam wrote:
> > >
> >
>
https://codereview.chromium.org/2889703002/diff/60001/content/browser/frame_h...
> > > content/browser/frame_host/navigation_handle_impl_browsertest.cc:1076: if
> > > (!IsBrowserSideNavigationEnabled())
> > > On 2017/05/18 05:14:14, nasko (out) wrote:
> > > > Hmm, I wonder if this behavior could be explaining the slowness in
> > > back/forward
> > > > navigations. Just thinking aloud, nothing for the CL :).
> > > 
> > > by behavior you mean that we create a NH? Curious what you mean would be
the
> > > slow part?
> > 
> > clamy@ and I have been chatting about whether the navigation start timing is
> > correctly set in PlzNavigate. Poking around that code it looks like we
create
> > the navigation timing parameters before creating the handle and asking for
the
> > beforeunload handler to be run. This is not the case without PlzNavigate, so
> the
> > next step should be looking at whether we use the same points in time for
the
> > timing data, which is what the metrics are based on. If we are capturing the
> > start time for browser initiated navigations (which history ones are) before
> we
> > execute the beforeunload, but we capture it afterwards without PlzNavigate,
> this
> > would explain why metrics say back/forward is slower (takes more time due to
> the
> > longer measurement), while in everyday usage it doesn't seem that it is
> > preceivable.
> 
> ah, ok thanks for the explanation. since we have UMA for how long it takes to
> execute beforeunload handler, seems like we can see if that is roughly the
delta
> between plznav and non-plznav forward/backward navigations?

The UMA I've looked at discounts the execution time of the beforeunload handler,
as it can be variable. It measures the roundtrip cost to the renderer process. I
will look around to see if we have other metric that might cover that, or just
read the navigation timing code and see :).

[commit-bot: I haz the power, 2017-05-18 21:24:54]

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

[commit-bot: I haz the power, 2017-05-18 21:24:57]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[jam, 2017-05-18 23:18:51]

The CQ bit was checked by jam@chromium.org

[commit-bot: I haz the power, 2017-05-18 23:20:26]

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-05-19 01:35:54]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495149531533900,
"parent_rev": "926e3b8b4f6523331bb200c6665bb953acc91134", "commit_rev":
"cb4ae15478ddb2f45a6e306a3de4ff53b6b46269"}

[commit-bot: I haz the power, 2017-05-19 01:36:08]

Description was changed from

==========
Ensure that all code paths which call FrameTreeNode::ResetNavigationRequest set
NavigationHandle::GetNetErrorCode().

The places were

1) NavigationRequest::OnRequestRedirected (redirected URL fails FilterURL check)
The non-PlzNavigate code was setting this in ResourceLoader::OnReceivedRedirect.
Added NavigationHandleImplBrowserTest.ErrorCodeOnRedirect to test this.

2) NavigationRequest::OnResponseStarted (cross-process commit cancelled by
embedder)
Neither PlzNavigate or old code path were setting this. Added this check to
existing test CrossSiteTransferTest.NoLeakOnCrossSiteCancel. Fixed PlzNavigate
in the above method, and non-Navigate in
RenderFrameHostManager::OnCrossSiteResponse.

3) NavigatorImpl::CancelNavigation (navigation aborted case, i.e. beforeunload
cancels it)
This codepath is only hit for PlzNavigate, since non-PlzNavigate doesn't create
a NavigationHandle until after beforeunload succeeds. Added test in
NavigationHandleImplBrowserTest.ErrorCodeOnAbortedNavigation.

4) RenderFrameHostImpl::DispatchBeforeUnload
This is a PlzNavigate-only code path. Added checks to existing test
BrowserSideNavigationBrowserTest.UnloadDuringNavigation.

5) RenderFrameHostManager::CancelPendingIfNecessary
Fixed in the above method for PlzNavigate, and in
RenderFrameHostImpl::DispatchBeforeUnload for old code path. Added checks to
existing test
SitePerProcessBrowserTest.RenderViewHostKeepsSwappedOutStateIfPendingRFHDies.

6) RenderFrameHostImpl::FailedNavigation is only called by
NavigationRequest::OnRequestFailed which already sets the error code. I've
audited all code path leading to this to check that they set an error code, and
added a DCHECK to confirm.

7) NavigatorImpl::OnBeginNavigation
This looks like dead code, I've sent https://codereview.chromium.org/2890613002/
to remove it.

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

to

==========
Ensure that all code paths which call FrameTreeNode::ResetNavigationRequest set
NavigationHandle::GetNetErrorCode().

The places were

1) NavigationRequest::OnRequestRedirected (redirected URL fails FilterURL check)
The non-PlzNavigate code was setting this in ResourceLoader::OnReceivedRedirect.
Added NavigationHandleImplBrowserTest.ErrorCodeOnRedirect to test this.

2) NavigationRequest::OnResponseStarted (cross-process commit cancelled by
embedder)
Neither PlzNavigate or old code path were setting this. Added this check to
existing test CrossSiteTransferTest.NoLeakOnCrossSiteCancel. Fixed PlzNavigate
in the above method, and non-Navigate in
RenderFrameHostManager::OnCrossSiteResponse.

3) NavigatorImpl::CancelNavigation (navigation aborted case, i.e. beforeunload
cancels it)
This codepath is only hit for PlzNavigate, since non-PlzNavigate doesn't create
a NavigationHandle until after beforeunload succeeds. Added test in
NavigationHandleImplBrowserTest.ErrorCodeOnAbortedNavigation.

4) RenderFrameHostImpl::DispatchBeforeUnload
This is a PlzNavigate-only code path. Added checks to existing test
BrowserSideNavigationBrowserTest.UnloadDuringNavigation.

5) RenderFrameHostManager::CancelPendingIfNecessary
Fixed in the above method for PlzNavigate, and in
RenderFrameHostImpl::DispatchBeforeUnload for old code path. Added checks to
existing test
SitePerProcessBrowserTest.RenderViewHostKeepsSwappedOutStateIfPendingRFHDies.

6) RenderFrameHostImpl::FailedNavigation is only called by
NavigationRequest::OnRequestFailed which already sets the error code. I've
audited all code path leading to this to check that they set an error code, and
added a DCHECK to confirm.

7) NavigatorImpl::OnBeginNavigation
This looks like dead code, I've sent https://codereview.chromium.org/2890613002/
to remove it.

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

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

[commit-bot: I haz the power, 2017-05-19 01:36:09]

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