Bug 1586612 properly handle tab events that happen prior to firing tabs.onCreated r=rpl,Gijs
authorShane Caraveo <scaraveo@mozilla.com>
Fri, 13 Dec 2019 20:17:39 +0000
changeset 506967 d6c47672bcf710e0b25157e808f035fa6349b868
parent 506966 0aba2e1d9ade11ab4f908cf58c8a4102753e25ad
child 506968 7d76a0d3d4027b6a0571a19c20e9956ba3b30120
push id103186
push userscaraveo@mozilla.com
push dateFri, 13 Dec 2019 20:33:54 +0000
treeherderautoland@d6c47672bcf7 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersrpl, Gijs
bugs1586612
milestone73.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 1586612 properly handle tab events that happen prior to firing tabs.onCreated r=rpl,Gijs Ensure that our delay in firing the onCreated (TabOpen) event does not result in other events happening prior to onCreated. Differential Revision: https://phabricator.services.mozilla.com/D55865
browser/base/content/tabbrowser.js
browser/components/extensions/parent/ext-browser.js
browser/components/extensions/parent/ext-tabs.js
browser/components/extensions/test/browser/browser_ext_tabs_events_order.js
browser/components/extensions/test/browser/browser_ext_tabs_onCreated.js
browser/components/extensions/test/browser/browser_ext_tabs_onUpdated.js
browser/components/extensions/test/browser/browser_ext_tabs_onUpdated_filter.js
--- a/browser/base/content/tabbrowser.js
+++ b/browser/base/content/tabbrowser.js
@@ -2593,16 +2593,19 @@
       if (relatedToCurrent == null) {
         relatedToCurrent = !!(referrerInfo && referrerInfo.originalReferrer);
       }
       let openerTab =
         (openerBrowser && this.getTabForBrowser(openerBrowser)) ||
         (relatedToCurrent && this.selectedTab);
 
       var t = document.createXULElement("tab", { is: "tabbrowser-tab" });
