Add custom interstitial for captive portals.

chrome/browser/captive_portal/captive_portal_browsertest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <map>
 #include <set>
 
 #include "base/basictypes.h"
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/compiler_specific.h"
 #include "base/files/file_path.h"
 #include "base/message_loop/message_loop.h"
 #include "base/path_service.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/captive_portal/captive_portal_service.h"
 #include "chrome/browser/captive_portal/captive_portal_service_factory.h"
 #include "chrome/browser/captive_portal/captive_portal_tab_helper.h"
 #include "chrome/browser/captive_portal/captive_portal_tab_reloader.h"
 #include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/interstitials/security_interstitial_page.h"
 #include "chrome/browser/net/url_request_mock_util.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ssl/captive_portal_blocking_page.h"
+#include "chrome/browser/ssl/ssl_blocking_page.h"
+#include "chrome/browser/ssl/ssl_error_handler.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/browser_navigator.h"
 #include "chrome/browser/ui/tab_contents/tab_contents_iterator.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/interstitial_page.h"
+#include "content/public/browser/interstitial_page_delegate.h"
 #include "content/public/browser/navigation_controller.h"
+#include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_types.h"
 #include "content/public/browser/render_frame_host.h"
+#include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/url_constants.h"
+#include "content/public/test/browser_test_utils.h"
 #include "net/base/net_errors.h"
 #include "net/http/transport_security_state.h"
 #include "net/test/url_request/url_request_failed_job.h"
 #include "net/test/url_request/url_request_mock_http_job.h"
 #include "net/url_request/url_request.h"
 #include "net/url_request/url_request_context.h"
 #include "net/url_request/url_request_context_getter.h"
 #include "net/url_request/url_request_filter.h"
 #include "net/url_request/url_request_job.h"
 #include "net/url_request/url_request_status.h"
@@ skipping 48 matching lines @@
     "https://mock.captive.portal.long.timeout2/title2.html";
 
 // Same as kMockHttpsUrl, except the timeout happens instantly.
 const char* const kMockHttpsQuickTimeoutUrl =
     "https://mock.captive.portal.quick.timeout/title2.html";
 
 // Expected title of a tab once an HTTPS load completes, when not behind a
 // captive portal.
 const char* const kInternetConnectedTitle = "Title Of Awesomeness";
 
