Further limit the amount of sessionStorage data we store and serialize

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
a month ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

unspecified
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf:p1])

Attachments

(2 attachments)

We have a pref called browser.sessionstore.dom_storage_limit that currently defaults to 10M. For localStorage however the default is 5M, so that doesn't seem to make a lot of sense. And this limit doesn't apply to partial sessionStorage updates at all, which probably completely bypasses the limit.

We should set a small limit, let's say 2kB, and shouldn't store/serialize DOMSessionStorage data beyond that. This will help with Google search pages spamming the file.

I can't imagine there being a lot of value for huge sessionStorage entries being persisted to disk. If a game stores textures they'll simply fetch them again, and search pages can preload next time too.

This should severely reduce I/O and make sessionstore.js a lot smaller. Also we will spend less time serializing hundreds of kilobytes of data.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Can I just make this a P1? I think it should be. Let's see.
Whiteboard: [qf:p1]
We'll also switch to using the API I introduced in bug 1361974.
Basically, I expect:

* FX_SESSION_RESTORE_FILE_SIZE_BYTES
* FX_SESSION_RESTORE_READ_FILE_MS
* FX_SESSION_RESTORE_SERIALIZE_DATA_MS
* FX_SESSION_RESTORE_WRITE_FILE_MS

to decrease. It might unfortunately take a while until this applies to existing sessions. If a tab isn't loaded, the limit won't apply to its DOMSessionStorage data. As soon as a tab is loaded however, we'll restore any previously collected DOMSessionStorage data and then check against the limit before writing it back to disk.
Comment on attachment 8864542 [details] [diff] [review]
0001-Bug-1362058-Further-limit-the-amount-of-sessionStora.patch

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

Nice! I'm I was following along with the DWU changes you made (new storage usage retrieval API).

As a very-much aside: have you thought about using MozReview in the future, perhaps? I don't care either way, but it makes your future landings a bit easier, perhaps.

::: browser/components/sessionstore/SessionStorage.jsm
@@ +184,5 @@
>        storage = null;
>      }
>  
> +    if (!storage || !storage.length) {
> +      return {};

nit: or just `return hostData;`? (Same in the early return below).

@@ +192,5 @@
> +    let usage = window.QueryInterface(Ci.nsIInterfaceRequestor)
> +                      .getInterface(Ci.nsIDOMWindowUtils)
> +                      .getStorageUsage(storage);
> +    Services.telemetry.getHistogramById("FX_SESSION_RESTORE_DOM_STORAGE_SIZE_ESTIMATE_CHARS").add(usage);
> +    if (usage > Preferences.get(DOM_STORAGE_LIMIT_PREF)) {

Why import Preferences.jsm when you already have `Services.prefs` at your disposal? You're not taking advantage of its default value or observer features, so I don't necessarily see an advantage to using it here.
Attachment #8864542 - Flags: review?(mdeboer) → review+
(In reply to Mike de Boer [:mikedeboer] from comment #5)
> As a very-much aside: have you thought about using MozReview in the future,
> perhaps? I don't care either way, but it makes your future landings a bit
> easier, perhaps.

Yes, yes, I have :) I was always a little too lazy to set it up. But I should.

> ::: browser/components/sessionstore/SessionStorage.jsm
> @@ +184,5 @@
> >        storage = null;
> >      }
> >  
> > +    if (!storage || !storage.length) {
> > +      return {};
> 
> nit: or just `return hostData;`? (Same in the early return below).

Sure, will do.

> @@ +192,5 @@
> > +    let usage = window.QueryInterface(Ci.nsIInterfaceRequestor)
> > +                      .getInterface(Ci.nsIDOMWindowUtils)
> > +                      .getStorageUsage(storage);
> > +    Services.telemetry.getHistogramById("FX_SESSION_RESTORE_DOM_STORAGE_SIZE_ESTIMATE_CHARS").add(usage);
> > +    if (usage > Preferences.get(DOM_STORAGE_LIMIT_PREF)) {
> 
> Why import Preferences.jsm when you already have `Services.prefs` at your
> disposal? You're not taking advantage of its default value or observer
> features, so I don't necessarily see an advantage to using it here.

Fair point, will fix.

Comment 7

2 years ago
Pushed by ttaubert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/262e008b0bc4
Further limit the amount of sessionStorage data we store and serialize r=mikedeboer

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/262e008b0bc4
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Comment on attachment 8873393 [details] [diff] [review]
0002-Bug-1362058-Update-FX_SESSION_RESTORE_DOM_STORAGE_SI.patch

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

Nice. Remember to file a follow-up bug for the removal/expiry extension of FX_SESSION_RESTORE_DOM_STORAGE_SIZE_ESTIMATE_CHARS in 60.
Attachment #8873393 - Flags: review?(chutten) → review+

Comment 11

2 years ago
Pushed by ttaubert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3cf72d18df9
Update FX_SESSION_RESTORE_DOM_STORAGE_SIZE_ESTIMATE_CHARS histogram metadata r=chutten
Depends on: 1432486
You need to log in before you can comment on or make changes to this bug.