+      // Tag the tab as being created so extension code can ignore events
+      // prior to TabOpen.
+      t.initializingTab = true;
       t.openerTab = openerTab;
 
       aURI = aURI || "about:blank";
       let aURIObject = null;
       try {
         aURIObject = Services.io.newURI(aURI);
       } catch (ex) {
         /* we'll try to fix up this URL later */
@@ -2856,16 +2859,17 @@
 
       // Hack to ensure that the about:newtab, and about:welcome favicon is loaded
       // instantaneously, to avoid flickering and improve perceived performance.
       this.setDefaultIcon(t, aURIObject);
 
       // Dispatch a new tab notification.  We do this once we're
       // entirely done, so that things are in a consistent state
       // even if the event listener opens or closes tabs.
+      delete t.initializingTab;
       let evt = new CustomEvent("TabOpen", {
         bubbles: true,
         detail: eventDetail || {},
       });
       t.dispatchEvent(evt);
 
       if (
         !usingPreloadedContent &&
--- a/browser/components/extensions/parent/ext-browser.js
+++ b/browser/components/extensions/parent/ext-browser.js
@@ -15,16 +15,21 @@ ChromeUtils.defineModuleGetter(
   "PrivateBrowsingUtils",
   "resource://gre/modules/PrivateBrowsingUtils.jsm"
 );
 ChromeUtils.defineModuleGetter(
   this,
   "BrowserWindowTracker",
   "resource:///modules/BrowserWindowTracker.jsm"
 );
+ChromeUtils.defineModuleGetter(
+  this,
+  "PromiseUtils",
+  "resource://gre/modules/PromiseUtils.jsm"
+);
 
 var { ExtensionError } = ExtensionUtils;
 
 var { defineLazyGetter } = ExtensionCommon;
 
 const READER_MODE_PREFIX = "about:reader";
 
 let tabTracker;
@@ -319,16 +324,17 @@ class WindowTracker extends WindowTracke
 class TabTracker extends TabTrackerBase {
   constructor() {
     super();
 
     this._tabs = new WeakMap();
     this._browsers = new WeakMap();
     this._tabIds = new Map();
     this._nextId = 1;
+    this._deferredTabOpenEvents = new WeakMap();
 
     this._handleTabDestroyed = this._handleTabDestroyed.bind(this);
   }
 
   init() {
     if (this.initialized) {
       return;
     }
@@ -484,16 +490,33 @@ class TabTracker extends TabTrackerBase 
    */
   setOpener(tab, openerTab) {
     if (tab.ownerDocument !== openerTab.ownerDocument) {
       throw new Error("Tab must be in the same window as its opener");
     }
     tab.openerTab = openerTab;
   }
 
+  deferredForTabOpen(nativeTab) {
+    let deferred = this._deferredTabOpenEvents.get(nativeTab);
+    if (!deferred) {
+      deferred = PromiseUtils.defer();
+      this._deferredTabOpenEvents.set(nativeTab, deferred);
+      deferred.promise.then(() => {
+        this._deferredTabOpenEvents.delete(nativeTab);
+      });
+    }
+    return deferred;
+  }
+
+  async maybeWaitForTabOpen(nativeTab) {
+    let deferred = this._deferredTabOpenEvents.get(nativeTab);
+    return deferred && deferred.promise;
+  }
+
   /**
    * @param {Event} event
    *        The DOM Event to handle.
    * @private
    */
   handleEvent(event) {
     let nativeTab = event.target;
 
@@ -521,17 +544,19 @@ class TabTracker extends TabTrackerBase 
           const currentTabSize = {
             width: frameLoader.lazyWidth,
             height: frameLoader.lazyHeight,
           };
 
           // We need to delay sending this event until the next tick, since the
           // tab can become selected immediately after "TabOpen", then onCreated
           // should be fired with `active: true`.
+          let deferred = this.deferredForTabOpen(event.originalTarget);
           Promise.resolve().then(() => {
+            deferred.resolve();
             if (!event.originalTarget.parentNode) {
               // If the tab is already be destroyed, do nothing.
               return;
             }
             this.emitCreated(event.originalTarget, currentTabSize);
           });
         }
         break;
@@ -547,29 +572,30 @@ class TabTracker extends TabTrackerBase 
         } else {
           this.emitRemoved(nativeTab, false);
         }
         break;
 
       case "TabSelect":
         // Because we are delaying calling emitCreated above, we also need to
         // delay sending this event because it shouldn't fire before onCreated.
-        Promise.resolve().then(() => {
+        this.maybeWaitForTabOpen(nativeTab).then(() => {
           if (!nativeTab.parentNode) {
             // If the tab is already be destroyed, do nothing.
             return;
           }
           this.emitActivated(nativeTab, event.detail.previousTab);
         });
         break;
 
       case "TabMultiSelect":
         if (this.has("tabs-highlighted")) {
           // Because we are delaying calling emitCreated above, we also need to
           // delay sending this event because it shouldn't fire before onCreated.
+          // event.target is gBrowser, so we don't use maybeWaitForTabOpen.
           Promise.resolve().then(() => {
             this.emitHighlighted(event.target.ownerGlobal);
           });
         }
         break;
     }
   }
 
--- a/browser/components/extensions/parent/ext-tabs.js
+++ b/browser/components/extensions/parent/ext-tabs.js
@@ -257,29 +257,39 @@ class TabsUpdateFilterEventManager exten
         }
         if (filter.urls) {
           // We check permission first because tab.uri is null if !hasTabPermission.
           return tab.hasTabPermission && filter.urls.matches(tab.uri);
         }
         return true;
       }
 
-      let fireForTab = (tab, changed) => {
+      let fireForTab = (tab, changed, nativeTab) => {
         // Tab may be null if private and not_allowed.
         if (!tab || !matchFilters(tab, changed)) {
           return;
         }
 
         let changeInfo = sanitize(extension, changed);
         if (changeInfo) {
-          fire.async(tab.id, changeInfo, tab.convert());
+          tabTracker.maybeWaitForTabOpen(nativeTab).then(() => {
+            if (!nativeTab.parentNode) {
+              // If the tab is already be destroyed, do nothing.
+              return;
+            }
+            fire.async(tab.id, changeInfo, tab.convert());
+          });
         }
       };
 
       let listener = event => {
+        // Ignore any events prior to TabOpen
+        if (event.originalTarget.initializingTab) {
+          return;
+        }
         if (!context.canAccessWindow(event.originalTarget.ownerGlobal)) {
           return;
         }
         let needed = [];
         if (event.type == "TabAttrModified") {
           let changed = event.detail.changed;
           if (
             changed.includes("image") &&
@@ -332,43 +342,43 @@ class TabsUpdateFilterEventManager exten
 
         let tab = tabManager.getWrapper(event.originalTarget);
 
         let changeInfo = {};
         for (let prop of needed) {
           changeInfo[prop] = tab[prop];
         }
 
-        fireForTab(tab, changeInfo);
+        fireForTab(tab, changeInfo, event.originalTarget);
       };
 
       let statusListener = ({ browser, status, url }) => {
         let { gBrowser } = browser.ownerGlobal;
         let tabElem = gBrowser.getTabForBrowser(browser);
         if (tabElem) {
           if (!context.canAccessWindow(tabElem.ownerGlobal)) {
             return;
           }
 
           let changed = { status };
           if (url) {
             changed.url = url;
           }
 
-          fireForTab(tabManager.wrapTab(tabElem), changed);
+          fireForTab(tabManager.wrapTab(tabElem), changed, tabElem);
         }
       };
 
       let isArticleChangeListener = (messageName, message) => {
         let { gBrowser } = message.target.ownerGlobal;
         let nativeTab = gBrowser.getTabForBrowser(message.target);
 
         if (nativeTab && context.canAccessWindow(nativeTab.ownerGlobal)) {
           let tab = tabManager.getWrapper(nativeTab);
-          fireForTab(tab, { isArticle: message.data.isArticle });
+          fireForTab(tab, { isArticle: message.data.isArticle }, nativeTab);
         }
       };
 
       let listeners = new Map();
       if (filter.properties.has("status")) {
         listeners.set("status", statusListener);
       }
       if (needsModified) {
--- a/browser/components/extensions/test/browser/browser_ext_tabs_events_order.js
+++ b/browser/components/extensions/test/browser/browser_ext_tabs_events_order.js
@@ -63,23 +63,31 @@ add_task(async function testTabEvents() 
       }
 
       browser.test.assertEq(
         expectedEvents.length,
         events[tabId].length,
         `Got expected number of events for ${tabId}`
       );
 
-      for (let [i, name] of expectedEvents.entries()) {
-        browser.test.assertEq(
-          name,
-          i in events[tabId] && events[tabId][i],
+      for (let name of expectedEvents) {
+        browser.test.assertTrue(
+          events[tabId].includes(name),
           `Got expected ${name} event`
         );
       }
+
+      if (expectedEvents.includes("onCreated")) {
+        browser.test.assertEq(
+          events[tabId].indexOf("onCreated"),
+          0,
+          "onCreated happened first"
+        );
+      }
+
       delete events[tabId];
     }
 
     /**
      * Opens a new tab and asserts that the correct events are fired.
      *
      * @param {number} windowId
      */
--- a/browser/components/extensions/test/browser/browser_ext_tabs_onCreated.js
+++ b/browser/components/extensions/test/browser/browser_ext_tabs_onCreated.js
@@ -3,24 +3,39 @@
 /* eslint-disable mozilla/no-arbitrary-setTimeout */
 "use strict";
 
 add_task(async function test_onCreated_active() {
   let extension = ExtensionTestUtils.loadExtension({
     manifest: {
       permissions: ["tabs"],
     },
-    background: function() {
+    async background() {
+      let created = false;
+      let tabIds = (await browser.tabs.query({})).map(t => t.id);
       browser.tabs.onCreated.addListener(tab => {
+        created = tab.id;
         browser.tabs.remove(tab.id);
         browser.test.sendMessage("onCreated", tab);
       });
+      // We always get at least one onUpdated event when creating tabs.
+      browser.tabs.onUpdated.addListener((tabId, changes, tab) => {
+        // ignore tabs created prior to extension startup
+        if (tabIds.includes(tabId)) {
+          return;
+        }
+        browser.test.assertTrue(created, tabId, "tab created before updated");
+        browser.test.notifyPass("onUpdated");
+      });
+      browser.test.sendMessage("ready");
     },
   });
 
   await extension.startup();
+  await extension.awaitMessage("ready");
   BrowserOpenTab();
 
   let tab = await extension.awaitMessage("onCreated");
   is(true, tab.active, "Tab should be active");
+  await extension.awaitFinish("onUpdated");
 
   await extension.unload();
 });
--- a/browser/components/extensions/test/browser/browser_ext_tabs_onUpdated.js
+++ b/browser/components/extensions/test/browser/browser_ext_tabs_onUpdated.js
@@ -245,9 +245,49 @@ add_task(async function test_without_tab
         }
       }
     });
 
     browser.tabs.reload(tab.id);
   }, false /* withPermissions */);
 });
 
