625016 - Users have app tab and panorama data loss depending on window close order

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[deos]

User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b8) Gecko/20100101 Firefox/4.0b8
Build Identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b8) Gecko/20100101 Firefox/4.0b8

When quitting the process after all windows got closed, all app tabs are gone when re-opening the browser

Reproducible: Always

Steps to Reproduce:
1. open browser
2. create some app tabs
3. close all windows
4. quit process (right click in dock: quit)
5. re-open browser
Actual Results:  
app tabs are gone

Expected Results:  
app tabs would persist

bug #587400 may be related

Comment 1

[George Carstoiu]

Build identifier: Mozilla/5.0 (Windows NT 6.1; rv:2.0b10pre) Gecko/20110116 Firefox/4.0b10pre

I was unable to reproduce this on Windows machine.

Comment 2

[deos]

thats why it says [OSX]
on mac, the firefox process continues to live even after the last window was closed. thats not the case on windows, so this cant happen on windows
this is a unix-only-bug

btw, i consider this as blocking! 
app tabs must never be forgotten in any case

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The issue here is that if all windows have been closed before shutdown, the "Recently closed Windows" submenu is disabled after a restart. So you cannot restore the window which has been closed before shutdown. The app tabs are getting restored successfully. This bug seems to be older and also exists in 3.6. I cannot find a bug right away.

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

Resummarizing, not blocking, we should try to find the dupe. The right solution here is to have the undo close window action saved and restored.

Comment 5

[deos]

you are right, its the session itself that cant be resumed, and as app tabs are stored in there, they get lost with it
i can also confirm this happens in FF 3.6 too

seems i found something bigger then i thought
as it causes dataloss, i think this should be fixed as soon as possible, though it may not be blocking

Comment 6

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

So app tabs are restored? Original description says no, comment 3 says yes. I could be mistaken, but I thought we specialized app tabs so they would be restored here.

As far as not resuming the session, that is intentional to match platform behavior. I'm not sure we want to repopulate recently closed windows (and if we do, it's definitely not happening for firefox 4 - we've been like that for a long time)

Comment 7

[deos]

as of beta 9 (Gecko/20100101 Firefox/4.0b9), the app tabs are NOT restored when doing the steps above
at least not for me

Comment 8

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

(In reply to comment #7)
> as of beta 9 (Gecko/20100101 Firefox/4.0b9), the app tabs are NOT restored when
> doing the steps above
> at least not for me

Indeed. I was wrong.

Comment 11

[Alex Faaborg [:faaborg] (Firefox UX)]

I'm expanding the summary of this bug to cover the larger issue which is this:

1) Create window A that contains many app tabs, and hours of panorama organization
2) Create second window B that contains a single tab
3) Shut down firefox by closing your windows (and by accident happen to close A, then B).
4) Notice that you have lost all of your work to organize everything, and session restore is no longer an option.

We need to find some quick and potentially messy way of addressing this problem soon.  It was out of scope for Firefox 4, but we really can't wait to long as users are experience pretty significant dataloss.

I'm fine with pretty much any quick solution (and we can go back and make it cleaner later).  For instance, restoring window B even though the user closed it right before closing the application (when they do a session restore).  Or having some threshold for if a window should be brought back anyway regardless of close order, like it containing app tabs, or panorama groups.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Hm, so why this isn't a dupe of bug 587400?

Comment 13

[deos]

it had not much to do with bug 587400 before Alex Faaborg extended the bug but i guess they depend on each other either way

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

I implemented a retention time for closed windows. So if the window is closed it's not stored as a closed window in sessionstore.js until the retention time (currently 60 seconds) is over. That's a pretty simple approach to prevent data loss when closing window after window to quit the browser.

We'd still need to talk to UX if that's an accepted solution. And Paul can tell if the patch is the right approach :)

Comment 15

[Frank Yan (:fryn)]

Comment on attachment 529314 [details] [diff] [review]
patch v1

