367605 - When quitting Firefox before all pages have loaded, session saving forgets pages that weren't loaded

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Joni Lehto]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.1) Gecko/20061204 Firefox/2.0.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.1) Gecko/20061204 Firefox/2.0.0.1

When starting Firefox with setting "When Firefox starts: Show my windows and tabs from last time" and then closing before all pages have been completely loaded or possibly some haven't even started to render, some of the unloaded pages are lost. Having a slow connection and lots of pages makes this happen very often.

Reproducible: Always

Steps to Reproduce:
1. Use browser setting When Firefox starts: Show my windows and tabs from last
time
2. Browse and open tabs you'd like to keep open, let's say 30 tabs for example
3. Close browser
4. Start browser
5. Close browser before all pages have been completely loaded or especially before some pages haven't even started to render
6. Start browser again
Actual Results:  
Some of the 30 tabs that weren't loaded before the browser was closed are now opened as about:blank.

Expected Results:  
All 30 tabs would have been opened properly, so that the address for not a single tab would be lost and browsing could be continued just as expected.

This bug has been in Firefox since 2.0 betas, so it's not caused by any newly-made code.

The Camino implementation of session manager suffers from the same problem. See  https://bugzilla.mozilla.org/show_bug.cgi?id=361092.

Comment 1

[Ria Klaassen (not reading all bugmail)]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.1) Gecko/20061204 Firefox/2.0.0.1
I could not reproduce this with my very fast cable connection so maybe this problem only occurs with slower connections. My computer is not very fast. That's why it took a few seconds before it reacted to my wish to close.

Comment 2

[Joni Lehto]

This bug only occurs when browser is shut down after startup before all pages have loaded. So more pages, slower servers and network connection and a faster computer make this a lot more likely to happen. If all pages would be loaded from a near-zero delay location, like from LAN, this problem wouldn't likely occur at all except when there is a problem with the connection.

But having lots of remote sites open from around the world with varying connections you don't often need to wait for one of the sites to have a network problem (being Slashdotted for example). Occasionally after starting Firefox you notice that it wasn't what you wanted to do or you have a hurry to go somewhere else physically or for any other reason you want to close the browser right away. Currently in such a situation you can only wait until all pages have started to load or close immediately and risk data loss.

Comment 3

[Simon Bünzli]

Attachment: prefer to-be-restored data over actually restored data for incompletely loaded tabs

This patch prevents incompletely loaded tabs not to be restored a second time by caching the data to be restored and saving that if the tab hasn't been fully loaded since. See bug 356050 for the same issue with complete windows.

BTW: This patch also includes a/the fix for bug 367389.

Comment 4

[Joni Lehto]

Excellent.

I'm eagerly waiting for a Windows version of Firefox in which I could test that bugfix. It could, though unlikely, fix the problem that when a page open in session saving is temporarily redirecting to a new address, the original one is forgotten and cannot be accessed via back button. My free studet ISP does that: it periodically stops serving pages so that every page is redirected to a form where user must type his/her account and password again to continue using. This is to prevent non-registered users to continuing to use the internet for long, but it causes me to lose the information in all but cached pages in open in session manager.

A fix would be to include the redirecting page in the history for back button. Back button currently doesn't remember redirecting pages at all, which is good for normal web browsing when a page has permanently moved (you can still follow back button to the original page you were browsing and not just to the redirecting page leading you nowhere), but disastrous for any kind of temporary redirects (you have no way of getting to the page you wanted even when it starts working).

A possible fix would be to remember the redirecting page in history, but make back button jump over it to the previous item (or next button to the next item after it), so that it would still be found in the dropdown menu displaying 10 next or previous pages (the small downwards arrow next to back or forward arrows). But before making a specific report of this bug I would like to clarify does this new patch happen to address it.

Is there a schedule when this last patch could be tested on Windows? Like in a certain nightly or in some other way.

Comment 5

[Dietrich Ayala (:dietrich)]

(In reply to comment #3)

>+      else if (browser.parentNode.__SS_data && browser.parentNode.__SS_data._tab) {
>+        // use the data to be restored when the tab hasn't been completely
>+        tabs.push(browser.parentNode.__SS_data);
>+        continue;
>+      }

hi simon, am i understanding this correctly: __SS_data is cleared when a tab loads, right? and this patch uses it as a heuristic for whether or not a tab has completed restoration, when saving data at shutdown (or window close, etc)?

if that's correct, i'd prefer a more explicit method of detecting whether a tab has completed restoration. even if it's just wrapping that check in a isTabRestored() method or something similar. doing this will make the code more self-documenting, easier to follow.

nit: please finish the "hasn't been completely" comment... :)

