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

Categories

(Firefox :: Session Restore, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 4.0b7
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: mats, Assigned: mats)

Details

Attachments

(2 files)

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.
Attached patch Like so?Splinter Review
Attachment #475838 - Flags: review?(paul)
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)
blocking2.0: --- → ?
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
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b7
blocking2.0: ? → betaN+
You need to log in before you can comment on or make changes to this bug.