Bug 528776 - discard stale windows before messing with the browser state. r=zeniko
authorDão Gottwald <dao@mozilla.com>
Sat, 28 Nov 2009 07:39:31 +0100
changeset 35664 d1891791ecd22a0fa28510c508b12d9b6b9b008a
parent 35663 2294a3f7932df0193fc0bc9b63562da81f6348f7
child 35665 6bd943c2c7bb5b3e909f40aff468d90260d8fde8
push id10670
push userdgottwald@mozilla.com
push dateSun, 13 Dec 2009 14:26:37 +0000
treeherdermozilla-central@d1891791ecd2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerszeniko
bugs528776
milestone1.9.3a1pre
Bug 528776 - discard stale windows before messing with the browser state. r=zeniko
browser/components/sessionstore/src/nsSessionStore.js
browser/components/sessionstore/test/browser/Makefile.in
browser/components/sessionstore/test/browser/browser_528776.js
--- a/browser/components/sessionstore/src/nsSessionStore.js
+++ b/browser/components/sessionstore/src/nsSessionStore.js
@@ -181,19 +181,16 @@ SessionStoreService.prototype = {
   _recentCrashes: 0,
 
   // whether we are in private browsing mode
   _inPrivateBrowsing: false,
 
   // whether we clearing history on shutdown
   _clearingOnShutdown: false,
 
-  // List of windows that are being closed during setBrowserState.
-  _closingWindows: [],
-
 #ifndef XP_MACOSX
   // whether the last window was closed and should be restored
   _restoreLastWindow: false,
 #endif
 
 /* ........ Global Event Handlers .............. */
 
   /**
@@ -335,27 +332,19 @@ SessionStoreService.prototype = {
     // for event listeners
     var _this = this;
 
     switch (aTopic) {
     case "domwindowopened": // catch new windows
       aSubject.addEventListener("load", function(aEvent) {
         aEvent.currentTarget.removeEventListener("load", arguments.callee, false);
         _this.onLoad(aEvent.currentTarget);
-        }, false);
+      }, false);
       break;
     case "domwindowclosed": // catch closed windows
-      if (this._closingWindows.length > 0) {
-        let index = this._closingWindows.indexOf(aSubject);
-        if (index != -1) {
-          this._closingWindows.splice(index, 1);
-          if (this._closingWindows.length == 0)
-            this._sendRestoreCompletedNotifications(true);
-        }
-      }
       this.onClose(aSubject);
       break;
     case "quit-application-requested":
       // get a current snapshot of all windows
       this._forEachBrowserWindow(function(aWindow) {
         this._collectWindowData(aWindow);
       });
       this._dirtyWindows = [];
@@ -896,47 +885,48 @@ SessionStoreService.prototype = {
 
 /* ........ nsISessionStore API .............. */
 
   getBrowserState: function sss_getBrowserState() {
     return this._toJSONString(this._getCurrentState());
   },
 
   setBrowserState: function sss_setBrowserState(aState) {
+    this._handleClosedWindows();
+
     try {
       var state = this._safeEval("(" + aState + ")");
     }
     catch (ex) { /* invalid state object - don't restore anything */ }
     if (!state || !state.windows)
       throw (Components.returnCode = Cr.NS_ERROR_INVALID_ARG);
 
     this._browserSetState = true;
 
     var window = this._getMostRecentBrowserWindow();
     if (!window) {
       this._restoreCount = 1;
       this._openWindowWithState(state);
       return;
     }
 
+    // close all other browser windows
+    this._forEachBrowserWindow(function(aWindow) {
+      if (aWindow != window) {
+        aWindow.close();
+        this.onClose(aWindow);
+      }
+    });
+
     // make sure closed window data isn't kept
     this._closedWindows = [];
 
     // determine how many windows are meant to be restored
     this._restoreCount = state.windows ? state.windows.length : 0;
 
-    var self = this;
-    // close all other browser windows
-    this._forEachBrowserWindow(function(aWindow) {
-      if (aWindow != window) {
-        self._closingWindows.push(aWindow);
-        aWindow.close();
-      }
-    });
-
     // restore to the given state
     this.restoreWindow(window, state, true);
   },
 
   getWindowState: function sss_getWindowState(aWindow) {
     if (!aWindow.__SSi && !aWindow.__SS_dyingCache)
       throw (Components.returnCode = Cr.NS_ERROR_INVALID_ARG);
     
@@ -1717,30 +1707,32 @@ SessionStoreService.prototype = {
 
   /**
    * serialize session data as Ini-formatted string
    * @param aUpdateAll
    *        Bool update all windows 
    * @returns string
    */
   _getCurrentState: function sss_getCurrentState(aUpdateAll) {
+    this._handleClosedWindows();
+
     var activeWindow = this._getMostRecentBrowserWindow();
     
     if (this._loadState == STATE_RUNNING) {
       // update the data for all windows with activities since the last save operation
       this._forEachBrowserWindow(function(aWindow) {
         if (!this._isWindowLoaded(aWindow)) // window data is still in _statesToRestore
           return;
         if (aUpdateAll || this._dirtyWindows[aWindow.__SSi] || aWindow == activeWindow) {
           this._collectWindowData(aWindow);
         }
         else { // always update the window features (whose change alone never triggers a save operation)
           this._updateWindowFeatures(aWindow);
         }
-      }, this);
+      });
       this._dirtyWindows = [];
     }
     
     // collect the data for all windows
     var total = [], windows = [];
     var nonPopupCount = 0;
     var ix;
     for (ix in this._windows) {
@@ -2666,16 +2658,34 @@ SessionStoreService.prototype = {
       if (!win.closed)
         return win;
     }
     return null;
 #endif
   },
 
   /**
+   * Calls onClose for windows that are determined to be closed but aren't
+   * destroyed yet, which would otherwise cause getBrowserState and
+   * setBrowserState to treat them as open windows.
+   */
+  _handleClosedWindows: function sss_handleClosedWindows() {
+    var windowMediator = Cc["@mozilla.org/appshell/window-mediator;1"].
+                         getService(Ci.nsIWindowMediator);
+    var windowsEnum = windowMediator.getEnumerator("navigator:browser");
+
+    while (windowsEnum.hasMoreElements()) {
+      var window = windowsEnum.getNext();
+      if (window.closed) {
+        this.onClose(window);
+      }
+    }
+  },
+
+  /**
    * open a new browser window for a given session state
    * called when restoring a multi-window session
    * @param aState
    *        Object containing session data
    */
   _openWindowWithState: function sss_openWindowWithState(aState) {
     var argString = Cc["@mozilla.org/supports-string;1"].
                     createInstance(Ci.nsISupportsString);
@@ -2876,27 +2886,26 @@ SessionStoreService.prototype = {
       // instead of evalInSandbox everywhere
       jsonString = jsonString.replace(/[\u2028\u2029]/g,
                                       function($0) "\\u" + $0.charCodeAt(0).toString(16));
     }
     
     return jsonString;
   },
 
-  _sendRestoreCompletedNotifications:
-  function sss_sendRestoreCompletedNotifications(aOnWindowClose) {
-    if (this._restoreCount && !aOnWindowClose)
+  _sendRestoreCompletedNotifications: function sss_sendRestoreCompletedNotifications() {
+    if (this._restoreCount) {
       this._restoreCount--;
-
-    if (this._restoreCount == 0 && this._closingWindows.length == 0) {
-      // This was the last window restored at startup, notify observers.
-      this._observerService.notifyObservers(null,
-        this._browserSetState ? NOTIFY_BROWSER_STATE_RESTORED : NOTIFY_WINDOWS_RESTORED,
-        "");
-      this._browserSetState = false;
+      if (this._restoreCount == 0) {
+        // This was the last window restored at startup, notify observers.
+        this._observerService.notifyObservers(null,
+          this._browserSetState ? NOTIFY_BROWSER_STATE_RESTORED : NOTIFY_WINDOWS_RESTORED,
+          "");
+        this._browserSetState = false;
+      }
     }
   },
 
   /**
    * @param aWindow
    *        Window reference
    * @returns whether this window's data is still cached in _statesToRestore
    *          because it's not fully loaded yet
--- a/browser/components/sessionstore/test/browser/Makefile.in
+++ b/browser/components/sessionstore/test/browser/Makefile.in
@@ -104,12 +104,13 @@ include $(topsrcdir)/config/rules.mk
 	browser_490040.js \
 	browser_491168.js \
 	browser_491577.js \
 	browser_493467.js \
 	browser_495495.js \
 	browser_514751.js \
 	browser_522545.js \
 	browser_526613.js \
+	browser_528776.js \
 	$(NULL)
 
 libs:: $(_BROWSER_TEST_FILES)
 	$(INSTALL) $(foreach f,$^,"$f") $(DEPTH)/_tests/testing/mochitest/browser/$(relativesrcdir)
new file mode 100644
--- /dev/null
+++ b/browser/components/sessionstore/test/browser/browser_528776.js
@@ -0,0 +1,29 @@
+var ss = Cc["@mozilla.org/browser/sessionstore;1"].getService(Ci.nsISessionStore);
+var wm = Cc["@mozilla.org/appshell/window-mediator;1"].getService(Ci.nsIWindowMediator);
+
+function browserWindowsCount(expected) {
+  var count = 0;
+  var e = wm.getEnumerator("navigator:browser");
+  while (e.hasMoreElements()) {
+    if (!e.getNext().closed)
+      ++count;
+  }
+  is(count, expected,
+     "number of open browser windows according to nsIWindowMediator");
+  is(JSON.parse(ss.getBrowserState()).windows.length, expected,
+     "number of open browser windows according to getBrowserState");
+}
+
+function test() {
+  waitForExplicitFinish();
+
+  browserWindowsCount(1);
+
+  var win = openDialog(location, "", "chrome,all,dialog=no");
+  win.addEventListener("load", function () {
+    browserWindowsCount(2);
+    win.close();
+    browserWindowsCount(1);
+    finish();
+  }, false);
+}