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

VERIFIED FIXED in Firefox 7

Status

()

Firefox
Session Restore
--
major
VERIFIED FIXED
7 years ago
4 years ago

People

(Reporter: deos, Assigned: zpao)

Tracking

(Blocks: 2 bugs, {dataloss})

Trunk
Firefox 7
dataloss
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 -)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

7 years ago
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
(Reporter)

Updated

7 years ago
Blocks: 551849
(Reporter)

Updated

7 years ago
Severity: normal → major
Keywords: dataloss
Hardware: x86 → x86_64

Comment 1

7 years ago
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.

Updated

7 years ago
Component: Tabbed Browser → Session Restore
QA Contact: tabbed.browser → session.restore
(Reporter)

Comment 2

7 years ago
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
blocking2.0: --- → ?
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.
Hardware: x86_64 → All
Version: unspecified → Trunk
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.
Status: UNCONFIRMED → NEW
blocking2.0: ? → -
Ever confirmed: true
Summary: [OSX] Quit process after last window was closed removes all app tabs → [OSX] if last window is closed before quitting Firefox, session is not saved
Whiteboard: DUPEME
(Reporter)

Comment 5

7 years ago
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
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)
(Reporter)

Comment 7

7 years ago
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
(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.
Duplicate of this bug: 645478

Updated

6 years ago
Duplicate of this bug: 645605
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.
Summary: [OSX] if last window is closed before quitting Firefox, session is not saved → Users have app tab and panorama data loss depending on window close order

Updated

6 years ago
OS: Mac OS X → All
Whiteboard: DUPEME
Hm, so why this isn't a dupe of bug 587400?
Whiteboard: [dupeme?] 587400

Updated

6 years ago
Depends on: 587400
(Reporter)

Comment 13

6 years ago
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
Assignee: nobody → tim.taubert
Status: NEW → ASSIGNED
Created attachment 529314 [details] [diff] [review]
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 :)
Attachment #529314 - Flags: review?(paul)

Comment 15

6 years ago
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.
Attachment #529314 - Flags: ui-review?(faaborg)
Comment on attachment 529314 [details] [diff] [review]
patch v1

Passed try:

http://tbpl.mozilla.org/?tree=Try&pusher=tim.taubert@gmx.de&rev=ad220387f71b

Builds are here:

https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tim.taubert@gmx.de-ad220387f71b/
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 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.
Attachment #529314 - Flags: review?(paul) → review-

Comment 19

6 years ago
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.
(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.
(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.
(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.
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

6 years ago
(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.
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).
Created attachment 532405 [details] [diff] [review]
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?
Assignee: tim.taubert → paul
Attachment #529314 - Attachment is obsolete: true
Attachment #532405 - Flags: feedback?(dietrich)
Attachment #529314 - Flags: ui-review?(faaborg)
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.
Attachment #532405 - Flags: feedback?(dietrich) → feedback+
(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.
Created attachment 532772 [details] [diff] [review]
Patch v2.1

Cleaned up a little bit and actually tested on Linux.
Attachment #532405 - Attachment is obsolete: true
Attachment #532772 - Flags: review?(dietrich)
Can you add a test? Should also do a try run.
Created attachment 533701 [details] [diff] [review]
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
Attachment #532772 - Attachment is obsolete: true
Attachment #533701 - Flags: review?(dietrich)
Attachment #532772 - Flags: review?(dietrich)
Comment on attachment 533701 [details] [diff] [review]
Patch v2.2

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

r=me, thanks!
Attachment #533701 - Flags: review?(dietrich) → review+
Whiteboard: [dupeme?] 587400 → [incoming]

Updated

6 years ago
Blocks: 587400
No longer depends on: 587400
Landed & backed out of mozilla-inbound. I pushed the wrong version of the patch. Will triple check & push to try again to confirm.
Whiteboard: [incoming]
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/be404c4dd873
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → Firefox 7

Comment 35

6 years ago
How can this be resolved fixed? I can still reproduce this issue.
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

6 years ago
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.)
(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

6 years ago
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 .
(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.
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

6 years ago
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

6 years ago
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?
(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.
(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

6 years ago
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.
Status: RESOLVED → VERIFIED
Blocks: 729281
You need to log in before you can comment on or make changes to this bug.