Comment 6

[Simon Bünzli]

(In reply to comment #5)
> __SS_data is cleared when a tab loads, right?

It's cleared whenever a page loads in a tab (cf. sss_onTabLoad). We already use it for caching a tab's history as you can see a few lines below the first code change.

> if that's correct, i'd prefer a more explicit method of detecting whether a
> tab has completed restoration.

I'd prefer to postpone such a change for when the __SS_data and __SS_text dependencies on the browser panels are refactored into SessionStore (cf. bug 343911). Currently we already use similar code in other locations and don't need the same heuristics anywhere else (officially a tab has completed loading when SSTabRestored is dispatched), IMO a comment explaining what's happening is thus sufficient.

> nit: please finish the "hasn't been completely" comment... :)

Yeah, there's a "loaded" missing. Should the patch be OK, please add it when checking it in - otherwise I'll fix it for the next patch.

Comment 8

[Dietrich Ayala (:dietrich)]

fixed on trunk

Checking in nsSessionStore.js;
/cvsroot/mozilla/browser/components/sessionstore/src/nsSessionStore.js,v  <--  nsSessionStore.js
new revision: 1.62; previous revision: 1.61
done

Comment 9

[Jeff Walden [:Waldo]]

There should be a Litmus test for this.

Comment 10

[Adam Guthrie]

http://litmus.mozilla.org/show_test.cgi?id=4423

Comment 11

[Majken Connor [:Kensie]]

Comment on attachment 253803 [details] [diff] [review]
prefer to-be-restored data over actually restored data for incompletely loaded tabs

So I wonder why more people haven't asked for this on branch yet, this patch was landed in Feb and no one's asked about getting in on branch before now, but, I hit this a few times a month, connection problems, kid problems. Can we try it for 2.0.0.7?

Comment 12

[Aaron Digulla]

I have a similar problem which might be related. I'd like to verify this before I open a new bug report.

When Firefox restores from a session and it takes a long time (> 15 minutes) to load a page (sometimes, many tabs just "hang" in the "loading" state, the circle turns but nothing ever happens), the URL is blank. If it wasn't blank, I could interrupt Firefox and tell it to reload the page manually. Since there is no URL in this case, I can't do this.

Does the patch above include a fix for my problem, too?

Comment 13

[Simon Bünzli]

(In reply to comment #12)
> Does the patch above include a fix for my problem, too?

No. Please file a new bug (if possible after having verified that the latest Firefox 3 Alpha indeed still contains the issue).

Comment 14

[Aaron Digulla]

(In reply to comment #13)

> No. Please file a new bug (if possible after having verified that the latest
> Firefox 3 Alpha indeed still contains the issue).

It's the completely fresh Bug 392373. I'll see if I can try with FF3.

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 253803 [details] [diff] [review]
prefer to-be-restored data over actually restored data for incompletely loaded tabs

approved for 1.8.1.7, a=dveditz for release-drivers

Comment 16

[:Gavin Sharp [email: [e-mail]]]

mozilla/browser/components/sessionstore/src/nsSessionStore.js 	1.5.2.47 

Comment 17

[Carsten Book [:Tomcat]]

verified fixed 1.8.1.8 using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.1.8) Gecko/2007100816 Firefox/2.0.0.8 and with the steps to reproduce

- adding verified keyword

Comment 18

[Jesus Cea]

This bug induces bug 401113.

Comment 19

[Jesus Cea]

This patch induces bug 401113.

Comment 20

[Jesus Cea]

This patch induces bug 401097.

Comment 21

[Michael Kraft [:morac]]

I'm nearly positive that this patch induces bug 402349.

Comment 22

[Majken Connor [:Kensie]]

I think this is broken again, getting some reports in #firefox

Comment 23

[Majken Connor [:Kensie]]

didn't specify that I think it's broken in *branch*

Comment 24

[Simon Bünzli]

(In reply to comment #22)
> I think this is broken again, getting some reports in #firefox

Please have those users who still see this issue file new bugs with proper steps to reproduce.

Comment 27

[Michael Kohler [:mkohler]]

Verified with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090617 Minefield/3.6a1pre ID:20090617031528