Closed Bug 497730 Opened 10 years ago Closed 10 years ago

Restore session from crash while loading multiple tabs opens multiple about:blanks

Categories

(Firefox :: Session Restore, defect, major)

3.5 Branch
defect
Not set
major

Tracking

()

RESOLVED FIXED
Firefox 3.7a1
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: ashughes, Assigned: zpao)

References

Details

(Keywords: dataloss, Whiteboard: [mocotestday])

Attachments

(2 files, 1 obsolete file)

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b99)
Gecko/20090605 Firefox/3.5b99

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b99) Gecko/20090605
Firefox/3.5b99

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b99) Gecko/20090605
Firefox/3.5b99

STR:
1. Start with a single tab open to the home page
2. Install Ted's Crashme extension (http://crashme.googlecode.com/files/crashme-advanced.xpi)
3. Restart Firefox
4. CTLR/Command + Click on the Latest Headlines bookmark
5. Opt to open in multiple tabs
6. Make sure a couple of the tabs have the page title while others still say "Loading"
7. Crash Firefox
8. Restart Firefox and restore the session if prompted to do so

RESULT:
Tabs which already had the page title are loaded.  All other tabs show up as about:blank.

EXPECTED:
about:blank tabs should remember the URL they were attempting to load and try to reload them.
Whiteboard: [mocotestday]
Attached image Screenshot
Screenshot showing some "BBC" and some "Untitled" tabs
I came here to report a "when my internet connection gets flaky, my pages are all about:blank and I don't get the URL until after they actually fail". I'm pretty sure that that problem and this are caused by the same underlying mechanic.

Seems like the fix is to prioritize assigning the URL to the view somehow, even if a connection hasn't yet been established?
Okay just confirmed in Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20090828 Minefield/3.7a1pre

I didn't get about:blank pages, but I got a bunch of "Untitled". I'm going to 'have at'.
Same with Seamonkey:

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.4pre) Gecko/20090829 NOT Firefox/2.0.0.20 SeaMonkey/2.0b2pre ID:20090829122807
Keywords: dataloss
I hit this *all* the time. I lose entire sessions this way. :(
Severity: normal → major
Flags: wanted-firefox3.6?
Flags: blocking-firefox3.6?
Paul, any ideas? Dolske mentioned that you thought there was an exception we weren't catching here ...
Flags: wanted-firefox3.6?
Flags: blocking-firefox3.6?
Flags: blocking-firefox3.6+
With any luck this is a dupe of bug 509315 and these STR will consistently work.
> With any luck this is a dupe of bug 509315
or rather the Firefox version: bug 514751
After further investigation, this isn't really a bug. It sorta is, but it's more that the last saved state is half opened. The timing works out that the tabs have been opened, but they didn't have any history when sessionstore tried to get it. I want to say it's invalid, but there might be something we can do.

I would need to check how the tabs are actually being opened here, but with session restore, we open a bunch of tabs and then fill in history. My guess is that the same sort of thing is happening here & with the browser slowing down from all that new activity, we're much more able to catch the session in this incomplete state.

Reed's problem is bug 514751. Travis's isn't this bug either. My guess is that Misak's is bug 509315.
Is there a way you can proactively save the history when creating or loading the tab instead of waiting for the session timer?
I think the important thing is to ensure that:

 - if a user opens multiple tabs, and crashes, we're restoring with all those tabs (even the ones that hadn't loaded)

 - if a user crashes while restoring, the subsequent restore doesn't load a bunch of about:blanks for tabs that hadn't completed loading last time
