Bug 625016 - Users have app tab and panorama data loss depending on window close order [r=dietrich]

browser/components/sessionstore/src/nsSessionStore.js

@@ -115,7 +115,8 @@ const CAPABILITIES = [
 
 // These keys are for internal use only - they shouldn't be part of the JSON
 // that gets saved to disk nor part of the strings returned by the API.
-const INTERNAL_KEYS = ["_tabStillLoading", "_hosts", "_formDataSaved"];
+const INTERNAL_KEYS = ["_tabStillLoading", "_hosts", "_formDataSaved",
+                       "_shouldRestore"];
 
 // These are tab events that we listen to.
 const TAB_EVENTS = ["TabOpen", "TabClose", "TabSelect", "TabShow", "TabHide",
@@ -515,6 +516,8 @@ SessionStoreService.prototype = {
       // Delete the private browsing backed up state, if any
       if ("_stateBackup" in this)
         delete this._stateBackup;
+
+      this._clearRestoringWindows();
       break;
     case "browser:purge-domain-data":
       // does a session history entry contain a url for the given domain?
@@ -565,6 +568,8 @@ SessionStoreService.prototype = {
       }
       if (this._loadState == STATE_RUNNING)
         this.saveState(true);
+
+      this._clearRestoringWindows();
       break;
     case "nsPref:changed": // catch pref changes
       switch (aData) {
@@ -640,6 +645,8 @@ SessionStoreService.prototype = {
         delete this._stateBackup;
         break;
       }
+
+      this._clearRestoringWindows();
       break;
     case "private-browsing-change-granted":
       if (aData == "enter") {
@@ -652,6 +659,8 @@ SessionStoreService.prototype = {
       // Make sure _tabsToRestore is cleared. It will be repopulated when
       // entering/exiting private browsing (by calls to setBrowserState).
       this._resetRestoringState();
+
+      this._clearRestoringWindows();
       break;
     }
   },
@@ -702,6 +711,8 @@ SessionStoreService.prototype = {
         this.saveStateDelayed(win);
         break;
     }
+
+    this._clearRestoringWindows();
   },
 
   /**
@@ -902,6 +913,12 @@ SessionStoreService.prototype = {
         this._updateCookies([winData]);
       }
 
+#ifndef XP_MACOSX
+      // Until we decide otherwise elsewhere, this window is part of a series
+      // of closing windows to quit.
+      winData._shouldRestore = true;
+#endif
+
       // save the window if it has multiple tabs or a single saveable tab
       if (winData.tabs.length > 1 ||
           (winData.tabs.length == 1 && this._shouldSaveTabState(winData.tabs[0]))) {
@@ -3372,6 +3389,22 @@ SessionStoreService.prototype = {
     if (!oState)
       return;
 
+#ifndef XP_MACOSX
+    // We want to restore closed windows that are marked with _shouldRestore.
+    // We're doing this here because we want to control this only when saving
+    // the file.
+    while (oState._closedWindows.length) {
+      let i = oState._closedWindows.length - 1;
+      if (oState._closedWindows[i]._shouldRestore) {
+        oState.windows.unshift(oState._closedWindows.pop());
+      }
+      else {
+        // We only need to go until we hit !needsRestore since we're going in reverse
+        break;
+      }
+    }
+#endif
+
     if (pinnedOnly) {
       // Save original resume_session_once preference for when quiting browser,
       // otherwise session will be restored next time browser starts and we
@@ -3959,6 +3992,12 @@ SessionStoreService.prototype = {
     this._closedWindows.splice(spliceTo);
   },
 
+  _clearRestoringWindows: function sss__clearRestoringWindows() {
+    for (let i = 0; i < this._closedWindows.length; i++) {
+      delete this._closedWindows[i]._shouldRestore;
+    }
+  },
+
   /**
    * Reset state to prepare for a new session state to be restored.
    */

browser/components/sessionstore/test/browser/Makefile.in

@@ -153,6 +153,7 @@ _BROWSER_TEST_FILES = \
 ifneq ($(OS_ARCH),Darwin)
 _BROWSER_TEST_FILES += \
 	browser_597071.js \
+	browser_625016.js \
 	$(NULL)
 endif
 

browser/components/sessionstore/test/browser/browser_625016.js

@@ -0,0 +1,53 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+let newWin;
+
+function test() {
+  /** Test for Bug 625016 - Restore windows closed in succession to quit (non-OSX only) **/
+
+  // We'll test this by opening a new window, waiting for the save event, then
+  // closing that window. We'll observe the "sessionstore-state-write" notification
+  // and check that the state contains no _closedWindows.
+
+  waitForExplicitFinish();
+
+  registerCleanupFunction(function() {
+  });
+
+  // We speed up the interval between session saves to ensure that the test
+  // runs quickly.
+  Services.prefs.setIntPref("browser.sessionstore.interval", 2000);
+
+  // We'll clear all closed windows to make sure our state is clean
+  while(SS_SVC.getClosedWindowCount()) {
+    SS_SVC.forgetClosedWindow();
+  }
+
+  // Open a new window, which should trigger a save event soon.
+  waitForSaveState(onSaveState);
+  newWin = openDialog(location, "_blank", "chrome,all,dialog=no", "about:robots");
+}
+
+function onSaveState() {
+  // Double check that we have no closed windows
+  is(SS_SVC.countClosedWindows(), 0, "no closed windows on first save");
+  Services.obs.addObserver(observe, "sessionstore-state-write", true);
+
+  // Now close the new window, which should trigger another save event
+  newWin.close();
+}
+
+// Our observer for "sessionstore-write-data"
+function observe(aSubject, aTopic, aData) {
+  dump(aSubject);
+  dump(aData);
+  let state = JSON.parse(aData);
+  done();
+}
+
+
+function done() {
+  Service.prefs.clearUserPref("browser.sessionstore.interval");
+  finish();
+}