+// Wait until all <script> tags have executed, including jstemplate.
+// This isn't ideal, but the same trick is used in
+// SafeBrowsingBlockingPageBrowserTest to wait for the interstitials.
+bool WaitForPageReady(content::RenderViewHost* rvh) {

[mmenke, 2014/12/15 20:46:07]

Suggest renaming this WaitForInterstitialReady, and checking that rvh is indeed
showing an interstitial (In fact, probably simpler just to take an interstitial
page).

[meacer, 2014/12/16 01:23:18]

Done.

+  if (!rvh)
+    return false;
+  std::string ready_state;

[mmenke, 2014/12/15 20:46:08]

Should include <string>

[meacer, 2014/12/16 01:23:19]

Done.

+  do {
+    scoped_ptr<base::Value> value = content::ExecuteScriptAndGetValue(

[mmenke, 2014/12/15 20:46:07]

Should include the scoped_ptr and base/values headers.

[meacer, 2014/12/16 01:23:18]

Done.

+        rvh->GetMainFrame(), "document.readyState");
+    if (!value.get() || !value->GetAsString(&ready_state))
+      return false;
+  } while (ready_state != "complete");

[mmenke, 2014/12/15 20:46:07]

Think you mentioned this option earlier, but I'd be happier injecting something
into the page that called back into us, rather than polling.

To do this:

context::ExecuteStringAndExtractBool(
  rvh->GetMainFrame(),
  "function() {"  // Begin anonymous namespace.
  "  var done = false;"
  "  function checkState() {"
  "    if (!done && document.readyState == 'complete') {"
  "      done = true;"
  "      window.domAutomationController.send(true);"
  "    }"
  "  }"
  "  checkState();"
  "  document.onreadystatechange = checkState();"
  "}();";  // End anonymous namespace.
);

And then checking the result as you currently do.  It's a bit more code, but
polling is an anti-pattern in tests.  WDYT?

document.addEventListener('readystatechange',...) is better than setting an
onreadystatechange event, if it works, since it doesn't preclude anything else
from listening to the event, unlike setting the onreadstatechange method on
document.

[meacer, 2014/12/16 01:23:18]

Yes, as I mentioned earlier, I was planning to do this in a separate CL so that
I could fix SafeBrowsing code as well. Done.

+  return true;
+}
+
 // A URL request job that hangs until FailJobs() is called.  Started jobs
 // are stored in a static class variable containing a linked list so that
 // FailJobs() can locate them.
 class URLRequestTimeoutOnDemandJob : public net::URLRequestJob,
                                      public base::NonThreadSafe {
  public:
   // net::URLRequestJob:
   void Start() override;
 
   // All the public static methods below can be called on any thread.
 
   // Waits for exactly |num_jobs|.
   static void WaitForJobs(int num_jobs);
 
   // Fails all active URLRequestTimeoutOnDemandJobs with connection timeouts.
   // There are expected to be exactly |expected_num_jobs| waiting for
-  // failure.  The only way to gaurantee this is with an earlier call to
+  // failure.  The only way to guarantee this is with an earlier call to
   // WaitForJobs, so makes sure there has been a matching WaitForJobs call.
   static void FailJobs(int expected_num_jobs);
 
+  // Fails all active URLRequestTimeoutOnDemandJobs with SSL cert errors.
+  // |expected_num_jobs| behaves just as in FailJobs.
+  static void FailJobsWithCertError(int expected_num_jobs);
+
   // Abandon all active URLRequestTimeoutOnDemandJobs.  |expected_num_jobs|
   // behaves just as in FailJobs.
   static void AbandonJobs(int expected_num_jobs);
 
  private:
   friend class URLRequestMockCaptivePortalJobFactory;
 
   // Operation to perform on jobs when removing them from |job_list_|.
   enum EndJobOperation {
     FAIL_JOBS,
     ABANDON_JOBS,
+    FAIL_JOBS_WITH_CERT_ERROR
   };
 
   URLRequestTimeoutOnDemandJob(net::URLRequest* request,
                                net::NetworkDelegate* network_delegate);
   ~URLRequestTimeoutOnDemandJob() override;
 
   // Attempts to removes |this| from |jobs_|.  Returns true if it was removed
   // from the list.
   bool RemoveFromList();
 
@@ skipping 66 matching lines @@
 // static
 void URLRequestTimeoutOnDemandJob::FailJobs(int expected_num_jobs) {
   content::BrowserThread::PostTask(
       content::BrowserThread::IO, FROM_HERE,
       base::Bind(&URLRequestTimeoutOnDemandJob::FailOrAbandonJobsOnIOThread,
                  expected_num_jobs,
                  FAIL_JOBS));
 }
 
 // static
+void URLRequestTimeoutOnDemandJob::FailJobsWithCertError(
+    int expected_num_jobs) {
+  content::BrowserThread::PostTask(
+      content::BrowserThread::IO, FROM_HERE,
+      base::Bind(&URLRequestTimeoutOnDemandJob::FailOrAbandonJobsOnIOThread,
+                 expected_num_jobs,
+                 FAIL_JOBS_WITH_CERT_ERROR));
+}
+
+// static
 void URLRequestTimeoutOnDemandJob::AbandonJobs(int expected_num_jobs) {
   content::BrowserThread::PostTask(
       content::BrowserThread::IO, FROM_HERE,
       base::Bind(&URLRequestTimeoutOnDemandJob::FailOrAbandonJobsOnIOThread,
                  expected_num_jobs,
                  ABANDON_JOBS));
 }
 
 URLRequestTimeoutOnDemandJob::URLRequestTimeoutOnDemandJob(
     net::URLRequest* request, net::NetworkDelegate* network_delegate)
@@ skipping 67 matching lines @@
   while (job_list_) {
     ++num_jobs;
     URLRequestTimeoutOnDemandJob* job = job_list_;
     // Since the error notification may result in the job's destruction, remove
     // it from the job list before the error.
     EXPECT_TRUE(job->RemoveFromList());
     if (end_job_operation == FAIL_JOBS) {
       job->NotifyStartError(net::URLRequestStatus(
                                 net::URLRequestStatus::FAILED,
                                 net::ERR_CONNECTION_TIMED_OUT));
+    } else if (end_job_operation == FAIL_JOBS_WITH_CERT_ERROR) {
+      DCHECK(job->request()->url().SchemeIs(url::kHttpsScheme));

[mmenke, 2014/12/15 20:46:07]

Currently this test fixture uses EXPECTs/ASSERTs for code run on the IO thread,
so should be consistent.  (I don't really care which we use, but I do feel we
shouldn't mix the two unless we have a well-considered reason to do so)

[meacer, 2014/12/16 01:23:19]

Done.

+      net::SSLInfo info;
+      info.cert_status = net::CERT_STATUS_COMMON_NAME_INVALID;
+      info.cert = new net::X509Certificate(
+          "bad.host", "CA", base::Time::Max(), base::Time::Max());
+      job->NotifySSLCertificateError(info, true);
     }
   }
 
   EXPECT_EQ(expected_num_jobs, num_jobs_started_);
   EXPECT_EQ(expected_num_jobs, num_jobs);
 
   num_jobs_started_ -= expected_num_jobs;
 }
 
 // URLRequestCaptivePortalJobFactory emulates captive portal behavior.
@@ skipping 356 matching lines @@
     base::MessageLoopForUI::current()->Quit();
   }
 }
 
 // An observer for watching the CaptivePortalService.  It tracks the last
 // received result and the total number of received results.
 class CaptivePortalObserver : public content::NotificationObserver {
  public:
   explicit CaptivePortalObserver(Profile* profile);
 
-  // Runs the message loop until until at exactly |update_count| capitive portal
-  // results have been received, since this creation of |this|.  Expects no
+  // Runs the message loop until exactly |update_count| captive portal
+  // results have been received, since the creation of |this|.  Expects no
   // additional captive portal results.
   void WaitForResults(int num_results_to_wait_for);
 
   int num_results_received() const { return num_results_received_; }
 
   CaptivePortalResult captive_portal_result() const {
     return captive_portal_result_;
   }
 
  private:
@@ skipping 97 matching lines @@
   CaptivePortalBrowserTest();
 
   // InProcessBrowserTest:
   void SetUpOnMainThread() override;
   void TearDownOnMainThread() override;
 
   // Sets the captive portal checking preference.  Does not affect the command
   // line flag, which is set in SetUpCommandLine.
   void EnableCaptivePortalDetection(Profile* profile, bool enabled);
 
+  // Enables or disables actual captive portal probes. Should only be called
+  // after captive portal service setup is done.

[mmenke, 2014/12/15 20:46:08]

Maybe "When disabled, probe requests are are silently ignored, never receiving a
response."

[meacer, 2014/12/16 01:23:19]

Done.

+  void EnablePortalRequests(bool enabled);

[mmenke, 2014/12/15 20:46:07]

Maybe RespondToPortalRequests?  Or RespondToProbeRequests

[meacer, 2014/12/16 01:23:18]

Done.

+
   // Sets up the captive portal service for the given profile so that
   // all checks go to |test_url|.  Also disables all timers.
   void SetUpCaptivePortalService(Profile* profile, const GURL& test_url);
 
   // Returns true if |browser|'s profile is currently running a captive portal
   // check.
   bool CheckPending(Browser* browser);
 
+  // Returns the type of the interstitial being shown.
+  const void* GetInterstitialType(WebContents* contents) const;
+
   // Returns the CaptivePortalTabReloader::State of |web_contents|.
   CaptivePortalTabReloader::State GetStateOfTabReloader(
       WebContents* web_contents) const;
 
   // Returns the CaptivePortalTabReloader::State of the indicated tab.
   CaptivePortalTabReloader::State GetStateOfTabReloaderAt(Browser* browser,
                                                           int index) const;
 
   // Returns the number of tabs with the given state, across all profiles.
   int NumTabsWithState(CaptivePortalTabReloader::State state) const;
@@ skipping 60 matching lines @@
                                    int expected_login_tab_navigations);
 
   // Just like SlowLoadBehindCaptivePortal, except the navigated tab has
   // a connection timeout rather having its time trigger, and the function
   // waits until that timeout occurs.
   void FastTimeoutBehindCaptivePortal(Browser* browser,
                                       bool expect_open_login_tab);
 
   // Much as above, but accepts a URL parameter and can be used for errors that
   // trigger captive portal checks other than timeouts.  |error_url| should
   // result in an error rather than hanging.