Introducing an arbitrary retention time seems wrong to me. Why should users expect a different behavior when they close a window and leave it closed for 60 seconds?

I'll defer to the UX team for more precise feedback.

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 529314 [details] [diff] [review]
patch v1

Passed try:

http://tbpl.mozilla.org/?tree=Try&pusher=[e-mail]&rev=ad220387f71b

Builds are here:

https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-ad220387f71b/

Comment 17

[Alex Faaborg [:faaborg] (Firefox UX)]

we need to not delete groups in panorama in Firefox 6, whatever it takes, regardless of how messed up and arbitrary. (but fwiw, if the user closes a series of windows, they are shutting the application down, which they will likely view that as a singular event, instead of a series of closely timed events in a carefully planned order.)

Comment 18

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

Comment on attachment 529314 [details] [diff] [review]
patch v1

First (before I go bashing things) thanks for taking a stab at this. Much appreciated.

The short: r- until ui-review+ (poke limi, faaborg is out for a little bit).

The long:
1. Don't lookup the pref everytime
2. We might actually want to do this in _getCurrentState (not 100% sure about that, but we might want to show the same state to API consumers...)
3. This should be non-OSX only

And the main thrust...
3. A pure retention time is not perfect. Nothing will be but this that seems a bit cludgy. If you close a window, then do something in a 2nd window (close some tabs, open the new email you just saw arrive), then close that window to quit, what should your session look like? Or if you close a window, then quit (file > exit), the window close probably shouldn't count (in other words, we should probably only care about this through the lastwindow-close-granted path).

I'd really like UX input here. I know faaborg wants _something_, but as is it just feels like the approach as implemented is too cludgy.

Comment 19

[ithinc]

We may move the app tabs to the remaining window if there is any when closing a window. Thus nothing special is needed in the session store service.

Comment 20

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

