Bug 1458918 - Prevent windows.getLastFocused from leaking tab being adopted by a new window. draft
authorLuca Greco <lgreco@mozilla.com>
Thu, 03 May 2018 16:24:02 +0200
changeset 792177 aa893e8070fd5aa9d7db6cb9d987a236de389f27
parent 791070 ce588e44f41599808a830db23d190d1ca474a781
push id109028
push userluca.greco@alcacoop.it
push dateMon, 07 May 2018 20:09:01 +0000
bugs1458918
milestone61.0a1
Bug 1458918 - Prevent windows.getLastFocused from leaking tab being adopted by a new window. MozReview-Commit-ID: 6Nj7J7aI06B
browser/base/content/tabbrowser.js
browser/components/extensions/parent/ext-browser.js
browser/components/extensions/test/browser/browser_ext_windows_create_tabId.js
toolkit/components/extensions/parent/ext-tabs-base.js
toolkit/components/extensions/parent/ext-webNavigation.js
--- a/browser/base/content/tabbrowser.js
+++ b/browser/base/content/tabbrowser.js
@@ -2992,16 +2992,22 @@ window._gBrowser = {
 
     return tab;
   },
 
   _blurTab(aTab) {
     this.selectedTab = this._findTabToBlurTo(aTab);
   },
 
