Bug 867118 - Remove browser.__SS_data and use a WeakMap instead; r=yoric

browser/components/sessionstore/src/SessionStore.jsm

@@ -1057,8 +1057,8 @@ let SessionStoreInternal = {
     this._lastSessionState = null;
     let openWindows = {};
     this._forEachBrowserWindow(function(aWindow) {
-      Array.forEach(aWindow.gBrowser.tabs, function(aTab) {
-        delete aTab.linkedBrowser.__SS_data;
+      Array.forEach(aWindow.gBrowser.tabs, aTab => {
+        RestoringTabsData.remove(aTab.linkedBrowser);
         delete aTab.linkedBrowser.__SS_formDataSaved;
         delete aTab.linkedBrowser.__SS_hostSchemeData;
         if (aTab.linkedBrowser.__SS_restoreState)
@@ -1249,7 +1249,7 @@ let SessionStoreInternal = {
     let mm = browser.messageManager;
     MESSAGES.forEach(msg => mm.removeMessageListener(msg, this));
 
-    delete browser.__SS_data;
+    RestoringTabsData.remove(aTab.linkedBrowser);
     delete browser.__SS_formDataSaved;
     delete browser.__SS_hostSchemeData;
 
@@ -1326,7 +1326,7 @@ let SessionStoreInternal = {
       return;
     }
 
-    delete aBrowser.__SS_data;
+    RestoringTabsData.remove(aBrowser);
     delete aBrowser.__SS_formDataSaved;
     this.saveStateDelayed(aWindow);
 
@@ -1635,10 +1635,13 @@ let SessionStoreInternal = {
     let data = {};
     if (aTab.__SS_extdata) {
       data = aTab.__SS_extdata;
-    }
-    else if (aTab.linkedBrowser.__SS_data && aTab.linkedBrowser.__SS_data.extData) {
-      // If the tab hasn't been fully restored, get the data from the to-be-restored data
-      data = aTab.linkedBrowser.__SS_data.extData;
+    } else {
+      let tabData = RestoringTabsData.get(aTab.linkedBrowser);
+      if (tabData && "extData" in tabData) {
+        // If the tab hasn't been fully restored yet,
+        // get the data from the to-be-restored data.
+        data = tabData.extData;
+      }
     }
     return data[aKey] || "";
   },
@@ -1649,13 +1652,14 @@ let SessionStoreInternal = {
     let saveTo;
     if (aTab.__SS_extdata) {
       saveTo = aTab.__SS_extdata;
-    }
-    else if (aTab.linkedBrowser.__SS_data && aTab.linkedBrowser.__SS_data.extData) {
-      saveTo = aTab.linkedBrowser.__SS_data.extData;
-    }
-    else {
-      aTab.__SS_extdata = {};
-      saveTo = aTab.__SS_extdata;
+    } else {
+      let tabData = RestoringTabsData.get(aTab.linkedBrowser);
+      if (tabData && "extData" in tabData) {
+        saveTo = tabData.extData;
+      } else {
+        aTab.__SS_extdata = {};
+        saveTo = aTab.__SS_extdata;
+      }
     }
     saveTo[aKey] = aStringValue;
     this.saveStateDelayed(aTab.ownerDocument.defaultView);
@@ -1663,14 +1667,16 @@ let SessionStoreInternal = {
 
   deleteTabValue: function ssi_deleteTabValue(aTab, aKey) {
     // We want to make sure that if data is accessed early, we attempt to delete
-    // that data from __SS_data as well. Otherwise we'll throw in cases where
-    // data can be set or read.
+    // that data from to-be-restored data as well. Otherwise we'll throw in
+    // cases where data can be set or read.
     let deleteFrom;
     if (aTab.__SS_extdata) {
       deleteFrom = aTab.__SS_extdata;
-    }
-    else if (aTab.linkedBrowser.__SS_data && aTab.linkedBrowser.__SS_data.extData) {
-      deleteFrom = aTab.linkedBrowser.__SS_data.extData;
+    } else {
+      let tabData = RestoringTabsData.get(aTab.linkedBrowser);
+      if (tabData && "extData" in tabData) {
+        deleteFrom = tabData.extData;
+      }
     }
 
     if (deleteFrom && deleteFrom[aKey])
@@ -1884,9 +1890,9 @@ let SessionStoreInternal = {
     if (!browser || !browser.currentURI)
       // can happen when calling this function right after .addTab()
       return tabData;
-    else if (browser.__SS_data) {
+    else if (RestoringTabsData.has(browser)) {
       // use the data to be restored when the tab hasn't been completely loaded
-      tabData = browser.__SS_data;
+      tabData = RestoringTabsData.get(browser);
       if (aTab.pinned)
         tabData.pinned = true;
       else
@@ -2164,7 +2170,7 @@ let SessionStoreInternal = {
   _updateTextAndScrollDataForTab:
     function ssi_updateTextAndScrollDataForTab(aWindow, aBrowser, aTabData, aFullData) {
     // we shouldn't update data for incompletely initialized tabs
-    if (aBrowser.__SS_data)
+    if (RestoringTabsData.has(aBrowser))
       return;
 
     var tabIndex = (aTabData.index || aTabData.entries.length) - 1;
@@ -2970,7 +2976,7 @@ let SessionStoreInternal = {
 
       // keep the data around to prevent dataloss in case
       // a tab gets closed before it's been properly restored
-      browser.__SS_data = tabData;
+      RestoringTabsData.set(browser, tabData);
       browser.__SS_restoreState = TAB_STATE_NEEDS_RESTORE;
       browser.setAttribute("pending", "true");
       tab.setAttribute("pending", "true");
@@ -3139,7 +3145,7 @@ let SessionStoreInternal = {
   restoreTab: function ssi_restoreTab(aTab) {
     let window = aTab.ownerDocument.defaultView;
     let browser = aTab.linkedBrowser;
-    let tabData = browser.__SS_data;
+    let tabData = RestoringTabsData.get(browser);
 
     // There are cases within where we haven't actually started a load. In that
     // that case we'll reset state changes we made and return false to the caller
@@ -4556,6 +4562,31 @@ let TabRestoreQueue = {
   }
 };
 
+// A map storing tabData belonging to xul:browsers of tabs. This will hold data
+// while a tab is restoring (i.e. loading). Because we can't query or use the
+// incomplete state of a loading tab we'll use data stored in the map if browser
+// state is collected while a tab is still restoring or if it's closed before
+// having restored fully.
+let RestoringTabsData = {
+  _data: new WeakMap(),
+
+  has: function (browser) {
+    return this._data.has(browser);
+  },
+
+  get: function (browser) {
+    return this._data.get(browser);
+  },
+
+  set: function (browser, data) {
+    this._data.set(browser, data);
+  },
+
+  remove: function (browser) {
+    this._data.delete(browser);
+  }
+};
+
 // This is used to help meter the number of restoring tabs. This is the control
 // point for telling the next tab to restore. It gets attached to each gBrowser
 // via gBrowser.addTabsProgressListener

browser/components/sessionstore/test/browser_579868.js

@@ -17,7 +17,6 @@ function test() {
 
     // Undo pinning
     gBrowser.unpinTab(tab1);
-    ok("__SS_data" in tab1.linkedBrowser, "tab should still be loading");
 
     // Close and restore tab
     gBrowser.removeTab(tab1);