PlzNavigate: Invoke didFailProvisionalLoad() in the renderer when a navigation request is cancelled…

PlzNavigate: Invoke didFailProvisionalLoad() in the renderer when a navigation request is cancelled or fails

Navigation requests can be cancelled or they could fail. In the latter case we do send an IPC FrameMsg_FailedNavigation
to the renderer where we display error pages etc. However we don't invoke didFailProvisionalLoad() which causes a number
of blink layout tests to fail. In any case we send out a didStartProvisionalLoad() notification in blink when plznavigate
handles navigation requests. We can safely call didFailProvisionalLoad() notification when the navigation fails.

For cancelled navigations like external URLs etc we invoke the didFailProvisionalLoad() handler for client side navigations in FrameLoader::stopAllLoaders()

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

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

[ananta, 2017-02-08 01:06:52]

Description was changed from

==========
PlzNavigate: Invoke didFailProvisionalLoad() in the renderer when a navigation
request is cancelled or fails

Navigation requests can be cancelled or they could fail. In the latter case we
do send an IPC FrameMsg_FailedNavigation
to the renderer where we display error pages etc. However we don't invoke
didFailProvisionalLoad() which causes a number
of blink layout tests to fail. In any case we send out a
didStartProvisionalLoad() notification in blink when plznavigate
handles navigation requests. We can safely call didFailProvisionalLoad()
notification when the navigation fails.

For cancelled navigations like external URLs etc we don't inform the renderer.
This patch adds an IPC message FrameMsg_AbortNavigationRequest
for the same. The renderer side handler for this invokes the
didFailProvisionalLoad() function.

BUG=640631
==========

to

==========
PlzNavigate: Invoke didFailProvisionalLoad() in the renderer when a navigation
request is cancelled or fails

Navigation requests can be cancelled or they could fail. In the latter case we
do send an IPC FrameMsg_FailedNavigation
to the renderer where we display error pages etc. However we don't invoke
didFailProvisionalLoad() which causes a number
of blink layout tests to fail. In any case we send out a
didStartProvisionalLoad() notification in blink when plznavigate
handles navigation requests. We can safely call didFailProvisionalLoad()
notification when the navigation fails.

For cancelled navigations like external URLs etc we don't inform the renderer.
This patch adds an IPC message FrameMsg_AbortNavigationRequest
for the same. The renderer side handler for this invokes the
didFailProvisionalLoad() function.

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

[ananta, 2017-02-08 01:08:24]

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

[ananta, 2017-02-08 01:08:45]

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

[commit-bot: I haz the power, 2017-02-08 01:09:43]

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-02-08 01:37:39]

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

[commit-bot: I haz the power, 2017-02-08 01:37:43]

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

[ananta, 2017-02-08 01:45:34]

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

[commit-bot: I haz the power, 2017-02-08 01:46:30]

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-02-08 04:07:01]

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

[commit-bot: I haz the power, 2017-02-08 04:07:05]

Dry run: This issue passed the CQ dry run.

[jam, 2017-02-08 17:05:29]

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

[jam, 2017-02-08 17:05:29]

I defer to Camille or Nasko on this.

[clamy, 2017-02-08 17:13:34]

Thanks! One question below.

