PlzNavigate: Show error pages when the navigation failed before commit

content/renderer/render_frame_impl.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/renderer/render_frame_impl.h"
 
 #include <map>
 #include <string>
 
 #include "base/auto_reset.h"
@@ skipping 1027 matching lines @@
     IPC_MESSAGE_HANDLER(FrameMsg_Reload, OnReload)
     IPC_MESSAGE_HANDLER(FrameMsg_TextSurroundingSelectionRequest,
                         OnTextSurroundingSelectionRequest)
     IPC_MESSAGE_HANDLER(FrameMsg_AddStyleSheetByURL,
                         OnAddStyleSheetByURL)
     IPC_MESSAGE_HANDLER(FrameMsg_SetAccessibilityMode,
                         OnSetAccessibilityMode)
     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)
     IPC_MESSAGE_HANDLER(FrameMsg_SelectPopupMenuItem, OnSelectPopupMenuItem)
     IPC_MESSAGE_HANDLER(InputMsg_CopyToFindPboard, OnCopyToFindPboard)
 #endif
   IPC_END_MESSAGE_MAP()
 
   return handled;
 }
@@ skipping 1433 matching lines @@
   //
   // Note: It is important this notification occur before DidStopLoading so the
   //       SSL manager can react to the provisional load failure before being
   //       notified the load stopped.
   //
   FOR_EACH_OBSERVER(RenderViewObserver, render_view_->observers(),
                     DidFailProvisionalLoad(frame, error));
   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)
+  if (!ShouldDisplayErrorPageForFailedLoad(error.reason, error.unreachableURL))
     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) {
-    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);
 
   DocumentState* document_state = DocumentState::FromDataSource(ds);
   NavigationStateImpl* navigation_state =
       static_cast<NavigationStateImpl*>(document_state->navigation_state());
 
   // If this is a failed back/forward/reload navigation, then we need to do a
   // 'replace' load.  This is necessary to avoid messing up session history.
   // Otherwise, we do a normal load, which simulates a 'go' navigation as far
@@ skipping 1527 matching lines @@
   request.setCheckForBrowserSideNavigation(false);
 
   // Record this before starting the load. A lower bound of this time is needed
   // to sanitize the navigationStart override set below.
   base::TimeTicks renderer_navigation_start = base::TimeTicks::Now();
   frame_->loadRequest(request);
   UpdateFrameNavigationTiming(frame_, request_params.browser_navigation_start,
                               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) {
 #ifdef OS_ANDROID
   // The handlenavigation API is deprecated and will be removed once
   // crbug.com/325351 is resolved.
   if (info.urlRequest.url() != GURL(kSwappedOutURL) &&
       GetContentClient()->renderer()->HandleNavigation(
           render_frame,
           static_cast<DocumentState*>(info.extraData),
@@ skipping 477 matching lines @@
         WebString::fromUTF8(charset),
         base_url,
         params.history_url_for_data_url,
         false);
   } else {
     CHECK(false) << "Invalid URL passed: "
                  << params.url.possibly_invalid_spec();
   }
 }
 
+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())
     return ds->unreachableURL();
 
   const WebURLRequest& request = ds->request();
   return request.url();
 }
 
 void RenderFrameImpl::PopulateDocumentStateFromPending(
@@ skipping 44 matching lines @@
   // initiated any load resulting from JS execution.
   if (!pending_navigation_params_->common_params.url.SchemeIs(
           url::kJavaScriptScheme)) {
     return NavigationStateImpl::CreateBrowserInitiated(
         pending_navigation_params_->common_params,
         pending_navigation_params_->start_params,
         pending_navigation_params_->request_params);
   }
   return NavigationStateImpl::CreateContentInitiated();
 }
+
 #if defined(OS_ANDROID)
 
 WebMediaPlayer* RenderFrameImpl::CreateAndroidWebMediaPlayer(
     const blink::WebURL& url,
     WebMediaPlayerClient* client,
     media::MediaPermission* media_permission,
     blink::WebContentDecryptionModule* initial_cdm) {
   GpuChannelHost* gpu_channel_host =
       RenderThreadImpl::current()->EstablishGpuChannelSync(
           CAUSE_FOR_GPU_LAUNCH_VIDEODECODEACCELERATOR_INITIALIZE);
@@ skipping 42 matching lines @@
 
 #if defined(ENABLE_BROWSER_CDMS)
 RendererCdmManager* RenderFrameImpl::GetCdmManager() {
   if (!cdm_manager_)
     cdm_manager_ = new RendererCdmManager(this);
   return cdm_manager_;
 }
 #endif  // defined(ENABLE_BROWSER_CDMS)
 
 }  // namespace content