Assignee: nobody → paul
(In reply to comment #10)
> Is there a way you can proactively save the history when creating or loading
> the tab instead of waiting for the session timer?

It's possible, but we really don't want to write to disk 37 times in the span of 10 seconds. Since we currently write out the whole file every time, this would be painful. That might become a more viable option with bug 508740.

(In reply to comment #11)
> I think the important thing is to ensure that:
> 
>  - if a user opens multiple tabs, and crashes, we're restoring with all those
> tabs (even the ones that hadn't loaded)

If the user changes their interval pref, they could potentially lose much more than 10 seconds of new tabs. It's unfortunate that the crash here happens when it does and we should do our best, but that's not necessarily going to mean we're perfect - not if we want to respect that pref.

If we knew the user was going to crash we'd do an emergency save, but we obviously can't know that.

>  - if a user crashes while restoring, the subsequent restore doesn't load a
> bunch of about:blanks for tabs that hadn't completed loading last time

Hopefully this isn't happening now. If it is, that's bad. I haven't seen any bugs on it, but I'll do some testing.

Now even with my negativity, hopefully there's something that can be done within some bounds (maybe we lose the last couple tabs, but the rest all have the right info). I have some things to try.
Ok, so it looks like this is most a problem when we it's been more than the pref delay between saves. After a tab is opened we call saveStateDelayed (with a min delay of 0). Since it's been longer than the pref delay, we kick off the save right away. So after the first tab is opened we kick off a save right away and gather info on the window. Not all of the tabs have actually started the navigating action yet, so have no actual history. So we end up storing an empty history & then restoring that.

So there are 2 options:
1. saveStateDelayed(aWindow, 2000) - Put a minimum delay in there so that we'll give other tabs a chance to do their thing. If we crash in those 2 seconds then we are worse off (no new tabs are added to the last saved session).
2. Use browser.userTypedValue - I think this is the better approach. We're still subject to race conditions, but we're better off. This worked pretty consistently in my testing today.
Attached patch Patch v0.1 (obsolete) — Splinter Review
Patch using option 2 from previous comment.
Attachment #399838 - Flags: review?(zeniko)
Attachment #399838 - Flags: review?(zeniko) → review+
Comment on attachment 399838 [details] [diff] [review]
Patch v0.1

>+      // This can happen if we open a lot of tabs simultaneously and try save
>+      // state before all of them are properly loaded. If we crash then we get
>+      // a bunch of about:blank tabs which isn't what we want.

Nit: These tabs weren't opened by "us" but by the user (i.e. they're not tabs being restored through SessionStore but newly created ones).

r+=me with the comment fixed. Thanks for looking into this, Paul.
With nit addressed.
Attachment #399838 - Attachment is obsolete: true
Attachment #399860 - Flags: review+
Pushed http://hg.mozilla.org/mozilla-central/rev/b11f8b61ce71. Will land on branch tonight/tomorrow.

Presumably we'll need a litmus test for this. The STR should be good enough. It might be worth noting that the browser should sit idle for ~10 seconds first before trying to crash to ensure the timing works such that we hit this case.
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-litmus?
Resolution: --- → FIXED
Whiteboard: [mocotestday] → [mocotestday] [needs 1.9.2 landing]
Target Milestone: --- → Firefox 3.7a1
Pushed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/2ff4cb479837
Whiteboard: [mocotestday] [needs 1.9.2 landing] → [mocotestday]
Blocks: 517139
Depends on: 522545
I should note that bug 522545 was a "regression" from this bug and so the expected behavior here is going to be a little different.

For cases where the browser has already gotten to the point of processing the URL (but not actually loaded it), we will save that state on quit. On startup that URL will be loaded again.

For the cases where the user has typed something (or like this case details, it's saved as a user typed value) but hasn't made it's way into processing it, we will restore such that the url is loaded into the address bar, but won't actually load the page.
Build identifier: Mozilla/5.0 (X11; Linux i686; rv:2.0b9) Gecko/20100101 Firefox/4.0b9

Issue present in above build.

Also the restored "blank" pages do not load with corresponding data until the user selects that respective tab.
(In reply to comment #21)
> Build identifier: Mozilla/5.0 (X11; Linux i686; rv:2.0b9) Gecko/20100101
> Firefox/4.0b9
> 
> Issue present in above build.

The issue as reported or something different? Follow the STR in comment 0.

If you follow the litmus tests linked above, do they fail?

> Also the restored "blank" pages do not load with corresponding data until the
> user selects that respective tab.

Perhaps related to your value of the browser.sessionstore.max_concurrent_tabs pref.
The litmus tests posted above Pass.

"about:blank" pages appear in restore page. If "Restore session" is pressed, all the tabs open. The ones that were still loading before the crash(that are named about:blank in restore page) appear as "New tab" and they load with data once it's their turn or when user selects a specific tab.

Question is, should the tabs appear in restore page with about:blank names and then with New Tab once the session is restored or should they have some sort of name that would indicate to the user what exactly is loading in those tabs?
(In reply to comment #23)
> Question is, should the tabs appear in restore page with about:blank names and
> then with New Tab once the session is restored or should they have some sort of
> name that would indicate to the user what exactly is loading in those tabs?

Probably the latter. Please file a new bug. This is somewhat related to cascaded session restore in the previously all tabs immediately showed "loading" in the tab title where now they show the saved page title until they start loading.
Logged Bug 625398 - Restoring multiple tabs causes abnormal behavior in their naming
Duplicate of this bug: 515134
You need to log in before you can comment on or make changes to this bug.