Add a new NavigationThrottle for HTTP-error navigations with empty body

Previously, error page navigations initiated by the renderer will commit
synchronously and only notify the browser at DidCommit time, making it
have to create a NavigationRequest for the navigation at DidCommit time.
One of the cases that do that is main-frame navigations with error HTTP
status code and an empty response body.

This CL makes it so that we detect that case in the browser side instead
of the renderer side by adding a NavigationThrottle that will defer main
frame navigations with an error HTTP status code until we can determine
if its response body is empty or not, and commit an error page instead
of the original (empty) page if so.

For more context:
doc: https://docs.google.com/document/d/1hf7b7OWlJMCpfFBk3xgxzHnskyNx3X8qjNWg6M1hr_0/edit

navigation-dev thread: https://groups.google.com/a/chromium.org/g/navigation-dev/c/WbNkf2alpPU/m/2tQQ-cXWBgAJ

Bug: 1133115
Change-Id: I09998edd984b86bad6294edc4c83a32c31e0197f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2487024
Commit-Queue: Rakina Zata Amni <rakina@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Emily Stark <estark@chromium.org>
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#823093}

android_webview/browser/aw_content_browser_client.cc

@@ -1067,6 +1067,10 @@ AwContentBrowserClient::CreateSpeechRecognitionManagerDelegate() {
   return new AwSpeechRecognitionManagerDelegate();
 }
 
