PlzNavigate: Show error pages when the navigation failed before commit

PlzNavigate: Show error pages when the navigation failed before commit

This CL enables showing error pages when browser-side navigation is enabled. It
introduces a new IPC FrameMsg_FailedNavigation used by the browser to ask a
renderer to display an appropriate error page for a navigation that just
failed.

BUG=459033
Committed: https://crrev.com/62b271de77990e9cb5c530c8a27ff41f21f9b1b3
Cr-Commit-Position: refs/heads/master@{#325425}

[clamy, 2015-03-31 15:40:26]

clamy@chromium.org changed reviewers:
+ creis@chromium.org, davidben@chromium.org, nasko@chromium.org

[clamy, 2015-03-31 15:40:27]

@creis, davidben: PTAL
@nasko: FYI

This is the patch to show error pages in PlzNavigate when failing before
navigation commit. I verified that it works by navigating Chrome to a page that
returns an error. I plan on adding some more unittests, but would like an
opinion on how the patch looks as it is now.

[clamy, 2015-03-31 15:41:07]

clamy@chromium.org changed reviewers:
+ carlosk@chromium.org, fdegans@chromium.org

[clamy, 2015-03-31 15:41:21]

+carlosk, fdegans: FYI

[Charlie Reis, 2015-03-31 23:16:40]

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

[Charlie Reis, 2015-03-31 23:16:40]

[+avi]

Sorry, I didn't get a chance to review this today.  However, Avi has been making
changes to DidFailProvisionalLoad this week, so I think it'd be worthwhile for
him to take a look.

I can try to review after that.

[clamy, 2015-04-01 11:57:46]

Patchset #6 (id:100001) has been deleted

[Avi (use Gerrit), 2015-04-01 16:45:42]

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

https://codereview.chromium.org/958083002/diff/120001/content/renderer/render...
content/renderer/render_frame_impl.cc:4097: ui::PAGE_TRANSITION_AUTO_SUBFRAME);
This definition of replace is wrong when we use it in didFailProvisionalLoad;
see http://crbug.com/471292 and my attempt to fix it in
https://codereview.chromium.org/1048963002/.

It seems to me that it's wrong here too. First, we're trying to kill page id, so
we shouldn't be using it here in the first place, and second, since we're
basically faking a load failure, common_params.transition wouldn't be set here.
(The AUTO_SUBFRAME value tested in didFailProvisionalLoad seems to be set in
didStartProvisionalLoad, and since we're bypassing it, would it ever be set?)

The definition of "replace" here is: If it was a history navigation or some
other navigation that was not creating a new history entry, then replace should
be true, else false. Given that OnFailedNavigation is triggered by an IPC from
the browser process, and the browser process *knows* what kind of navigation
just failed, we should have the browser process tell us the replacement value,
not try to deduce it ourselves in the renderer process.

https://codereview.chromium.org/958083002/diff/120001/content/renderer/render...
content/renderer/render_frame_impl.cc:4619: // from being dumb, WebCore doesn't
expect it and it will cause a crash.
Blink

[Avi (use Gerrit), 2015-04-01 18:08:09]

As a side note, you don't need to commit the browsertest from
https://codereview.chromium.org/1048963002/ (I'm still working on it) but you
might want to run it locally when working on this area.

[clamy, 2015-04-02 08:21:10]

Thanks! One question on your comment.

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

https://codereview.chromium.org/958083002/diff/120001/content/renderer/render...
content/renderer/render_frame_impl.cc:4097: ui::PAGE_TRANSITION_AUTO_SUBFRAME);
On 2015/04/01 16:45:42, Avi wrote:
> This definition of replace is wrong when we use it in didFailProvisionalLoad;
> see http://crbug.com/471292 and my attempt to fix it in
> https://codereview.chromium.org/1048963002/.
> 
> It seems to me that it's wrong here too. First, we're trying to kill page id,
so
> we shouldn't be using it here in the first place, and second, since we're
> basically faking a load failure, common_params.transition wouldn't be set
here.
> (The AUTO_SUBFRAME value tested in didFailProvisionalLoad seems to be set in
> didStartProvisionalLoad, and since we're bypassing it, would it ever be set?)
> 
> The definition of "replace" here is: If it was a history navigation or some
> other navigation that was not creating a new history entry, then replace
should
> be true, else false. Given that OnFailedNavigation is triggered by an IPC from
> the browser process, and the browser process *knows* what kind of navigation
> just failed, we should have the browser process tell us the replacement value,
> not try to deduce it ourselves in the renderer process.

Then can we just use should_replace_current_entry (which would move from
StartNavigationParams to RequestNavigationParams that we get here)?

[Avi (use Gerrit), 2015-04-02 15:05:23]

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

https://codereview.chromium.org/958083002/diff/120001/content/renderer/render...
content/renderer/render_frame_impl.cc:4097: ui::PAGE_TRANSITION_AUTO_SUBFRAME);
On 2015/04/02 08:21:10, clamy wrote:
> Then can we just use should_replace_current_entry (which would move from
> StartNavigationParams to RequestNavigationParams that we get here)?

