Default typed omnibox navigations to HTTPS: Don't wait forever for HTTPS

When upgrading a typed omnibox navigation to HTTPS, we observe the HTTPS
load and fallback to HTTP if it fails. For slow HTTPS sites, this will
introduce a very user visible delay.

Instead of indefinitely waiting for the HTTPS navigation to succeed or
finish, this CL implements a timeout so that we can cancel the HTTPS
navigation (i.e. the "upgrade") and fall back to HTTP.

Bug: 1141691
Change-Id: I8ec6d4bdd42abd2a9b8f90a802b9280cc0ded6bc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2577633
Commit-Queue: Mustafa Emre Acer <meacer@chromium.org>
Reviewed-by: Chris Thompson <cthomp@chromium.org>
Reviewed-by: Emily Stark <estark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#840353}

chrome/browser/ssl/typed_navigation_upgrade_throttle.cc

@@ -6,6 +6,7 @@
 
 #include "base/check_op.h"
 #include "base/memory/ptr_util.h"
+#include "base/metrics/field_trial_params.h"
 #include "base/metrics/histogram_functions.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/task/post_task.h"
@@ -20,11 +21,19 @@
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_user_data.h"
 #include "ui/base/page_transition_types.h"
-#include "url/gurl.h"
 #include "url/url_constants.h"
 
 namespace {
 
+// Delay in milliseconds before falling back to the HTTP URL.
+// This can be changed in tests.
+// - If the HTTPS load finishes successfully during this time, the timer is
+//   cleared and no more work is done.
+// - Otherwise, a new navigation to the the fallback HTTP URL is started.
+constexpr base::FeatureParam<base::TimeDelta> kFallbackDelay{
+    &omnibox::kDefaultTypedNavigationsToHttps, "timeout",
+    base::TimeDelta::FromSeconds(3)};
+
 bool IsNavigationUsingHttpsAsDefaultScheme(content::NavigationHandle* handle) {
   content::NavigationUIData* ui_data = handle->GetNavigationUIData();
   // UI data can be null in the case of navigations to interstitials.
@@ -54,7 +63,12 @@ class TypedNavigationUpgradeLifetimeHelper
     return weak_factory_.GetWeakPtr();
   }
 
-  void Navigate(const content::OpenURLParams& url_params) {
+  void Navigate(const content::OpenURLParams& url_params,
+                bool stop_navigation) {
+    if (stop_navigation) {
+      // This deletes the NavigationThrottle and NavigationHandle.
+      web_contents_->Stop();
+    }
     web_contents_->OpenURL(url_params);
   }
 
@@ -123,36 +137,47 @@ content::NavigationThrottle::ThrottleCheckResult
 TypedNavigationUpgradeThrottle::WillStartRequest() {
   DCHECK_EQ(url::kHttpsScheme, navigation_handle()->GetURL().scheme());
   RecordUMA(Event::kHttpsLoadStarted);
+  timer_.Start(FROM_HERE, kFallbackDelay.Get(), this,
+               &TypedNavigationUpgradeThrottle::OnHttpsLoadTimeout);
   return content::NavigationThrottle::PROCEED;
 }
 
 content::NavigationThrottle::ThrottleCheckResult
 TypedNavigationUpgradeThrottle::WillFailRequest() {
   DCHECK_EQ(url::kHttpsScheme, navigation_handle()->GetURL().scheme());
-  // Cancel and fall back to HTTP in case of SSL errors or other net/ errors.
+  // Cancel the request, stop the timer and fall back to HTTP in case of SSL
+  // errors or other net/ errors.
+  timer_.Stop();
   // If there was no certificate error, SSLInfo will be empty.
   const net::SSLInfo info =
       navigation_handle()->GetSSLInfo().value_or(net::SSLInfo());
   int cert_status = info.cert_status;
-  if (net::IsCertStatusError(cert_status)) {
-    RecordUMA(Event::kHttpsLoadFailedWithCertError);
-    FallbackToHttp();
-    return content::NavigationThrottle::CANCEL_AND_IGNORE;
+  if (!net::IsCertStatusError(cert_status) &&
+      navigation_handle()->GetNetErrorCode() == net::OK) {
+    return content::NavigationThrottle::PROCEED;
   }
 
-  if (navigation_handle()->GetNetErrorCode() != net::OK) {
+  if (net::IsCertStatusError(cert_status)) {
+    RecordUMA(Event::kHttpsLoadFailedWithCertError);
+  } else if (navigation_handle()->GetNetErrorCode() != net::OK) {
     RecordUMA(Event::kHttpsLoadFailedWithNetError);
-    FallbackToHttp();
-    return content::NavigationThrottle::CANCEL_AND_IGNORE;
   }
-  return content::NavigationThrottle::PROCEED;
+
+  // Fallback to Http without stopping the navigation. The return value of this
+  // method takes care of that, and we don't need to call WebContents::Stop on a
+  // navigation that's already about to fail.
+  FallbackToHttp(false);
+
+  // Do not add any code after here, |this| is deleted.
+  return content::NavigationThrottle::CANCEL_AND_IGNORE;
 }
 
 content::NavigationThrottle::ThrottleCheckResult
 TypedNavigationUpgradeThrottle::WillProcessResponse() {
   DCHECK_EQ(url::kHttpsScheme, navigation_handle()->GetURL().scheme());
-  // If we got here, HTTPS load succeeded.
+  // If we got here, HTTPS load succeeded. Stop the timer.
   RecordUMA(Event::kHttpsLoadSucceeded);
+  timer_.Stop();
   return content::NavigationThrottle::PROCEED;
 }
 
@@ -172,13 +197,25 @@ bool TypedNavigationUpgradeThrottle::
 
 TypedNavigationUpgradeThrottle::TypedNavigationUpgradeThrottle(
     content::NavigationHandle* handle)
-    : content::NavigationThrottle(handle) {}
+    : content::NavigationThrottle(handle),
+      http_url_(GetHttpUrl(handle->GetURL())) {}
+
+void TypedNavigationUpgradeThrottle::OnHttpsLoadTimeout() {
+  RecordUMA(Event::kHttpsLoadTimedOut);
+  // Stop the current navigation and load the HTTP URL. We explicitly stop the
+  // navigation here as opposed to WillFailRequest because the timeout happens
+  // in the middle of a navigation where we can't return a ThrottleCheckResult.
+  FallbackToHttp(true);
+
+  // Once the fallback navigation starts, |this| will be deleted. Be careful
+  // adding code here -- any async task posted hereafter may never run.
+}
 
-void TypedNavigationUpgradeThrottle::FallbackToHttp() {
-  const GURL http_url = GetHttpUrl(navigation_handle()->GetURL());
+void TypedNavigationUpgradeThrottle::FallbackToHttp(bool stop_navigation) {
+  DCHECK_EQ(url::kHttpScheme, http_url_.scheme()) << http_url_;
   content::OpenURLParams params =
       content::OpenURLParams::FromNavigationHandle(navigation_handle());
-  params.url = http_url;
+  params.url = http_url_;
 
   content::WebContents* web_contents = navigation_handle()->GetWebContents();
   // According to crbug.com/1058303, web_contents could be null but we don't
@@ -195,7 +232,12 @@ void TypedNavigationUpgradeThrottle::FallbackToHttp() {
   TypedNavigationUpgradeLifetimeHelper::CreateForWebContents(web_contents);
   TypedNavigationUpgradeLifetimeHelper* helper =
       TypedNavigationUpgradeLifetimeHelper::FromWebContents(web_contents);
+
   base::SequencedTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::BindOnce(&TypedNavigationUpgradeLifetimeHelper::Navigate,
-                                helper->GetWeakPtr(), std::move(params)));
+      FROM_HERE,
+      base::BindOnce(&TypedNavigationUpgradeLifetimeHelper::Navigate,
+                     helper->GetWeakPtr(), std::move(params), stop_navigation));
+
+  // Once the fallback navigation starts, |this| will be deleted. Be careful
+  // adding code here -- any async task posted hereafter may never run.
 }

chrome/browser/ssl/typed_navigation_upgrade_throttle.h

@@ -7,7 +7,9 @@
 
 #include <memory>
 
+#include "base/timer/timer.h"
 #include "content/public/browser/navigation_throttle.h"
+#include "url/gurl.h"
 
 namespace content {
 class NavigationHandle;
@@ -30,7 +32,10 @@ class TypedNavigationUpgradeThrottle : public content::NavigationThrottle {
     // Failed to load the upgraded HTTPS URL because of a net error, fell back
     // to the HTTP URL.
     kHttpsLoadFailedWithNetError,
-    kMaxValue = kHttpsLoadFailedWithNetError,
+    // Failed to load the upgraded HTTPS URL within the timeout window, fell
+    // back to the HTTP URL.
+    kHttpsLoadTimedOut,
+    kMaxValue = kHttpsLoadTimedOut,
   };
 
   static std::unique_ptr<content::NavigationThrottle> MaybeCreateThrottleFor(
@@ -61,9 +66,15 @@ class TypedNavigationUpgradeThrottle : public content::NavigationThrottle {
   TypedNavigationUpgradeThrottle& operator=(
       const TypedNavigationUpgradeThrottle&) = delete;
 
-  // Stops the current navigation and initiates a new navigation to the HTTP
-  // version of the original navigation's URL.
-  void FallbackToHttp();
+  void OnHttpsLoadTimeout();
+
+  // Initiates a new navigation to the HTTP version of the original navigation's
+  // URL. If |stop_navigation| is true, also stops any pending navigation in the
+  // current WebContents.
+  void FallbackToHttp(bool stop_navigation);
+
+  const GURL http_url_;
+  base::OneShotTimer timer_;
 };
 
 #endif  // CHROME_BROWSER_SSL_TYPED_NAVIGATION_UPGRADE_THROTTLE_H_