+  // Used in WebExtensions internals to check if a new window is currently adopting
+  // an existent tab as its first tab.
+  isAdoptingFirstTab() {
+    return window.arguments && window.arguments[0] instanceof window.XULElement;
+  },
+
   swapBrowsersAndCloseOther(aOurTab, aOtherTab) {
     // Do not allow transfering a private tab to a non-private window
     // and vice versa.
     if (PrivateBrowsingUtils.isWindowPrivate(window) !=
       PrivateBrowsingUtils.isWindowPrivate(aOtherTab.ownerGlobal))
       return;
 
     let ourBrowser = this.getBrowserForTab(aOurTab);
--- a/browser/components/extensions/parent/ext-browser.js
+++ b/browser/components/extensions/parent/ext-browser.js
@@ -395,21 +395,17 @@ class TabTracker extends TabTrackerBase 
    * A private method which is called whenever a new browser window is opened,
    * and dispatches the necessary events for it.
    *
    * @param {DOMWindow} window
    *        The window being opened.
    * @private
    */
   _handleWindowOpen(window) {
-    if (window.arguments && window.arguments[0] instanceof window.XULElement) {
-      // If the first window argument is a XUL element, it means the
-      // window is about to adopt a tab from another window to replace its
-      // initial tab.
-      //
+    if (window.gBrowser.isAdoptingFirstTab()) {
       // Note that this event handler depends on running before the
       // delayed startup code in browser.js, which is currently triggered
       // by the first MozAfterPaint event. That code handles finally
       // adopting the tab, and clears it from the arguments list in the
       // process, so if we run later than it, we're too late.
       let nativeTab = window.arguments[0];
       let adoptedBy = window.gBrowser.tabs[0];
 
@@ -849,26 +845,38 @@ class Window extends WindowBase {
         break;
 
       default:
         throw new Error(`Unexpected window state: ${state}`);
     }
   }
 
   * getTabs() {
+    // Return an empty iterator while a new window is adopting an existing tab
+    // as its first tab (See Bug 1458918 for a rationale).
+    if (this.window.gBrowser.isAdoptingFirstTab()) {
+      return;
+    }
+
     let {tabManager} = this.extension;
 
     for (let nativeTab of this.window.gBrowser.tabs) {
       yield tabManager.getWrapper(nativeTab);
     }
   }
 
   get activeTab() {
     let {tabManager} = this.extension;
 
+    // Do not create a TabWrapper for the first tab of a new window that is currently
+    // adopting its first tab (See Bug 1458918 for rationale).
+    if (this.window.gBrowser.isAdoptingFirstTab()) {
+      return null;
+    }
+
     return tabManager.getWrapper(this.window.gBrowser.selectedTab);
   }
 
   getTabAtIndex(index) {
     let nativeTab = this.window.gBrowser.tabs[index];
     if (nativeTab) {
       return this.extension.tabManager.getWrapper(nativeTab);
     }
--- a/browser/components/extensions/test/browser/browser_ext_windows_create_tabId.js
+++ b/browser/components/extensions/test/browser/browser_ext_windows_create_tabId.js
@@ -218,8 +218,87 @@ add_task(async function testWebNavigatio
 
   await extension.startup();
 
   await extension.sendMessage("testTabURLs", testURLs);
 
   await extension.awaitFinish("webNavigation-on-window-create-tabId");
   await extension.unload();
 });
+
+add_task(async function testGetLastFocusedDoesNotLeakDuringTabAdoption() {
+  async function background() {
+    const allTabs = await browser.tabs.query({});
+
+    browser.test.onMessage.addListener(async (msg, testTabURL) => {
+      if (msg !== "testTabURL") {
+        return;
+      }
+
+      let tab = allTabs.filter(tab => tab.url === testTabURL).pop();
+
+      // Keep calling getLastFocused while browser.windows.create is creating
+      // a new window to adopt the test tab, so that the test recreates
+      // conditions similar to the extension that has been triggered this leak
+      // (See Bug 1458918 for a rationale).
+      Promise.resolve().then(async () => {
+        while (true) {
+          browser.windows.getLastFocused({populate: true});
+          // eslint-disable-next-line mozilla/no-arbitrary-setTimeout
+          await new Promise(resolve => setTimeout(resolve, 50));
+        }
+      });
+
+      // Create a new window which adopt an existent tab and wait the tab to
+      // be fully attached to the new window.
+      await Promise.all([
+        new Promise(resolve => {
+          const listener = () => {
+            browser.tabs.onAttached.removeListener(listener);
+            resolve();
+          };
+          browser.tabs.onAttached.addListener(listener);
+        }),
+        browser.windows.create({tabId: tab.id}),
+      ]);
+
+      // Check that getLastFocused populate the tabs property once the tab adoption
+      // has been completed.
+      const lastFocusedPopulate = await browser.windows.getLastFocused({populate: true});
+      browser.test.assertEq(1, lastFocusedPopulate.tabs.length,
+                            "Got the expected number of tabs from windows.getLastFocused");
+
+      // Remove the test tab.
+      await browser.tabs.remove(tab.id);
+
+      browser.test.notifyPass("tab-adopted");
+    });
+  }
+
+  await BrowserTestUtils.openNewForegroundTab(gBrowser, "http://example.com/");
+
+  let extension = ExtensionTestUtils.loadExtension({
+    manifest: {
+      "permissions": ["tabs", "webNavigation"],
+    },
+    background,
+  });
+
+  await extension.startup();
+
+  extension.sendMessage("testTabURL", "http://example.com/");
+
+  await extension.awaitFinish("tab-adopted");
+
+  // Check that no tabs have been leaked by the internal tabTracker helper class.
+  const {ExtensionParent} = ChromeUtils.import("resource://gre/modules/ExtensionParent.jsm", {});
+  const {tabTracker} = ExtensionParent.apiManager.global;
+
+  for (const [tabId, nativeTab] of tabTracker._tabIds) {
+    if (!nativeTab.ownerGlobal) {
+      ok(false, `A tab with tabId ${tabId} has been leaked by the WebExtensions' tabTracker`);
+    }
+  }
+
+  is(tabTracker._tabIds.size, 1, "Got the expected number of tab ids in tabTracker");
+
+  await extension.unload();
+});
--- a/toolkit/components/extensions/parent/ext-tabs-base.js
+++ b/toolkit/components/extensions/parent/ext-tabs-base.js
@@ -1008,17 +1008,19 @@ class WindowBase {
 
   /**
    * @property {nsIURI | null} title
    *        Returns the current title of this window if the extension has permission
    *        to read it, or null otherwise.
    *        @readonly
    */
   get title() {
-    if (this.activeTab.hasTabPermission) {
+    // activeTab may be null when a new window is adopting an existing tab as its first tab
+    // (See Bug 1458918 for rationale).
+    if (this.activeTab && this.activeTab.hasTabPermission) {
       return this._title;
     }
   }
 
   // The JSDoc validator does not support @returns tags in abstract functions or
   // star functions without return statements.
   /* eslint-disable valid-jsdoc */
   /**
--- a/toolkit/components/extensions/parent/ext-webNavigation.js
+++ b/toolkit/components/extensions/parent/ext-webNavigation.js
@@ -118,17 +118,19 @@ class WebNavigationEventManager extends 
 
         if (data.sourceFrameId != undefined) {
           data2.sourceFrameId = data.sourceFrameId;
         }
 
         // Do not send a webNavigation event when the data.browser is related to a tab from a
         // new window opened to adopt an existent tab (See Bug 1443221 for a rationale).
         const chromeWin = data.browser.ownerGlobal;
-        if (chromeWin && chromeWin.arguments && chromeWin.arguments[0] instanceof chromeWin.XULElement &&
+
+        if (chromeWin && chromeWin.gBrowser &&
+            chromeWin.gBrowser.isAdoptingFirstTab() &&
             chromeWin.gBrowser.selectedBrowser === data.browser) {
           return;
         }
 
         // Fills in tabId typically.
         Object.assign(data2, tabTracker.getBrowserData(data.browser));
         if (data2.tabId < 0) {
           return;