[mmenke, 2014/12/15 20:46:08]

disable_portal_check_until_interstitial needs some documentation.  Maybe "If
|disable_portal_check_until_interstitial| is true, captive portal requests are
ignored until the interstitial is shown, at which point a captive portal result
is sent.  This allows testing in conjunction with the certificate error
interstitial."

[meacer, 2014/12/16 01:23:19]

Done.

-  void FastErrorBehindCaptivePortal(Browser* browser,
-                                    bool expect_open_login_tab,
-                                    const GURL& error_url);
+  void FastErrorBehindCaptivePortal(
+      Browser* browser,
+      bool expect_open_login_tab,
+      const GURL& error_url,
+      bool disable_portal_check_until_interstitial);

[mmenke, 2014/12/15 20:46:08]

"disable_portal_check_until_interstitial" could be clearer, as this is different
from normally disabling the checks at runtime, which makes the service claim
we're always online.

Maybe "delay_portal_response_until_interstital"

[meacer, 2014/12/16 01:23:18]

Done.

+
+  // Navigates the active tab to an SSL error page which triggers an
+  // interstitial timer. Also disables captive portal checks indefinitely, so
+  // the page appears to be hanging.
+  void FastErrorWithInterstitialTimer(Browser* browser,
+                                      const GURL& cert_error_url);
 
   // Navigates the login tab without logging in.  The login tab must be the
   // specified browser's active tab.  Expects no other tab to change state.
   // |num_loading_tabs| and |num_timed_out_tabs| are used as extra checks
   // that nothing has gone wrong prior to the function call.
   void NavigateLoginTab(Browser* browser,
                         int num_loading_tabs,
                         int num_timed_out_tabs);
 
   // Simulates a login by updating the URLRequestMockCaptivePortalJob's
   // behind captive portal state, and navigating the login tab.  Waits for
   // all broken but not loading tabs to be reloaded.
   // |num_loading_tabs| and |num_timed_out_tabs| are used as extra checks
   // that nothing has gone wrong prior to the function call.
   void Login(Browser* browser, int num_loading_tabs, int num_timed_out_tabs);
 
+  // Simulates a login when the broken tab shows an SSL or captive portal
+  // interstitial. Can't use Login() in those cases because the interstitial
+  // tab looks like a cross between a hung tab (Load was never committed) and a
+  // tab at an error page (The load was stopped).
+  void LoginCertError(Browser* browser);

[mmenke, 2014/12/15 20:46:07]

Why wasn't this needed for the cert error test before?

[meacer, 2014/12/16 01:23:19]

It was part of the cert error test, I just pulled the common login code into
this method so that it can be used by ShowCaptivePortalInterstitialOnCertError
as well.

+
   // Makes the slow SSL loads of all active tabs time out at once, and waits for
   // them to finish both that load and the automatic reload it should trigger.
   // There should be no timed out tabs when this is called.
   void FailLoadsAfterLogin(Browser* browser, int num_loading_tabs);
 
   // Makes the slow SSL loads of all active tabs time out at once, and waits for
   // them to finish displaying their error pages.  The login tab should be the
   // active tab.  There should be no timed out tabs when this is called.
   void FailLoadsWithoutLogin(Browser* browser, int num_loading_tabs);
 
@@ skipping 54 matching lines @@
 void CaptivePortalBrowserTest::TearDownOnMainThread() {
   // No test should have a captive portal check pending on quit.
   EXPECT_FALSE(CheckPending(browser()));
 }
 
 void CaptivePortalBrowserTest::EnableCaptivePortalDetection(
     Profile* profile, bool enabled) {
   profile->GetPrefs()->SetBoolean(prefs::kAlternateErrorPagesEnabled, enabled);
 }
 
+void CaptivePortalBrowserTest::EnablePortalRequests(bool enabled) {
+  if (enabled) {
+    EXPECT_EQ(CaptivePortalService::IGNORE_REQUESTS_FOR_TESTING,
+              CaptivePortalService::get_state_for_testing());
+    CaptivePortalService::set_state_for_testing(
+        CaptivePortalService::SKIP_OS_CHECK_FOR_TESTING);
+  } else {
+    EXPECT_EQ(CaptivePortalService::SKIP_OS_CHECK_FOR_TESTING,
+              CaptivePortalService::get_state_for_testing());
+    CaptivePortalService::set_state_for_testing(
+        CaptivePortalService::IGNORE_REQUESTS_FOR_TESTING);
+  }
+}
+
 void CaptivePortalBrowserTest::SetUpCaptivePortalService(Profile* profile,
                                                          const GURL& test_url) {
   CaptivePortalService* captive_portal_service =
       CaptivePortalServiceFactory::GetForProfile(profile);
   captive_portal_service->set_test_url(test_url);
 
   // Don't use any non-zero timers.  Timers are checked in unit tests.
   CaptivePortalService::RecheckPolicy* recheck_policy =
       &captive_portal_service->recheck_policy();
   recheck_policy->initial_backoff_no_portal_ms = 0;
   recheck_policy->initial_backoff_portal_ms = 0;
   recheck_policy->backoff_policy.maximum_backoff_ms = 0;
 }
 
 bool CaptivePortalBrowserTest::CheckPending(Browser* browser) {
   CaptivePortalService* captive_portal_service =
       CaptivePortalServiceFactory::GetForProfile(browser->profile());
 
   return captive_portal_service->DetectionInProgress() ||
       captive_portal_service->TimerRunning();
 }
 
