467409 - (backslashplosion) Nested about:sessionrestore instances causes huge sessionstore.js file

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Martijn Wargers (dead)]

I had a very slow startup of Firefox and I noticed that it used >500MB internal memory.
It turned out this was happening because Firefox was trying to load the about:sessionrestore page.
When I looked into my profile, I noticed that the sessionstore.js file had a size of 50.6MB (!).
After I removed that file, I could start-up again normally.

So it seems to me that session restore should be able to handle 50MB sessionstore.js files just smoothly or else there should be a cap on the size of that file or something.

Comment 1

[Martijn Wargers (dead)]

Perhaps there is a relation to bug 464350 here.

Comment 3

[Simon Bünzli]

Martijn: Have you ever seen this issue again? If so, would you mind sharing that sessionstore.js for further inspection?

Comment 4

[Ben O'Brien]

could somebody tell me how to get the sessionstore.js

Comment 5

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

(In reply to comment #4)
> could somebody tell me how to get the sessionstore.js

sessionstore.js is in your profile folder
http://support.mozilla.com/en-US/kb/Profiles#How_to_find_your_profile

Comment 7

[Simon Bünzli]

This is no DUPE of bug 477564. The bug is caused by several instances of about:sessionrestore being contained in each other, leading to a huge sessionstore.js and thus slow startup (see also bug 511135).

Comment 8

[Wayne Mery (:wsmwk)]

I think current trunk restores large sessions a bit faster than builds from 2-3 months ago.  What do you think Martijn? Is it my imagination?

Paul, is there a bug to do the "second half" implied by bug 464350?

Comment 9

[Martijn Wargers (dead)]

Sorry, I currently have history disabled, so I haven't really tested it, lately.

Comment 10

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

(In reply to comment #8)
> Paul, is there a bug to do the "second half" implied by bug 464350?

No idea, but I saw you asked there. I still stand by my stance that a better workflow needs to be found for cases like yours. The nesting of about:sessionrestore instances is bad.

Comment 11

[Wayne Mery (:wsmwk)]

(In reply to comment #10)
> (In reply to comment #8)
> > Paul, is there a bug to do the "second half" implied by bug 464350?
> 
> No idea, but I saw you asked there. I still stand by my stance that a better
> workflow needs to be found for cases like yours. The nesting of
> about:sessionrestore instances is bad.

datapoint - the problem goes slightly beyond use case...I recently lost power twice in a short span of time, and each time restarted firefox but did not restore session before losing power again...resulting in large sessionstore.js

Comment 12

[Dietrich Ayala (:dietrich)]

This blocks. If possible we should cap the number of nested session restore pages.

Comment 13

[John J. Barton]

I posted my 84MB sessionstore.js on Bug 581510. 

In my case I have 1-3 tabs open, one typically is the restore page. I kill the browser every few minutes.

Comment 14

[Wayne Mery (:wsmwk)]

(In reply to comment #12)
> This blocks. If possible we should cap the number of nested session restore
> pages.

from the perspective of a slightly informed user, I'm not sure that anything other a fairly low value of nesting will suffice, if nesting is the only measure used. It seems to me some combination of #tabs, size of sessionstore.js and #nest-levels is required. And even then that might not suffice, because of how quickly the sessionstore size can grow. It only takes a couple quick crashes to go way over the edge, as I note in bug 581510 in my power failure example.

Comment 15

[Granville Moore]

I have also been bitten by this issue, which can cause a very noticable slowdown.

No-one has mentioned possible workarounds (other than deleting sessionstore.js), so here are a couple of suggestions:

1. To flush an existing sessionstore.js of unwanted closed sessions, use about:config to temporarily set browser.sessionstore.max_tabs_undo and browser.sessionstore.max_windows_undo to zero, and then back to their original values.  This causes any recently-closed windows/tabs to be removed from sessionstore.js whilst preserving windows/tabs that are currently open.

2. To reduce the frequency with which the problem arises, increase browser.sessionstore.max_resumed_crashes to a larger value, say 6 (default 1).  This means that sessionstore.js will be used to automatically restore windows/tabs after a crash without opening the "Restore Sessions" window, up to a maximum of 6 times in a 6-hour window, rather than after the 2nd crash.  Because this avoids opening the "Restore Sessions" window after crashes, sessionstore.js doesn't get "bulked up".  The downside of this is that if a condition arises that crashes Firefox on startup, you have to restart it 6 times before you get the "Restore Sessions" window, but that's a price I'm willing to pay to prevent this slowdown issue.

Hope this is useful,

GVM

Comment 16

[Robert Sayre]

This needs an assignee.

Comment 17

[Ben O'Brien]

Could somebody get me off the mailing list for this. I keep on doing it

Comment 18

[Ben O'Brien]

whoops... to continue, I keep on getting emails even though I no longer have anything to do with this bug.

Comment 19

[Johnathan Nightingale [:johnath]]

mak - want this one or, failing that, want to suggest an alternate? I know Paul's been poking session restore a lot lately, too.

Comment 20

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

I could probably take this. Session restore is currently my life.

Shaver and I talked about this few weeks ago, and the result of that was to just cap nested about:sessionrestore.

Not sure exactly how we want to do it. It might come down to just doing that on about:sessionrestore itself so the data doesn't get saved in the first place. Or do some special casing for about:sessionrestore if it's a current entry when quitting (will mean we need to do some extra processing to JSON.parse but that would only be an issue if it's a current entry). A marginally slower shutdown in a very particular case instead of a very slow startup in that particular case sounds like a net win to me.

I don't really want to make this preffable, but I know there are some very ingrained habits in a minority of users that a change like this could really bother. It would be very datalossy.

Comment 21

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

Attachment: Patch v0.1 (WIP)

So I'm pretty sure this is working, even if it is quite ugly right now.

Comment 22

[Dietrich Ayala (:dietrich)]

Comment on attachment 493734 [details] [diff] [review]
Patch v0.1 (WIP)

good enough to move forward, feedback+. a pref is unnecessary IMO.

Comment 23

[Dietrich Ayala (:dietrich)]

This is not a regression, not holding the release for it. I understand that the worse case scenario can be really bad, but it's not worse than 3.6, so shouldn't prevent us from shipping all the other new goodness in Fx4 to users. Would take this fix if it can get finished up in time.

Comment 24

[Tomas]

Any progress?

Comment 25

[Wayne Mery (:wsmwk)]

The patch implements comment 20?
It avoids huge memory usage and no dataloss if it has been shutdown normal?

Comment 26

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

(In reply to comment #24)
> Any progress?

Not recently. Work on Firefox 4 blockers is prioritized over this.

(In reply to comment #25)
> The patch implements comment 20?
> It avoids huge memory usage and no dataloss if it has been shutdown normal?

It does implement comment 20. It will strip out about:sessionrestore entries on normal shutdown, which is typically the cause of huge sessionrestore.js files (and in turn should decrease the chance of us hitting OOM errors when writing the file and use less memory to read it).

Comment 27

[Wayne Mery (:wsmwk)]

meaning the *current* session will not be saved?

Comment 28

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

(In reply to comment #27)
> meaning the *current* session will not be saved?

Your current session (open tabs, recently closed windows/tabs) will be saved. Your previous session (anything in about:sessionrestore) will not be saved unless it is restored prior to quitting.

The patch as is has a pref so that people like you can control the depth of about:sessionrestores allowed. Not sure if we're going to keep it as a pref or just say we're not going to save about:sessionrestore.

Comment 30

[Wayne Mery (:wsmwk)]

(In reply to comment #28)
> (In reply to comment #27)
> > meaning the *current* session will not be saved?
> 
> Your current session (open tabs, recently closed windows/tabs) will be saved.
> Your previous session (anything in about:sessionrestore) will not be saved
> unless it is restored prior to quitting.
> 
> The patch as is has a pref so that people like you can control the depth of
> about:sessionrestores allowed. Not sure if we're going to keep it as a pref or
> just say we're not going to save about:sessionrestore.

A problem with this approach, prior to restoring about:sessionrestore, is one might already be at the limit of what firefox can when it comes time to restore

Comment 31

[Granville Moore]

(In reply to comment #28)
> (In reply to comment #27)
> > meaning the *current* session will not be saved?
> 
> Your current session (open tabs, recently closed windows/tabs) will be saved.
> Your previous session (anything in about:sessionrestore) will not be saved
> unless it is restored prior to quitting.
> 
> The patch as is has a pref so that people like you can control the depth of
> about:sessionrestores allowed. Not sure if we're going to keep it as a pref or
> just say we're not going to save about:sessionrestore.

That's a significant change in behaviour - I'd be most "disappointed" if I were to lose an open about:sessionrestore without warning if I'd decided (for whatever reason) to close Firefox before fully restoring the session.  It's something I know that I've done in the past (e.g. deciding to reboot before reloading the hundreds (literally) of tabs in the restored session).

If this is adopted, I'd suggest warning the user about the potential loss before quitting Firefox, and offering to abort or preserve the session.

Comment 32

[Patrick Walton (:pcwalton)]

As a simple alternative, could we not store the previous JSON as a string? The biggest problem is all the '\' escaping necessary to store JSON inside JSON. This leads to exponential growth (in my file, over 90% of it is the character '\'). By storing the previous session store as a JSON object, we could change this exponential growth to linear.

Comment 33

[Patrick Walton (:pcwalton)]

(In reply to comment #32)
> The
> biggest problem is all the '\' escaping necessary to store JSON inside JSON.

That is, all the '\' escaping necessary to store JSON inside a JSON *string*.

Comment 34

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

(In reply to comment #32)
> As a simple alternative, could we not store the previous JSON as a string? The
> biggest problem is all the '\' escaping necessary to store JSON inside JSON.
> This leads to exponential growth (in my file, over 90% of it is the character
> '\'). By storing the previous session store as a JSON object, we could change
> this exponential growth to linear.

To clarify (since I confused myself reading that, too much JSON & string)... 

You're suggesting we special case that form field for about:sessionrestore and not save it as a string, but instead JSON.parse that value before we save it to sessionstore.js.

That's a good idea to at least tackle the largest issue of file size. We might be able to let the nested about:sessionrestore's keep nesting (for now). Nested sessions are still going to hurt though since we'll be stringifying larger JS objects on a regular basis as well as taking more memory to keep the JS object alive as opposed to a string (at least I assume that will be more memory)

Comment 35

[harnack]

I have used the patch for a month. It works fine. When will it be applied to the official release of Firefox? Now I have to modify nsSessionStore.js manually everytime after updating Firefox.

Comment 37

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

Do we have an updated TM when we want to land this? Seems like the work has been stalled in the last 5 month.

Comment 38

[Dietrich Ayala (:dietrich)]

Patrick's approach sounds sound. Paul, what's involved? It'd be great to get this fixed soon.

Comment 39

[Wayne Mery (:wsmwk)]

somewhere in the last few months (late 2010? / panorama?) in the runup to FF 4 the number of tabs where "out of memory" is seen in error console dropped by about 1/4 to 1/3, such that now I can encounter it at say 120 tabs and 6MB sessionstore.js.  Previously I didn't see this until in the 400-500 tab range.  (ref bug 581510)

Comment 40

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Reading the description of this bug, I think I have encountered this before, prob with about 20 or 30 or so tabs open.  Some of which were open to flash sites and it would happen when Firefox would update and try to restart.  This was with the current nightly for Mac.

Comment 41

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

Attachment: Patch v0.1b (WIP)

Took Patrick's approach. I think we can do a bit of testing here (open about:sessionrestore with state, then look at the saved state and make sure it's not a string for formdata["#sessionData"]).

The only thing here is that it only works going forward. We're not going to look back through somebody's current session and clean it up.

Comment 42

[Dietrich Ayala (:dietrich)]

Comment on attachment 558659 [details] [diff] [review]
Patch v0.1b (WIP)

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

totally ok with special-casing this.

Comment 43

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

Attachment: Patch v0.2b

Now with a test. An existing test caught a backwards compatibility issue, so I made sure we only stringify when restoring the form if it's an object. I updated the existing test to use the now proper way to do it and added a backwards compat test for data that's already been stringified (eg, old sessions)

Comment 44

[Emanuel Hoogeveen [:ehoogeveen]]

Will this do anything for bug 669034?

Comment 45

[Dietrich Ayala (:dietrich)]

Comment on attachment 558970 [details] [diff] [review]
Patch v0.2b

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

::: browser/components/sessionstore/src/nsSessionStore.js
@@ +3277,5 @@
>  
>          let value = aData[key];
> +
> +        // for about:sessionrestore we saved the field as JSON to avoid nested
> +        // instances causing hugmongous sessionstore.js files. cf. bug 467409

hug·mon·gous [hug-mung-gus] adjective - portmanteau of huggable and humungous. Ex: "I just saw Monsters Inc., Sully is hugmongous!"

::: browser/components/sessionstore/test/browser/Makefile.in
@@ +97,5 @@
>  	browser_465215.js \
>  	browser_465223.js \
>  	browser_466937.js \
>  	browser_466937_sample.html \
> +	browser_467409-backsplashplosion.js \

I see what you're doing here.

::: browser/components/sessionstore/test/browser/browser_467409-backsplashplosion.js
@@ +1,4 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +// Test Summary:

Every test should have a summary like this. Gold star.

@@ +12,5 @@
> +// 2b. Check that there are no backslashes in the current state
> +//
> +// 3.  [backwards compat] Use a stringified state as formdata when opening about:sessionrestore
> +// 3a. Make sure there are nodes in the tree on about:sessionrestore
> +// 3b. Check that formdate via getBrowserState doesn't require JSON.parse

pico-nit: formdata

Comment 46

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Emanuel Hoogeveen from comment #44)
> Will this do anything for bug 669034?

Not really, no.  That's a much bigger issue.

Comment 47

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

Updating the title to better reflect what's going on. We're not really addressing the problem of existing large sessionstore.js files, but rather preventing them from coming into existence for (specifically) nested about:sessionrestore instances.

Comment 48

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

Comment on attachment 558970 [details] [diff] [review]
Patch v0.2b

For the record, this test failed on try previously (that's why it didn't land weeks ago). I'm pretty that was due to having panorama data in the state from previous tests, so I pushed an updated version to try that that only looks at formdata when checking for escape chars, not the whole state.

Comment 49

[Mozilla RelEng Bot]

Try run for 11c602d4404b is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=11c602d4404b
Results (out of 4 total builds):
    success: 2
    warnings: 2
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-11c602d4404b

Comment 50

[Mozilla RelEng Bot]

Try run for 8d9959a645f5 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=8d9959a645f5
Results (out of 4 total builds):
    success: 1
    warnings: 3
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-8d9959a645f5

Comment 51

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

(after figuring out orange on try due to panorama) https://hg.mozilla.org/integration/fx-team/rev/6f4b69ed92f2

Comment 52

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/6f4b69ed92f2