Improve captive portal test stability

Tests expecting a new popup window (just secure DNS tests) were
expecting the new window to always be the "last active" Browser window,
but that is apparently not the case despite everything appearing to work
correctly with creation of the new popup. Remove checks against
GetLastActive().

Also fix cases where it was assumed GetLastActive() could be used as a
shortcut to retrieve the popup window. Instead the code for validating
creation of the popup window can optionally return a pointer to the
created window.

Bug: 11139282
Change-Id: Ic900d0deb881eacc53099de07c5e8cdd68b39be9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2606525
Commit-Queue: Eric Orth <ericorth@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#840215}

chrome/browser/captive_portal/captive_portal_browsertest.cc

@@ -656,9 +656,14 @@ class CaptivePortalBrowserTest : public InProcessBrowserTest {
   // 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 WaitForJobs having been called.
+  //
+  // If not null, `out_login_browser` will be set to the browser window used for
+  // the login tab. If no login tab was opened, or if the window is
+  // undetermined, will set to null.
   void SlowLoadBehindCaptivePortal(Browser* browser,
                                    bool expect_open_login_tab,
-                                   bool expect_new_login_browser);
+                                   bool expect_new_login_browser = false,
+                                   Browser** out_login_browser = nullptr);
 
   // Same as above, but takes extra parameters.
   //
@@ -667,12 +672,17 @@ class CaptivePortalBrowserTest : public InProcessBrowserTest {
   // |expected_portal_checks| and |expected_login_tab_navigations| allow
   // client-side redirects to be tested.  |expected_login_tab_navigations| is
   // ignored when |expect_open_login_tab| is false.
+  //
+  // If not null, `out_login_browser` will be set to the browser window used for
+  // the login tab. If no login tab was opened, or if the window is
+  // undetermined, will set to null.
   void SlowLoadBehindCaptivePortal(Browser* browser,
                                    bool expect_open_login_tab,
                                    bool expect_new_login_browser,
                                    const GURL& hanging_url,
                                    int expected_portal_checks,
-                                   int expected_login_tab_navigations);
+                                   int expected_login_tab_navigations,
+                                   Browser** out_login_browser = nullptr);
 
   // Just like SlowLoadBehindCaptivePortal, except the navigated tab has
   // a connection timeout rather having its time trigger, and the function
@@ -683,10 +693,15 @@ 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.
+  //
+  // If not null, `out_login_browser` will be set to the browser window used for
+  // the login tab. If no login tab was opened, or if the window is
+  // undetermined, will set to null.
   void FastErrorBehindCaptivePortal(Browser* browser,
                                     bool expect_open_login_tab,
                                     bool expect_new_login_browser,
-                                    const GURL& error_url);
+                                    const GURL& error_url,
+                                    Browser** out_login_browser = nullptr);
 
   // Navigates the active tab to an SSL error page which triggers an
   // interstitial timer. Also disables captive portal checks indefinitely, so
