Bug 625016 - Restore windows closed in succession to quit (non-OSX only) [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",
@@ -524,6 +525,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?
@@ -574,6 +577,8 @@ SessionStoreService.prototype = {
       }
       if (this._loadState == STATE_RUNNING)
         this.saveState(true);
+
+      this._clearRestoringWindows();
       break;
     case "nsPref:changed": // catch pref changes
       switch (aData) {
@@ -649,6 +654,8 @@ SessionStoreService.prototype = {
         delete this._stateBackup;
         break;
       }
+
+      this._clearRestoringWindows();
       break;
     case "private-browsing-change-granted":
       if (aData == "enter") {
@@ -661,6 +668,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;
     }
   },
@@ -711,6 +720,8 @@ SessionStoreService.prototype = {
         this.saveStateDelayed(win);
         break;
     }
+
+    this._clearRestoringWindows();
   },
 
   /**
@@ -910,7 +921,13 @@ SessionStoreService.prototype = {
                                                   tabbrowser.selectedTab);
         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]))) {
@@ -3385,6 +3402,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
@@ -3973,6 +4006,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,108 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+let newWin;
+let newTab;
+
+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. We'll then add a new
+  // tab and make sure that the state following that was reset and the closed
+  // window is now in _closedWindows.
+
+  waitForExplicitFinish();
+
+  // 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
+  // forgetClosedWindow doesn't trigger a delayed save
+  while(SS_SVC.getClosedWindowCount()) {
+    SS_SVC.forgetClosedWindow(0);
+  }
+  is(SS_SVC.getClosedWindowCount(), 0, "starting with no closed windows");
+
+  // 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.getClosedWindowCount(), 0, "no closed windows on first save");
+
+  Services.obs.addObserver(observe1, "sessionstore-state-write", false);
+
+  // Now close the new window, which should trigger another save event
+  newWin.close();
+}
+
+function observe1(aSubject, aTopic, aData) {
+  info("observe1: " + aTopic);
+  switch(aTopic) {
+    case "sessionstore-state-write":
+      aSubject.QueryInterface(Ci.nsISupportsString);
+      let state = JSON.parse(aSubject.data);
+      is(state.windows.length, 2,
+         "observe1: 2 windows in data being writted to disk");
+      is(state._closedWindows.length, 0,
+         "observe1: no closed windows in data being writted to disk");
+
+      // The API still treats the closed window as closed, so ensure that window is there
+      is(SS_SVC.getClosedWindowCount(), 1,
+         "observe1: 1 closed window according to API");
+      Services.obs.removeObserver(observe1, "sessionstore-state-write", false);
+      Services.obs.addObserver(observe1, "sessionstore-state-write-complete", false);
+      break;
+    case "sessionstore-state-write-complete":
+      Services.obs.removeObserver(observe1, "sessionstore-state-write-complete", false);
+      openTab();
+      break;
+  }
+}
+
+function observe2(aSubject, aTopic, aData) {
+  info("observe2: " + aTopic);
+  switch(aTopic) {
+    case "sessionstore-state-write":
+      aSubject.QueryInterface(Ci.nsISupportsString);
+      let state = JSON.parse(aSubject.data);
+      is(state.windows.length, 1,
+         "observe2: 1 window in data being writted to disk");
+      is(state._closedWindows.length, 1,
+         "observe2: 1 closed window in data being writted to disk");
+
+      // The API still treats the closed window as closed, so ensure that window is there
+      is(SS_SVC.getClosedWindowCount(), 1,
+         "observe2: 1 closed window according to API");
+      Services.obs.removeObserver(observe2, "sessionstore-state-write", false);
+      Services.obs.addObserver(observe2, "sessionstore-state-write-complete", false);
+      break;
+    case "sessionstore-state-write-complete":
+      Services.obs.removeObserver(observe2, "sessionstore-state-write-complete", false);
+      done();
+      break;
+  }
+}
+
+// We'll open a tab, which should trigger another state save which would wipe
+// the _shouldRestore attribute from the closed window
+function openTab() {
+  Services.obs.addObserver(observe2, "sessionstore-state-write", false);
+  newTab = gBrowser.addTab("about:mozilla");
+}
+
+function done() {
+  Services.prefs.clearUserPref("browser.sessionstore.interval");
+  gBrowser.removeTab(newTab);
+  // The API still represents the closed window as closed, so we can clear it
+  // with the API, but just to make sure...
+  is(SS_SVC.getClosedWindowCount(), 1, "1 closed window according to API");
+  SS_SVC.forgetClosedWindow(0);
+  executeSoon(finish);
+}
+