+const void* CaptivePortalBrowserTest::GetInterstitialType(
+    WebContents* contents) const {
+  if (!contents->ShowingInterstitialPage())
+    return NULL;
+  SecurityInterstitialPage* blocking_page =
+      static_cast<SecurityInterstitialPage*>(
+          contents->GetInterstitialPage()->GetDelegateForTesting());
+  DCHECK(blocking_page);
+  return blocking_page->GetTypeForTesting();
+}
+
 CaptivePortalTabReloader::State CaptivePortalBrowserTest::GetStateOfTabReloader(
     WebContents* web_contents) const {
   return GetTabReloader(web_contents)->state();
 }
 
 CaptivePortalTabReloader::State
 CaptivePortalBrowserTest::GetStateOfTabReloaderAt(Browser* browser,
                                                   int index) const {
   return GetStateOfTabReloader(
       browser->tab_strip_model()->GetWebContentsAt(index));
@@ skipping 212 matching lines @@
 
   // Reset the load time to be large, so the timer won't trigger on a reload.
   SetSlowSSLLoadTime(tab_reloader, base::TimeDelta::FromHours(1));
 }
 
 void CaptivePortalBrowserTest::FastTimeoutBehindCaptivePortal(
     Browser* browser,
     bool expect_open_login_tab) {
   FastErrorBehindCaptivePortal(browser,
                                expect_open_login_tab,
-                               GURL(kMockHttpsQuickTimeoutUrl));
+                               GURL(kMockHttpsQuickTimeoutUrl),
+                               false);
 }
 
 void CaptivePortalBrowserTest::FastErrorBehindCaptivePortal(
     Browser* browser,
     bool expect_open_login_tab,
-    const GURL& error_url) {
+    const GURL& error_url,
+    bool disable_portal_check_until_interstitial) {
   TabStripModel* tab_strip_model = browser->tab_strip_model();
   // Calling this on a tab that's waiting for a load to manually be timed out
   // will result in a hang.
   ASSERT_FALSE(tab_strip_model->GetActiveWebContents()->IsLoading());
 
   // Set the load time to be large, so the timer won't trigger.  The value is
   // not restored at the end of the function.
   CaptivePortalTabReloader* tab_reloader =
       GetTabReloader(tab_strip_model->GetActiveWebContents());
   ASSERT_TRUE(tab_reloader);
   SetSlowSSLLoadTime(tab_reloader, base::TimeDelta::FromHours(1));
 
   // Number of tabs expected to be open after the captive portal checks
   // have completed.
   int initial_tab_count = tab_strip_model->count();
   int initial_active_index = tab_strip_model->active_index();
   int initial_loading_tabs = NumLoadingTabs();
   int expected_broken_tabs = NumBrokenTabs();
   if (CaptivePortalTabReloader::STATE_BROKEN_BY_PORTAL !=
           GetStateOfTabReloader(tab_strip_model->GetActiveWebContents())) {
     ++expected_broken_tabs;
   }
 
+  CaptivePortalService* captive_portal_service =
+      CaptivePortalServiceFactory::GetForProfile(browser->profile());
+  if (disable_portal_check_until_interstitial)
+    EnablePortalRequests(false);
+
   MultiNavigationObserver navigation_observer;
   CaptivePortalObserver portal_observer(browser->profile());
   ui_test_utils::NavigateToURLWithDisposition(browser,
                                               error_url,
                                               CURRENT_TAB,
                                               ui_test_utils::BROWSER_TEST_NONE);
+
+  if (disable_portal_check_until_interstitial) {
+    EXPECT_EQ(CaptivePortalTabReloader::STATE_NONE,
+              GetStateOfTabReloaderAt(browser, initial_active_index));
+    // Once the interstitial is attached, probe for captive portal.
+    WaitForInterstitialAttach(tab_strip_model->GetActiveWebContents());
+    EnablePortalRequests(true);
+    captive_portal_service->DetectCaptivePortal();
+  }
+
   portal_observer.WaitForResults(1);
 
   if (expect_open_login_tab) {
     navigation_observer.WaitForNavigations(2);
     ASSERT_EQ(initial_tab_count + 1, tab_strip_model->count());
     EXPECT_EQ(initial_tab_count, tab_strip_model->active_index());
     // Make sure that the originally active tab and the captive portal tab have
     // each loaded once.
     EXPECT_EQ(1, navigation_observer.NumNavigationsForTab(
                      tab_strip_model->GetWebContentsAt(initial_active_index)));
@@ skipping 107 matching lines @@
   EXPECT_EQ(initial_tab_count, tab_strip_model->count());
   EXPECT_EQ(CaptivePortalTabReloader::STATE_NONE,
             GetStateOfTabReloaderAt(browser, login_tab_index));
   EXPECT_FALSE(IsLoginTab(tab_strip_model->GetWebContentsAt(login_tab_index)));
 
   // Make sure there were no unexpected navigations of the login tab.
   EXPECT_EQ(1, navigation_observer.NumNavigationsForTab(
                    tab_strip_model->GetWebContentsAt(login_tab_index)));
 }
 
+void CaptivePortalBrowserTest::LoginCertError(Browser* browser) {

[mmenke, 2014/12/15 20:46:08]

Could we order the code here more like CaptivePortalBrowserTest::Login, so it's
easier to compare the two.

[meacer, 2014/12/16 01:23:19]

Done.

+  TabStripModel* tab_strip_model = browser->tab_strip_model();
+  URLRequestMockCaptivePortalJobFactory::SetBehindCaptivePortal(false);
+  MultiNavigationObserver navigation_observer;
+  CaptivePortalObserver portal_observer(browser->profile());
+
+  content::RenderFrameHost* render_frame_host =
+      tab_strip_model->GetActiveWebContents()->GetMainFrame();
+  render_frame_host->ExecuteJavaScript(base::ASCIIToUTF16("submitForm()"));
+
+  // The captive portal tab navigation will trigger a captive portal check,
+  // and reloading the original tab will bring up the interstitial page again,
+  // triggering a second captive portal check.
+  portal_observer.WaitForResults(2);
+
+  // Wait for both tabs to finish loading.
+  navigation_observer.WaitForNavigations(2);
+  EXPECT_EQ(2, portal_observer.num_results_received());
+  EXPECT_FALSE(CheckPending(browser));
+  EXPECT_EQ(captive_portal::RESULT_INTERNET_CONNECTED,
+            portal_observer.captive_portal_result());
+
+  // Check state of tabs.  While the first tab is still displaying an
+  // interstitial page, since no portal was found, it should be in STATE_NONE,
+  // as should the login tab.
+  ASSERT_EQ(2, tab_strip_model->count());
+  EXPECT_EQ(CaptivePortalTabReloader::STATE_NONE,
+            GetStateOfTabReloaderAt(browser, 0));
+  EXPECT_FALSE(IsLoginTab(tab_strip_model->GetWebContentsAt(1)));
+  EXPECT_EQ(CaptivePortalTabReloader::STATE_NONE,
+            GetStateOfTabReloaderAt(browser, 1));
+
+  // Make sure only one navigation was for the login tab.
+  EXPECT_EQ(1, navigation_observer.NumNavigationsForTab(
+                   tab_strip_model->GetWebContentsAt(1)));
+}
+
 void CaptivePortalBrowserTest::FailLoadsAfterLogin(Browser* browser,
                                                    int num_loading_tabs) {
   ASSERT_EQ(num_loading_tabs, NumLoadingTabs());
   ASSERT_EQ(num_loading_tabs, NumNeedReloadTabs());
   EXPECT_EQ(0, NumBrokenTabs());
 
   TabStripModel* tab_strip_model = browser->tab_strip_model();
   int initial_num_tabs = tab_strip_model->count();
   int initial_active_tab = tab_strip_model->active_index();
 
@@ skipping 278 matching lines @@
 }
 
 // Checks the unlikely case that the tab times out before the timer triggers.
 // This most likely won't happen, but should still work:
 IN_PROC_BROWSER_TEST_F(CaptivePortalBrowserTest, LoginFastTimeout) {
   FastTimeoutBehindCaptivePortal(browser(), true);
   Login(browser(), 0, 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) {
-  // Need an HTTP TestServer to handle a dynamically created server redirect.
-  ASSERT_TRUE(test_server()->Start());
-
+// tab.
+IN_PROC_BROWSER_TEST_F(CaptivePortalBrowserTest,
+                       ShowCaptivePortalInterstitialOnCertError) {
   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);
+  // Set SSL interstitial delay long enough so that a captive portal result
+  // is guaranteed to arrive during this window, and a captive portal
+  // error page is displayed instead of an SSL interstitial.
+  SSLErrorHandler::SetInterstitialDisplayDelayForTest(
+      base::TimeDelta::FromHours(1));

[mmenke, 2014/12/15 20:46:08]

Suggest moving this into the constructor for the test fixture.  Shouldn't affect
any of the other tests, anyways, but I'm paranoid, and it's one less cause of
variability between test runs.

[meacer, 2014/12/16 01:23:19]

Done.

+  TabStripModel* tab_strip_model = browser()->tab_strip_model();
+  WebContents* broken_tab_contents = tab_strip_model->GetActiveWebContents();
+
+  // The path does not matter.
+  GURL cert_error_url = https_server.GetURL(kTestServerLoginPath);
+  int cert_error_tab_index = tab_strip_model->active_index();
   // The interstitial should trigger a captive portal check when it opens, just
   // like navigating to kMockHttpsQuickTimeoutUrl.
-  FastErrorBehindCaptivePortal(browser(), true, cert_error_url);
-
-  // Simulate logging in.  Can't use Login() because the interstitial tab looks
-  // like a cross between a hung tab (Load was never committed) and a tab at an
-  // error page (The load was stopped).
-  URLRequestMockCaptivePortalJobFactory::SetBehindCaptivePortal(false);
-  MultiNavigationObserver navigation_observer;
+  FastErrorBehindCaptivePortal(browser(), true, cert_error_url, false);
+
+  EXPECT_EQ(CaptivePortalBlockingPage::kTypeForTesting,
+            GetInterstitialType(broken_tab_contents));
+
+  int login_tab_index = tab_strip_model->active_index();

[mmenke, 2014/12/15 20:46:07]

Suggest moving this below the comment.

[meacer, 2014/12/16 01:23:19]

Done.

+  // Switch to the interstitial and click the "Connect" button. Should switch
+  // active tab to the captive portal landing page.
+  tab_strip_model->ActivateTabAt(cert_error_tab_index, false);
+

[mmenke, 2014/12/15 20:46:08]

Remove this line break (The comment above applies to more than just the line
immediately below it.  Removing the line break makes that clearer)

[meacer, 2014/12/16 01:23:19]

Done.

+  content::RenderViewHost* rvh =
+      broken_tab_contents->GetInterstitialPage()->GetRenderViewHostForTesting();
+  EXPECT_TRUE(WaitForPageReady(rvh));

[mmenke, 2014/12/15 20:46:08]

Suggest a comment above this line, about having to wait for the interstitial to
load so it has the button to click.

[meacer, 2014/12/16 01:23:19]

Done.

+  EXPECT_TRUE(
+      content::ExecuteScript(
+          rvh->GetMainFrame(),
+          "document.getElementById('primary-button').click();"));
+  EXPECT_EQ(login_tab_index, tab_strip_model->active_index());

[mmenke, 2014/12/15 20:46:07]

Should we have a test where this opens another tab?

[meacer, 2014/12/16 01:23:19]

Added that scenario here: After the first click on the "connect" button in the
error page, this test now closes the login tab, clicks the "connect" button
again and expects a new login tab to open.

+
+  LoginCertError(browser());
+
+  // Once logged in, broken tab should reload and display the SSL interstitial.
+  WaitForInterstitialAttach(broken_tab_contents);
+  tab_strip_model->ActivateTabAt(cert_error_tab_index, false);
+
+  EXPECT_EQ(SSLBlockingPage::kTypeForTesting,
+            GetInterstitialType(tab_strip_model->GetActiveWebContents()));
+
+  // Trigger another captive portal check while the SSL interstitial is showing.
+  // At this point the user is logged in to the captive portal, so the captive
+  // portal interstitial shouldn't get recreated.
   CaptivePortalObserver portal_observer(browser()->profile());
-
-  TabStripModel* tab_strip_model = browser()->tab_strip_model();
-  content::RenderFrameHost* render_frame_host =
-      tab_strip_model->GetActiveWebContents()->GetMainFrame();
-  render_frame_host->ExecuteJavaScript(base::ASCIIToUTF16("submitForm()"));
-
-  // The captive portal tab navigation will trigger a captive portal check,
-  // and reloading the original tab will bring up the interstitial page again,
-  // triggering a second captive portal check.
-  portal_observer.WaitForResults(2);
-
-  // Wait for both tabs to finish loading.
-  navigation_observer.WaitForNavigations(2);
-  EXPECT_EQ(2, portal_observer.num_results_received());
-  EXPECT_FALSE(CheckPending(browser()));
-  EXPECT_EQ(captive_portal::RESULT_INTERNET_CONNECTED,
-            portal_observer.captive_portal_result());
-
-  // Check state of tabs.  While the first tab is still displaying an
-  // interstitial page, since no portal was found, it should be in STATE_NONE,
-  // as should the login tab.
-  ASSERT_EQ(2, tab_strip_model->count());
-  EXPECT_EQ(CaptivePortalTabReloader::STATE_NONE,
-            GetStateOfTabReloaderAt(browser(), 0));
-  EXPECT_FALSE(IsLoginTab(tab_strip_model->GetWebContentsAt(1)));
-  EXPECT_EQ(CaptivePortalTabReloader::STATE_NONE,
-            GetStateOfTabReloaderAt(browser(), 1));
-
-  // Make sure only one navigation was for the login tab.
-  EXPECT_EQ(1, navigation_observer.NumNavigationsForTab(
-                   tab_strip_model->GetWebContentsAt(1)));
+  CaptivePortalService* captive_portal_service =
+      CaptivePortalServiceFactory::GetForProfile(browser()->profile());
+  captive_portal_service->DetectCaptivePortal();
+  portal_observer.WaitForResults(1);
+  EXPECT_EQ(SSLBlockingPage::kTypeForTesting,
+            GetInterstitialType(broken_tab_contents));
+
+  // A captive portal appears. Trigger a final captive portal check. The
+  // captive portal interstitial should still not get recreated.
+  URLRequestMockCaptivePortalJobFactory::SetBehindCaptivePortal(true);
+  CaptivePortalObserver final_portal_observer(browser()->profile());
+  captive_portal_service->DetectCaptivePortal();
+  final_portal_observer.WaitForResults(1);
+  EXPECT_EQ(SSLBlockingPage::kTypeForTesting,
+            GetInterstitialType(broken_tab_contents));
+}
+
+void CaptivePortalBrowserTest::FastErrorWithInterstitialTimer(

[mmenke, 2014/12/15 20:46:07]

This should go up with the other CaptivePortalBrowserTest methods, in the same
order it is declared.

[meacer, 2014/12/16 01:23:18]

Done.

+    Browser* browser,
+    const GURL& cert_error_url) {
+  TabStripModel* tab_strip_model = browser->tab_strip_model();
+  WebContents* broken_tab_contents = tab_strip_model->GetActiveWebContents();
+
+  // Disable captive portal checks indefinitely.
+  EnablePortalRequests(false);
+
+  content::WindowedNotificationObserver signal(
+      chrome::NOTIFICATION_SSL_INTERSTITIAL_TIMER_FIRED,
+      content::Source<WebContents>(broken_tab_contents));
+  ui_test_utils::NavigateToURLWithDisposition(browser,
+                                              cert_error_url,
+                                              CURRENT_TAB,
+                                              ui_test_utils::BROWSER_TEST_NONE);
+  signal.Wait();
+  ASSERT_EQ(broken_tab_contents,
+            content::Source<WebContents>(signal.source()).ptr());
+
+  // 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
+  // are disabled and timer set to expire after a very long time, the tab should
+  // hang indefinitely.
+  EXPECT_TRUE(broken_tab_contents->IsLoading());
+  EXPECT_EQ(1, NumLoadingTabs());
+}
+
+// Tests this scenario:
+// - Portal checks are disabled.

[mmenke, 2014/12/15 20:46:08]

Should clarify this - The CaptivePortalService has its own notion of "disabled"
which differs strongly from the concept here.

[meacer, 2014/12/16 01:23:18]

Done.

+// - A cert error triggers an interstitial timer with a very long timeout.
+// - No captive portal results arrive, causing the tab to appear as loading
+//   indefinitely.
+// - Stopping the page load shouldn't result in any interstitials.
+IN_PROC_BROWSER_TEST_F(CaptivePortalBrowserTest,
+                       InterstitialTimerStopNavigationWhileLoading) {
+  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);
+
+  SSLErrorHandler::SetInterstitialDisplayDelayForTest(
+      base::TimeDelta::FromHours(1));
+  TabStripModel* tab_strip_model = browser()->tab_strip_model();
+  WebContents* broken_tab_contents = tab_strip_model->GetActiveWebContents();
+
+  CaptivePortalObserver portal_observer1(browser()->profile());
+  FastErrorWithInterstitialTimer(browser(), cert_error_url);
+
+  // Page appears loading. Stop the navigation. There should be no interstitial.
+  MultiNavigationObserver test_navigation_observer;
+  broken_tab_contents->Stop();
+  test_navigation_observer.WaitForNavigations(1);
+
+  EXPECT_FALSE(broken_tab_contents->ShowingInterstitialPage());
+  EXPECT_FALSE(broken_tab_contents->IsLoading());
+  EXPECT_EQ(0, portal_observer1.num_results_received());
+  EXPECT_EQ(0, NumLoadingTabs());
+  EXPECT_FALSE(CheckPending(browser()));
+  EXPECT_EQ(1, browser()->tab_strip_model()->count());
+  EXPECT_EQ(CaptivePortalTabReloader::STATE_NONE,
+            GetStateOfTabReloaderAt(browser(), 0));
+
+  // Re-enable captive portal checks and fire one. The result should be ignored.
+  EnablePortalRequests(true);
+  CaptivePortalObserver portal_observer2(browser()->profile());
+  CaptivePortalService* captive_portal_service =
+      CaptivePortalServiceFactory::GetForProfile(browser()->profile());
+  captive_portal_service->DetectCaptivePortal();
+  portal_observer2.WaitForResults(1);
+
+  EXPECT_FALSE(broken_tab_contents->ShowingInterstitialPage());
+  EXPECT_FALSE(broken_tab_contents->IsLoading());
+  EXPECT_EQ(1, portal_observer2.num_results_received());
+  EXPECT_EQ(captive_portal::RESULT_BEHIND_CAPTIVE_PORTAL,
+            portal_observer2.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 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,
+                       InterstitialTimerReloadWhileLoading) {
+  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);
+
+  // Set SSL interstitial delay long enough so that a captive portal result
+  // is guaranteed to arrive during this window, and a captive portal
+  // error page is displayed instead of an SSL interstitial.
+  SSLErrorHandler::SetInterstitialDisplayDelayForTest(
+      base::TimeDelta::FromHours(1));
+  TabStripModel* tab_strip_model = browser()->tab_strip_model();
+  WebContents* broken_tab_contents = tab_strip_model->GetActiveWebContents();
+
+  CaptivePortalObserver portal_observer(browser()->profile());
+  FastErrorWithInterstitialTimer(browser(), cert_error_url);
+
+  // Page appears loading. Reloading should cancel the navigation and there
+  // should be no interstitial.
+  MultiNavigationObserver test_navigation_observer;
+  chrome::Reload(browser(), CURRENT_TAB);
+  test_navigation_observer.WaitForNavigations(2);
+
+  EXPECT_FALSE(broken_tab_contents->ShowingInterstitialPage());
+  EXPECT_FALSE(broken_tab_contents->IsLoading());
+  EXPECT_EQ(0, portal_observer.num_results_received());
+  EXPECT_EQ(2, test_navigation_observer.num_navigations());
+  EXPECT_EQ(0, NumLoadingTabs());
+  EXPECT_FALSE(CheckPending(browser()));
+  EXPECT_EQ(1, browser()->tab_strip_model()->count());
+  EXPECT_EQ(CaptivePortalTabReloader::STATE_NONE,
+            GetStateOfTabReloaderAt(browser(), 0));
+
+  // Re-enable captive portal checks and fire one. The result should be ignored.
+  EnablePortalRequests(true);

[mmenke, 2014/12/15 20:46:08]

Why don't we have a new SSLErrorHandler waiting on a probe result here?

[meacer, 2014/12/16 01:23:19]

Reloading effectively stops the navigation (this is the normal behavior with
slow loading pages). Since the load is stopped, there is no cert error and no
SSLErrorHandler.

+  CaptivePortalObserver portal_observer2(browser()->profile());
+  CaptivePortalService* captive_portal_service =
+      CaptivePortalServiceFactory::GetForProfile(browser()->profile());
+  captive_portal_service->DetectCaptivePortal();
+  portal_observer2.WaitForResults(1);
+
+  EXPECT_FALSE(broken_tab_contents->ShowingInterstitialPage());
+  EXPECT_FALSE(broken_tab_contents->IsLoading());
+  EXPECT_EQ(1, portal_observer2.num_results_received());
+  EXPECT_EQ(captive_portal::RESULT_BEHIND_CAPTIVE_PORTAL,
+            portal_observer2.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 instead of reloading, the page is navigated away. The new
+// page should load, and no interstitials shoudl be shown.

[mmenke, 2014/12/15 20:46:08]

nit:  shoudl -> should

[meacer, 2014/12/16 01:23:18]

Done.

+IN_PROC_BROWSER_TEST_F(CaptivePortalBrowserTest,
+                       InterstitialTimerNavigateAwayWhileLoading) {
+  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);
+
+  // Set SSL interstitial delay long enough so that a captive portal result
+  // is guaranteed to arrive during this window, and a captive portal
+  // error page is displayed instead of an SSL interstitial.
+  SSLErrorHandler::SetInterstitialDisplayDelayForTest(
+      base::TimeDelta::FromHours(1));
+  TabStripModel* tab_strip_model = browser()->tab_strip_model();
+  WebContents* broken_tab_contents = tab_strip_model->GetActiveWebContents();
+
+  CaptivePortalObserver portal_observer(browser()->profile());
+  FastErrorWithInterstitialTimer(browser(), cert_error_url);
+
+  // Page appears loading. Navigating away shouldn't result in any interstitial.
+  // Can't use ui_test_utils::NavigateToURLWithDisposition because it waits for
+  // a load stop notification before starting a new navigation.
+  MultiNavigationObserver test_navigation_observer;
+  browser()->OpenURL(content::OpenURLParams(
+      URLRequestMockHTTPJob::GetMockUrl(
+          base::FilePath(FILE_PATH_LITERAL("title2.html"))),
+      content::Referrer(),
+      CURRENT_TAB,
+      ui::PAGE_TRANSITION_TYPED, false));
+  test_navigation_observer.WaitForNavigations(2);

[mmenke, 2014/12/15 20:46:08]

Why are there two navigations here?

[meacer, 2014/12/16 01:23:19]

First one is for stopping the hanging navigation, second one is for completing
the second page load. The second one is generated because of the
FrameHostMsg_DidStopLoading IPC.

+
+  EXPECT_FALSE(broken_tab_contents->ShowingInterstitialPage());
+  EXPECT_FALSE(broken_tab_contents->IsLoading());
+  EXPECT_EQ(0, portal_observer.num_results_received());
+  EXPECT_EQ(2, test_navigation_observer.num_navigations());
+  EXPECT_EQ(0, NumLoadingTabs());
+  EXPECT_FALSE(CheckPending(browser()));
+  EXPECT_EQ(1, browser()->tab_strip_model()->count());
+  EXPECT_EQ(CaptivePortalTabReloader::STATE_NONE,
+            GetStateOfTabReloaderAt(browser(), 0));
+
+  // Re-enable captive portal checks and fire one. The result should be ignored.
+  EnablePortalRequests(true);
+  CaptivePortalObserver portal_observer2(browser()->profile());
+  CaptivePortalService* captive_portal_service =
+      CaptivePortalServiceFactory::GetForProfile(browser()->profile());
+  captive_portal_service->DetectCaptivePortal();
+  portal_observer2.WaitForResults(1);
+
+  EXPECT_FALSE(broken_tab_contents->ShowingInterstitialPage());
+  EXPECT_FALSE(broken_tab_contents->IsLoading());
+  EXPECT_EQ(1, portal_observer2.num_results_received());
+  EXPECT_EQ(captive_portal::RESULT_BEHIND_CAPTIVE_PORTAL,
+            portal_observer2.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));
+}
+
+// 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) {
+  // Need an HTTP TestServer to handle a dynamically created server redirect.
+  ASSERT_TRUE(test_server()->Start());
+
+  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());
+
+  // Set SSL interstitial delay to zero so that a captive portal result can not
+  // arrive during this window, so an SSL interstitial is displayed instead
+  // of a captive portal error page.
+  SSLErrorHandler::SetInterstitialDisplayDelayForTest(base::TimeDelta());
+  TabStripModel* tab_strip_model = browser()->tab_strip_model();
+  WebContents* broken_tab_contents = tab_strip_model->GetActiveWebContents();
+
+  // Setting the delay to zero above has a race condition: A captive portal
+  // result triggered by a cert error can arrive before the SSL interstitial
+  // display timer is fired, even though it's set to zero.
+  // To avoid this, disable captive portal checks until the SSL interstitial is
+  // displayed. Once it's displayed, enable portal checks and fire one.
+  bool disable_portal_check_until_interstitial = true;
+
+  // The path does not matter.
+  GURL cert_error_url = https_server.GetURL(kTestServerLoginPath);
+  // The interstitial should trigger a captive portal check when it opens, just
+  // like navigating to kMockHttpsQuickTimeoutUrl.