+add_task(async function test_onUpdated_after_onRemoved() {
+  let extension = ExtensionTestUtils.loadExtension({
+    manifest: {
+      permissions: ["tabs"],
+    },
+    async background() {
+      const url =
+        "http://mochi.test:8888/browser/browser/components/extensions/test/browser/context_tabs_onUpdated_page.html";
+      let removed = false;
+      let tab;
+
+      // If remove happens fast and we never receive onUpdated, that is ok, but
+      // we never want to receive onUpdated after onRemoved.
+      browser.tabs.onUpdated.addListener(function onUpdated(tabId, changeInfo) {
+        if (!tab || tab.id !== tabId) {
+          return;
+        }
+        browser.test.assertFalse(
+          removed,
+          "tab has not been removed before onUpdated"
+        );
+      });
+
+      browser.tabs.onRemoved.addListener((tabId, removedInfo) => {
+        if (!tab || tab.id !== tabId) {
+          return;
+        }
+        removed = true;
+        browser.test.notifyPass("onRemoved");
+      });
+
+      tab = await browser.tabs.create({ url });
+      browser.tabs.remove(tab.id);
+    },
+  });
+  await extension.startup();
+  await extension.awaitFinish("onRemoved");
+  await extension.unload();
+});
+
 add_task(forceGC);
