This came out of bug 596592, where entering PB mode with just a popup and then leaving causes the last opened window to reopen. _getCurrentState is actually lying about the current state when there is a single window open and it's a popup (and there was a recently closed non-popup). It shifts a closed window into the open window list http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStore.js#1953 This is fine for when quitting and we've been doing such for a long time. But I don't think it's the right thing to do when entering/exiting private browsing mode.
How can we detect whether _getCurrentState is called during shutdown?
if (_this._loadState == STATE_QUITTING) // move last closed non-popup I think we also want to check something about _restoreLastWindow (for the all browser windows are closed, but view-source was left open), since we'll still have STATE_RUNNING.
Created attachment 476464 [details] [diff] [review] Patch v0.1 Fix it all up and it even includes a test (win/linux only). I didn't have a windows machine handy, but Sean has run this and says it passes with the fix, and fails without.
Comment on attachment 476464 [details] [diff] [review] Patch v0.1 Dietrich / Dolske: whoever gets to it first wins! I know the timing is pretty crappy, but Panorama needs this to fix a b7 blocker (it really just makes test behavior for some private browsing tests accurate). It is a bit changy - API consumers will get an accurate state as opposed to the fake state we've been providing.
Comment on attachment 476464 [details] [diff] [review] Patch v0.1 Alas, I think dietrich should do the review here, I don't know session restore well enough to judge the impact of this. Even though it's a 1-line change. >+++ b/browser/components/sessionstore/test/browser/browser_597071.js ... >+ // Test is Win/Linux only >+ // if ("nsILocalFileMac" in Ci) >+ // return; Really? :)
I pushed a workaround for this in bug 596592: http://hg.mozilla.org/mozilla-central/rev/f1eec85dcfdc It should be backed out when this bug is fixed.
Comment on attachment 476464 [details] [diff] [review] Patch v0.1 This makes sense, makes the API more consistent. To make the test only run for Win/Linux, move the check to the makefile so that it doesn't run at all. r=me w/ that change.
Sent to try with 595236
Try server failed this bug's test (timeout) on linux, see try build http://hg.mozilla.org/try/rev/617debdc2f5e
Rather, here's the proper build log: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1285017295.1285018434.11888.gz&fulltext=1
Created attachment 477282 [details] [diff] [review] Patch v1.0 (for checkin) Used #ifndef XP_MACOSX in the Makefile to target win/linux. Also moved the listener off the tabContainer to gBrowser, since that's what was most likely causing the timeout (tabContainer doesn't capture page loads, just favicons)
Pushed http://hg.mozilla.org/mozilla-central/rev/7b85cdce3ea6 to followup on the bustage. I can't do what I had done in the Makefile (don't know why it worked locally!)