Add custom interstitial for captive portals.

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 2a8749ec8fd09f1f53dd64776238fa4d0ede991b..340ac2a47395b143dcaef6527aeb327aebbc6443 100644
--- a/chrome/browser/captive_portal/captive_portal_browsertest.cc
+++ b/chrome/browser/captive_portal/captive_portal_browsertest.cc
@@ -19,8 +19,12 @@
 #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"
@@ -33,14 +37,19 @@
 #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"
@@ -109,6 +118,22 @@ const char* const kMockHttpsQuickTimeoutUrl =
 // 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.
@@ -125,10 +150,14 @@ class URLRequestTimeoutOnDemandJob : public net::URLRequestJob,
 
   // 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);
@@ -140,6 +169,7 @@ class URLRequestTimeoutOnDemandJob : public net::URLRequestJob,
   enum EndJobOperation {
     FAIL_JOBS,
     ABANDON_JOBS,
+    FAIL_JOBS_WITH_CERT_ERROR
   };
 
   URLRequestTimeoutOnDemandJob(net::URLRequest* request,
@@ -226,6 +256,16 @@ void URLRequestTimeoutOnDemandJob::FailJobs(int expected_num_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,
@@ -313,6 +353,13 @@ void URLRequestTimeoutOnDemandJob::FailOrAbandonJobsOnIOThread(
       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);
     }
   }
 
@@ -689,8 +736,8 @@ 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);
 
@@ -808,6 +855,10 @@ class CaptivePortalBrowserTest : public InProcessBrowserTest {
   // 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);
@@ -816,6 +867,9 @@ class CaptivePortalBrowserTest : public InProcessBrowserTest {
   // 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;
@@ -897,9 +951,17 @@ class CaptivePortalBrowserTest : public InProcessBrowserTest {
   // 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.
@@ -916,6 +978,12 @@ class CaptivePortalBrowserTest : public InProcessBrowserTest {
   // 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.
@@ -990,6 +1058,20 @@ void CaptivePortalBrowserTest::EnableCaptivePortalDetection(
   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 =
@@ -1012,6 +1094,17 @@ bool CaptivePortalBrowserTest::CheckPending(Browser* browser) {
       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();
@@ -1244,13 +1337,15 @@ void CaptivePortalBrowserTest::FastTimeoutBehindCaptivePortal(
     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.
@@ -1274,12 +1369,27 @@ void CaptivePortalBrowserTest::FastErrorBehindCaptivePortal(
     ++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) {
@@ -1407,6 +1517,43 @@ void CaptivePortalBrowserTest::Login(Browser* browser,
                    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());
@@ -1705,11 +1852,9 @@ IN_PROC_BROWSER_TEST_F(CaptivePortalBrowserTest, LoginFastTimeout) {
 }
 
 // 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;
@@ -1718,49 +1863,326 @@ IN_PROC_BROWSER_TEST_F(CaptivePortalBrowserTest, SSLCertErrorLogin) {
       base::FilePath(FILE_PATH_LITERAL("chrome/test/data")));
   ASSERT_TRUE(https_server.Start());
 
+  // 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);
+  FastErrorBehindCaptivePortal(browser(), true, cert_error_url, false);
 
-  // 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;
+  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());
+  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();
-  content::RenderFrameHost* render_frame_host =
-      tab_strip_model->GetActiveWebContents()->GetMainFrame();
-  render_frame_host->ExecuteJavaScript(base::ASCIIToUTF16("submitForm()"));
+  WebContents* broken_tab_contents = tab_strip_model->GetActiveWebContents();
 
-  // 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);
+  CaptivePortalObserver portal_observer1(browser()->profile());
+  FastErrorWithInterstitialTimer(browser(), cert_error_url);
 
-  // Wait for both tabs to finish loading.
-  navigation_observer.WaitForNavigations(2);
-  EXPECT_EQ(2, portal_observer.num_results_received());
+  // 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(captive_portal::RESULT_INTERNET_CONNECTED,
-            portal_observer.captive_portal_result());
+  EXPECT_EQ(1, browser()->tab_strip_model()->count());
+  EXPECT_EQ(CaptivePortalTabReloader::STATE_NONE,
+            GetStateOfTabReloaderAt(browser(), 0));
 
-  // 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());
+  // 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));
-  EXPECT_FALSE(IsLoginTab(tab_strip_model->GetWebContentsAt(1)));
+}
+
+// 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(), 1));
+            GetStateOfTabReloaderAt(browser(), 0));
 
-  // Make sure only one navigation was for the login tab.
-  EXPECT_EQ(1, navigation_observer.NumNavigationsForTab(
-                   tab_strip_model->GetWebContentsAt(1)));
+  // 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
@@ -2213,3 +2635,42 @@ IN_PROC_BROWSER_TEST_F(CaptivePortalBrowserTest, HstsLogin) {
   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));
+}