(In reply to comment #19)
> We may move the app tabs to the remaining window if there is any when closing a
> window. Thus nothing special is needed in the session store service.

I don't think that's happening. That idea had been discussed but I never heard any real favor for that. I was under the impression that we would make app tabs global before doing that.

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #20)
> I don't think that's happening. That idea had been discussed but I never heard
> any real favor for that. I was under the impression that we would make app tabs
> global before doing that.

Yeah, so app tabs are going to be global and Panorama is going to show tab groups from all windows. That would solve all of our problems but seems we need an intermediate solution now as the real solutions probably won't make it to Fx 6.

Comment 22

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

(In reply to comment #21)
> (In reply to comment #20)
> > I don't think that's happening. That idea had been discussed but I never heard
> > any real favor for that. I was under the impression that we would make app tabs
> > global before doing that.
> 
> Yeah, so app tabs are going to be global and Panorama is going to show tab
> groups from all windows. That would solve all of our problems but seems we
> need an intermediate solution now as the real solutions probably won't make
> it to Fx 6.

I still don't think moving app tabs around is the right solution, and it wouldn't solve the Panorama loss. I think your patch is the right route to go, it just needs to be more fine grained.

Comment 23

[Alex Limi (:limi) — Firefox UX Team]

The intermediate solution that we discussed with Paul earlier (just to have it documented) is to restore all windows if the close operation was the only thing that happened before the last window was closed.

If you close a window, open a new tab, navigate, or do something else in the next window, the previously closed window is discarded, and not restored.

And yes, the proper solution is global app tabs and global Panorama, but we need to do something in the meantime. :)

Comment 24

[Frank Yan (:fryn)]

(In reply to comment #23)
> The intermediate solution that we discussed with Paul earlier (just to have
> it documented) is to restore all windows if the close operation was the only
> thing that happened before the last window was closed.

I find this solution much more palatable. It's action-based instead of arbitrarily time-based.

Comment 25

[Alex Faaborg [:faaborg] (Firefox UX)]

yeah, action based is fine as well, important thing is that we get it landed and users don't lose all their data (I honestly lose my panorama state often enough that I kind of kick myself for spending time organizing stuff in there).

Comment 26

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

Attachment: Patch v2.0 (WIP)

Went with the other approach. Will need a bit of tweaking and we're going to be limited by max_closed_windows with this approach (we were with Tim's too), but I think that's probably OK.

Dietrich, if you get a chance, can you take a look to make review faster come next week?

Comment 27

[Dietrich Ayala (:dietrich)]

Comment on attachment 532405 [details] [diff] [review]
Patch v2.0 (WIP)

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

::: browser/components/sessionstore/src/nsSessionStore.js
@@ +434,5 @@
> +    let clearRestoringWindows = true;
> +    if (["domwindowclosed", "quit-application-requested", "quit-application-granted",
> +         "quit-application", "browser-lastwindow-close-granted", "nsPref:changed",
> +         "timer-callback"].indexOf(aTopic) != -1)
> +      clearRestoringWindows = false;

patch generally looks fine. i don't like how this approach means that it's not explicit in the code which notifications will actually clear restoring windows. please either document it here, or for the triggering notification. f=me otherwise.

Comment 28

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

(In reply to comment #27)
> Comment on attachment 532405 [details] [diff] [review] [review]
> Patch v2.0 (WIP)
> 
> Review of attachment 532405 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/sessionstore/src/nsSessionStore.js
> @@ +434,5 @@
> > +    let clearRestoringWindows = true;
> > +    if (["domwindowclosed", "quit-application-requested", "quit-application-granted",
> > +         "quit-application", "browser-lastwindow-close-granted", "nsPref:changed",
> > +         "timer-callback"].indexOf(aTopic) != -1)
> > +      clearRestoringWindows = false;
> 
> patch generally looks fine. i don't like how this approach means that it's
> not explicit in the code which notifications will actually clear restoring
> windows. please either document it here, or for the triggering notification.
> f=me otherwise.

That was the quick and dirty "put everything together so it's easy to change" approach. I'll add comments and set clearRestoringWindows explicitly in the switch cases.

Comment 29

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

Attachment: Patch v2.1

Cleaned up a little bit and actually tested on Linux.

Comment 30

[Dietrich Ayala (:dietrich)]

Can you add a test? Should also do a try run.

Comment 31

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

Attachment: Patch v2.2

Now with a test. It's not comprehensive because there are a number of actions that will cause a reset, but it shows that it works.

Comment in the file should explain it:
We'll test this by opening a new window, waiting for the save event, then closing that window. We'll observe the "sessionstore-state-write" notification and check that the state contains no _closedWindows. We'll then add a new tab and make sure that the state following that was reset and the closed window is now in _closedWindows.

Try was green: http://tbpl.mozilla.org/?tree=Try&rev=1cb8e8f01152

Comment 32

[Dietrich Ayala (:dietrich)]

Comment on attachment 533701 [details] [diff] [review]
Patch v2.2

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

r=me, thanks!

Comment 33

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

Landed & backed out of mozilla-inbound. I pushed the wrong version of the patch. Will triple check & push to try again to confirm.

Comment 34

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/be404c4dd873

Comment 35

[Vlad [QA]]

How can this be resolved fixed? I can still reproduce this issue.

Comment 36

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Reproduce with what? You are lacking information which is necessary to investigate your issue. Keep in mind that this will be fixed in todays nightly build which hasn't been released yet.

Comment 37

[Wyatt Childers]

This can still be reproduced. Steps:
1.) Open firefox
2.) Open a new window in the nightly menu
3.) Close all the windows and if the process is killed with the window opened by firefox data loss will occur

I believe Bug 587400 certainly needs a fix before this is completely patched.

