596592 - Leaving Private Browsing mode resurrects a window that was closed at the time of entering PB

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Mats Palmgren (inactive)]

Attachment: browser-chrome mochitest

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.

Comment 1

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

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.

Comment 2

[Mats Palmgren (inactive)]

You're right, leaving PB is what creates the window, not setWindowState.
The bug occurs on Windows and Linux, but not OSX.

Comment 3

[Mats Palmgren (inactive)]

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 4

[Mats Palmgren (inactive)]

Attachment: Like so?

Comment 5

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

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.

Comment 6

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

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 7

[(no longer active)]

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.

Comment 8

[Mats Palmgren (inactive)]

Added a comment in the testcase, and in bug 597071.
http://hg.mozilla.org/mozilla-central/rev/f1eec85dcfdc