Bug 1366290 - Fix the ordering of tabs.onActivated and tabs.onRemoved, r=kmag
authorBob Silverberg <bsilverberg@mozilla.com>
Wed, 31 May 2017 12:01:58 -0400
changeset 430379 cf7a1267d47b09edb0da713b213e2509c0f9dc7f
parent 430378 d68bccc30f7e0af24fb7f43950ecd2832981914b
child 430380 7d519539e6684c9a26b674ff62140e2f13de2ef5
push id7761
push userjlund@mozilla.com
push dateFri, 15 Sep 2017 00:19:52 +0000
treeherdermozilla-beta@c38455951db4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskmag
bugs1366290
milestone57.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 1366290 - Fix the ordering of tabs.onActivated and tabs.onRemoved, r=kmag Currently tabs.onActivated (for the tab that becomes active after a tab is removed) fires before tabs.onRemoved (for the tab that was removed). This is neither the order in which Chrome fires these events, nor is it the order in which the internal TabSelect and TabClose happen in Firefox. This bug fixes this so tabs.onActivated fires *after* tabs.onRemoved. Note that this does introduce an issue in in-process mode, where window.close() will not trigger a tabs.onRemoved event for the window, but Kris says "Meh" about that. MozReview-Commit-ID: CrFR3jqL2u5
browser/components/extensions/ext-browser.js
browser/components/extensions/test/browser/browser_ext_tabs_events.js
toolkit/components/extensions/test/mochitest/test_ext_tab_teardown.html
--- a/browser/components/extensions/ext-browser.js
+++ b/browser/components/extensions/ext-browser.js
@@ -494,28 +494,17 @@ class TabTracker extends TabTrackerBase 
    *        True if the tab is being removed because the browser window is
    *        closing.
    * @private
    */
   emitRemoved(nativeTab, isWindowClosing) {
     let windowId = windowTracker.getId(nativeTab.ownerGlobal);
     let tabId = this.getId(nativeTab);
 
-    // When addons run in-process, `window.close()` is synchronous. Most other
-    // addon-invoked calls are asynchronous since they go through a proxy
-    // context via the message manager. This includes event registrations such
-    // as `tabs.onRemoved.addListener`.
-    //
-    // So, even if `window.close()` were to be called (in-process) after calling
-    // `tabs.onRemoved.addListener`, then the tab would be closed before the
-    // event listener is registered. To make sure that the event listener is
-    // notified, we dispatch `tabs.onRemoved` asynchronously.
-    Services.tm.dispatchToMainThread(() => {
-      this.emit("tab-removed", {nativeTab, tabId, windowId, isWindowClosing});
-    });
+    this.emit("tab-removed", {nativeTab, tabId, windowId, isWindowClosing});
   }
 
   getBrowserData(browser) {
     if (browser.ownerDocument.documentURI === "about:addons") {
       // When we're loaded into a <browser> inside about:addons, we need to go up
       // one more level.
       browser = browser.ownerDocument.docShell.chromeEventHandler;
     }
--- a/browser/components/extensions/test/browser/browser_ext_tabs_events.js
+++ b/browser/components/extensions/test/browser/browser_ext_tabs_events.js
@@ -243,42 +243,52 @@ add_task(async function testTabEventsSiz
   }
 
   await extension.unload();
   SpecialPowers.clearUserPref(RESOLUTION_PREF);
 });
 
 add_task(async function testTabRemovalEvent() {
   async function background() {
+    let events = [];
+
     function awaitLoad(tabId) {
       return new Promise(resolve => {
         browser.tabs.onUpdated.addListener(function listener(tabId_, changed, tab) {
           if (tabId == tabId_ && changed.status == "complete") {
             browser.tabs.onUpdated.removeListener(listener);
             resolve();
           }
         });
       });
     }
 
     chrome.tabs.onRemoved.addListener((tabId, info) => {
+      browser.test.assertEq(0, events.length, "No events recorded before onRemoved.");
+      events.push("onRemoved");
       browser.test.log("Make sure the removed tab is not available in the tabs.query callback.");
       chrome.tabs.query({}, tabs => {
         for (let tab of tabs) {
           browser.test.assertTrue(tab.id != tabId, "Tab query should not include removed tabId");
         }
-        browser.test.notifyPass("tabs-events");
       });
     });
 
     try {
       let url = "http://example.com/browser/browser/components/extensions/test/browser/context.html";
       let tab = await browser.tabs.create({url: url});
       await awaitLoad(tab.id);
 
+      chrome.tabs.onActivated.addListener(info => {
+        browser.test.assertEq(1, events.length, "One event recorded before onActivated.");
+        events.push("onActivated");
+        browser.test.assertEq("onRemoved", events[0], "onRemoved fired before onActivated.");
+        browser.test.notifyPass("tabs-events");
+      });
+
       await browser.tabs.remove(tab.id);
     } catch (e) {
       browser.test.fail(`${e} :: ${e.stack}`);
       browser.test.notifyFail("tabs-events");
     }
   }
 
   let extension = ExtensionTestUtils.loadExtension({
--- a/toolkit/components/extensions/test/mochitest/test_ext_tab_teardown.html
+++ b/toolkit/components/extensions/test/mochitest/test_ext_tab_teardown.html
@@ -103,45 +103,50 @@ add_task(async function test_extension_p
   // This tests whether a context that is opened via window.open is properly
   // disposed when the tab closes.
   // The background page cannot use window.open (bugzil.la/1282021), so we open
   // another extension page that manages the window.open-tab for testing.
   function background() {
     chrome.tabs.create({url: "window.open.html"});
   }
 
-  function windowOpenScript() {
+  function windowOpenScript(remote) {
     let win;
     browser.test.onMessage.addListener(msg => {
       if (msg === "open extension page") {
         win = window.open("page.html");
       } else if (msg === "reload extension page") {
         win.location.reload();
       } else if (msg === "close extension page") {
         browser.tabs.onRemoved.addListener(function listener() {
           browser.tabs.onRemoved.removeListener(listener);
           browser.test.sendMessage("closed extension page");
         });
         win.close();
+        // In in-process mode, the tabs.onRemoved above won't fire, so we need
+        // to send the message after the window closes.
+        if (!remote) {
+          browser.test.sendMessage("closed extension page");
+        }
       }
     });
     browser.test.sendMessage("setup-intermediate-tab");
   }
 
   function pageScript() {
     browser.test.sendMessage("extension page loaded", document.URL);
   }
 
   let extensionData = {
     background,
     files: {
       "page.html": `<!DOCTYPE html><meta charset="utf-8"><script src="page.js"><\/script>`,
       "page.js": pageScript,
       "window.open.html": `<!DOCTYPE html><meta charset="utf-8"><script src="window.open.js"><\/script>`,
-      "window.open.js": windowOpenScript,
+      "window.open.js": `(${windowOpenScript})(${remote})`,
     },
   };
   let extension = ExtensionTestUtils.loadExtension(extensionData);
   await extension.startup();
   await extension.awaitMessage("setup-intermediate-tab");
   await runTabReloadAndCloseTest(extension);
 });
 </script>