Bug 1463751 - Tab-specific data not updated when tab is moved to another window and old window closes draft
authorOriol Brufau <oriol-bugzilla@hotmail.com>
Sat, 26 May 2018 21:57:58 +0200
changeset 802110 b66720efa1dd4392fecccaf401c5c15aaf5dfd53
parent 800319 4e9446f9e8f0a75c7ffe063f1dfb311cc90d56cf
push id111820
push userbmo:oriol-bugzilla@hotmail.com
push dateThu, 31 May 2018 11:30:37 +0000
bugs1463751
milestone62.0a1
Bug 1463751 - Tab-specific data not updated when tab is moved to another window and old window closes MozReview-Commit-ID: IUC8OwV6YHY
browser/components/extensions/parent/ext-browser.js
browser/components/extensions/test/browser/browser_ext_browserAction_context.js
browser/components/extensions/test/browser/browser_ext_pageAction_context.js
browser/components/extensions/test/browser/browser_ext_sidebarAction_context.js
--- a/browser/components/extensions/parent/ext-browser.js
+++ b/browser/components/extensions/parent/ext-browser.js
@@ -137,18 +137,18 @@ global.TabContext = class extends EventE
 
     this.getDefaultPrototype = getDefaultPrototype;
 
     this.tabData = new WeakMap();
 
     windowTracker.addListener("progress", this);
     windowTracker.addListener("TabSelect", this);
 