+bool AwContentBrowserClient::HasErrorPage(int http_status_code) {
+  return http_status_code >= 400;
+}
+
 // static
 void AwContentBrowserClient::DisableCreatingThreadPool() {
   g_should_create_thread_pool = false;

android_webview/browser/aw_content_browser_client.h

@@ -225,14 +225,14 @@ class AwContentBrowserClient : public content::ContentBrowserClient {
                                    blink::mojom::WebFeature feature) override;
   bool IsOriginTrialRequiredForAppCache(
       content::BrowserContext* browser_text) override;
+  content::SpeechRecognitionManagerDelegate*
+  CreateSpeechRecognitionManagerDelegate() override;
+  bool HasErrorPage(int http_status_code) override;
 
   AwFeatureListCreator* aw_feature_list_creator() {
     return aw_feature_list_creator_;
   }
 
-  content::SpeechRecognitionManagerDelegate*
-  CreateSpeechRecognitionManagerDelegate() override;
-
   static void DisableCreatingThreadPool();
 
  private:

android_webview/renderer/aw_content_renderer_client.cc

@@ -177,10 +177,6 @@ void AwContentRendererClient::RenderViewCreated(
   AwRenderViewExt::RenderViewCreated(render_view);
 }
 
-bool AwContentRendererClient::HasErrorPage(int http_status_code) {
-  return http_status_code >= 400;
-}
-
 void AwContentRendererClient::PrepareErrorPage(
     content::RenderFrame* render_frame,
     const blink::WebURLError& error,

android_webview/renderer/aw_content_renderer_client.h

@@ -37,7 +37,6 @@ class AwContentRendererClient : public content::ContentRendererClient,
   void ExposeInterfacesToBrowser(mojo::BinderMap* binders) override;
   void RenderFrameCreated(content::RenderFrame* render_frame) override;
   void RenderViewCreated(content::RenderView* render_view) override;
-  bool HasErrorPage(int http_status_code) override;
   void PrepareErrorPage(content::RenderFrame* render_frame,
                         const blink::WebURLError& error,
                         const std::string& http_method,

chrome/browser/chrome_content_browser_client.cc

@@ -209,7 +209,9 @@
 #include "components/dom_distiller/core/dom_distiller_switches.h"
 #include "components/dom_distiller/core/url_constants.h"
 #include "components/embedder_support/switches.h"
+#include "components/error_page/common/error.h"
 #include "components/error_page/common/error_page_switches.h"
+#include "components/error_page/common/localized_error.h"
 #include "components/error_page/content/browser/net_error_auto_reloader.h"
 #include "components/feature_engagement/public/feature_constants.h"
 #include "components/feature_engagement/public/feature_list.h"
@@ -5863,3 +5865,9 @@ void ChromeContentBrowserClient::GetHyphenationDictionary(
       GetHyphenationDictionary(std::move(callback));
 #endif
 }
+
+bool ChromeContentBrowserClient::HasErrorPage(int http_status_code) {
+  // Use an internal error page, if we have one for the status code.
+  return error_page::LocalizedError::HasStrings(
+      error_page::Error::kHttpErrorDomain, http_status_code);
+}

chrome/browser/chrome_content_browser_client.h

@@ -705,6 +705,7 @@ class ChromeContentBrowserClient : public content::ContentBrowserClient {
 
   void GetHyphenationDictionary(
       base::OnceCallback<void(const base::FilePath&)>) override;
+  bool HasErrorPage(int http_status_code) override;
 
   StartupData* startup_data() { return &startup_data_; }
 

chrome/browser/ui/login/login_handler_browsertest.cc

@@ -147,8 +147,11 @@ void TestProxyAuth(Browser* browser, const GURL& test_page) {
     auth_cancelled_waiter.Wait();
     reload_observer.Wait();
     if (https) {
-      EXPECT_EQ(true, content::EvalJs(contents, "document.body === null"));
+      EXPECT_EQ(
+          "<head></head><body></body>",
+          content::EvalJs(contents, "document.documentElement.innerHTML"));
     }
+
     EXPECT_FALSE(browser->location_bar_model()->GetFormattedFullURL().empty());
   }
 

chrome/browser/ui/login/login_tab_helper.cc

@@ -162,6 +162,21 @@ LoginTabHelper::WillProcessMainFrameUnauthorizedResponse(
     // because the navigation entry ID will change once the refresh finishes.
     navigation_handle_id_with_cancelled_prompt_ =
         navigation_handle->GetNavigationId();
+
+    int response_code =
+        navigation_handle->GetResponseHeaders()->response_code();
+    // For HTTPS navigations with 407 responses, we want to show an empty
+    // page. We need to cancel the navigation and commit an empty error
+    // page directly here, because otherwise the HttpErrorNavigationThrottle
+    // will see that the response body is empty (because the network stack
+    // refuses to read the response body) and will try to commit a generic
+    // non-empty error page instead.
+    if (navigation_handle->GetURL().SchemeIs(url::kHttpsScheme) &&
+        response_code ==
+            net::HttpStatusCode::HTTP_PROXY_AUTHENTICATION_REQUIRED) {
+      return {content::NavigationThrottle::CANCEL,
+              net::ERR_INVALID_AUTH_CREDENTIALS, "<html></html>"};
+    }
     return content::NavigationThrottle::PROCEED;
   }
 

chrome/renderer/chrome_content_renderer_client.cc

@@ -1206,12 +1206,6 @@ void ChromeContentRendererClient::ReportNaClAppType(bool is_pnacl,
 }
 #endif  // BUILDFLAG(ENABLE_NACL)
 
-bool ChromeContentRendererClient::HasErrorPage(int http_status_code) {
-  // Use an internal error page, if we have one for the status code.
-  return error_page::LocalizedError::HasStrings(
-      error_page::Error::kHttpErrorDomain, http_status_code);
-}
-
 void ChromeContentRendererClient::PrepareErrorPage(
     content::RenderFrame* render_frame,
     const blink::WebURLError& web_error,
@@ -1232,14 +1226,13 @@ void ChromeContentRendererClient::PrepareErrorPage(
 
 void ChromeContentRendererClient::PrepareErrorPageForHttpStatusError(
     content::RenderFrame* render_frame,
-    const GURL& unreachable_url,
+    const blink::WebURLError& error,
     const std::string& http_method,
     int http_status,
     std::string* error_html) {
   NetErrorHelper::Get(render_frame)
-      ->PrepareErrorPage(
-          error_page::Error::HttpError(unreachable_url, http_status),
-          http_method == "POST", error_html);
+      ->PrepareErrorPage(error_page::Error::HttpError(error.url(), http_status),
+                         http_method == "POST", error_html);
 }
 
 void ChromeContentRendererClient::PostIOThreadCreated(

chrome/renderer/chrome_content_renderer_client.h

@@ -104,13 +104,12 @@ class ChromeContentRendererClient
   blink::WebPlugin* CreatePluginReplacement(
       content::RenderFrame* render_frame,
       const base::FilePath& plugin_path) override;
-  bool HasErrorPage(int http_status_code) override;
   void PrepareErrorPage(content::RenderFrame* render_frame,
                         const blink::WebURLError& error,
                         const std::string& http_method,
                         std::string* error_html) override;
   void PrepareErrorPageForHttpStatusError(content::RenderFrame* render_frame,
-                                          const GURL& unreachable_url,
+                                          const blink::WebURLError& error,
                                           const std::string& http_method,
                                           int http_status,
                                           std::string* error_html) override;

components/error_page/content/browser/net_error_auto_reloader.cc

@@ -51,7 +51,10 @@ bool ShouldAutoReload(content::NavigationHandle* handle) {
          // TODO(crbug.com/1016164): Explore how to allow reloads for secure DNS
          // network errors without interfering with the captive portal probe
          // state.
-         !handle->GetResolveErrorInfo().is_secure_network_error;
+         !handle->GetResolveErrorInfo().is_secure_network_error &&
+         // Don't auto reload if the error is caused by the server returning a
+         // non-2xx HTTP response code.
+         net_error != net::ERR_HTTP_RESPONSE_CODE_FAILURE;
 }
 
 base::TimeDelta GetNextReloadDelay(size_t reload_count) {

components/prerender/browser/prerender_contents.cc

@@ -441,21 +441,16 @@ void PrerenderContents::DidFinishLoad(
 void PrerenderContents::DidFinishNavigation(
     content::NavigationHandle* navigation_handle) {
   if (!navigation_handle->IsInMainFrame() ||
-      !navigation_handle->HasCommitted() || navigation_handle->IsErrorPage()) {
+      !navigation_handle->HasCommitted()) {
     return;
   }
 
-  if (navigation_handle->GetResponseHeaders() &&
-      navigation_handle->GetResponseHeaders()->response_code() >= 400) {
+  if (navigation_handle->IsErrorPage()) {
     // Maintain same behavior as old navigation API when the URL is unreachable
-    // and leads to an error page. While there will be a subsequent navigation
-    // that has navigation_handle->IsErrorPage(), it'll be too late to wait for
-    // it as the renderer side will consider this prerender complete. This
-    // object would therefore have been destructed already and so instead look
-    // for the error response code now.
-    // Also maintain same final status code that previous navigation API
-    // returned, which was reached because the URL for the error page was
-    // kUnreachableWebDataURL and that was interpreted as unsupported scheme.
+    // and leads to an error page. Also maintain same final status code that
+    // previous navigation API returned, which was reached because the URL for
+    // the error page was kUnreachableWebDataURL and that was interpreted as
+    // unsupported scheme.
     Destroy(FINAL_STATUS_UNSUPPORTED_SCHEME);
     return;
   }

content/browser/BUILD.gn

@@ -1296,6 +1296,8 @@ source_set("browser") {
     "renderer_host/frame_tree_node_blame_context.h",
     "renderer_host/hit_test_debug_key_event_observer.cc",
     "renderer_host/hit_test_debug_key_event_observer.h",
+    "renderer_host/http_error_navigation_throttle.cc",
+    "renderer_host/http_error_navigation_throttle.h",
     "renderer_host/input/fling_controller.cc",
     "renderer_host/input/fling_controller.h",
     "renderer_host/input/fling_scheduler.cc",

content/browser/renderer_host/http_error_navigation_throttle.cc

@@ -0,0 +1,106 @@
+// Copyright 2020 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/browser/renderer_host/http_error_navigation_throttle.h"
+
+#include "content/browser/renderer_host/navigation_request.h"
+#include "content/public/common/content_client.h"
+
+namespace content {
+
+// static
+std::unique_ptr<NavigationThrottle>
+HttpErrorNavigationThrottle::MaybeCreateThrottleFor(
+    NavigationHandle& navigation_handle) {
+  // We only care about main frame navigations.
+  if (!navigation_handle.IsInMainFrame())
+    return nullptr;
+  return base::WrapUnique(new HttpErrorNavigationThrottle(navigation_handle));
+}
+
+HttpErrorNavigationThrottle::HttpErrorNavigationThrottle(
+    NavigationHandle& navigation_handle)
+    : NavigationThrottle(&navigation_handle),
+      task_runner_(base::ThreadTaskRunnerHandle::Get()),
+      body_consumer_watcher_(FROM_HERE,
+                             mojo::SimpleWatcher::ArmingPolicy::MANUAL,
+                             task_runner_) {}
+
+const char* HttpErrorNavigationThrottle::GetNameForLogging() {
+  return "HttpErrorNavigationThrottle";
+}
+
+HttpErrorNavigationThrottle::~HttpErrorNavigationThrottle() = default;
+
+NavigationThrottle::ThrottleCheckResult
+HttpErrorNavigationThrottle::WillProcessResponse() {
+  // We've received the response head, but the response body might not be
+  // readable yet. We should wait for the body, but only if the response might
+  // result in an error page (response_code indicates an error, and the embedder
+  // has a custom error page for it).
+  const network::mojom::URLResponseHead* response =
+      NavigationRequest::From(navigation_handle())->response();
+  DCHECK(response);
+  if (!response->headers)
+    return PROCEED;
+  int response_code = response->headers->response_code();
+  if (response_code < 400 ||
+      !GetContentClient()->browser()->HasErrorPage(response_code)) {
+    return PROCEED;
+  }
+
+  // Defer the navigation until we can determine if the response body is empty
+  // or not, by waiting until it becomes readable or the connection is closed.
+  body_consumer_watcher_.Watch(
+      NavigationRequest::From(navigation_handle())->response_body(),
+      MOJO_HANDLE_SIGNAL_READABLE | MOJO_HANDLE_SIGNAL_PEER_CLOSED,
+      base::BindRepeating(&HttpErrorNavigationThrottle::OnBodyReadable,
+                          base::Unretained(this)));
+  body_consumer_watcher_.ArmOrNotify();
+  return DEFER;
+}
+
+void HttpErrorNavigationThrottle::OnBodyReadable(MojoResult) {
+  const mojo::DataPipeConsumerHandle& body =
+      NavigationRequest::From(navigation_handle())->response_body();
+  // See how many bytes are in the body, without consuming anything from the
+  // response body data pipe.
+  uint32_t num_bytes = 0;
+  MojoResult result =
+      body.ReadData(nullptr, &num_bytes, MOJO_READ_DATA_FLAG_QUERY);
+
+  switch (result) {
+    case MOJO_RESULT_OK:
+      break;
+    case MOJO_RESULT_FAILED_PRECONDITION:
+      // Failed reading the result, can be due to the connection being closed.
+      // We should treat this as a signal that the body is empty.
+      DCHECK_EQ(num_bytes, 0u);
+      break;
+    case MOJO_RESULT_SHOULD_WAIT:
+      // Wait for the next signal to try and read the body again.
+      body_consumer_watcher_.ArmOrNotify();
+      return;
+    default:
+      NOTREACHED();
+      return;
+  }
+
+  // Stop watching for signals.
+  body_consumer_watcher_.Cancel();
+
+  if (num_bytes == 0) {
+    // The response body is empty, so cancel the navigation and commit an error
+    // page instead. The error page's content will be generated in the renderer
+    // at commit time, so we only need to pass the error code in the call below.
+    CancelDeferredNavigation({content::NavigationThrottle::CANCEL,
+                              net::ERR_HTTP_RESPONSE_CODE_FAILURE});
+  } else {
+    // There's at least one byte in the body, which means it's not empty. The
+    // navigation should continue normally.
+    Resume();
+  }
+}
+
+}  // namespace content

content/browser/renderer_host/http_error_navigation_throttle.h

@@ -0,0 +1,44 @@
+// Copyright 2020 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.
+
+#ifndef CONTENT_BROWSER_RENDERER_HOST_HTTP_ERROR_NAVIGATION_THROTTLE_H_
+#define CONTENT_BROWSER_RENDERER_HOST_HTTP_ERROR_NAVIGATION_THROTTLE_H_
+
+#include "content/public/browser/navigation_handle.h"
+#include "content/public/browser/navigation_throttle.h"
+#include "mojo/public/cpp/system/simple_watcher.h"
+
+namespace content {
+
+// If a navigation received a response with a bad HTTP status code, we will
+// still display the contents from the response body sent by the site (e.g. some
+// sites have custom 404 pages). In cases where the response's body is empty,
+// however, we should display an error page if possible (instead of showing a
+// blank page, which might confuse users). This throttle will defer main frame
+// potentially-empty HTTP error navigations until we can determine if its
+// response body is empty or not.
+class HttpErrorNavigationThrottle : public NavigationThrottle {
+ public:
+  static std::unique_ptr<NavigationThrottle> MaybeCreateThrottleFor(
+      NavigationHandle& navigation_handle);
+
+  ~HttpErrorNavigationThrottle() override;
+
+ private:
+  explicit HttpErrorNavigationThrottle(NavigationHandle& navigation_handle);
+
+  // NavigationThrottle overrides.
+  const char* GetNameForLogging() override;
+  ThrottleCheckResult WillProcessResponse() override;
+
+  void OnBodyReadable(MojoResult);
+
+ private:
+  scoped_refptr<base::SequencedTaskRunner> task_runner_;
+  mojo::SimpleWatcher body_consumer_watcher_;
+};
+
+}  // namespace content
+
+#endif  // CONTENT_BROWSER_RENDERER_HOST_HTTP_ERROR_NAVIGATION_THROTTLE_H_

content/browser/renderer_host/navigation_controller_impl.cc

@@ -3355,7 +3355,7 @@ NavigationControllerImpl::CreateNavigationRequestFromLoadParams(
               network::mojom::WebClientHintsType>() /* enabled_client_hints */,
           false /* is_cross_browsing_instance */,
           std::vector<std::string>() /* forced_content_security_policies */,
-          nullptr /* old_page_info */);
+          nullptr /* old_page_info */, -1 /* http_response_code */);
 #if defined(OS_ANDROID)
   if (ValidateDataURLAsString(params.data_url_as_string)) {
     commit_params->data_url_as_string = params.data_url_as_string->data();