https://codereview.chromium.org/2687593002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/2687593002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:1033:
render_frame_host->AbortNavigationRequest(
How many tests fail without this? I'm not terribly fan of adding this. I don't
think we should do an IPC to the renderer to tell it that something was aborted,
especially a browser-initiated navigations.

Instead, would the following work? We send a Stop IPC to the renderer if the
request is renderer-initiated. In the renderer, we have a boolean that records
that the navigation is being handled by the client. Could we fire a
didFailProvisionalLoad with net::ERR_ABORTED when we're told to stop and we
expected the client to be navigating for us?

[ananta, 2017-02-08 22:56:01]

Description was changed from

==========
PlzNavigate: Invoke didFailProvisionalLoad() in the renderer when a navigation
request is cancelled or fails

Navigation requests can be cancelled or they could fail. In the latter case we
do send an IPC FrameMsg_FailedNavigation
to the renderer where we display error pages etc. However we don't invoke
didFailProvisionalLoad() which causes a number
of blink layout tests to fail. In any case we send out a
didStartProvisionalLoad() notification in blink when plznavigate
handles navigation requests. We can safely call didFailProvisionalLoad()
notification when the navigation fails.

For cancelled navigations like external URLs etc we don't inform the renderer.
This patch adds an IPC message FrameMsg_AbortNavigationRequest
for the same. The renderer side handler for this invokes the
didFailProvisionalLoad() function.

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

to

==========
PlzNavigate: Invoke didFailProvisionalLoad() in the renderer when a navigation
request is cancelled or fails

Navigation requests can be cancelled or they could fail. In the latter case we
do send an IPC FrameMsg_FailedNavigation
to the renderer where we display error pages etc. However we don't invoke
didFailProvisionalLoad() which causes a number
of blink layout tests to fail. In any case we send out a
didStartProvisionalLoad() notification in blink when plznavigate
handles navigation requests. We can safely call didFailProvisionalLoad()
notification when the navigation fails.

For cancelled navigations like external URLs etc we invoke the
didFailProvisionalLoad() handler for client side navigations in
FrameLoader::stopAllLoaders()

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

[ananta, 2017-02-08 23:02:19]

https://codereview.chromium.org/2687593002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/2687593002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:1033:
render_frame_host->AbortNavigationRequest(
On 2017/02/08 17:13:34, clamy (slow - catching up) wrote:
> How many tests fail without this? I'm not terribly fan of adding this. I don't
> think we should do an IPC to the renderer to tell it that something was
aborted,
> especially a browser-initiated navigations.
> 
> Instead, would the following work? We send a Stop IPC to the renderer if the
> request is renderer-initiated. In the renderer, we have a boolean that records
> that the navigation is being handled by the client. Could we fire a
> didFailProvisionalLoad with net::ERR_ABORTED when we're told to stop and we
> expected the client to be navigating for us?

Thanks. FrameLoader::stopAllLoaders() now sends failed provisional load
notifications for client side navigations. 

We still need the provisional load failed notification in
RenderFrameImpl::OnFailedNavigation(). I added a check for doing this only for
renderer initiated navigations. This covers the
http\tests\loading\bad-server-subframe.html layout test

[ananta, 2017-02-08 23:03:12]

+nasko for FrameLoader.cpp

[ananta, 2017-02-08 23:17:11]

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

[commit-bot: I haz the power, 2017-02-08 23:17:59]

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

[nasko, 2017-02-08 23:52:21]

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

[nasko, 2017-02-08 23:52:22]

This seems reasonable to me, but I'd defer to clamy@ for final approval.

You also need japhet@ for FrameLoader.cpp, I'm not an owner in Blink-land.

https://codereview.chromium.org/2687593002/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2687593002/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:3508: //
nit: You don't need this extra "// " line.

https://codereview.chromium.org/2687593002/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:5227: // For renderer initiated
navigations, we send out a didFailProvisionalLoad()
nit: Empty line before the comment.

[ananta, 2017-02-08 23:59:21]

https://codereview.chromium.org/2687593002/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2687593002/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:3508: //
On 2017/02/08 23:52:22, nasko (very slow) wrote:
> nit: You don't need this extra "// " line.

Thanks. Removed

https://codereview.chromium.org/2687593002/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:5227: // For renderer initiated
navigations, we send out a didFailProvisionalLoad()
On 2017/02/08 23:52:22, nasko (very slow) wrote:
> nit: Empty line before the comment.

Done.

[ananta, 2017-02-08 23:59:45]

+japhet for FrameLoader.cpp

[ananta, 2017-02-09 00:00:07]

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

[commit-bot: I haz the power, 2017-02-09 00:01:07]

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

[Nate Chapin, 2017-02-09 00:10:07]

I'm not thrilled with sending a didFailProvisionalLoad() when there isn't a
provisionalDataSource(), but it's consistent with the rest of the
m_navigationHandledByClient logic, so LGTM.

https://codereview.chromium.org/2687593002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2687593002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1234: if
(m_isNavigationHandledByClient && client()) {
I'm ~99% confident client() should be guaranteed non-null at this point.

[ananta, 2017-02-09 00:22:46]

https://codereview.chromium.org/2687593002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2687593002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1234: if
(m_isNavigationHandledByClient && client()) {
On 2017/02/09 00:10:07, Nate Chapin wrote:
> I'm ~99% confident client() should be guaranteed non-null at this point.

Thanks. Removed the null check

[ananta, 2017-02-09 01:00:03]

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

[commit-bot: I haz the power, 2017-02-09 01:00:39]

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-02-09 06:14:19]

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

[commit-bot: I haz the power, 2017-02-09 06:14:21]

Dry run: This issue passed the CQ dry run.

[clamy, 2017-02-09 10:15:33]

Thanks! Lgtm.

[ananta, 2017-02-09 19:06:47]

The CQ bit was checked by ananta@chromium.org

[ananta, 2017-02-09 19:06:48]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/2687593002/#ps120001
(title: "Remove null client check")

[commit-bot: I haz the power, 2017-02-09 19:07:35]

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-02-09 19:17:24]

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

[commit-bot: I haz the power, 2017-02-09 19:17:25]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[ananta, 2017-02-09 19:42:15]

The CQ bit was checked by ananta@chromium.org

[ananta, 2017-02-09 19:42:16]

The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org,
japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/2687593002/#ps140001
(title: "Remove null client check")

[commit-bot: I haz the power, 2017-02-09 19:43:05]

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-02-09 21:23:14]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1486669335878830,
"parent_rev": "6cd4cd4c30979c6fc520b090f1b64e3bcd1d0868", "commit_rev":
"a85730c84d15b89b04bf58ed6efda2050c9de619"}

[commit-bot: I haz the power, 2017-02-09 21:23:49]

Description was changed from

==========
PlzNavigate: Invoke didFailProvisionalLoad() in the renderer when a navigation
request is cancelled or fails

Navigation requests can be cancelled or they could fail. In the latter case we
do send an IPC FrameMsg_FailedNavigation
to the renderer where we display error pages etc. However we don't invoke
didFailProvisionalLoad() which causes a number
of blink layout tests to fail. In any case we send out a
didStartProvisionalLoad() notification in blink when plznavigate
handles navigation requests. We can safely call didFailProvisionalLoad()
notification when the navigation fails.

For cancelled navigations like external URLs etc we invoke the
didFailProvisionalLoad() handler for client side navigations in
FrameLoader::stopAllLoaders()

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

to

==========
PlzNavigate: Invoke didFailProvisionalLoad() in the renderer when a navigation
request is cancelled or fails

Navigation requests can be cancelled or they could fail. In the latter case we
do send an IPC FrameMsg_FailedNavigation
to the renderer where we display error pages etc. However we don't invoke
didFailProvisionalLoad() which causes a number
of blink layout tests to fail. In any case we send out a
didStartProvisionalLoad() notification in blink when plznavigate
handles navigation requests. We can safely call didFailProvisionalLoad()
notification when the navigation fails.

For cancelled navigations like external URLs etc we invoke the
didFailProvisionalLoad() handler for client side navigations in
FrameLoader::stopAllLoaders()

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

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

[commit-bot: I haz the power, 2017-02-09 21:23:51]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/a85730c84d15b89b04bf58ed6efd...