1362058 - Further limit the amount of sessionStorage data we store and serialize

Status: RESOLVED FIXED

(Firefox :: Session Restore, enhancement)

Description

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Can I just make this a P1? I think it should be. Let's see.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

We'll also switch to using the API I introduced in bug 1361974.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1362058-Further-limit-the-amount-of-sessionStora.patch

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

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 5

[Mike de Boer [:mikedeboer]]

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.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

(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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/262e008b0bc4
Further limit the amount of sessionStorage data we store and serialize r=mikedeboer

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/262e008b0bc4

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1362058-Update-FX_SESSION_RESTORE_DOM_STORAGE_SI.patch

Comment 10

[Chris H-C :chutten]

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.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3cf72d18df9
Update FX_SESSION_RESTORE_DOM_STORAGE_SIZE_ESTIMATE_CHARS histogram metadata r=chutten

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d3cf72d18df9