[mmenke, 2014/12/15 20:46:07]

Wonder about this comment - we actually trigger the check in
FastErrorBehindCaptivePortal itself, rather than relying on the interstitial's
check.

[meacer, 2014/12/16 01:23:19]

The comment is from the current SSLCertErrorLogin code (even though rietveld
shows this as new code, it's not). I admit I don't quite understand what it
referred to before, but tried to clarify.

+  FastErrorBehindCaptivePortal(
+      browser(),
+      true,
+      cert_error_url,
+      disable_portal_check_until_interstitial);
+
+  EXPECT_EQ(SSLBlockingPage::kTypeForTesting,
+            GetInterstitialType(broken_tab_contents));
+
+  LoginCertError(browser());
 }
 
 // Tries navigating both the tab that encounters an SSL timeout and the
 // login tab twice, only logging in the second time.
 IN_PROC_BROWSER_TEST_F(CaptivePortalBrowserTest, LoginExtraNavigations) {
   FastTimeoutBehindCaptivePortal(browser(), true);
 
   // Activate the timed out tab and navigate it to a timeout again.
   TabStripModel* tab_strip_model = browser()->tab_strip_model();
   tab_strip_model->ActivateTabAt(0, true);
@@ skipping 432 matching lines @@
   content::BrowserThread::PostTask(
       content::BrowserThread::IO, FROM_HERE,
       base::Bind(&AddHstsHost,
                  make_scoped_refptr(browser()->profile()->GetRequestContext()),
                  http_timeout_url.host()));
 
   SlowLoadBehindCaptivePortal(browser(), true, http_timeout_url, 1, 1);
   Login(browser(), 1, 0);
   FailLoadsAfterLogin(browser(), 1);
 }