@@ -728,7 +743,12 @@ class CaptivePortalBrowserTest : public InProcessBrowserTest {
   // 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);
+  //
+  // If non-null, `captive_portal_browser` specifies a separate popup window
+  // used for the captive portal login tab.
+  void FailLoadsWithoutLogin(Browser* browser,
+                             int num_loading_tabs,
+                             Browser* captive_portal_browser = nullptr);
 
   // Navigates |browser|'s active tab to |starting_url| while not behind a
   // captive portal.  Then navigates to |interrupted_url|, which should create
@@ -1286,10 +1306,11 @@ void CaptivePortalBrowserTest::FastTimeoutNoCaptivePortal(
 void CaptivePortalBrowserTest::SlowLoadBehindCaptivePortal(
     Browser* browser,
     bool expect_open_login_tab,
-    bool expect_new_login_browser = false) {
-  SlowLoadBehindCaptivePortal(browser, expect_open_login_tab,
-                              expect_new_login_browser, GURL(kMockHttpsUrl), 1,
-                              1);
+    bool expect_new_login_browser,
+    Browser** out_login_browser) {
+  return SlowLoadBehindCaptivePortal(
+      browser, expect_open_login_tab, expect_new_login_browser,
+      GURL(kMockHttpsUrl), 1, 1, out_login_browser);
 }
 
 void CaptivePortalBrowserTest::SlowLoadBehindCaptivePortal(
@@ -1298,7 +1319,8 @@ void CaptivePortalBrowserTest::SlowLoadBehindCaptivePortal(
     bool expect_new_login_browser,
     const GURL& hanging_url,
     int expected_portal_checks,
-    int expected_login_tab_navigations) {
+    int expected_login_tab_navigations,
+    Browser** out_login_browser) {
   ASSERT_GE(expected_portal_checks, 1);
   TabStripModel* tab_strip_model = browser->tab_strip_model();
   // Calling this on a tab that's waiting for a load to manually be timed out
@@ -1330,6 +1352,7 @@ void CaptivePortalBrowserTest::SlowLoadBehindCaptivePortal(
       ui_test_utils::BROWSER_TEST_NONE);
   portal_observer.WaitForResults(expected_portal_checks);
 
+  Browser* login_browser = nullptr;
   if (expect_open_login_tab) {
     ASSERT_GE(expected_login_tab_navigations, 1);
     navigation_observer.WaitForNavigations(expected_login_tab_navigations);
@@ -1341,14 +1364,11 @@ void CaptivePortalBrowserTest::SlowLoadBehindCaptivePortal(
       // Check the original browser
       ASSERT_EQ(initial_tab_count, tab_strip_model->count());
       EXPECT_EQ(initial_tab_count - 1, tab_strip_model->active_index());
-      EXPECT_NE(browser_list_->get(initial_browser_count - 1),
-                browser_list_->GetLastActive());
 
       // Check the new popup browser
-      Browser* popup_browser = browser_list_->get(initial_browser_count);
-      EXPECT_EQ(popup_browser, browser_list_->GetLastActive());
-      EXPECT_EQ(Browser::TYPE_POPUP, popup_browser->type());
-      login_tab = popup_browser->tab_strip_model()->GetWebContentsAt(0);
+      login_browser = browser_list_->get(initial_browser_count);
+      EXPECT_EQ(Browser::TYPE_POPUP, login_browser->type());
+      login_tab = login_browser->tab_strip_model()->GetWebContentsAt(0);
       EXPECT_TRUE(
           captive_portal::CaptivePortalTabHelper::FromWebContents(login_tab)
               ->is_captive_portal_window());
@@ -1359,6 +1379,7 @@ void CaptivePortalBrowserTest::SlowLoadBehindCaptivePortal(
       ASSERT_EQ(initial_tab_count + 1, tab_strip_model->count());
       EXPECT_EQ(initial_tab_count, tab_strip_model->active_index());
       login_tab = tab_strip_model->GetWebContentsAt(initial_tab_count);
+      login_browser = browser;
     }
 
     EXPECT_EQ(expected_login_tab_navigations,
@@ -1390,21 +1411,29 @@ void CaptivePortalBrowserTest::SlowLoadBehindCaptivePortal(
 
   // Reset the load time to be large, so the timer won't trigger on a reload.
   SetSlowSSLLoadTime(tab_reloader, base::TimeDelta::FromHours(1));
+
+  if (out_login_browser)
+    *out_login_browser = login_browser;
 }
 
 void CaptivePortalBrowserTest::FastTimeoutBehindCaptivePortal(
     Browser* browser,
     bool expect_open_login_tab) {
+  Browser* login_browser = nullptr;
   FastErrorBehindCaptivePortal(browser, expect_open_login_tab,
                                false /* expect_new_login_browser */,
-                               GURL(kMockHttpsQuickTimeoutUrl));
+                               GURL(kMockHttpsQuickTimeoutUrl), &login_browser);
+
+  // Expected result of setting `expect_new_login_browser` to false.
+  DCHECK(!login_browser || login_browser == browser);
 }
 
 void CaptivePortalBrowserTest::FastErrorBehindCaptivePortal(
     Browser* browser,
     bool expect_open_login_tab,
     bool expect_new_login_browser,
-    const GURL& error_url) {
+    const GURL& error_url,
+    Browser** out_login_browser) {
   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.
@@ -1437,6 +1466,7 @@ void CaptivePortalBrowserTest::FastErrorBehindCaptivePortal(
 
   portal_observer.WaitForResults(1);
 
+  Browser* login_browser = nullptr;
   if (expect_open_login_tab) {
     navigation_observer.WaitForNavigations(2);
     WebContents* login_tab;
@@ -1447,14 +1477,11 @@ void CaptivePortalBrowserTest::FastErrorBehindCaptivePortal(
       // Check the original browser
       ASSERT_EQ(initial_tab_count, tab_strip_model->count());
       EXPECT_EQ(initial_tab_count - 1, tab_strip_model->active_index());
-      EXPECT_NE(browser_list_->get(initial_browser_count - 1),
-                browser_list_->GetLastActive());
 
       // Check the new popup browser
-      Browser* popup_browser = browser_list_->get(initial_browser_count);
-      EXPECT_EQ(popup_browser, browser_list_->GetLastActive());
-      EXPECT_EQ(Browser::TYPE_POPUP, popup_browser->type());
-      login_tab = popup_browser->tab_strip_model()->GetWebContentsAt(0);
+      login_browser = browser_list_->get(initial_browser_count);
+      EXPECT_EQ(Browser::TYPE_POPUP, login_browser->type());
+      login_tab = login_browser->tab_strip_model()->GetWebContentsAt(0);
       EXPECT_TRUE(
           captive_portal::CaptivePortalTabHelper::FromWebContents(login_tab)
               ->is_captive_portal_window());
@@ -1465,6 +1492,7 @@ void CaptivePortalBrowserTest::FastErrorBehindCaptivePortal(
       ASSERT_EQ(initial_tab_count + 1, tab_strip_model->count());
       EXPECT_EQ(initial_tab_count, tab_strip_model->active_index());
       login_tab = tab_strip_model->GetWebContentsAt(initial_tab_count);
+      login_browser = browser;
     }
 
     EXPECT_EQ(1, navigation_observer.NumNavigationsForTab(
@@ -1492,6 +1520,9 @@ void CaptivePortalBrowserTest::FastErrorBehindCaptivePortal(
 
   EXPECT_EQ(captive_portal::CaptivePortalTabReloader::STATE_BROKEN_BY_PORTAL,
             GetStateOfTabReloaderAt(browser, initial_active_index));
+
+  if (out_login_browser)
+    *out_login_browser = login_browser;
 }
 
 void CaptivePortalBrowserTest::FastErrorWithInterstitialTimer(
@@ -1688,13 +1719,17 @@ void CaptivePortalBrowserTest::FailLoadsAfterLogin(Browser* browser,
   EXPECT_EQ(0, NumLoadingTabs());
 }
 
-void CaptivePortalBrowserTest::FailLoadsWithoutLogin(Browser* browser,
-                                                     int num_loading_tabs) {
+void CaptivePortalBrowserTest::FailLoadsWithoutLogin(
+    Browser* browser,
+    int num_loading_tabs,
+    Browser* captive_portal_browser) {
+  if (!captive_portal_browser)
+    captive_portal_browser = browser;
+
   ASSERT_EQ(num_loading_tabs, NumLoadingTabs());
   ASSERT_EQ(0, NumNeedReloadTabs());
   EXPECT_EQ(num_loading_tabs, NumBrokenTabs());
 
-  Browser* captive_portal_browser = browser_list_->GetLastActive();
   TabStripModel* tab_strip_model = captive_portal_browser->tab_strip_model();
   int initial_num_tabs = NumTabs();
   int login_tab = tab_strip_model->active_index();
@@ -2840,9 +2875,12 @@ IN_PROC_BROWSER_TEST_F(CaptivePortalBrowserTest,
   local_state->SetString(prefs::kDnsOverHttpsTemplates,
                          "https://bar.test/dns-query{?dns}");
 
+  Browser* login_browser = nullptr;
   SlowLoadBehindCaptivePortal(browser(), true /* expect_open_login_tab */,
-                              true /* expect_new_login_browser */);
-  FailLoadsWithoutLogin(browser(), 1);
+                              true /* expect_new_login_browser */,
+                              &login_browser);
+  ASSERT_TRUE(login_browser);
+  FailLoadsWithoutLogin(browser(), 1, login_browser);
 
   // A second navigation on the same tab should not cause a new captive portal
   // window to open.
@@ -2858,11 +2896,11 @@ IN_PROC_BROWSER_TEST_F(CaptivePortalBrowserTest,
                               false /* expect_open_login_tab */,
                               false /* expect_new_login_browser */);
   // Check that the existing captive portal popup is visible.
-  EXPECT_TRUE(browser_list_->get(1)->window()->IsVisible());
+  EXPECT_TRUE(login_browser->window()->IsVisible());
 
   // Login to the captive portal.
-  Login(browser_list_->get(1), 2 /* num_loading_tabs */,
-        0 /* num_timed_out_tabs */, 1 /* expected_portal_checks */);
+  Login(login_browser, 2 /* num_loading_tabs */, 0 /* num_timed_out_tabs */,
+        1 /* expected_portal_checks */);
 }
 
 // An HTTP load results in a secure DNS error, which triggers a captive portal
@@ -2878,9 +2916,12 @@ IN_PROC_BROWSER_TEST_F(CaptivePortalBrowserTest, SecureDnsErrorTriggersCheck) {
   TabStripModel* tab_strip_model = browser()->tab_strip_model();
   WebContents* broken_tab_contents = tab_strip_model->GetActiveWebContents();
 
+  Browser* login_browser = nullptr;
   FastErrorBehindCaptivePortal(browser(), true /* expect_open_login_tab */,
                                true /* expect_new_login_browser */,
-                               GURL(kMockHttpConnectionSecureDnsErr));
+                               GURL(kMockHttpConnectionSecureDnsErr),
+                               &login_browser);
+  ASSERT_TRUE(login_browser);
 
   // The navigated tab should be displaying an error page.
   EXPECT_TRUE(broken_tab_contents->GetController()
@@ -2890,8 +2931,8 @@ IN_PROC_BROWSER_TEST_F(CaptivePortalBrowserTest, SecureDnsErrorTriggersCheck) {
   // Login to the captive portal. The captive portal tab navigation will trigger
   // a captive portal check, and reloading the original tab will produce the
   // same secure DNS error, triggering a second captive portal check.
-  Login(browser_list_->get(1), 0 /* num_loading_tabs */,
-        1 /* num_timed_out_tabs */, 2 /* expected_portal_checks */);
+  Login(login_browser, 0 /* num_loading_tabs */, 1 /* num_timed_out_tabs */,
+        2 /* expected_portal_checks */);
 
   // The reload of the original page should have produced another DNS error
   // page.
@@ -2921,8 +2962,7 @@ IN_PROC_BROWSER_TEST_F(CaptivePortalBrowserTest,
   FailJobs(1, net::ERR_NAME_NOT_RESOLVED,
            net::ResolveErrorInfo(net::ERR_CERT_COMMON_NAME_INVALID, true));
   navigation_observer.WaitForNavigations(1);
-  WebContents* tab =
-      browser_list_->get(0)->tab_strip_model()->GetWebContentsAt(0);
+  WebContents* tab = browser()->tab_strip_model()->GetWebContentsAt(0);
   EXPECT_EQ(1, navigation_observer.NumNavigationsForTab(tab));
   EXPECT_TRUE(tab->GetController().GetLastCommittedEntry()->GetPageType() ==
               content::PAGE_TYPE_ERROR);
@@ -2942,12 +2982,15 @@ IN_PROC_BROWSER_TEST_F(CaptivePortalBrowserTest,
   local_state->SetString(prefs::kDnsOverHttpsMode,
                          SecureDnsConfig::kModeSecure);
 
+  Browser* login_browser = nullptr;
   SlowLoadBehindCaptivePortal(browser(), true /* expect_open_login_tab */,
-                              true /* expect_new_login_browser */);
+                              true /* expect_new_login_browser */,
+                              &login_browser);
+  ASSERT_TRUE(login_browser);
 
   // Login to the captive portal.
-  Login(browser_list_->get(1), 1 /* num_loading_tabs */,
-        0 /* num_timed_out_tabs */, 1 /* expected_portal_checks */);
+  Login(login_browser, 1 /* num_loading_tabs */, 0 /* num_timed_out_tabs */,
+        1 /* expected_portal_checks */);
 
   // Connection finally hits a secure DNS error. It should reload without
   // sending a new captive portal check.
@@ -2955,8 +2998,7 @@ IN_PROC_BROWSER_TEST_F(CaptivePortalBrowserTest,
   FailJobs(1, net::ERR_NAME_NOT_RESOLVED,
            net::ResolveErrorInfo(net::ERR_CERT_COMMON_NAME_INVALID, true));
   navigation_observer.WaitForNavigations(1);
-  WebContents* tab =
-      browser_list_->get(0)->tab_strip_model()->GetWebContentsAt(0);
+  WebContents* tab = browser()->tab_strip_model()->GetWebContentsAt(0);
   EXPECT_EQ(1, navigation_observer.NumNavigationsForTab(tab));
   EXPECT_TRUE(tab->GetController().GetLastCommittedEntry()->GetPageType() ==
               content::PAGE_TYPE_NORMAL);