Bug 1586612 properly handle tab events that happen prior to firing ta…

…bs.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

@@ -2598,6 +2598,9 @@
         (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";
@@ -2861,6 +2864,7 @@
       // 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 || {},

browser/components/extensions/parent/ext-browser.js

@@ -20,6 +20,11 @@ ChromeUtils.defineModuleGetter(
   "BrowserWindowTracker",
   "resource:///modules/BrowserWindowTracker.jsm"
 );
+ChromeUtils.defineModuleGetter(
+  this,
+  "PromiseUtils",
+  "resource://gre/modules/PromiseUtils.jsm"
+);
 
 var { ExtensionError } = ExtensionUtils;
 
@@ -324,6 +329,7 @@ class TabTracker extends TabTrackerBase {
     this._browsers = new WeakMap();
     this._tabIds = new Map();
     this._nextId = 1;
+    this._deferredTabOpenEvents = new WeakMap();
 
     this._handleTabDestroyed = this._handleTabDestroyed.bind(this);
   }
@@ -489,6 +495,23 @@ class TabTracker extends TabTrackerBase {
     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.
@@ -526,7 +549,9 @@ class TabTracker extends TabTrackerBase {
           // 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;
@@ -552,7 +577,7 @@ class TabTracker extends TabTrackerBase {
       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;
@@ -565,6 +590,7 @@ class TabTracker extends TabTrackerBase {
         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);
           });

browser/components/extensions/parent/ext-tabs.js

@@ -262,19 +262,29 @@ class TabsUpdateFilterEventManager extends EventManager {
         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;
         }
@@ -337,7 +347,7 @@ class TabsUpdateFilterEventManager extends EventManager {
           changeInfo[prop] = tab[prop];
         }
 
-        fireForTab(tab, changeInfo);
+        fireForTab(tab, changeInfo, event.originalTarget);
       };
 
       let statusListener = ({ browser, status, url }) => {
@@ -353,7 +363,7 @@ class TabsUpdateFilterEventManager extends EventManager {
             changed.url = url;
           }
 
-          fireForTab(tabManager.wrapTab(tabElem), changed);
+          fireForTab(tabManager.wrapTab(tabElem), changed, tabElem);
         }
       };
 
@@ -363,7 +373,7 @@ class TabsUpdateFilterEventManager extends EventManager {
 
         if (nativeTab && context.canAccessWindow(nativeTab.ownerGlobal)) {
           let tab = tabManager.getWrapper(nativeTab);
-          fireForTab(tab, { isArticle: message.data.isArticle });
+          fireForTab(tab, { isArticle: message.data.isArticle }, nativeTab);
         }
       };
 

browser/components/extensions/test/browser/browser_ext_tabs_events_order.js

@@ -68,13 +68,21 @@ add_task(async function testTabEvents() {
         `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];
     }
 

browser/components/extensions/test/browser/browser_ext_tabs_onCreated.js

@@ -8,19 +8,34 @@ add_task(async function test_onCreated_active() {
     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();
 });

browser/components/extensions/test/browser/browser_ext_tabs_onUpdated.js

@@ -250,4 +250,44 @@ add_task(async function test_without_tabs_permission() {
   }, 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);

browser/components/extensions/test/browser/browser_ext_tabs_onUpdated_filter.js

@@ -288,7 +288,7 @@ add_task(async function test_filter_property() {
     manifest: {
       permissions: ["tabs"],
     },
-    background() {
+    async background() {
       // We expect only status updates, anything else is a failure.
       let properties = new Set([
         "audible",
@@ -301,8 +301,22 @@ add_task(async function test_filter_property() {
         "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))) {
@@ -318,9 +332,11 @@ add_task(async function test_filter_property() {
         },
         { properties: ["status"] }
       );
+      browser.test.sendMessage("ready");
     },
   });
   await extension.startup();
+  await extension.awaitMessage("ready");
   let ok = extension.awaitFinish("onUpdated");
 
   let tab = await BrowserTestUtils.openNewForegroundTab(