Bug 1463751 - Tab-specific data not updated when tab is moved to another window and old window closes r=mixedpuppy
authorOriol Brufau <oriol-bugzilla@hotmail.com>
Sat, 26 May 2018 21:57:58 +0200
changeset 420673 548c628a1059cf0c57fd73a3e4e141eed06bd685
parent 420672 930b30d0d0aab0905fabfb7cb61e39489dc5b1de
child 420674 4ae4cc41c1ad65a6a9735bff436ab97cc1dfa5ee
push id64697
push userapavel@mozilla.com
push dateThu, 31 May 2018 13:15:45 +0000
treeherderautoland@548c628a1059 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmixedpuppy
bugs1463751
milestone62.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 1463751 - Tab-specific data not updated when tab is moved to another window and old window closes r=mixedpuppy 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":
@@ -443,19 +470,17 @@ class TabTracker extends TabTrackerBase 
     if (tabToAdopt) {
       // 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 = tabToAdopt;
       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"},