+
+// A slow SSL load starts. The reloader triggers a captive portal check, finds a
+// captive portal. The SSL commits with a cert error, triggering another captive
+// portal check.
+// The second check finds no captive portal. The reloader triggers a reload at
+// the same time SSL error handler tries to show an interstitial. Should result
+// in an SSL interstitial.
+IN_PROC_BROWSER_TEST_F(CaptivePortalBrowserTest,
+                       InterstitialTimerCertErrorAfterSlowLoad) {
+  // Use a url that triggers a slow load, instead of creating an https server.
+  GURL cert_error_url = GURL(kMockHttpsUrl);
+
+  TabStripModel* tab_strip_model = browser()->tab_strip_model();
+  int broken_tab_index = tab_strip_model->active_index();
+  WebContents* broken_tab_contents = tab_strip_model->GetActiveWebContents();
+  CaptivePortalTabReloader* tab_reloader = GetTabReloader(broken_tab_contents);
+  SetSlowSSLLoadTime(tab_reloader, base::TimeDelta());

[mmenke, 2014/12/15 20:46:07]

Shouldn't we be setting the 1 hour time here?

[meacer, 2014/12/16 01:23:18]

This is the timeout for the slow ssl timer, not the interstitial. Anyways,
removed because it's not actually necessary here, as SlowLoadBehindCaptivePortal
already sets it to zero.

+
+  SlowLoadBehindCaptivePortal(browser(), true, cert_error_url, 1, 1);
+
+  // No longer behind a captive portal. Committing the SSL page should trigger
+  // an SSL interstitial which triggers a new captive portal check. Since there
+  // is no captive portal anymore, should end up with an SSL interstitial.
+  URLRequestMockCaptivePortalJobFactory::SetBehindCaptivePortal(false);
+
+  CaptivePortalObserver portal_observer(browser()->profile());
+  MultiNavigationObserver navigation_observer;
+  URLRequestTimeoutOnDemandJob::FailJobsWithCertError(1);
+  navigation_observer.WaitForNavigations(1);
+
+  EXPECT_EQ(CaptivePortalTabReloader::STATE_NEEDS_RELOAD,
+            GetStateOfTabReloaderAt(browser(), broken_tab_index));
+
+  WaitForInterstitialAttach(broken_tab_contents);
+  portal_observer.WaitForResults(1);
+
+  EXPECT_EQ(SSLBlockingPage::kTypeForTesting,
+            GetInterstitialType(broken_tab_contents));
+}