User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20121204 Firefox/20.0 Build ID: 20121204030754 Steps to reproduce: (Per-Window Private Browsing testday) 1. In Options -> General, have Firefox start with a homepage or a blank page. 2. Visit a few sites in tabs. 3. Go to the File menu and select "New Private Window" and visit some different sites. 4. In the Private Window, go to the File menu and select "Exit". Both windows close. 5. Start Firefox. 6. From the History menu, choose "Restore Previous Session". Actual results: At Step 6, no tabs are restored. However, repeating Step 6 will correctly restore the tabs from the non-Private session. Expected results: The tabs should be restored first time.

There is also a problem when you restore previous session by default. If you close a private window after closing all normal ones and you restart the browser, the session is forgotten (even pinned tabs).

This is what I get in the console: JavaScript error: chrome://browser/content/browser.js, line 9727: NS_ERROR_XPC_JS_THREW_JS_OBJECT: 'TypeError: winState is undefined' when calling method: [nsISessionStore::restoreLastSession] Andres, can you please look into this? Thanks!

This issue was caused because the selectedWindow was not updated when the browser state is saved and private windows are removed.

Comment on attachment 691114 [details] [diff] [review] Patch > // Restore into windows or open new ones as needed. > for (let i = 0; i < lastSessionState.windows.length; i++) { > let winState = lastSessionState.windows[i]; >+ // Make sure the window exist >+ if (winState) { make this: if (!winState) continue;

Comment on attachment 691114 [details] [diff] [review] Patch Review of attachment 691114 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, but I'd prefer Tim to review it.

Comment on attachment 691114 [details] [diff] [review] Patch Review of attachment 691114 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me but please update the patch and follow Dão's advice. It's always preferable to not lose too many blame information for small changes like this.

Comment on attachment 691114 [details] [diff] [review] Patch Review of attachment 691114 [details] [diff] [review]: ----------------------------------------------------------------- Wait, if the core issue is that .selectedWindow isn't updated properly we should do exactly that on line http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/SessionStore.jsm#3713 Something like: if (oState.selectedWindow >= i) { oState.selectedWindow--; } Also, I'd really like to have a test for this.

- Updated the patch according to the suggestions. - I'll post a new patch for the test case later.

Removed all white spaces changes from previous patch

Comment on attachment 691412 [details] [diff] [review] Patch v2 Review of attachment 691412 [details] [diff] [review]: ----------------------------------------------------------------- Waiting for the test to make sure this works. I just assume you tested this manually? Thanks! ::: browser/components/sessionstore/src/SessionStore.jsm @@ +1757,5 @@ > for (let i = 0; i < lastSessionState.windows.length; i++) { > let winState = lastSessionState.windows[i]; > + // Make sure the window exist > + if (!winState) > + continue; We don't really need this anymore, do we? I'd rather have this fail than silently swallowing errors.

Bellindira, are you going to provide an updated patch here with tests? Just want to make sure this does not fall off the radar. Thanks!

Yes, I'm going to update the patch with the test. I'm working on it. Thanks

Comment on attachment 692152 [details] [diff] [review] Patch + testcase Review of attachment 692152 [details] [diff] [review]: ----------------------------------------------------------------- LGTM in general but the test fails when run in single-mode (as do browser_394759_perwindowpb.js and browser_354894_perwindowpb.js). I'd be fine with fixing those three in a follow-up. r=me with a follow-up filed. Please push to try before marking as checkin-needed. ::: browser/components/sessionstore/test/browser_819510_perwindowpb.js @@ +1,3 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ Tests should have a public domain header: /* Any copyright is dedicated to the Public Domain. * http://creativecommons.org/publicdomain/zero/1.0/ */ @@ +40,5 @@ > + let currentTest = tests.shift(); > + waitForBrowserState(testState, currentTest); > + } > + else { > + Services.prefs.clearUserPref("browser.sessionstore.interval"); Please put this in the test() function and wrap it in a registerCleanupFunction() call. So we're sure this is called when we time out. @@ +65,5 @@ > + is (curState.windows[4].isPrivate, true, "Last window is private"); > + is (curState.selectedWindow, 5, "Last window opened is the one selected"); > + > + Services.obs.addObserver(function (aSubject, aTopic, aData) { > + Services.obs.removeObserver(arguments.callee, aTopic); Please assign a function name and use that instead of arguments.callee, that's deprecated. @@ +101,5 @@ > + is (curState.windows[2].isPrivate, true, "Window 2 is private"); > + is (curState.selectedWindow, 3, "Last window opened is the one selected"); > + > + Services.obs.addObserver(function (aSubject, aTopic, aData) { > + Services.obs.removeObserver(arguments.callee, aTopic); Please assign a function name and use that instead of arguments.callee. @@ +133,5 @@ > + is (curState.selectedWindow, 4, "Last window opened is the one selected"); > + > + normalWindow.close(); > + Services.obs.addObserver(function (aSubject, aTopic, aData) { > + Services.obs.removeObserver(arguments.callee, aTopic); Please assign a function name and use that instead of arguments.callee.

- Fixed nits. - Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=51639e8772df - Follow up Bug 821847

The try job was based on an older m-c which did not have MOZ_PER_WINDOW_PRIVATE_BROWSING set by default, so it was actually not testing anything useful. I'll push this to try again.

Comment on attachment 692422 [details] [diff] [review] Patch + testcase (r+ usually means that you don't need to ask for another review after addressing the nits...)

Backed out for intermittent test failures (bug 822093). Before this relands the test either needs fixing, or the reviewer would need to approve landing without tests. https://hg.mozilla.org/integration/mozilla-inbound/rev/10cb0e3aac02

Please may we also s/writted/written/ when this relands?

(In reply to Ed Morley [UTC+0; email:edmorley@moco] from comment #21) > Backed out for intermittent test failures (bug 822093). Before this relands > the test either needs fixing, or the reviewer would need to approve landing > without tests. > > https://hg.mozilla.org/integration/mozilla-inbound/rev/10cb0e3aac02 https://hg.mozilla.org/mozilla-central/rev/10cb0e3aac02

I added the test fix in Bug 822093 comment 16 and is all green on try. See: https://tbpl.mozilla.org/?tree=Try&rev=17c1e98fec0f Let me know if the whole patch fixed is needed.

(In reply to Andres Hernandez [:andreshm] from comment #24) > I added the test fix in Bug 822093 comment 16 and is all green on try. > See: https://tbpl.mozilla.org/?tree=Try&rev=17c1e98fec0f > > Let me know if the whole patch fixed is needed. The failure was intermittent (rather than permanently orange), so one green run may not be sufficient to tell if this is fixed. I've retriggered some of the runs on https://tbpl.mozilla.org/?tree=Try&rev=17c1e98fec0f to help prove either way. Thank you for looking at this :-)

Rebased patch with latest fixes in Bug 822093.

Added a new check to ensure the window is closed. Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=a70b8f394033

All green on try: https://tbpl.mozilla.org/?tree=Try&rev=a70b8f394033