Destroy SSLErrorHandler on new navigations so that it can properly be recreated.

Since |SSLErrorHandler| was a |WebContentsUserData| tied to the lifetime of a
|WebContents|, it wasn't properly deleted when the |WebContents| was reloaded
or navigated to another page. This CL removes the error handler explicitly and
adds browser tests to assert reload/navigate behavior.

BUG=453875

Review URL: https://codereview.chromium.org/1004283004

Cr-Commit-Position: refs/heads/master@{#321603}

chrome/browser/captive_portal/captive_portal_browsertest.cc

@@ -810,18 +810,18 @@ void CaptivePortalObserver::Observe(
   }
 }
 
-// This observer waits for the SSLErrorHandler to fire an interstitial timer for
-// the given web contents.
+// This observer waits for the SSLErrorHandler to start an interstitial timer
+// for the given web contents.
 class SSLInterstitialTimerObserver {
  public:
   explicit SSLInterstitialTimerObserver(content::WebContents* web_contents);
   ~SSLInterstitialTimerObserver();
 
-  // Waits until the interstitial delay timer in SSLErrorHandler is fired.
-  void WaitForTimerFired();
+  // Waits until the interstitial delay timer in SSLErrorHandler is started.
+  void WaitForTimerStarted();
 
  private:
-  void OnTimerFired(content::WebContents* web_contents);
+  void OnTimerStarted(content::WebContents* web_contents);
 
   const content::WebContents* web_contents_;
   SSLErrorHandler::TimerStartedCallback callback_;
@@ -835,7 +835,7 @@ SSLInterstitialTimerObserver::SSLInterstitialTimerObserver(
     content::WebContents* web_contents)
     : web_contents_(web_contents),
       message_loop_runner_(new content::MessageLoopRunner) {
-  callback_ = base::Bind(&SSLInterstitialTimerObserver::OnTimerFired,
+  callback_ = base::Bind(&SSLInterstitialTimerObserver::OnTimerStarted,
                          base::Unretained(this));
   SSLErrorHandler::SetInterstitialTimerStartedCallbackForTest(&callback_);
 }
@@ -844,11 +844,11 @@ SSLInterstitialTimerObserver::~SSLInterstitialTimerObserver() {
   SSLErrorHandler::SetInterstitialTimerStartedCallbackForTest(nullptr);
 }
 
-void SSLInterstitialTimerObserver::WaitForTimerFired() {
+void SSLInterstitialTimerObserver::WaitForTimerStarted() {
   message_loop_runner_->Run();
 }
 
-void SSLInterstitialTimerObserver::OnTimerFired(
+void SSLInterstitialTimerObserver::OnTimerStarted(
     content::WebContents* web_contents) {
   if (web_contents_ == web_contents && message_loop_runner_.get())
     message_loop_runner_->Quit();
@@ -948,19 +948,20 @@ class CaptivePortalBrowserTest : public InProcessBrowserTest {
   // returns, until URLRequestTimeoutOnDemandJob::FailJobs() is called,
   // at which point it will timeout.
   //
-  // When |expect_login_tab| is false, no login tab is expected to be opened,
-  // because one already exists, and the function returns once the captive
-  // portal test is complete.
+  // When |expect_open_login_tab| is false, no login tab is expected to be
+  // opened, because one already exists, and the function returns once the
+  // captive portal test is complete.
   //
-  // If |expect_login_tab| is true, a login tab is then expected to be opened.
-  // It waits until both the login tab has finished loading, and two captive
-  // portal tests complete.  The second test is triggered by the load of the
-  // captive portal tab completing.
+  // If |expect_open_login_tab| is true, a login tab is then expected to be
+  // opened. It waits until both the login tab has finished loading, and two
+  // captive portal tests complete.  The second test is triggered by the load of
+  // the captive portal tab completing.
   //
   // This function must not be called when the active tab is currently loading.
   // Waits for the hanging request to be issued, so other functions can rely
   // on URLRequestTimeoutOnDemandJob::WaitForJobs having been called.
-  void SlowLoadBehindCaptivePortal(Browser* browser, bool expect_login_tab);
+  void SlowLoadBehindCaptivePortal(Browser* browser,
+                                   bool expect_open_login_tab);
 
   // Same as above, but takes extra parameters.
   //
@@ -1486,7 +1487,7 @@ void CaptivePortalBrowserTest::FastErrorWithInterstitialTimer(
                                               cert_error_url,
                                               CURRENT_TAB,
                                               ui_test_utils::BROWSER_TEST_NONE);
-  interstitial_timer_observer.WaitForTimerFired();
+  interstitial_timer_observer.WaitForTimerStarted();
 
   // The tab should be in loading state, waiting for the interstitial timer to
   // expire or a captive portal result to arrive. Since captive portal checks
@@ -2070,19 +2071,10 @@ IN_PROC_BROWSER_TEST_F(CaptivePortalBrowserTest,
             GetStateOfTabReloaderAt(browser(), 0));
 }
 
-// This test is very flaky on Linux and is disabled.
-// https://crbug.com/453875
-#if defined(OS_LINUX)
-#define MAYBE_InterstitialTimerReloadWhileLoading \
-        DISABLED_InterstitialTimerReloadWhileLoading
-#else
-#define MAYBE_InterstitialTimerReloadWhileLoading \
-        InterstitialTimerReloadWhileLoading
-#endif
 // Same as above, but instead of stopping, the loading page is reloaded. The end
 // result is the same. (i.e. page load stops, no interstitials shown)
 IN_PROC_BROWSER_TEST_F(CaptivePortalBrowserTest,
-                       MAYBE_InterstitialTimerReloadWhileLoading) {
+                       InterstitialTimerReloadWhileLoading) {
   net::SpawnedTestServer::SSLOptions https_options;
   https_options.server_certificate =
       net::SpawnedTestServer::SSLOptions::CERT_MISMATCHED_NAME;
@@ -2135,20 +2127,11 @@ IN_PROC_BROWSER_TEST_F(CaptivePortalBrowserTest,
             GetStateOfTabReloaderAt(browser(), 0));
 }
 
-// This test is very flaky on Linux and is disabled.
-// https://crbug.com/453875
-#if defined(OS_LINUX)
-#define MAYBE_InterstitialTimerNavigateAwayWhileLoading \
-        DISABLED_InterstitialTimerNavigateAwayWhileLoading
-#else
-#define MAYBE_InterstitialTimerNavigateAwayWhileLoading \
-        InterstitialTimerNavigateAwayWhileLoading
-#endif
-
-// Same as above, but instead of reloading, the page is navigated away. The new
-// page should load, and no interstitials should be shown.
+// Same as |InterstitialTimerReloadWhileLoading_NoSSLError|, but instead of
+// reloading, the page is navigated away. The new page should load, and no
+// interstitials should be shown.
 IN_PROC_BROWSER_TEST_F(CaptivePortalBrowserTest,
-                       MAYBE_InterstitialTimerNavigateAwayWhileLoading) {
+                       InterstitialTimerNavigateAwayWhileLoading) {
   net::SpawnedTestServer::SSLOptions https_options;
   https_options.server_certificate =
       net::SpawnedTestServer::SSLOptions::CERT_MISMATCHED_NAME;
@@ -2209,6 +2192,119 @@ IN_PROC_BROWSER_TEST_F(CaptivePortalBrowserTest,
             GetStateOfTabReloaderAt(browser(), 0));
 }
 
+// Same as above, but the hanging load is interrupted by a navigation to the
+// same page, this time committing the navigation. This should end up with an
+// SSL interstitial when not behind a captive portal. This ensures that a new
+// |SSLErrorHandler| is created on a new navigation, even though the tab's
+// WebContents doesn't change.
+IN_PROC_BROWSER_TEST_F(
+    CaptivePortalBrowserTest,
+    InterstitialTimerNavigateWhileLoading_EndWithSSLInterstitial) {
+  net::SpawnedTestServer::SSLOptions https_options;
+  https_options.server_certificate =
+      net::SpawnedTestServer::SSLOptions::CERT_MISMATCHED_NAME;
+  net::SpawnedTestServer https_server(
+      net::SpawnedTestServer::TYPE_HTTPS, https_options,
+      base::FilePath(FILE_PATH_LITERAL("chrome/test/data")));
+  ASSERT_TRUE(https_server.Start());
+  // The path does not matter.
+  GURL cert_error_url = https_server.GetURL(kTestServerLoginPath);
+  URLRequestMockCaptivePortalJobFactory::SetBehindCaptivePortal(false);
+
+  TabStripModel* tab_strip_model = browser()->tab_strip_model();
+  WebContents* broken_tab_contents = tab_strip_model->GetActiveWebContents();
+
+  FastErrorWithInterstitialTimer(browser(), cert_error_url);
+  // Page appears loading. Turn on response to probe request again, and navigate
+  // to the same page. This should result in a cert error which should
+  // instantiate an |SSLErrorHandler| and end up showing an SSL interstitial.
+  RespondToProbeRequests(true);
+  // Can't have ui_test_utils do the navigation because it will wait for loading
+  // tabs to stop loading before navigating.
+  CaptivePortalObserver portal_observer(browser()->profile());
+  MultiNavigationObserver test_navigation_observer;
+  browser()->OpenURL(content::OpenURLParams(cert_error_url, content::Referrer(),
+                                            CURRENT_TAB,
+                                            ui::PAGE_TRANSITION_TYPED, false));
+  // Expect two navigations: First one for stopping the hanging page, second one
+  // for completing the load of the above navigation.
+  test_navigation_observer.WaitForNavigations(2);
+  // Should end up with an SSL interstitial.
+  WaitForInterstitialAttach(broken_tab_contents);
+  ASSERT_TRUE(broken_tab_contents->ShowingInterstitialPage());
+  EXPECT_EQ(SSLBlockingPage::kTypeForTesting,
+            broken_tab_contents->GetInterstitialPage()
+                ->GetDelegateForTesting()
+                ->GetTypeForTesting());
+  EXPECT_FALSE(broken_tab_contents->IsLoading());
+  EXPECT_EQ(1, portal_observer.num_results_received());
+  EXPECT_EQ(captive_portal::RESULT_INTERNET_CONNECTED,
+            portal_observer.captive_portal_result());
+  EXPECT_EQ(0, NumLoadingTabs());
+  EXPECT_FALSE(CheckPending(browser()));
+  EXPECT_EQ(1, browser()->tab_strip_model()->count());
+  EXPECT_EQ(CaptivePortalTabReloader::STATE_NONE,
+            GetStateOfTabReloaderAt(browser(), 0));
+}
+
+// Same as above, but this time behind a captive portal.
+IN_PROC_BROWSER_TEST_F(
+    CaptivePortalBrowserTest,
+    InterstitialTimerNavigateWhileLoading_EndWithCaptivePortalInterstitial) {
+  net::SpawnedTestServer::SSLOptions https_options;
+  https_options.server_certificate =
+      net::SpawnedTestServer::SSLOptions::CERT_MISMATCHED_NAME;
+  net::SpawnedTestServer https_server(
+      net::SpawnedTestServer::TYPE_HTTPS, https_options,
+      base::FilePath(FILE_PATH_LITERAL("chrome/test/data")));
+  ASSERT_TRUE(https_server.Start());
+  // The path does not matter.
+  GURL cert_error_url = https_server.GetURL(kTestServerLoginPath);
+  URLRequestMockCaptivePortalJobFactory::SetBehindCaptivePortal(true);
+
+  TabStripModel* tab_strip_model = browser()->tab_strip_model();
+  WebContents* broken_tab_contents = tab_strip_model->GetActiveWebContents();
+  int initial_tab_count = tab_strip_model->count();
+
+  FastErrorWithInterstitialTimer(browser(), cert_error_url);
+  // Page appears loading. Turn on response to probe request again, and navigate
+  // to the same page. This should result in a cert error which should
+  // instantiate an |SSLErrorHandler| and end up showing an SSL.
+  RespondToProbeRequests(true);
+  // Can't have ui_test_utils do the navigation because it will wait for loading
+  // tabs to stop loading before navigating.
+  CaptivePortalObserver portal_observer(browser()->profile());
+  MultiNavigationObserver test_navigation_observer;
+  browser()->OpenURL(content::OpenURLParams(cert_error_url, content::Referrer(),
+                                            CURRENT_TAB,
+                                            ui::PAGE_TRANSITION_TYPED, false));
+  // Expect three navigations:
+  // 1- For stopping the hanging page.
+  // 2- For completing the load of the above navigation.
+  // 3- For completing the load of the login tab.
+  test_navigation_observer.WaitForNavigations(3);
+  // Should end up with a captive portal interstitial and a new login tab.
+  WaitForInterstitialAttach(broken_tab_contents);
+  ASSERT_TRUE(broken_tab_contents->ShowingInterstitialPage());
+  EXPECT_EQ(CaptivePortalBlockingPage::kTypeForTesting,
+            broken_tab_contents->GetInterstitialPage()
+                ->GetDelegateForTesting()
+                ->GetTypeForTesting());
+  ASSERT_EQ(initial_tab_count + 1, tab_strip_model->count());
+  EXPECT_EQ(initial_tab_count, tab_strip_model->active_index());
+  EXPECT_FALSE(broken_tab_contents->IsLoading());
+  EXPECT_EQ(1, portal_observer.num_results_received());
+  EXPECT_EQ(captive_portal::RESULT_BEHIND_CAPTIVE_PORTAL,
+            portal_observer.captive_portal_result());
+  EXPECT_EQ(0, NumLoadingTabs());
+  EXPECT_FALSE(CheckPending(browser()));
+  EXPECT_EQ(CaptivePortalTabReloader::STATE_BROKEN_BY_PORTAL,
+            GetStateOfTabReloaderAt(browser(), 0));
+  EXPECT_EQ(CaptivePortalTabReloader::STATE_NONE,
+            GetStateOfTabReloaderAt(browser(), 1));
+  EXPECT_TRUE(IsLoginTab(tab_strip_model->GetWebContentsAt(1)));
+}
+
 // A cert error triggers a captive portal check and results in opening a login
 // tab.  The user then logs in and the page with the error is reloaded.
 IN_PROC_BROWSER_TEST_F(CaptivePortalBrowserTest, SSLCertErrorLogin) {

chrome/browser/ssl/ssl_error_handler.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/ssl/ssl_error_handler.h"
 
+#include "base/callback_helpers.h"
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram.h"
 #include "base/time/time.h"
@@ -126,7 +127,8 @@ SSLErrorHandler::SSLErrorHandler(content::WebContents* web_contents,
                                  const GURL& request_url,
                                  int options_mask,
                                  const base::Callback<void(bool)>& callback)
-    : web_contents_(web_contents),
+    : content::WebContentsObserver(web_contents),
+      web_contents_(web_contents),
       cert_error_(cert_error),
       ssl_info_(ssl_info),
       request_url_(request_url),
@@ -223,3 +225,17 @@ void SSLErrorHandler::Observe(
   }
 #endif
 }
+
+// Destroy the error handler on all new navigations. This ensures that the
+// handler is properly recreated when a hanging page is navigated to an SSL
+// error, even when the tab's WebContents doesn't change.
+void SSLErrorHandler::DidStartNavigationToPendingEntry(
+    const GURL& url,
+    content::NavigationController::ReloadType reload_type) {
+  // Need to explicity deny the certificate via the callback, otherwise memory
+  // is leaked.
+  if (!callback_.is_null()) {
+    base::ResetAndReturn(&callback_).Run(false);
+  }
+  web_contents_->RemoveUserData(UserDataKey());
+}

chrome/browser/ssl/ssl_error_handler.h

@@ -13,6 +13,7 @@
 #include "chrome/browser/chrome_notification_types.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
+#include "content/public/browser/web_contents_observer.h"
 #include "content/public/browser/web_contents_user_data.h"
 #include "net/ssl/ssl_info.h"
 #include "url/gurl.h"
@@ -35,6 +36,7 @@ class WebContents;
 // captive_portal::CaptivePortalService which can only be accessed on the UI
 // thread.
 class SSLErrorHandler : public content::WebContentsUserData<SSLErrorHandler>,
+                        public content::WebContentsObserver,
                         public content::NotificationObserver {
  public:
   // Type of the delay to display the SSL interstitial.
@@ -91,12 +93,17 @@ class SSLErrorHandler : public content::WebContentsUserData<SSLErrorHandler>,
       const content::NotificationSource& source,
       const content::NotificationDetails& details) override;
 
+  // content::WebContentsObserver:
+  void DidStartNavigationToPendingEntry(
+      const GURL& url,
+      content::NavigationController::ReloadType reload_type) override;
+
   content::WebContents* web_contents_;
   const int cert_error_;
   const net::SSLInfo ssl_info_;
   const GURL request_url_;
   const int options_mask_;
-  const base::Callback<void(bool)> callback_;
+  base::Callback<void(bool)> callback_;
 
   content::NotificationRegistrar registrar_;
   base::OneShotTimer<SSLErrorHandler> timer_;