[HTTPS-Upgrades] Fix crash or hang during certain redirects

When HTTPS-Upgrades are applied multiple times across redirects to
different sites within the same navigation, old remote/receiver
instances were being held onto and either triggering a DCHECK or
causing the navigation to hang (in non-DCHECK builds) by failing to continue the next redirect step. This CL removes these DCHECKs and
instead explicitly reset()'s the Remote and Receiver in
HttpsUpgradesInterceptor::RedirectHandler() before re-binding them to
trigger the upgrade redirect.

This also adds a regression test that triggered these DCHECKS (or
navigation hang) before the fix was applied.

Bug: 1431026
Change-Id: If487631e217ad06aefabe72e84c6c138902a5b06
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4418407
Commit-Queue: Chris Thompson <[email protected]>
Reviewed-by: Mustafa Emre Acer <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1129533}

chrome/browser/ssl/https_upgrades_browsertest.cc

@@ -5,6 +5,7 @@
 #include <algorithm>
 #include <vector>
 
+#include "base/strings/strcat.h"
 #include "base/test/bind.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/simple_test_clock.h"
@@ -106,6 +107,9 @@ class HttpsUpgradesBrowserTest
     mock_cert_verifier_.mock_cert_verifier()->AddResultForCertAndHost(
         cert, "bad-https.test", verify_result,
         net::ERR_CERT_COMMON_NAME_INVALID);
+    mock_cert_verifier_.mock_cert_verifier()->AddResultForCertAndHost(
+        cert, "www.bad-https.test", verify_result,
+        net::ERR_CERT_COMMON_NAME_INVALID);
 
     http_server_.AddDefaultHandlers(GetChromeTestDataDir());
     https_server_.AddDefaultHandlers(GetChromeTestDataDir());
@@ -1533,6 +1537,48 @@ IN_PROC_BROWSER_TEST_P(HttpsUpgradesBrowserTest,
   }
 }
 
+// Regression test for crbug.com/1431026. Triggers a navigation where HTTPS
+// upgrades applied multiple times across redirects to different sites.
+// Should not crash when DCHECKS are enabled.
+IN_PROC_BROWSER_TEST_P(HttpsUpgradesBrowserTest, crbug1431026) {
+  GURL www_bad_https_url =
+      https_server()->GetURL("www.bad-https.test", "/simple.html");
+  GURL www_http_url =
+      http_server()->GetURL("www.bad-https.test", "/simple.html");
+
+  // Configure HTTP and bad-HTTPS URLs which redirect to www. subdomain.
+  std::string www_redirect_path =
+      base::StrCat({"/server-redirect?", www_http_url.spec()});
+  GURL redirecting_bad_https_url =
+      https_server()->GetURL("bad-https.test", www_redirect_path);
+  GURL redirecting_http_url =
+      http_server()->GetURL("bad-https.test", www_redirect_path);
+
+  // A good HTTPS URL which redirects to an HTTP URL, which also redirects.
+  GURL initial_redirecting_good_https_url = https_server()->GetURL(
+      "good-https.test",
+      base::StrCat({"/server-redirect-301?", redirecting_http_url.spec()}));
+
+  auto* contents = browser()->tab_strip_model()->GetActiveWebContents();
+  EXPECT_FALSE(
+      content::NavigateToURL(contents, initial_redirecting_good_https_url));
+
+  if (IsHttpInterstitialEnabled()) {
+    // Should be showing interstitial on http://bad-https.test/.
+    EXPECT_EQ(redirecting_http_url, contents->GetLastCommittedURL());
+    EXPECT_TRUE(
+        chrome_browser_interstitials::IsShowingHttpsFirstModeInterstitial(
+            contents));
+  } else {
+    // Either due to no upgrades, or due to fast fallback to HTTP, this should
+    // end up on http://www.bad-https.test.
+    EXPECT_EQ(www_http_url, contents->GetLastCommittedURL());
+    EXPECT_FALSE(
+        chrome_browser_interstitials::IsShowingHttpsFirstModeInterstitial(
+            contents));
+  }
+}
+
 // A simple test fixture that ensures the kHttpsFirstModeV2 feature is enabled
 // and constructs a HistogramTester (so that it gets initialized before browser
 // startup). Used for testing pref tracking logic.

chrome/browser/ssl/https_upgrades_interceptor.cc

@@ -563,10 +563,13 @@ void HttpsUpgradesInterceptor::RedirectHandler(
     mojo::PendingReceiver<network::mojom::URLLoader> receiver,
     mojo::PendingRemote<network::mojom::URLLoaderClient> client) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  DCHECK(!receiver_.is_bound());
-  DCHECK(!client_.is_bound());
 
-  // Set up Mojo connection and initiate the redirect.
+  // Set up Mojo connection and initiate the redirect. `client_` and `receiver_`
+  // may have been previously bound from handling a previous upgrade earlier in
+  // the same navigation, so reset them before re-binding them to handle a new
+  // redirect.
+  receiver_.reset();
+  client_.reset();
   receiver_.Bind(std::move(receiver));
   receiver_.set_disconnect_handler(
       base::BindOnce(&HttpsUpgradesInterceptor::OnConnectionClosed,