--- a/browser/components/extensions/test/browser/browser_ext_tabs_onUpdated_filter.js
+++ b/browser/components/extensions/test/browser/browser_ext_tabs_onUpdated_filter.js
@@ -283,49 +283,65 @@ add_task(async function test_filter_isAr
   await BrowserTestUtils.removeTab(tab);
 });
 
 add_task(async function test_filter_property() {
   let extension = ExtensionTestUtils.loadExtension({
     manifest: {
       permissions: ["tabs"],
     },
-    background() {
+    async background() {
       // We expect only status updates, anything else is a failure.
       let properties = new Set([
         "audible",
         "discarded",
         "favIconUrl",
         "hidden",
         "isArticle",
         "mutedInfo",
         "pinned",
         "sharingState",
         "title",
       ]);
+
+      // Test that updated only happens after created.
+      let created = false;
+      let tabIds = (await browser.tabs.query({})).map(t => t.id);
+      browser.tabs.onCreated.addListener(tab => {
+        created = tab.id;
+      });
+
       browser.tabs.onUpdated.addListener(
         (tabId, changeInfo) => {
+          // ignore tabs created prior to extension startup
+          if (tabIds.includes(tabId)) {
+            return;
+          }
+          browser.test.assertEq(created, tabId, "tab created before updated");
+
           browser.test.log(`got onUpdated ${JSON.stringify(changeInfo)}`);
           browser.test.assertTrue(!!changeInfo.status, "changeInfo has status");
           if (Object.keys(changeInfo).some(p => properties.has(p))) {
             browser.test.fail(
               `received unexpected onUpdated event ${JSON.stringify(
                 changeInfo
               )}`
             );
           }
           if (changeInfo.status === "complete") {
             browser.test.notifyPass("onUpdated");
           }
         },
         { properties: ["status"] }
       );
+      browser.test.sendMessage("ready");
     },
   });
   await extension.startup();
+  await extension.awaitMessage("ready");
   let ok = extension.awaitFinish("onUpdated");
 
   let tab = await BrowserTestUtils.openNewForegroundTab(
     gBrowser,
     "http://mochi.test:8888/"
   );
   await ok;