Quitting from a Private Browsing window makes Restore Previous Session fail once

RESOLVED FIXED in Firefox 20

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: mozbz, Assigned: bellindira)

Tracking

20 Branch
Firefox 20
x86_64
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [appcoast])

Attachments

(1 attachment, 6 obsolete attachments)

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.
Component: Untriaged → Private Browsing
Blocks: fxPBnGen
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!
Blocks: PBnGen, 722985
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [appcoast]
Assignee: nobody → bellindira
Status: NEW → ASSIGNED
Posted patch Patch (obsolete) — Splinter Review
This issue was caused because the selectedWindow was not updated when the browser state is saved and private windows are removed.
Attachment #691114 - Flags: review?(ehsan)
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.
Attachment #691114 - Flags: review?(ehsan) → review?(ttaubert)
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.
Attachment #691114 - Flags: review?(ttaubert) → review+
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.
Attachment #691114 - Flags: review+ → review-
Posted patch Patch v2 (obsolete) — Splinter Review
- Updated the patch according to the suggestions.
- I'll post a new patch for the test case later.
Attachment #691114 - Attachment is obsolete: true
Attachment #691407 - Flags: review?(ttaubert)
Posted patch Patch v2 (obsolete) — Splinter Review
Removed all white spaces changes from previous patch
Attachment #691407 - Attachment is obsolete: true
Attachment #691407 - Flags: review?(ttaubert)
Attachment #691412 - Flags: review?(ttaubert)
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.
Attachment #691412 - Flags: review?(ttaubert) → review+
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
Posted patch Patch + testcase (obsolete) — Splinter Review
Attachment #691412 - Attachment is obsolete: true
Attachment #692152 - Flags: review?(ttaubert)
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.
Attachment #692152 - Flags: review?(ttaubert) → review+
Blocks: 821847
Posted patch Patch + testcase (obsolete) — Splinter Review
- Fixed nits.
- Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=51639e8772df
- Follow up Bug 821847
Attachment #692152 - Attachment is obsolete: true
Attachment #692422 - Flags: review?(ttaubert)
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...)
Attachment #692422 - Flags: review?(ttaubert)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e28f0f4b25a4
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
No longer blocks: 822093
Depends on: 822093
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 20 → ---
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 :-)
Posted patch Patch v5 (obsolete) — Splinter Review
Rebased patch with latest fixes in Bug 822093.
Attachment #692422 - Attachment is obsolete: true
Duplicate of this bug: 823127
Posted patch Patch v6Splinter Review
Added a new check to ensure the window is closed. 

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=a70b8f394033
Attachment #693940 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/e7b858cfccc2
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
You need to log in before you can comment on or make changes to this bug.