Closed Bug 1081135 Opened 10 years ago Closed 10 years ago

Restore Previous Session opens multiple instances

Categories

(Firefox :: Session Restore, defect)

35 Branch
defect
Not set
normal
Points:
2

Tracking

()

VERIFIED FIXED
Firefox 36
Iteration:
35.3
Tracking Status
firefox34 --- unaffected
firefox35 + verified
firefox36 --- verified

People

(Reporter: mozilla, Assigned: ttaubert)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(2 files)

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20141010030201

Steps to reproduce:

1. Perform clean boot.
2. Click on Nightly icon on taskbar or desktop to open Nightly.
3. If desired, click on Webroot icon at top right to open Webroot addon.
4. Check how many instances of Nightly are running (should be only one).
5. Click on Restore Previous Session icon at bottom of screen that opens.



Actual results:

Previous session actually is restored, but two (or now, three) identical instances of Nightly are launched.


Expected results:

Previous session should be restored (which it is), but only one instance of Nightly should be launched.
This can include tabs from private windows.
Severity: normal → major
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
OS: Windows 7 → All
The workaround is to not set "Show my windows and tabs from last time" in Options > General.
Reg range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f0bb13ef0ee4&tochange=50b689feab5f


My guess goes to:
Tim Taubert — Bug 1073992 - Keep track of revivable windows separately to allow reviving more windows than the max_undo_windows pref allows r=yoric
Blocks: 1073992
Component: Untriaged → Session Restore
I don't know whether it's relevant, but when I started having this problem I had an enormous number of tabs open (close to 100). I wondered whether this might have something to do with the problem, so I closed a large number of tabs, then quit and restarted Nightly. However, I still had the problem with few windows open.
I had only 2 or 3 tabs open, at the next startup, I got 2 windows with the last 2-3 tabs.
It would be nice to have a respin, because this regression is really annoying to test Nightly, I can't restore my last session without having a bunch on windows to close.
I see this too, even after changing the "When Nightly Starts:" setting, restarting, changing it back, then restarting again.

Cleaning the session etc in CCleaner didn't stop it either.
Flags: needinfo?(ttaubert)
I see this even though I have it set to "Show my home page". Related to my pinned tabs, perhaps?
in onClose we now have


 -#ifndef XP_MACOSX
        // Until we decide otherwise elsewhere, this window is part of a series
        // of closing windows to quit.
 -      winData._shouldRestore = true;
 -#endif
 +      RevivableWindows.add(winData);

and later in the same function:

-        let isLastWindow =
-          Object.keys(this._windows).length == 1 &&
-          !this._closedWindows.some(win => win._shouldRestore || false);
+        let numOpenWindows = Object.keys(this._windows).length;
+        let isLastWindow = numOpenWindows == 1 && RevivableWindows.isEmpty;