should_replace_current_entry is a browser-side flag to treat a NEW_PAGE
navigation by the renderer as if it were replacing the current entry. The
browser is keeping the renderer informed with that flag, and the renderer can
use it to keep bookkeeping up-to-date, but the renderer should not be making
navigation decisions with that flag.

I've been working in this area in https://codereview.chromium.org/1048963002/.
In previous patch sets I tried using should_replace_current_entry to make
navigation decisions in the renderer, and it led to nothing but repeated
headaches, heartaches, and pain. Definitely not recommended. So in patch set 3 I
settled on

bool replace = commit_type != blink::WebStandardCommit;

which seems to be the right thing to do, because it makes error loads navigate
in the same way as normal loads.

In this CL (your CL), we're bypassing Blink to load an error page, so we need to
set |replace| to mean "a new navigation entry would have been created". Perhaps
the right thing to do is set it if we're going to a new nav entry? The problem I
have with that is the whole "reload via hitting enter" scenario, where a pending
nav entry wouldn't be an accurate sign. (This possibly calls for the "turn it
into a reload" refactor which Charlie was pondering.)

I'm off on holiday today and tomorrow. My recommendation is to try the new
browsertest that I added in my CL, and try to make it work with this CL, as that
test exercises the error page codepath. I'll think about this further and let
you know.

[clamy, 2015-04-08 12:53:49]

Thanks!

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

https://codereview.chromium.org/958083002/diff/120001/content/renderer/render...
content/renderer/render_frame_impl.cc:4097: ui::PAGE_TRANSITION_AUTO_SUBFRAME);
On 2015/04/02 15:05:23, Avi wrote:
> On 2015/04/02 08:21:10, clamy wrote:
> > Then can we just use should_replace_current_entry (which would move from
> > StartNavigationParams to RequestNavigationParams that we get here)?
> 
> should_replace_current_entry is a browser-side flag to treat a NEW_PAGE
> navigation by the renderer as if it were replacing the current entry. The
> browser is keeping the renderer informed with that flag, and the renderer can
> use it to keep bookkeeping up-to-date, but the renderer should not be making
> navigation decisions with that flag.
> 
> I've been working in this area in https://codereview.chromium.org/1048963002/.
> In previous patch sets I tried using should_replace_current_entry to make
> navigation decisions in the renderer, and it led to nothing but repeated
> headaches, heartaches, and pain. Definitely not recommended. So in patch set 3
I
> settled on
> 
> bool replace = commit_type != blink::WebStandardCommit;
> 
> which seems to be the right thing to do, because it makes error loads navigate
> in the same way as normal loads.
> 
> In this CL (your CL), we're bypassing Blink to load an error page, so we need
to
> set |replace| to mean "a new navigation entry would have been created".
Perhaps
> the right thing to do is set it if we're going to a new nav entry? The problem
I
> have with that is the whole "reload via hitting enter" scenario, where a
pending
> nav entry wouldn't be an accurate sign. (This possibly calls for the "turn it
> into a reload" refactor which Charlie was pondering.)
> 
> I'm off on holiday today and tomorrow. My recommendation is to try the new
> browsertest that I added in my CL, and try to make it work with this CL, as
that
> test exercises the error page codepath. I'll think about this further and let
> you know.