Firefox: 7.0a1 (2011-06-25)
OS:Windows Vista (That's why there is no doc step.)

Comment 38

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

(In reply to comment #37)
> This can still be reproduced. Steps:
> 1.) Open firefox
> 2.) Open a new window in the nightly menu
> 3.) Close all the windows and if the process is killed with the window
> opened by firefox data loss will occur
> 
> I believe Bug 587400 certainly needs a fix before this is completely patched.
> 
> Firefox: 7.0a1 (2011-06-25)
> OS:Windows Vista (That's why there is no doc step.)

This doesn't fix all cases (I meant to adjust the bug title to reflect what the patch actually does). The case you're specifically talking about - crashing - is never going to be perfect. What was "fixed" here is the quitting case where you close multiple windows in a row to quit.

What we really need is bug 587873, which I'm working on.

Comment 39

[Vlad [QA]]

I can reproduce this on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0a1) Gecko/20110626 Firefox/7.0a1

I've followed the steps from the description and the issue is still reproducible for me .

Comment 40

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

(In reply to comment #39)
> I can reproduce this on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6;
> rv:7.0a1) Gecko/20110626 Firefox/7.0a1
> 
> I've followed the steps from the description and the issue is still
> reproducible for me .

This bug was transformed beyond the OSX case into something a bit different. Yes the original steps are still reproducible, but that's under the umbrella of a different bug which still needs to be fixed.

Comment 41

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Paul, do you have this bug number off-hand? It would be important.

In case of the fix for this bug, what has been fixed exactly? Does the summary need an update? Which steps we would have to use to verify the fix? Thanks.

Comment 42

[George Carstoiu]

Mozilla/5.0 (X11; Linux i686; rv:6.0) Gecko/20100101 Firefox/6.0

Paul, can you please answer Henrik's questions? The answers would be very helpful.

Thanks!

Comment 43

[George Carstoiu]

Mozilla/5.0 (X11; Linux i686; rv:6.0) Gecko/20100101 Firefox/6.0 - beta 5

Verified this issue using the following steps on a clean profile:
1. Opened Window A containing several groups and a few AppTabs
2. Opened Window B containing a few opened tabs
3. Close Window A
4. Close Window B
5. Open Firefox

What happens - Window B is restored. By going to the History menu the user is capable of restoring Window A also. The restored window contains all the AppTabs and all the groups that were previously set.

Is this what was expected?

Comment 44

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

(In reply to George Carstoiu from comment #43)
> Mozilla/5.0 (X11; Linux i686; rv:6.0) Gecko/20100101 Firefox/6.0 - beta 5
> 
> Verified this issue using the following steps on a clean profile:
> 1. Opened Window A containing several groups and a few AppTabs
> 2. Opened Window B containing a few opened tabs
> 3. Close Window A
> 4. Close Window B
> 5. Open Firefox
> 
> What happens - Window B is restored. By going to the History menu the user
> is capable of restoring Window A also. The restored window contains all the
> AppTabs and all the groups that were previously set.
> 
> Is this what was expected?

That's what is expected on Firefox 6. The patch here landed for Firefox 7, where it's expected that Window A and Window B are both opened following the restore.

Comment 45

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

(In reply to Henrik Skupin (:whimboo) from comment #41)
> Paul, do you have this bug number off-hand? It would be important.

It could sort of fall under bug 587873, but more the OSX case has always been a bit special - when you close all of your windows it's treated mostly as a blank slate (following OS conventions). Making app tabs global will fix that part of it, but we'll likely still wipe out other tabs & panorama data.

Comment 46

[Vlad [QA]]

I have followed the steps from comment43 and Firefox behaves as expected. See comment44.
Both windows are restored after Firefox starts.

Mozilla/5.0 (X11; Linux x86_64; rv:7.0) Gecko/20100101 Firefox/7.0 beta 2
Mozilla/5.0 (Windows NT 5.1; rv:7.0) Gecko/20100101 Firefox/7.0 beta 2
Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0 beta 2
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0) Gecko/20100101 Firefox/7.0 beta 2

Setting resolution to Verified Fixed.