-    this.tabDetached = this.tabDetached.bind(this);
-    tabTracker.on("tab-detached", this.tabDetached);
+    this.tabAdopted = this.tabAdopted.bind(this);
+    tabTracker.on("tab-adopted", this.tabAdopted);
   }
 
   /**
    * Returns the context data associated with `keyObject`.
    *
    * @param {XULElement|ChromeWindow} keyObject
    *        Browser tab or browser chrome window.
    * @returns {Object}
@@ -183,34 +183,45 @@ global.TabContext = class extends EventE
   onLocationChange(browser, webProgress, request, locationURI, flags) {
     let gBrowser = browser.ownerGlobal.gBrowser;
     let tab = gBrowser.getTabForBrowser(browser);
     // fromBrowse will be false in case of e.g. a hash change or history.pushState
     let fromBrowse = !(flags & Ci.nsIWebProgressListener.LOCATION_CHANGE_SAME_DOCUMENT);
     this.emit("location-change", tab, fromBrowse);
   }
 
-  tabDetached(eventType, {nativeTab, adoptedBy}) {
-    if (!this.tabData.has(nativeTab)) {
+  /**
+   * Persists context data when a tab is moved between windows.
+   *
+   * @param {string} eventType
+   *        Event type, should be "tab-adopted".
+   * @param {NativeTab} adoptingTab
+   *        The tab which is being opened and adopting `adoptedTab`.
+   * @param {NativeTab} adoptedTab
+   *        The tab which is being closed and adopted by `adoptingTab`.
+   */
+  tabAdopted(eventType, adoptingTab, adoptedTab) {
+    if (!this.tabData.has(adoptedTab)) {
       return;
     }
     // Create a new object (possibly with different inheritance) when a tab is moved
     // into a new window. But then reassign own properties from the old object.
-    let newData = this.get(adoptedBy);
-    let oldData = this.tabData.get(nativeTab);
+    let newData = this.get(adoptingTab);
+    let oldData = this.tabData.get(adoptedTab);
+    this.tabData.delete(adoptedTab);
     Object.assign(newData, oldData);
   }
 
   /**
    * Makes the TabContext instance stop emitting events.
    */
   shutdown() {
     windowTracker.removeListener("progress", this);
     windowTracker.removeListener("TabSelect", this);
-    tabTracker.off("tab-detached", this.tabDetached);
+    tabTracker.off("tab-adopted", this.tabAdopted);
   }
 };
 
 
 class WindowTracker extends WindowTrackerBase {
   addProgressListener(window, listener) {
     window.gBrowser.addTabsProgressListener(listener);
   }
@@ -299,16 +310,34 @@ class TabTracker extends TabTrackerBase 
   setId(nativeTab, id) {
     this._tabs.set(nativeTab, id);
     if (nativeTab.linkedBrowser) {
       this._browsers.set(nativeTab.linkedBrowser, id);
     }
     this._tabIds.set(id, nativeTab);
   }
 
+  /**
+   * Handles tab adoption when a tab is moved between windows.
+   * Ensures the new tab will have the same ID as the old one,
+   * and emits a "tab-adopted" event.
+   *
+   * @param {NativeTab} adoptingTab
+   *        The tab which is being opened and adopting `adoptedTab`.
+   * @param {NativeTab} adoptedTab
+   *        The tab which is being closed and adopted by `adoptingTab`.
+   */
+  adopt(adoptingTab, adoptedTab) {
+    if (!this.adoptedTabs.has(adoptedTab)) {
+      this.adoptedTabs.set(adoptedTab, adoptingTab);
+      this.setId(adoptingTab, this.getId(adoptedTab));
+      this.emit("tab-adopted", adoptingTab, adoptedTab);
+    }
+  }
+
   _handleTabDestroyed(event, {nativeTab}) {
     let id = this._tabs.get(nativeTab);
     if (id) {
       this._tabs.delete(nativeTab);
       if (this._tabIds.get(id) === nativeTab) {
         this._tabIds.delete(id);
       }
     }
@@ -358,21 +387,19 @@ class TabTracker extends TabTrackerBase 
    */
   handleEvent(event) {
     let nativeTab = event.target;
 
     switch (event.type) {
       case "TabOpen":
         let {adoptedTab} = event.detail;
         if (adoptedTab) {
-          this.adoptedTabs.set(adoptedTab, event.target);
-
           // This tab is being created to adopt a tab from a different window.
-          // Copy the ID from the old tab to the new.
-          this.setId(nativeTab, this.getId(adoptedTab));
+          // Handle the adoption.
+          this.adopt(nativeTab, adoptedTab);
 
           adoptedTab.linkedBrowser.messageManager.sendAsyncMessage("Extension:SetFrameData", {
             windowId: windowTracker.getId(nativeTab.ownerGlobal),
           });
         }
 
         // Save the current tab, since the newly-created tab will likely be
         // active by the time the promise below resolves and the event is
@@ -389,20 +416,20 @@ class TabTracker extends TabTrackerBase 
           }
         });
         break;
 
       case "TabClose":
         let {adoptedBy} = event.detail;
         if (adoptedBy) {
           // This tab is being closed because it was adopted by a new window.
-          // Copy its ID to the new tab, in case it was created as the first tab
-          // of a new window, and did not have an `adoptedTab` detail when it was
+          // Handle the adoption in case it was created as the first tab of a
+          // new window, and did not have an `adoptedTab` detail when it was
           // opened.
-          this.setId(adoptedBy, this.getId(nativeTab));
+          this.adopt(adoptedBy, nativeTab);
 
           this.emitDetached(nativeTab, adoptedBy);
         } else {
           this.emitRemoved(nativeTab, false);
         }
         break;
 
       case "TabSelect":
@@ -446,19 +473,17 @@ class TabTracker extends TabTrackerBase 
       //
       // 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];
-
-      this.adoptedTabs.set(nativeTab, adoptedBy);
-      this.setId(adoptedBy, this.getId(nativeTab));
+      this.adopt(adoptedBy, nativeTab);
 
       // We need to be sure to fire this event after the onDetached event
       // for the original tab.
       let listener = (event, details) => {
         if (details.nativeTab === nativeTab) {
           this.off("tab-detached", listener);
 
           Promise.resolve().then(() => {
--- a/browser/components/extensions/test/browser/browser_ext_browserAction_context.js
+++ b/browser/components/extensions/test/browser/browser_ext_browserAction_context.js
@@ -661,23 +661,29 @@ add_task(async function testMultipleWind
         async expect => {
           browser.test.log("Move tab from old window to the new one. Tab-specific data"
             + " is preserved but inheritance is from the new window");
           await browser.tabs.move(tabs[1], {windowId: windows[1], index: -1});
           await browser.tabs.update(tabs[1], {active: true});
           expect(details[3], details[2], null, details[0]);
         },
         async expect => {
-          browser.test.log("Close the tab, expect window values.");
-          await browser.tabs.remove(tabs[1]);
-          expect(null, details[2], null, details[0]);
+          browser.test.log("Close the initial tab of the new window.");
+          let [{id}] = await browser.tabs.query({windowId: windows[1], index: 0});
+          await browser.tabs.remove(id);
+          expect(details[3], details[2], null, details[0]);
         },
         async expect => {
-          browser.test.log("Close the new window and go back to the previous one.");
-          await browser.windows.remove(windows[1]);
+          browser.test.log("Move the previous tab to a 3rd window, the 2nd one will close.");
+          await browser.windows.create({tabId: tabs[1]});
+          expect(details[3], null, null, details[0]);
+        },
+        async expect => {
+          browser.test.log("Close the tab, go back to the 1st window.");
+          await browser.tabs.remove(tabs[1]);
           expect(null, details[1], null, details[0]);
         },
         async expect => {
           browser.test.log("Assert failures for bad parameters. Expect no change");
 
           let calls = {
             setIcon: {path: "default.png"},
             setPopup: {popup: "default.html"},
--- a/browser/components/extensions/test/browser/browser_ext_pageAction_context.js
+++ b/browser/components/extensions/test/browser/browser_ext_pageAction_context.js
@@ -251,18 +251,29 @@ add_task(async function testMultipleWind
         },
         async expect => {
           browser.test.log("Move tab from old window to the new one, expect old values.");
           await browser.tabs.move(tabs[1], {windowId: windows[1], index: -1});
           await browser.tabs.update(tabs[1], {active: true});
           expect(details[1]);
         },
         async expect => {
-          browser.test.log("Close the new window and go back to the previous one.");
-          await browser.windows.remove(windows[1]);
+          browser.test.log("Close the initial tab of the new window.");
+          let [{id}] = await browser.tabs.query({windowId: windows[1], index: 0});
+          await browser.tabs.remove(id);
+          expect(details[1]);
+        },
+        async expect => {
+          browser.test.log("Move the previous tab to a 3rd window, the 2nd one will close.");
+          await browser.windows.create({tabId: tabs[1]});
+          expect(details[1]);
+        },
+        async expect => {
+          browser.test.log("Close the tab, go back to the 1st window.");
+          await browser.tabs.remove(tabs[1]);
           expect(null);
         },
       ];
     },
   });
 });
 
 add_task(async function testNavigationClearsData() {
--- a/browser/components/extensions/test/browser/browser_ext_sidebarAction_context.js
+++ b/browser/components/extensions/test/browser/browser_ext_sidebarAction_context.js
@@ -560,23 +560,29 @@ add_task(async function testMultipleWind
         async expect => {
           browser.test.log("Move tab from old window to the new one. Tab-specific data"
             + " is preserved but inheritance is from the new window");
           await browser.tabs.move(tabs[1], {windowId: windows[1], index: -1});
           await browser.tabs.update(tabs[1], {active: true});
           expect(details[3], details[2], null, details[0]);
         },
         async expect => {
-          browser.test.log("Close the tab, expect window values.");
-          await browser.tabs.remove(tabs[1]);
-          expect(null, details[2], null, details[0]);
+          browser.test.log("Close the initial tab of the new window.");
+          let [{id}] = await browser.tabs.query({windowId: windows[1], index: 0});
+          await browser.tabs.remove(id);
+          expect(details[3], details[2], null, details[0]);
         },
         async expect => {
-          browser.test.log("Close the new window and go back to the previous one.");
-          await browser.windows.remove(windows[1]);
+          browser.test.log("Move the previous tab to a 3rd window, the 2nd one will close.");
+          await browser.windows.create({tabId: tabs[1]});
+          expect(details[3], null, null, details[0]);
+        },
+        async expect => {
+          browser.test.log("Close the tab, go back to the 1st window.");
+          await browser.tabs.remove(tabs[1]);
           expect(null, details[1], null, details[0]);
         },
         async expect => {
           browser.test.log("Assert failures for bad parameters. Expect no change");
 
           let calls = {
             setIcon: {path: "default.png"},
             setPanel: {panel: "default.html"},