with this code isLastWindow is always false, RevivableWindows.isEmpty is false after you add a window to RevivableWindows before in the same function
If this bug is not going to be fixed in the next days, could we back out bug 1073992 for the next nightlies?
Also "restoring" multiple really old small pop-up windows.
(In reply to tabmix.onemen from comment #10)
> with this code isLastWindow is always false, RevivableWindows.isEmpty is
> false after you add a window to RevivableWindows before in the same function

Great analysis, thanks!
Flags: needinfo?(ttaubert)
Assignee: nobody → ttaubert
Severity: major → normal
Hardware: x86 → All
Added a test that ensures we don't put private window in the RevivableWindows bucket. We regressed bug 495123 as well but unfortunately we can't check |isLastWindow| without proper shutdown tests :(
Attachment #8503633 - Flags: review?(dteller)
Iteration: --- → 35.3
Points: --- → 2
QA Whiteboard: [qa+]
Status: NEW → ASSIGNED
QA Whiteboard: [qa+]
Flags: qe-verify+
Flags: firefox-backlog+
Comment on attachment 8503633 [details] [diff] [review]
0001-Bug-1081135-Don-t-put-private-windows-in-the-revivab.patch

Review of attachment 8503633 [details] [diff] [review]:
-----------------------------------------------------------------

Good catch. I wonder what made it work before the previous patch.
Attachment #8503633 - Flags: review?(dteller) → review+
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #16)
> Good catch. I wonder what made it work before the previous patch.

We previously set _shouldRestore=true on the winData object but only unshift()'ed it later to _closedWindows[]. That's why it worked... Good that this stuff is a little clearer now.
Just testing this patch, and it does NOT fix the problem.

Each close/re-open of the browser is generating more and more windows.  Started out with main and 1 extra, now opening the main and 3 more besides.....

win7 x64 cset: https://hg.mozilla.org/integration/fx-team/rev/44cb72be622d
Deleting sessionstore.js with Firefox closed, restored my last session anyhow, was expecting no session to restore thinking it may be corrupt.  Still opened 4 windows...
Also tried deleting sessionstore.js and sessionCheckpoints.json and my session is still getting restored.

After deleting sessionCheckpoins.json I only got one extra window, closing and re-opening resulted in main + 2 windows, seems to grow to a max of 4 windows being restored.

I'm quite puzzled why its restoring 'anything' with the session deleted.
Same thing occurs in a new Profile.

Open the browser 
Close
Re-open main single tab window + a duplicate
Close again and re-open, now a main window + 2 more...
more problems...

1. in onClose you should call RevivableWindows.add(winData) just before you set this._closedWindows.unshift(winData). otherwise you can add winData with hasSaveableTabs false to RevivableWindows list.

2. SessionSaver._saveState call to getCurrentState that push the last closed window back to state.windows
and since you update RevivableWindows we end up duplicate windows when you push all RevivableWindows into state.windows in SessionSaver._saveState.

3. there are places in the code that can change _closedWindows without changing RevivableWindows. for example
forgetClosedWindow, and undoCloseWindow.
I can return to functioning state by doing the following:

1. Close all windows except the one you want to restore normally.
2. Open a new tab. (This really needs to be done)
3. Close Firefox with it's own Exit function (not window controls)

Next time you start it up only that single window is restored. Likewise, your other opened windows are correctly restored as long as Firefox is closed with it's own exit functionality.

I didn't even see this problem for days as I don't normally use window control buttons at all.
For me, I find this only shows up when there is a pinned tab present (or sessionstore.js is not deleted before next opening... thus clearing pinned tabs) on exiting Firefox (regardless of how Firefox is closed). So it seems to be a problem between sessionstore and pinned tabs. I noticed this started on Thursday, October 9th.

However, it seems to exponentially open more and more windows each time Firefox is re-opened (meaning: first open = 1 win + 1 pinnned tab, second open = 2 win + 1 pin tab each, third open = 4 win + 1 pin tab each, etc.).

I did try this what Jastekken suggested, but got the same thing happening.

So far the only thing that seems to work is: don't have pinned tabs at close or delete sessionstore.js before next start of Firefox (which is basically the same thing).

I'm on Arch Linux x64.
In my experiments also closing a tab works instead of opening one, so I guess you have to do something to "refresh" sessionstore.

 Note: opening/closing a tab must be done after all excess windows are closed.

This is on Win7 SP1 x64
I have no pinned tabs this problem occurs if you have just the default start page open close the browser then start a new on and click on restore previous session.
I think we should just wait until the issues/questions from comment 23 are addressed and see what happens rather than add more noise to the bug.
https://hg.mozilla.org/mozilla-central/rev/44cb72be622d
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #32)
> https://hg.mozilla.org/mozilla-central/rev/44cb72be622d

Well, at this point I think it would be prudent to backout the change from bug 1073992 after Mondays uplift, so that Firefox 35 would not be affected, and change the Target Milestone here to be Firefox36.
And why is this 
now marked as fixed???
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
OH I guess because the patch that did not fix this was landed.
Status: REOPENED → ASSIGNED
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #32)
> https://hg.mozilla.org/mozilla-central/rev/44cb72be622d
Sorry, guess you were just doing a merge and not following the bug so doing the normal check-in update thing.
(In reply to tabmix.onemen from comment #23)
> 1. in onClose you should call RevivableWindows.add(winData) just before you
> set this._closedWindows.unshift(winData). otherwise you can add winData with
> hasSaveableTabs false to RevivableWindows list.

Yes, wow that's a stupid mistake.

> 2. SessionSaver._saveState call to getCurrentState that push the last closed
> window back to state.windows
> and since you update RevivableWindows we end up duplicate windows when you
> push all RevivableWindows into state.windows in SessionSaver._saveState.

Yes, another oversight.

> 3. there are places in the code that can change _closedWindows without
> changing RevivableWindows. for example
> forgetClosedWindow, and undoCloseWindow.

So many edge cases.
Keywords: leave-open
Going to back this and bug 1073992 out.
Please also backout on mozilla-central.
(In reply to TheRave from comment #41)
> Please also backout on mozilla-central.

We will wait until these backouts merge to m-c. Hopefully everything's back to normal with tomorrow's Nightly.
Now that we've branched, do we need to backout on Aurora, or did the backout make it in time?
(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #45)
> Now that we've branched, do we need to backout on Aurora, or did the backout
> make it in time?

43633c475f58 is not present on aurora (eg https://hg.mozilla.org/releases/mozilla-aurora/rev/43633c475f58) so this will need backing out there too.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 35 → Firefox 36
(In reply to Ed Morley [:edmorley] from comment #46)
> 43633c475f58 is not present on aurora (eg
> https://hg.mozilla.org/releases/mozilla-aurora/rev/43633c475f58) so this
> will need backing out there too.

Thanks! This definitely shouldn't hit the Aurora userbase - Tim, could you do the honors?
Flags: needinfo?(ttaubert)
Approval Request Comment
[Feature/regressing bug #]: this bug
[User impact if declined]: broken sessionstore behavior on Linux/Windows opening duplicate windows
[Describe test coverage new/current, TBPL]:
[Risks and why]: Low risk, backout of a feature
[String/UUID change made/needed]: None.

The backouts didn't make the Aurora merge. We should back out the changesets of this and bug 1073992 to fix the sessionstore regression.
Attachment #8504652 - Flags: approval-mozilla-aurora?
Flags: needinfo?(ttaubert)
verified fixed in 36.0a1, 20141014030201 build.
Status: RESOLVED → VERIFIED
[Tracking Requested - why for this release]:
Attachment #8504652 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
verified fixed in 35.0a2 20141017004001.
You need to log in before you can comment on or make changes to this bug.