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

chrome/browser/captive_portal/captive_portal_browsertest.cc

Index: chrome/browser/captive_portal/captive_portal_browsertest.cc
diff --git a/chrome/browser/captive_portal/captive_portal_browsertest.cc b/chrome/browser/captive_portal/captive_portal_browsertest.cc
index 32b297190e169019fcb9a0838dd6ad5333b2cfc2..5066c0fe6268d5dbfaf01ed2c579bcbba50af831 100644
--- a/chrome/browser/captive_portal/captive_portal_browsertest.cc
+++ b/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) {