PlzNavigate: Show error pages when the navigation failed before commit

content/renderer/render_frame_impl.cc

Index: content/renderer/render_frame_impl.cc
diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc
index c215b0822fefbbdf44443c90f3f20610796301ba..88f4aa062fbfdff258b747153d5c884d28554c6a 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -1045,6 +1045,7 @@ bool RenderFrameImpl::OnMessageReceived(const IPC::Message& msg) {
     IPC_MESSAGE_HANDLER(FrameMsg_DisownOpener, OnDisownOpener)
     IPC_MESSAGE_HANDLER(FrameMsg_CommitNavigation, OnCommitNavigation)
     IPC_MESSAGE_HANDLER(FrameMsg_DidUpdateSandboxFlags, OnDidUpdateSandboxFlags)
+    IPC_MESSAGE_HANDLER(FrameMsg_FailedNavigation, OnFailedNavigation)
 #if defined(OS_ANDROID)
     IPC_MESSAGE_HANDLER(FrameMsg_SelectPopupMenuItems, OnSelectPopupMenuItems)
 #elif defined(OS_MACOSX)
@@ -2498,45 +2499,10 @@ void RenderFrameImpl::didFailProvisionalLoad(blink::WebLocalFrame* frame,
   FOR_EACH_OBSERVER(RenderFrameObserver, observers_,
                     DidFailProvisionalLoad(error));
 
-  bool show_repost_interstitial =
-      (error.reason == net::ERR_CACHE_MISS &&
-       EqualsASCII(failed_request.httpMethod(), "POST"));
+  SendFailedProvisionalLoad(failed_request, error, frame);
 
-  FrameHostMsg_DidFailProvisionalLoadWithError_Params params;
-  params.error_code = error.reason;
-  GetContentClient()->renderer()->GetNavigationErrorStrings(
-      render_view_.get(),
-      frame,
-      failed_request,
-      error,
-      NULL,
-      &params.error_description);
-  params.url = error.unreachableURL;
-  params.showing_repost_interstitial = show_repost_interstitial;
-  Send(new FrameHostMsg_DidFailProvisionalLoadWithError(
-      routing_id_, params));
-
-  // Don't display an error page if this is simply a cancelled load.  Aside
-  // from being dumb, WebCore doesn't expect it and it will cause a crash.
-  if (error.reason == net::ERR_ABORTED)
-    return;
-
-  // Don't display "client blocked" error page if browser has asked us not to.
-  if (error.reason == net::ERR_BLOCKED_BY_CLIENT &&
-      render_view_->renderer_preferences_.disable_client_blocked_error_page) {
+  if (!ShouldDisplayErrorPageForFailedLoad(error.reason, error.unreachableURL))
     return;
-  }
-
-  // Allow the embedder to suppress an error page.
-  if (GetContentClient()->renderer()->ShouldSuppressErrorPage(this,
-          error.unreachableURL)) {
-    return;
-  }
-
-  if (RenderThreadImpl::current() &&
-      RenderThreadImpl::current()->layout_test_mode()) {
-    return;
-  }
 
   // Make sure we never show errors in view source mode.
   frame->enableViewSourceMode(false);
@@ -4085,6 +4051,53 @@ void RenderFrameImpl::OnCommitNavigation(
                               renderer_navigation_start);
 }
 
+void RenderFrameImpl::OnFailedNavigation(
+    const CommonNavigationParams& common_params,
+    const RequestNavigationParams& request_params,
+    bool has_stale_copy_in_cache,
+    int error_code) {
+  bool is_reload = false;
+  bool is_history_navigation = request_params.page_state.IsValid();
+  WebURLRequest::CachePolicy cache_policy =
+      WebURLRequest::UseProtocolCachePolicy;
+  if (!RenderFrameImpl::PrepareRenderViewForNavigation(
+          common_params.url, is_history_navigation, request_params, &is_reload,
+          &cache_policy)) {
+    return;
+  }
+
+  GetContentClient()->SetActiveURL(common_params.url);
+
+  pending_navigation_params_.reset(new NavigationParams(
+      common_params, StartNavigationParams(), request_params));
+
+  // Inform the browser of the start of the provisional load. This is needed so
+  // that the load is properly tracked by the WebNavigation API.
+  // TODO(clamy): Properly set is_transition_navigation.
+  Send(new FrameHostMsg_DidStartProvisionalLoadForFrame(
+      routing_id_, common_params.url, false));
+
+  // Send the provisional load failure.
+  blink::WebURLError error =
+      CreateWebURLError(common_params.url, has_stale_copy_in_cache, error_code);
+  WebURLRequest failed_request = CreateURLRequestForNavigation(
+      common_params, scoped_ptr<StreamOverrideParameters>(),
+      frame_->isViewSourceModeEnabled());
+  SendFailedProvisionalLoad(failed_request, error, frame_);
+
+  if (!ShouldDisplayErrorPageForFailedLoad(error_code, common_params.url))
+    return;
+
+  // Make sure errors are not shown in view source mode.
+  frame_->enableViewSourceMode(false);
+
+  bool replace =
+      request_params.page_id != -1 ||
+      ui::PageTransitionCoreTypeIs(common_params.transition,
+                                   ui::PAGE_TRANSITION_AUTO_SUBFRAME);

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

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.

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

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]

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! 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).

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

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.

