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

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[u279076]

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.

Comment 1

[u279076]

Attachment: Screenshot

Screenshot showing some "BBC" and some "Untitled" tabs

Comment 2

[Travis Briggs]

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?

Comment 3

[Travis Briggs]

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'.

Comment 4

[Misak Khachatryan]

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

Comment 5

[Reed Loden [:reed]]

I hit this *all* the time. I lose entire sessions this way. :(

Comment 6

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

Paul, any ideas? Dolske mentioned that you thought there was an exception we weren't catching here ...

Comment 7

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

With any luck this is a dupe of bug 509315 and these STR will consistently work.

Comment 8

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

> With any luck this is a dupe of bug 509315
or rather the Firefox version: bug 514751

Comment 9

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

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.

Comment 10

[[not reading bugmail]]

Is there a way you can proactively save the history when creating or loading the tab instead of waiting for the session timer?

Comment 11

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

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

Comment 12

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

(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.

Comment 13

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

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.

Comment 14

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

Attachment: Patch v0.1

Patch using option 2 from previous comment.

Comment 15

[Simon Bünzli]

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.

Comment 16

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

Attachment: Patch v0.2 (for check-in)

With nit addressed.

Comment 17

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

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.

Comment 18

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

Pushed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/2ff4cb479837

Comment 19

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

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.

Comment 20

[u279076]

4 test cases added:
https://litmus.mozilla.org/show_test.cgi?id=11507
https://litmus.mozilla.org/show_test.cgi?id=11508
https://litmus.mozilla.org/show_test.cgi?id=11509
https://litmus.mozilla.org/show_test.cgi?id=11510

Marking in-litmus+

Comment 21

[George Carstoiu]

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.

Comment 22

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

(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.

Comment 23

[George Carstoiu]

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?

Comment 24

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

(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.

Comment 25

[George Carstoiu]

Logged Bug 625398 - Restoring multiple tabs causes abnormal behavior in their naming