Bug 1303775 - Fix race conditions prevalent with closing captive portal tabs that re-direct to the canonicalURL after successful login/abort. r=johannh,nhnt11

Differential Revision: https://phabricator.services.mozilla.com/D65554

--HG--
extra : moz-landing-system : lando

browser/base/content/browser-captivePortal.js

@@ -22,6 +22,10 @@ var CaptivePortalWatcher = {
   // This flag exists so that tests can appropriately simulate a recheck.
   _waitingForRecheck: false,
 
+  // This holds a weak reference to the captive portal tab so we can close the tab
+  // after successful login if we're redirected to the canonicalURL.
+  _previousCaptivePortalTab: null,
+
   get _captivePortalNotification() {
     return gHighPriorityNotificationBox.getNotificationWithValue(
       this.PORTAL_NOTIFICATION_VALUE
@@ -108,6 +112,45 @@ var CaptivePortalWatcher = {
     }
   },
 
+  onLocationChange(browser) {
+    if (!this._previousCaptivePortalTab) {
+      return;
+    }
+
+    let tab = this._previousCaptivePortalTab.get();
+    if (!tab || !tab.linkedBrowser) {
+      return;
+    }
+
+    if (browser != tab.linkedBrowser) {
+      return;
+    }
+
+    // There is a race between the release of captive portal i.e.
+    // the time when success/abort events are fired and the time when
+    // the captive portal tab redirects to the canonicalURL. We check for
+    // both conditions to be true and also check that we haven't already removed
+    // the captive portal tab in the success/abort event handlers before we remove
+    // it in the callback below. A tick is added to avoid removing the tab before
+    // onLocationChange handlers across browser code are executed.
+    Services.tm.dispatchToMainThread(() => {
+      if (!this._previousCaptivePortalTab) {
+        return;
+      }
+
+      tab = this._previousCaptivePortalTab.get();
+      let canonicalURI = Services.io.newURI(this.canonicalURL);
+      if (
+        tab &&
+        tab.linkedBrowser.currentURI.equalsExceptRef(canonicalURI) &&
+        (this._cps.state == this._cps.UNLOCKED_PORTAL ||
+          this._cps.state == this._cps.UNKNOWN)
+      ) {
+        gBrowser.removeTab(tab);
+      }
+    });
+  },
+
   _captivePortalDetected() {
     if (this._delayedCaptivePortalDetectedInProgress) {
       return;
@@ -178,9 +221,24 @@ var CaptivePortalWatcher = {
   },
 
   _captivePortalGone() {
-    this._captivePortalTab = null;
     this._cancelDelayedCaptivePortal();
     this._removeNotification();
+
+    if (!this._captivePortalTab) {
+      return;
+    }
+
+    let tab = this._captivePortalTab.get();
+    let canonicalURI = Services.io.newURI(this.canonicalURL);
+    if (
+      tab &&
+      tab.linkedBrowser &&
+      tab.linkedBrowser.currentURI.equalsExceptRef(canonicalURI)
+    ) {
+      this._previousCaptivePortalTab = null;
+      gBrowser.removeTab(tab);
+    }
+    this._captivePortalTab = null;
   },
 
   _cancelDelayedCaptivePortal() {
@@ -286,28 +344,9 @@ var CaptivePortalWatcher = {
         disableTRR: true,
       });
       this._captivePortalTab = Cu.getWeakReference(tab);
+      this._previousCaptivePortalTab = Cu.getWeakReference(tab);
     }
 
     gBrowser.selectedTab = tab;
-
-    let canonicalURI = Services.io.newURI(this.canonicalURL);
-
-    // When we are no longer captive, close the tab if it's at the canonical URL.
-    let tabCloser = () => {
-      Services.obs.removeObserver(tabCloser, "captive-portal-login-abort");
-      Services.obs.removeObserver(tabCloser, "captive-portal-login-success");
-      if (
-        !tab ||
-        tab.closing ||
-        !tab.parentNode ||
-        !tab.linkedBrowser ||
-        !tab.linkedBrowser.currentURI.equalsExceptRef(canonicalURI)
-      ) {
-        return;
-      }
-      gBrowser.removeTab(tab);
-    };
-    Services.obs.addObserver(tabCloser, "captive-portal-login-abort");
-    Services.obs.addObserver(tabCloser, "captive-portal-login-success");
   },
 };

browser/base/content/browser.js

@@ -5930,6 +5930,7 @@ var TabsProgressListener = {
     gBrowser.getNotificationBox(aBrowser).removeTransientNotifications();
 
     FullZoom.onLocationChange(aLocationURI, false, aBrowser);
+    CaptivePortalWatcher.onLocationChange(aBrowser);
   },
 
   onLinkIconAvailable(browser, dataURI, iconURI) {

browser/base/content/test/captivePortal/browser.ini

@@ -8,3 +8,4 @@ skip-if = os == "win" # Bug 1313894
 skip-if = os == "win" # Bug 1313894
 [browser_captivePortal_certErrorUI.js]
 [browser_captivePortalTabReference.js]
+[browser_closeCapPortalTabCanonicalURL.js]

browser/base/content/test/captivePortal/browser_closeCapPortalTabCanonicalURL.js

@@ -0,0 +1,203 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+const { HttpServer } = ChromeUtils.import("resource://testing-common/httpd.js");
+const LOGIN_LINK = `<html><body><a href="/unlock">login</a></body></html>`;
+const BAD_CERT_PAGE = "https://expired.example.com/";
+const LOGIN_URL = "http://localhost:8080/login";
+const CANONICAL_SUCCESS_URL = "http://localhost:8080/success";
+const CPS = Cc["@mozilla.org/network/captive-portal-service;1"].getService(
+  Ci.nsICaptivePortalService
+);
+
+let server;
+let loginPageShown = false;
+
+async function openCaptivePortalErrorTab() {
+  await portalDetected();
+
+  // Open a page with a cert error.
+  let browser;
+  let certErrorLoaded;
+  let errorTab = await BrowserTestUtils.openNewForegroundTab(
+    gBrowser,
+    () => {
+      let tab = BrowserTestUtils.addTab(gBrowser, BAD_CERT_PAGE);
+      gBrowser.selectedTab = tab;
+      browser = gBrowser.selectedBrowser;
+      certErrorLoaded = BrowserTestUtils.waitForErrorPage(browser);
+      return tab;
+    },
+    false
+  );
+  await certErrorLoaded;
+  info("A cert error page was opened");
+  await SpecialPowers.spawn(errorTab.linkedBrowser, [], async () => {
+    let doc = content.document;
+    let loginButton = doc.getElementById("openPortalLoginPageButton");
+    await ContentTaskUtils.waitForCondition(
+      () => loginButton && doc.body.className == "captiveportal",
+      "Captive portal error page UI is visible"
+    );
+  });
+  info("Captive portal error page UI is visible");
+
+  return errorTab;
+}
+
+async function openCaptivePortalLoginTab(errorTab) {
+  let portalTabPromise = BrowserTestUtils.waitForNewTab(gBrowser, LOGIN_URL);
+
+  await SpecialPowers.spawn(errorTab.linkedBrowser, [], async () => {
+    let doc = content.document;
+    let loginButton = doc.getElementById("openPortalLoginPageButton");
+    info("Click on the login button on the captive portal error page");
+    await EventUtils.synthesizeMouseAtCenter(loginButton, {}, content);
+  });
+
+  let portalTab = await portalTabPromise;
+  is(
+    gBrowser.selectedTab,
+    portalTab,
+    "Captive Portal login page is now open in a new foreground tab."
+  );
+
+  return portalTab;
+}
+
+function redirectHandler(request, response) {
+  if (loginPageShown) {
+    return;
+  }
+  response.setStatusLine(request.httpVersion, 302, "captive");
+  response.setHeader("Content-Type", "text/html");
+  response.setHeader("Location", LOGIN_URL);
+}
+
+function loginHandler(request, response) {
+  response.setHeader("Content-Type", "text/html");
+  response.bodyOutputStream.write(LOGIN_LINK, LOGIN_LINK.length);
+  loginPageShown = true;
+}
+
+function unlockHandler(request, response) {
+  response.setStatusLine(request.httpVersion, 302, "login complete");
+  response.setHeader("Content-Type", "text/html");
+  response.setHeader("Location", CANONICAL_SUCCESS_URL);
+}
+
+add_task(async function setup() {
+  // Set up a mock server for handling captive portal redirect.
+  server = new HttpServer();
+  server.registerPathHandler("/success", redirectHandler);
+  server.registerPathHandler("/login", loginHandler);
+  server.registerPathHandler("/unlock", unlockHandler);
+  server.start(8080);
+  info("Mock server is now set up for captive portal redirect");
+
+  await SpecialPowers.pushPrefEnv({
+    set: [
+      ["captivedetect.canonicalURL", CANONICAL_SUCCESS_URL],
+      ["captivedetect.canonicalContent", CANONICAL_CONTENT],
+    ],
+  });
+});
+
+// This test checks if the captive portal tab is removed after the
+// sucess/abort events are fired, assuming the tab has already redirected
+// to the canonical URL before they are fired.
+add_task(async function checkCaptivePortalTabCloseOnCanonicalURL_one() {
+  let errorTab = await openCaptivePortalErrorTab();
+  let tab = await openCaptivePortalLoginTab(errorTab);
+  let browser = tab.linkedBrowser;
+
+  let redirectedToCanonicalURL = BrowserTestUtils.browserLoaded(
+    browser,
+    false,
+    CANONICAL_SUCCESS_URL
+  );
+  let errorPageReloaded = BrowserTestUtils.waitForErrorPage(
+    errorTab.linkedBrowser
+  );
+
+  await SpecialPowers.spawn(browser, [], async () => {
+    let doc = content.document;
+    let loginButton = doc.querySelector("a");
+    await ContentTaskUtils.waitForCondition(
+      () => loginButton,
+      "Login button on the captive portal tab is visible"
+    );
+    info("Clicking the login button on the captive portal tab page");
+    await EventUtils.synthesizeMouseAtCenter(loginButton, {}, content);
+  });
+
+  await redirectedToCanonicalURL;
+  info(
+    "Re-direct to canonical URL in the captive portal tab was succcessful after login"
+  );
+
+  let tabClosed = BrowserTestUtils.waitForTabClosing(tab);
+  Services.obs.notifyObservers(null, "captive-portal-login-success");
+  await tabClosed;
+  info(
+    "Captive portal tab was closed on re-direct to canonical URL after login as expected"
+  );
+
+  await errorPageReloaded;
+  info("Captive portal error page was reloaded");
+  gBrowser.removeTab(errorTab);
+});
+
+// This test checks if the captive portal tab is removed on location change
+// i.e. when it is re-directed to the canonical URL long after success/abort
+// event handlers are executed.
+add_task(async function checkCaptivePortalTabCloseOnCanonicalURL_two() {
+  loginPageShown = false;
+  let errorTab = await openCaptivePortalErrorTab();
+  let tab = await openCaptivePortalLoginTab(errorTab);
+  let browser = tab.linkedBrowser;
+
+  let redirectedToCanonicalURL = BrowserTestUtils.waitForLocationChange(
+    gBrowser,
+    CANONICAL_SUCCESS_URL
+  );
+  let errorPageReloaded = BrowserTestUtils.waitForErrorPage(
+    errorTab.linkedBrowser
+  );
+
+  Services.obs.notifyObservers(null, "captive-portal-login-success");
+  await TestUtils.waitForCondition(
+    () => CPS.state == CPS.UNLOCKED_PORTAL,
+    "Captive portal is released"
+  );
+
+  let tabClosed = BrowserTestUtils.waitForTabClosing(tab);
+  await SpecialPowers.spawn(browser, [], async () => {
+    let doc = content.document;
+    let loginButton = doc.querySelector("a");
+    await ContentTaskUtils.waitForCondition(
+      () => loginButton,
+      "Login button on the captive portal tab is visible"
+    );
+    info("Clicking the login button on the captive portal tab page");
+    await EventUtils.synthesizeMouseAtCenter(loginButton, {}, content);
+  });
+
+  await redirectedToCanonicalURL;
+  info(
+    "Re-direct to canonical URL in the captive portal tab was succcessful after login"
+  );
+  await tabClosed;
+  info(
+    "Captive portal tab was closed on re-direct to canonical URL after login as expected"
+  );
+
+  await errorPageReloaded;
+  info("Captive portal error page was reloaded");
+  gBrowser.removeTab(errorTab);
+
+  // Stop the server.
+  await new Promise(r => server.stop(r));
+});