+  LoadNavigationErrorPage(failed_request, error, replace);
+}
+
 WebNavigationPolicy RenderFrameImpl::DecidePolicyForNavigation(
     RenderFrame* render_frame,
     const NavigationPolicyInfo& info) {
@@ -4582,6 +4595,51 @@ void RenderFrameImpl::LoadDataURL(const CommonNavigationParams& params,
   }
 }
 
+void RenderFrameImpl::SendFailedProvisionalLoad(
+    const blink::WebURLRequest& request,
+    const blink::WebURLError& error,
+    blink::WebLocalFrame* frame) {
+  bool show_repost_interstitial = (error.reason == net::ERR_CACHE_MISS &&
+                                   EqualsASCII(request.httpMethod(), "POST"));
+
+  FrameHostMsg_DidFailProvisionalLoadWithError_Params params;
+  params.error_code = error.reason;
+  GetContentClient()->renderer()->GetNavigationErrorStrings(
+      render_view_.get(), frame, request, error, NULL,
+      &params.error_description);
+  params.url = error.unreachableURL;
+  params.showing_repost_interstitial = show_repost_interstitial;
+  Send(new FrameHostMsg_DidFailProvisionalLoadWithError(routing_id_, params));
+}
+
+bool RenderFrameImpl::ShouldDisplayErrorPageForFailedLoad(
+    int error_code,
+    const GURL& unreachable_url) {
+  // Don't display an error page if this is simply a cancelled load.  Aside
+  // from being dumb, WebCore doesn't expect it and it will cause a crash.

[Avi (use Gerrit), 2015/04/01 16:45:41]

Blink

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

Done.

+  if (error_code == net::ERR_ABORTED)
+    return false;
+
+  // Don't display "client blocked" error page if browser has asked us not to.
+  if (error_code == net::ERR_BLOCKED_BY_CLIENT &&
+      render_view_->renderer_preferences_.disable_client_blocked_error_page) {
+    return false;
+  }
+
+  // Allow the embedder to suppress an error page.
+  if (GetContentClient()->renderer()->ShouldSuppressErrorPage(
+          this, unreachable_url)) {
+    return false;
+  }
+
+  if (RenderThreadImpl::current() &&
+      RenderThreadImpl::current()->layout_test_mode()) {
+    return false;
+  }
+
+  return true;
+}
+
 GURL RenderFrameImpl::GetLoadingUrl() const {
   WebDataSource* ds = frame_->dataSource();
   if (ds->hasUnreachableURL())
@@ -4646,6 +4704,7 @@ NavigationState* RenderFrameImpl::CreateNavigationStateFromPending() {
   }
   return NavigationStateImpl::CreateContentInitiated();
 }
+
 #if defined(OS_ANDROID)
 
 WebMediaPlayer* RenderFrameImpl::CreateAndroidWebMediaPlayer(