Closed Bug 596592 Opened 9 years ago Closed 9 years ago
Leaving Private Browsing mode resurrects a window that was closed at the time of entering PB
nsISessionStore::setWindowState resurrects a window that was closed at the time of the getWindowState. The browser-chrome mochitest browser/components/privatebrowsing/test/browser/browser_privatebrowsing_popupmode.js does something like this: let stateBackup = ss.getWindowState(window); // do a bunch of tests ... ss.setWindowState(window, stateBackup, true); the problem is that when restoring the 'stateBackup' a window that was closed at the time of the 'getWindowState()' is opened. This is what bit me in bug 449734. The tests I add in that bug create a tab, detaches it using 'replaceTabWithWindow', and finally close the new window. My tests work fine when run individually, but when running the whole browser-chrome suite "browser_privatebrowsing_popupmode.js" which runs some 40 or 50 tests later, resurrected a window from my test. I have combined the essence of the two tests into a new test to demonstrate the problem, see the attached patch. STEPS TO REPRODUCE 1. apply the attached patch 2. rebuild browser/base/content/test/ 3. run "mochitest --browser-chrome --autorun --test=browser/base/content/test/browser_setWindowState_BUG.js" WORKAROUND After you 'close()' the window created by 'replaceTabWithWindow' you can do: Cc["@mozilla.org/browser/sessionstore;1"].getService(Ci.nsISessionStore).forgetClosedWindow(0); to avoid it causing a problem with 'get/setWindowState' later.
Just from a quick glance, I'm going to guess this is related to turning a window into a popup and/or the fact that you're calling setWindowState with an empty state. Though I guess that's the private browsing test, not you. And I bet the problem is actually showing up after you do pb.enabled = false, which calls ss.setBrowserState. We have code that make sures a non-popup is reopened if the last window was a popup (on Windows) when restoring.
You're right, leaving PB is what creates the window, not setWindowState. The bug occurs on Windows and Linux, but not OSX.
Summary: nsISessionStore::setWindowState resurrects a window that was closed at the time of the getWindowState → Leaving Private Browsing mode resurrects a window that was closed at the time of entering PB
The problem does not occur if I remove this block: http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStore.js#1953 The code is probably fine, I can't reproduce the problem with normal usage of the browser. So, I suggest we should just fix the test.
Comment on attachment 475838 [details] [diff] [review] Like so? Looks OK to me, but I'll give it to ehsan since it's in a PB test and that's him. I'm thinking it might be a good idea to fix this properly, such that setBrowserState doesn't follow the one-non-popup rule. I filed bug 597071 for that and know exactly how it should be fixed & tested. I don't have time to fix it this second, so I'll support this workaround for now.
Attachment #475838 - Flags: review?(paul) → review?(ehsan)
Looking more like we want this ASAP. That test seems to be causing an issue for a tab candy test as well (since it opens a window, then closes it). All the more reason to fix it properly...
Comment on attachment 475838 [details] [diff] [review] Like so? r=me if you add a comment pointing out that this needs to be removed once bug 597071 is fixed.
Attachment #475838 - Flags: review?(ehsan) → review+
Added a comment in the testcase, and in bug 597071. http://hg.mozilla.org/mozilla-central/rev/f1eec85dcfdc
Assignee: nobody → matspal
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b7
You need to log in before you can comment on or make changes to this bug.