Trigger captive portal checks on secure DNS network failures.

These captive portal checks may occur on either HTTP or HTTPS
navigations. The CaptivePortalTabHelper no longer tracks navigations
that are renderer-initiated to avoid interference from link doctor and
renderer-initiated reload attempts (these navigations, which can be
triggered by DNS failures, may otherwise reset the tab state before a
captive portal probe result is received).

Bug: 10161646
Change-Id: Ia2ac3715d43e9b1be71df1dea7a1882f98033888
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1870013Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: Katharine Daly <dalyk@google.com>
Cr-Commit-Position: refs/heads/master@{#737843}

chrome/renderer/net/net_error_helper_core.cc

@@ -172,7 +172,10 @@ bool ShouldUseFixUrlServiceForError(const error_page::Error& error,
     *error_param = "http404";
     return true;
   }
-  if (IsNetDnsError(error)) {
+  // Don't use the link doctor for secure DNS network errors, since the
+  // additional navigation may interfere with the captive portal probe state.
+  if (IsNetDnsError(error) &&
+      !error.resolve_error_info().is_secure_network_error) {
     *error_param = "dnserror";
     return true;
   }
@@ -475,7 +478,13 @@ bool NetErrorHelperCore::IsReloadableError(
          info.error.reason() != net::ERR_INVALID_AUTH_CREDENTIALS &&
          // Don't auto-reload non-http/https schemas.
          // https://crbug.com/471713
-         url.SchemeIsHTTPOrHTTPS();
+         url.SchemeIsHTTPOrHTTPS() &&
+         // Don't auto reload if the error was a secure DNS network error, since
+         // the reload may interfere with the captive portal probe state.
+         // TODO(crbug.com/1016164): Explore how to allow reloads for secure DNS
+         // network errors without interfering with the captive portal probe
+         // state.
+         !info.error.resolve_error_info().is_secure_network_error;
 }
 
 NetErrorHelperCore::NetErrorHelperCore(Delegate* delegate,

components/captive_portal/content/captive_portal_tab_helper.cc

@@ -122,7 +122,8 @@ void CaptivePortalTabHelper::DidFinishNavigation(
   }
 
   if (navigation_handle->HasCommitted()) {
-    tab_reloader_->OnLoadCommitted(navigation_handle->GetNetErrorCode());
+    tab_reloader_->OnLoadCommitted(navigation_handle->GetNetErrorCode(),
+                                   navigation_handle->GetResolveErrorInfo());
   } else {
     tab_reloader_->OnAbort();
   }

components/captive_portal/content/captive_portal_tab_helper_unittest.cc

@@ -45,7 +45,7 @@ class MockCaptivePortalTabReloader : public CaptivePortalTabReloader {
       : CaptivePortalTabReloader(nullptr, nullptr, base::Callback<void()>()) {}
 
   MOCK_METHOD1(OnLoadStart, void(bool));
-  MOCK_METHOD1(OnLoadCommitted, void(int));
+  MOCK_METHOD2(OnLoadCommitted, void(int, net::ResolveErrorInfo));
   MOCK_METHOD0(OnAbort, void());
   MOCK_METHOD1(OnRedirect, void(bool));
   MOCK_METHOD2(OnCaptivePortalResults,
@@ -91,7 +91,9 @@ class CaptivePortalTabHelperTest : public content::RenderViewHostTestHarness {
     auto navigation = content::NavigationSimulator::CreateBrowserInitiated(
         url, web_contents());
     navigation->Start();
-    EXPECT_CALL(mock_reloader(), OnLoadCommitted(net::OK)).Times(1);
+    EXPECT_CALL(mock_reloader(),
+                OnLoadCommitted(net::OK, net::ResolveErrorInfo(net::OK)))
+        .Times(1);
     navigation->Commit();
   }
 
@@ -102,7 +104,27 @@ class CaptivePortalTabHelperTest : public content::RenderViewHostTestHarness {
     auto navigation = content::NavigationSimulator::CreateBrowserInitiated(
         url, web_contents());
     navigation->Fail(net::ERR_TIMED_OUT);
-    EXPECT_CALL(mock_reloader(), OnLoadCommitted(net::ERR_TIMED_OUT)).Times(1);
+    EXPECT_CALL(
+        mock_reloader(),
+        OnLoadCommitted(net::ERR_TIMED_OUT, net::ResolveErrorInfo(net::OK)))
+        .Times(1);
+    navigation->CommitErrorPage();
+  }
+
+  // Simulates a secure DNS network error while requesting |url|.
+  void SimulateSecureDnsNetworkError(const GURL& url) {
+    EXPECT_CALL(mock_reloader(), OnLoadStart(url.SchemeIsCryptographic()))
+        .Times(1);
+    auto navigation = content::NavigationSimulator::CreateBrowserInitiated(
+        url, web_contents());
+    navigation->SetResolveErrorInfo({net::ERR_CERT_COMMON_NAME_INVALID,
+                                     true /* is_secure_network_error */});
+    navigation->Fail(net::ERR_NAME_NOT_RESOLVED);
+    EXPECT_CALL(mock_reloader(),
+                OnLoadCommitted(net::ERR_NAME_NOT_RESOLVED,
+                                net::ResolveErrorInfo(
+                                    net::ERR_CERT_COMMON_NAME_INVALID, true)))
+        .Times(1);
     navigation->CommitErrorPage();
   }
 
@@ -189,6 +211,13 @@ TEST_F(CaptivePortalTabHelperTest, HttpsTimeout) {
   EXPECT_FALSE(tab_helper()->IsLoginTab());
 }
 
+TEST_F(CaptivePortalTabHelperTest, HttpsSecureDnsNetworkError) {
+  SimulateSecureDnsNetworkError(GURL(kHttpsUrl));
+  // Make sure no state was carried over from the secure DNS network error.
+  SimulateSuccess(GURL(kHttpsUrl));
+  EXPECT_FALSE(tab_helper()->IsLoginTab());
+}
+
 TEST_F(CaptivePortalTabHelperTest, HttpsAbort) {
   SimulateAbort(GURL(kHttpsUrl));
   // Make sure no state was carried over from the abort.
@@ -261,7 +290,9 @@ TEST_F(CaptivePortalTabHelperTest, UnexpectedProvisionalLoad) {
   // The cross-process navigation fails.
   cross_process_navigation->Fail(net::ERR_FAILED);
 
-  EXPECT_CALL(mock_reloader(), OnLoadCommitted(net::ERR_FAILED)).Times(1);
+  EXPECT_CALL(mock_reloader(),
+              OnLoadCommitted(net::ERR_FAILED, net::ResolveErrorInfo(net::OK)))
+      .Times(1);
   cross_process_navigation->CommitErrorPage();
 }
 
@@ -298,7 +329,9 @@ TEST_F(CaptivePortalTabHelperTest, UnexpectedCommit) {
   EXPECT_CALL(mock_reloader(),
               OnLoadStart(same_site_url.SchemeIsCryptographic()))
       .Times(1);
-  EXPECT_CALL(mock_reloader(), OnLoadCommitted(net::OK)).Times(1);
+  EXPECT_CALL(mock_reloader(),
+              OnLoadCommitted(net::OK, net::ResolveErrorInfo(net::OK)))
+      .Times(1);
   same_site_navigation->Commit();
 }
 
@@ -355,7 +388,9 @@ TEST_F(CaptivePortalTabHelperTest, HttpsSubframeParallelError) {
 
   // Error page load finishes.
   subframe_navigation->CommitErrorPage();
-  EXPECT_CALL(mock_reloader(), OnLoadCommitted(net::ERR_UNEXPECTED)).Times(1);
+  EXPECT_CALL(mock_reloader(), OnLoadCommitted(net::ERR_UNEXPECTED,
+                                               net::ResolveErrorInfo(net::OK)))
+      .Times(1);
   main_frame_navigation->CommitErrorPage();
 }
 
@@ -373,7 +408,9 @@ TEST_F(CaptivePortalTabHelperTest, HttpToHttpsRedirectTimeout) {
 
   navigation->Fail(net::ERR_TIMED_OUT);
 
-  EXPECT_CALL(mock_reloader(), OnLoadCommitted(net::ERR_TIMED_OUT)).Times(1);
+  EXPECT_CALL(mock_reloader(), OnLoadCommitted(net::ERR_TIMED_OUT,
+                                               net::ResolveErrorInfo(net::OK)))
+      .Times(1);
   navigation->CommitErrorPage();
 }
 
@@ -391,7 +428,9 @@ TEST_F(CaptivePortalTabHelperTest, HttpsToHttpRedirect) {
       .Times(1);
   navigation->Redirect(http_url);
 
-  EXPECT_CALL(mock_reloader(), OnLoadCommitted(net::OK)).Times(1);
+  EXPECT_CALL(mock_reloader(),
+              OnLoadCommitted(net::OK, net::ResolveErrorInfo(net::OK)))
+      .Times(1);
   navigation->Commit();
 }
 
@@ -408,7 +447,9 @@ TEST_F(CaptivePortalTabHelperTest, HttpToHttpRedirect) {
       .Times(1);
   navigation->Redirect(http_url);
 
-  EXPECT_CALL(mock_reloader(), OnLoadCommitted(net::OK)).Times(1);
+  EXPECT_CALL(mock_reloader(),
+              OnLoadCommitted(net::OK, net::ResolveErrorInfo(net::OK)))
+      .Times(1);
   navigation->Commit();
 }
 
@@ -431,7 +472,9 @@ TEST_F(CaptivePortalTabHelperTest, SubframeRedirect) {
   GURL https_url(kHttpsUrl);
   subframe_navigation->Redirect(https_url);
 
-  EXPECT_CALL(mock_reloader(), OnLoadCommitted(net::OK)).Times(1);
+  EXPECT_CALL(mock_reloader(),
+              OnLoadCommitted(net::OK, net::ResolveErrorInfo(net::OK)))
+      .Times(1);
   main_frame_navigation->Commit();
 }
 

components/captive_portal/content/captive_portal_tab_reloader.cc

@@ -70,10 +70,18 @@ void CaptivePortalTabReloader::OnLoadStart(bool is_ssl) {
     SetState(STATE_TIMER_RUNNING);
 }
 
-void CaptivePortalTabReloader::OnLoadCommitted(int net_error) {
+void CaptivePortalTabReloader::OnLoadCommitted(
+    int net_error,
+    net::ResolveErrorInfo resolve_error_info) {
   provisional_main_frame_load_ = false;
   ssl_url_in_redirect_chain_ = false;
 
+  // There was a secure DNS network error, so maybe check for a captive portal.
+  if (resolve_error_info.is_secure_network_error) {
+    OnSecureDnsNetworkError();
+    return;
+  }
+
   if (state_ == STATE_NONE)
     return;
 
@@ -181,6 +189,19 @@ void CaptivePortalTabReloader::OnSlowSSLConnect() {
   SetState(STATE_MAYBE_BROKEN_BY_PORTAL);
 }
 
+void CaptivePortalTabReloader::OnSecureDnsNetworkError() {
+  if (state_ == STATE_NONE || state_ == STATE_TIMER_RUNNING) {
+    SetState(STATE_MAYBE_BROKEN_BY_PORTAL);
+    return;
+  }
+
+  if (state_ == STATE_NEEDS_RELOAD) {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::BindOnce(&CaptivePortalTabReloader::ReloadTabIfNeeded,
+                                  weak_factory_.GetWeakPtr()));
+  }
+}
+
 void CaptivePortalTabReloader::SetState(State new_state) {
   // Stop the timer even when old and new states are the same.
   if (state_ == STATE_TIMER_RUNNING) {
@@ -192,7 +213,8 @@ void CaptivePortalTabReloader::SetState(State new_state) {
   // Check for unexpected state transitions.
   switch (state_) {
     case STATE_NONE:
-      DCHECK(new_state == STATE_NONE || new_state == STATE_TIMER_RUNNING);
+      DCHECK(new_state == STATE_NONE || new_state == STATE_TIMER_RUNNING ||
+             new_state == STATE_MAYBE_BROKEN_BY_PORTAL);
       break;
     case STATE_TIMER_RUNNING:
       DCHECK(new_state == STATE_NONE ||

components/captive_portal/content/captive_portal_tab_reloader.h

@@ -12,6 +12,7 @@
 #include "base/time/time.h"
 #include "base/timer/timer.h"
 #include "components/captive_portal/content/captive_portal_service.h"
+#include "net/dns/public/resolve_error_info.h"
 
 namespace content {
 class WebContents;
@@ -85,15 +86,17 @@ class CaptivePortalTabReloader {
   // loads and for error pages.
   virtual void OnLoadStart(bool is_ssl);
 
-  // Called when the main frame is committed.  |net_error| will be net::OK in
-  // the case of a successful load.  For an errror page, the entire 3-step
+  // Called when the main frame is committed. |net_error| will be net::OK in
+  // the case of a successful load. |resolve_error_info| contains information
+  // about any hostname resolution error. For an error page, the entire 3-step
   // process of getting the error, starting a new provisional load for the error
   // page, and committing the error page is treated as a single commit.
   //
   // The Link Doctor page will typically be one OnLoadCommitted with an error
   // code, followed by another OnLoadCommitted with net::OK for the Link Doctor
   // page.
-  virtual void OnLoadCommitted(int net_error);
+  virtual void OnLoadCommitted(int net_error,
+                               net::ResolveErrorInfo resolve_error_info);
 
   // This is called when the current provisional main frame load is canceled.
   // Sets state to STATE_NONE, unless this is a login tab.
@@ -138,6 +141,9 @@ class CaptivePortalTabReloader {
   // while to commit.
   void OnSlowSSLConnect();
 
+  // Called when a main frame loads with a secure DNS network error.
+  void OnSecureDnsNetworkError();
+
   // Reloads the tab if there's no provisional load going on and the current
   // state is STATE_NEEDS_RELOAD.  Not safe to call synchronously when called
   // by from a WebContentsObserver function, since the WebContents is currently

content/public/test/navigation_simulator.h

@@ -12,6 +12,7 @@
 #include "content/public/browser/navigation_throttle.h"
 #include "content/public/browser/reload_type.h"
 #include "mojo/public/cpp/bindings/pending_receiver.h"
+#include "net/dns/public/resolve_error_info.h"
 #include "services/service_manager/public/cpp/interface_provider.h"
 #include "third_party/blink/public/mojom/referrer.mojom.h"
 #include "ui/base/page_transition_types.h"
@@ -296,6 +297,10 @@ class NavigationSimulator {
   // in throttles deferring the navigation with a call to Wait().
   virtual void SetAutoAdvance(bool auto_advance) = 0;
 
+  // Sets the ResolveErrorInfo to be set on the URLLoaderCompletionStatus.
+  virtual void SetResolveErrorInfo(
+      const net::ResolveErrorInfo& resolve_error_info) = 0;
+
   // Sets the SSLInfo to be set on the response. This should be called before
   // Commit().
   virtual void SetSSLInfo(const net::SSLInfo& ssl_info) = 0;

content/test/navigation_simulator_impl.cc

@@ -684,6 +684,7 @@ void NavigationSimulatorImpl::FailWithResponseHeaders(
       static_cast<TestNavigationURLLoader*>(request_->loader_for_testing());
   CHECK(url_loader);
   network::URLLoaderCompletionStatus status(error_code);
+  status.resolve_error_info = resolve_error_info_;
   status.ssl_info = ssl_info_;
   url_loader->SimulateErrorWithStatus(status);
 
@@ -916,6 +917,11 @@ void NavigationSimulatorImpl::SetAutoAdvance(bool auto_advance) {
   auto_advance_ = auto_advance;
 }
 
+void NavigationSimulatorImpl::SetResolveErrorInfo(
+    const net::ResolveErrorInfo& resolve_error_info) {
+  resolve_error_info_ = resolve_error_info;
+}
+
 void NavigationSimulatorImpl::SetSSLInfo(const net::SSLInfo& ssl_info) {
   ssl_info_ = ssl_info;
 }

content/test/navigation_simulator_impl.h

@@ -20,6 +20,7 @@
 #include "mojo/public/cpp/bindings/pending_receiver.h"
 #include "net/base/host_port_pair.h"
 #include "net/base/ip_endpoint.h"
+#include "net/dns/public/resolve_error_info.h"
 #include "services/service_manager/public/cpp/interface_provider.h"
 #include "third_party/blink/public/mojom/referrer.mojom-forward.h"
 #include "url/gurl.h"
@@ -95,6 +96,8 @@ class NavigationSimulatorImpl : public NavigationSimulator,
       override;
   void SetContentsMimeType(const std::string& contents_mime_type) override;
   void SetAutoAdvance(bool auto_advance) override;
+  void SetResolveErrorInfo(
+      const net::ResolveErrorInfo& resolve_error_info) override;
   void SetSSLInfo(const net::SSLInfo& ssl_info) override;
 
   NavigationThrottle::ThrottleCheckResult GetLastThrottleCheckResult() override;
@@ -295,6 +298,7 @@ class NavigationSimulatorImpl : public NavigationSimulator,
       network::mojom::CSPDisposition::CHECK;
   net::HttpResponseInfo::ConnectionInfo http_connection_info_ =
       net::HttpResponseInfo::CONNECTION_INFO_UNKNOWN;
+  net::ResolveErrorInfo resolve_error_info_ = net::ResolveErrorInfo(net::OK);
   base::Optional<net::SSLInfo> ssl_info_;
   base::Optional<PageState> page_state_;
   base::Optional<url::Origin> origin_;