Bug 1303775 - Fix race conditions prevalent with closing captive portal tabs that re-direct to the canonicalURL after successful login/abort. r=johannh,nhnt11
authorprathiksha <prathikshaprasadsuman@gmail.com>
Thu, 26 Mar 2020 19:04:59 +0000
changeset 520634 942cf82e21982304603725eb51d6543caa144dd4
parent 520633 9b0691360c54670250d56409e5e767a2fcf0c16e
child 520635 753c5e83ea11ca696f43fc9626defa61c6b6e534
push id37254
push usernerli@mozilla.com
push dateFri, 27 Mar 2020 04:48:07 +0000
treeherdermozilla-central@2d758b42bd73 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjohannh, nhnt11
bugs1303775
milestone76.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
browser/base/content/browser-captivePortal.js
browser/base/content/browser.js
browser/base/content/test/captivePortal/browser.ini
browser/base/content/test/captivePortal/browser_closeCapPortalTabCanonicalURL.js
--- a/browser/base/content/browser-captivePortal.js
+++ b/browser/base/content/browser-captivePortal.js
@@ -17,16 +17,20 @@ var CaptivePortalWatcher = {
    * another notification while waiting, we don't do things twice.
    */
   _delayedCaptivePortalDetectedInProgress: false,
 
   // In the situation above, this is set to true while we wait for the recheck.
   // 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
     );
   },
 
   get canonicalURL() {
     return Services.prefs.getCharPref("captivedetect.canonicalURL");
@@ -103,16 +107,55 @@ var CaptivePortalWatcher = {
         this._captivePortalGone();
         break;
       case "delayed-captive-portal-handled":
         this._cancelDelayedCaptivePortal();
         break;
     }
   },
 
+  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;
     }
 
     let win = BrowserWindowTracker.getTopWindow();
 
     // Used by tests: ignore the main test window in order to enable testing of
@@ -173,19 +216,34 @@ var CaptivePortalWatcher = {
         // focus a tab with the login page with no noticeable delay.
         this.ensureCaptivePortalTab();
       }
     };
     Services.obs.addObserver(observer, "captive-portal-check-complete");
   },
 
   _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() {
     if (this._delayedCaptivePortalDetectedInProgress) {
       this._delayedCaptivePortalDetectedInProgress = false;
       Services.obs.removeObserver(this, "delayed-captive-portal-handled");
       window.removeEventListener("activate", this);
     }
@@ -281,33 +339,14 @@ var CaptivePortalWatcher = {
         triggeringPrincipal: Services.scriptSecurityManager.createNullPrincipal(
           {
             userContextId: gBrowser.contentPrincipal.userContextId,
           }
         ),
         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");
   },
 };
--- a/browser/base/content/browser.js
+++ b/browser/base/content/browser.js
@@ -5925,16 +5925,17 @@ var TabsProgressListener = {
     let tab = gBrowser.getTabForBrowser(aBrowser);
     if (tab && tab._sharingState) {
       gBrowser.resetBrowserSharing(aBrowser);
     }
 
     gBrowser.getNotificationBox(aBrowser).removeTransientNotifications();
 
     FullZoom.onLocationChange(aLocationURI, false, aBrowser);
+    CaptivePortalWatcher.onLocationChange(aBrowser);
   },
 
   onLinkIconAvailable(browser, dataURI, iconURI) {
     if (!iconURI) {
       return;
     }
     if (browser == gBrowser.selectedBrowser) {
       // If the "Add Search Engine" page action is in the urlbar, its image
--- a/browser/base/content/test/captivePortal/browser.ini
+++ b/browser/base/content/test/captivePortal/browser.ini
@@ -3,8 +3,9 @@ support-files =
   head.js
 
 [browser_CaptivePortalWatcher.js]
 skip-if = os == "win" # Bug 1313894
 [browser_CaptivePortalWatcher_1.js]
 skip-if = os == "win" # Bug 1313894
 [browser_captivePortal_certErrorUI.js]
 [browser_captivePortalTabReference.js]
+[browser_closeCapPortalTabCanonicalURL.js]
new file mode 100644
--- /dev/null
+++ b/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));
+});