Thanks! I've tried to emulate what blink::WebStandardCommit means based on the
information received from the browser. Unfortunately, the browser test times out
with PlzNavigate enabled so I could not use ti to test the code (I suspect it's
because we don't send the load events properly and it interferes with the helper
functions).

https://codereview.chromium.org/958083002/diff/120001/content/renderer/render...
content/renderer/render_frame_impl.cc:4619: // from being dumb, WebCore doesn't
expect it and it will cause a crash.
On 2015/04/01 16:45:41, Avi wrote:
> Blink

Done.

[Avi (use Gerrit), 2015-04-08 14:12:13]

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

https://codereview.chromium.org/958083002/diff/120001/content/renderer/render...
content/renderer/render_frame_impl.cc:4097: ui::PAGE_TRANSITION_AUTO_SUBFRAME);
On 2015/04/08 12:53:49, clamy wrote:
> Unfortunately, the browser test times out
> with PlzNavigate enabled so I could not use ti to test the code

Can you manually test?

Type aslkdfjalskdjflasdkjflaskdjf.com (or another failing URL) into the address
bar, and tap return to load (and fail). Keep clicking in the address bar over
and over, and hitting return over and over to keep retrying the load. No matter
how many times you do that, it shouldn't create a new entry.

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

https://codereview.chromium.org/958083002/diff/160001/content/renderer/render...
content/renderer/render_frame_impl.cc:4081: (frame_->parent() &&
request_params.page_id == -1);
:(

Can we please not add more page id uses? I am _this_close_ to killing it.

[clamy, 2015-04-08 15:02:22]

Thanks! I tested manually that spurious entries are not created when hitting
enter in the address bar with the same address or hitting reload. This required
checking the url against the loading url of the frame for the adress bar case.

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

https://codereview.chromium.org/958083002/diff/160001/content/renderer/render...
content/renderer/render_frame_impl.cc:4081: (frame_->parent() &&
request_params.page_id == -1);
On 2015/04/08 14:12:13, Avi wrote:
> :(
> 
> Can we please not add more page id uses? I am _this_close_ to killing it.

It's for the case of subframes, so I'm not completely confident what to do there
yet. Removed it and added a TODO instead.

[Avi (use Gerrit), 2015-04-08 15:22:11]

LGTM; this looks reasonable.

It is all going to have to change to fix http://crbug.com/474261 anyway.

[Avi (use Gerrit), 2015-04-08 15:22:30]

On 2015/04/08 15:22:11, Avi wrote:
> LGTM; this looks reasonable.
> 
> It is all going to have to change to fix http://crbug.com/474261 anyway.

Also, get Charlie to look this over too.

[clamy, 2015-04-08 15:23:36]

Thanks!
@creis: PTAL

[Charlie Reis, 2015-04-09 00:07:30]

Thanks, seems reasonable to me as well.  (I'm not very familiar with the
existing error page logic, but it looks like it should work.)  Can you add a
test?

LGTM apart from that and some nits.

https://codereview.chromium.org/958083002/diff/180001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/958083002/diff/180001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:206: void
NavigationRequest::OnRequestFailed(bool in_cache, int net_error) {
Please use the same name as in Navigator (has_stale_copy_in_cache).  These names
sound like they mean very different things.

https://codereview.chromium.org/958083002/diff/180001/content/browser/frame_h...
File content/browser/frame_host/navigator.h (right):

https://codereview.chromium.org/958083002/diff/180001/content/browser/frame_h...
content/browser/frame_host/navigator.h:148: // page.
Please add what has_stale_copy_in_cache means.  Is there a copy of the page in
the cache?  Of the error page?  Why does it matter?

https://codereview.chromium.org/958083002/diff/180001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_delegate.h (right):

https://codereview.chromium.org/958083002/diff/180001/content/browser/loader/...
content/browser/loader/navigation_url_loader_delegate.h:41: virtual void
OnRequestFailed(bool in_cache, int net_error) = 0;
Same.

https://codereview.chromium.org/958083002/diff/180001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/958083002/diff/180001/content/common/frame_me...
content/common/frame_messages.h:543: int /* error_code */)
Shouldn't this comment line up with the others, as in request_params on the
struct above?  Or we could match the other structs where the comments aren't
lined up.  Let's pick one or the other, though.

[clamy, 2015-04-10 13:28:40]

Patchset #11 (id:220001) has been deleted

[clamy, 2015-04-10 14:04:50]

Thanks! I added a simple browser test to exercise the navigation failure.

https://codereview.chromium.org/958083002/diff/180001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/958083002/diff/180001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:206: void
NavigationRequest::OnRequestFailed(bool in_cache, int net_error) {
On 2015/04/09 00:07:30, Charlie Reis (slow until 4-15) wrote:
> Please use the same name as in Navigator (has_stale_copy_in_cache).  These
names
> sound like they mean very different things.

Done.

https://codereview.chromium.org/958083002/diff/180001/content/browser/frame_h...
File content/browser/frame_host/navigator.h (right):

https://codereview.chromium.org/958083002/diff/180001/content/browser/frame_h...
content/browser/frame_host/navigator.h:148: // page.
On 2015/04/09 00:07:30, Charlie Reis (slow until 4-15) wrote:
> Please add what has_stale_copy_in_cache means.  Is there a copy of the page in
> the cache?  Of the error page?  Why does it matter?

Done. This is needed by the WebURLError, which is used by embedders of content.

https://codereview.chromium.org/958083002/diff/180001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_delegate.h (right):

https://codereview.chromium.org/958083002/diff/180001/content/browser/loader/...
content/browser/loader/navigation_url_loader_delegate.h:41: virtual void
OnRequestFailed(bool in_cache, int net_error) = 0;
On 2015/04/09 00:07:30, Charlie Reis (slow until 4-15) wrote:
> Same.

Done.

https://codereview.chromium.org/958083002/diff/180001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/958083002/diff/180001/content/common/frame_me...
content/common/frame_messages.h:543: int /* error_code */)
On 2015/04/09 00:07:30, Charlie Reis (slow until 4-15) wrote:
> Shouldn't this comment line up with the others, as in request_params on the
> struct above?  Or we could match the other structs where the comments aren't
> lined up.  Let's pick one or the other, though.

Done. Weirdly this is produced by git cl format.

https://codereview.chromium.org/958083002/diff/240001/content/browser/browser...
File content/browser/browser_side_navigation_browsertest.cc (right):

https://codereview.chromium.org/958083002/diff/240001/content/browser/browser...
content/browser/browser_side_navigation_browsertest.cc:176: GURL
url("http://127.0.0.1:4");
The url was chosen to get a network error (otherwise the test server returns a
404 which does not result in a navigation failure).

https://codereview.chromium.org/958083002/diff/240001/content/public/test/tes...
File content/public/test/test_navigation_observer.h (right):

https://codereview.chromium.org/958083002/diff/240001/content/public/test/tes...
content/public/test/test_navigation_observer.h:89: int navigations_failed_;
I had to track the number of navigations that failed since loading the error
page results in a CommitProvisionalLoad message which makes it appear as if the
last navigation succeeded when it did not. This may be due to the fact that
PlzNavigate does not call DidStart/StopLoading properly.

[Charlie Reis, 2015-04-10 22:54:44]

https://codereview.chromium.org/958083002/diff/240001/content/browser/browser...
File content/browser/browser_side_navigation_browsertest.cc (right):

https://codereview.chromium.org/958083002/diff/240001/content/browser/browser...
content/browser/browser_side_navigation_browsertest.cc:176: GURL
url("http://127.0.0.1:4");
On 2015/04/10 14:04:49, clamy wrote:
> The url was chosen to get a network error (otherwise the test server returns a
> 404 which does not result in a navigation failure).

I think we have better ways to do this.  Please see
NavigationControllerBrowserTest.ErrorPageReplacement:

  GURL error_url(
      net::URLRequestFailedJob::GetMockHttpUrl(net::ERR_CONNECTION_RESET));
  net::URLRequestFailedJob::AddUrlHandler();

(I'm not 100% sure how that works, but I think this is what it's for.)

https://codereview.chromium.org/958083002/diff/240001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_delegate.h (right):

https://codereview.chromium.org/958083002/diff/240001/content/browser/loader/...
content/browser/loader/navigation_url_loader_delegate.h:41: // copy of the
unreachable page in cache.
nit: Unexpected newline after stale

https://codereview.chromium.org/958083002/diff/240001/content/public/test/tes...
File content/public/test/test_navigation_observer.h (right):

https://codereview.chromium.org/958083002/diff/240001/content/public/test/tes...
content/public/test/test_navigation_observer.h:89: int navigations_failed_;
On 2015/04/10 14:04:49, clamy wrote:
> I had to track the number of navigations that failed since loading the error
> page results in a CommitProvisionalLoad message which makes it appear as if
the
> last navigation succeeded when it did not. This may be due to the fact that
> PlzNavigate does not call DidStart/StopLoading properly.

Can you check the page type of the committed NavigationEntry instead?  Something
like this from NavigationControllerBrowserTest.ErrorPageReplacement:

EXPECT_EQ(PAGE_TYPE_ERROR, entry->GetPageType());

That might make changing TestNavigationObserver unnecessary.

[clamy, 2015-04-14 11:56:25]

Thanks! The test has been updated.
@nasko: PTAL a look at the changes in frame_messages

https://codereview.chromium.org/958083002/diff/240001/content/browser/browser...
File content/browser/browser_side_navigation_browsertest.cc (right):

https://codereview.chromium.org/958083002/diff/240001/content/browser/browser...
content/browser/browser_side_navigation_browsertest.cc:176: GURL
url("http://127.0.0.1:4");
On 2015/04/10 22:54:44, Charlie Reis (slow until 4-15) wrote:
> On 2015/04/10 14:04:49, clamy wrote:
> > The url was chosen to get a network error (otherwise the test server returns
a
> > 404 which does not result in a navigation failure).
> 
> I think we have better ways to do this.  Please see
> NavigationControllerBrowserTest.ErrorPageReplacement:
> 
>   GURL error_url(
>       net::URLRequestFailedJob::GetMockHttpUrl(net::ERR_CONNECTION_RESET));
>   net::URLRequestFailedJob::AddUrlHandler();
> 
> (I'm not 100% sure how that works, but I think this is what it's for.)

Done.

https://codereview.chromium.org/958083002/diff/240001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_delegate.h (right):

https://codereview.chromium.org/958083002/diff/240001/content/browser/loader/...
content/browser/loader/navigation_url_loader_delegate.h:41: // copy of the
unreachable page in cache.
On 2015/04/10 22:54:44, Charlie Reis (slow until 4-15) wrote:
> nit: Unexpected newline after stale

Done.

https://codereview.chromium.org/958083002/diff/240001/content/public/test/tes...
File content/public/test/test_navigation_observer.h (right):

https://codereview.chromium.org/958083002/diff/240001/content/public/test/tes...
content/public/test/test_navigation_observer.h:89: int navigations_failed_;
On 2015/04/10 22:54:44, Charlie Reis (slow until 4-15) wrote:
> On 2015/04/10 14:04:49, clamy wrote:
> > I had to track the number of navigations that failed since loading the error
> > page results in a CommitProvisionalLoad message which makes it appear as if
> the
> > last navigation succeeded when it did not. This may be due to the fact that
> > PlzNavigate does not call DidStart/StopLoading properly.
> 
> Can you check the page type of the committed NavigationEntry instead? 
Something
> like this from NavigationControllerBrowserTest.ErrorPageReplacement:
> 
> EXPECT_EQ(PAGE_TYPE_ERROR, entry->GetPageType());
> 
> That might make changing TestNavigationObserver unnecessary.

Done.

[nasko, 2015-04-15 21:34:32]

LGTM

[Charlie Reis, 2015-04-15 21:37:43]

Thanks-- still LGTM.

[clamy, 2015-04-16 09:44:27]

The CQ bit was checked by clamy@chromium.org

[clamy, 2015-04-16 09:44:27]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org
Link to the patchset: https://codereview.chromium.org/958083002/#ps280001
(title: "Updated test")

[commit-bot: I haz the power, 2015-04-16 09:44:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/958083002/280001

[commit-bot: I haz the power, 2015-04-16 11:55:04]

Committed patchset #13 (id:280001)

[commit-bot: I haz the power, 2015-04-16 11:56:01]

Patchset 13 (id:??) landed as
https://crrev.com/62b271de77990e9cb5c530c8a27ff41f21f9b1b3
Cr-Commit-Position: refs/heads/master@{#325425}