1216250 - Limit amount of DOM Storage data stored by Session Restore

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Attachment: MozReview Request: Bug 1216250 - Limit amount of DOM Storage data stored by Session Restore;r=ttaubert

Bug 1216250 - Limit amount of DOM Storage data stored by Session Restore;r?ttaubert

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8676203 [details]
MozReview Request: Bug 1216250 - Limit amount of DOM Storage data stored by Session Restore;r=ttaubert

https://reviewboard.mozilla.org/r/22627/#review20135

::: browser/components/sessionstore/content/content-sessionStore.js:24
(Diff revision 1)
> +XPCOMUtils.defineLazyModuleGetter(this, "DocShellCapabilities",
> +  "resource:///modules/sessionstore/DocShellCapabilities.jsm");

Why are you moving this? The includes were alphabetically ordered before.

::: browser/components/sessionstore/content/content-sessionStore.js:723
(Diff revision 1)
> +        let size = 0;
> +        if (typeof collected == "object" && collected != null) {
> +          for (let host of Object.keys(collected)) {
> +            size += host.length;
> +            let perHost = collected[host];
> +            for (let key of Object.keys(perHost)) {
> +              size += key.length;
> +              let perKey = perHost[key];
> +              size += perKey.length;
> +            }
> +          }
> +        }

This doesn't seem like the right place for this code. I think we should move it into the SessionStorageListener and just ignore it there before pushing it onto the MessageQueue.

::: browser/components/sessionstore/content/content-sessionStore.js:738
(Diff revision 1)
> +        if (size > Preferences.get("browser.sessionstore.dom_storage_limit", DOM_STORAGE_IS_TOO_LARGE)) {
> +          // Discard. DOM storage will be recollected the next time and stored
> +          // if it is now small enough.
> +          continue;
> +        }

I wouldn't default to introducing new preferences just because we think someone might want it. 1) Having less prefs is always preferable and 2) I don't think that anyone or any website really relies on us restoring DOMSessionStorage anyway.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

https://reviewboard.mozilla.org/r/22627/#review20135

> Why are you moving this? The includes were alphabetically ordered before.

Well, they were semi-alphabetical, mixing resource:///modules/sessionstore and resource://gre/modules. I don't have to do that, of course, your call.

> This doesn't seem like the right place for this code. I think we should move it into the SessionStorageListener and just ignore it there before pushing it onto the MessageQueue.

If we do that, we lose the opportunity to send Telemetry to the parent process. There may be a way around it, but it seems more awkward. What do you think?

> I wouldn't default to introducing new preferences just because we think someone might want it. 1) Having less prefs is always preferable and 2) I don't think that anyone or any website really relies on us restoring DOMSessionStorage anyway.

Actually, the preference was for testing purposes.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #3)
> > Why are you moving this? The includes were alphabetically ordered before.
> 
> Well, they were semi-alphabetical, mixing resource:///modules/sessionstore
> and resource://gre/modules. I don't have to do that, of course, your call.

I don't have a strong opinion here, was just wondering.

> > This doesn't seem like the right place for this code. I think we should move it into the SessionStorageListener and just ignore it there before pushing it onto the MessageQueue.
> 
> If we do that, we lose the opportunity to send Telemetry to the parent
> process. There may be a way around it, but it seems more awkward. What do
> you think?

Hmmm, true. What about doing MessageQueue.push("telemetry", { ... }) and handling that separately in MessageQueue.send()? This way we could extend other Listeners/Collectors with telemetry in the future if needed.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8676203 [details]
MozReview Request: Bug 1216250 - Limit amount of DOM Storage data stored by Session Restore;r=ttaubert

Bug 1216250 - Limit amount of DOM Storage data stored by Session Restore;r?ttaubert

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8676203 [details]
MozReview Request: Bug 1216250 - Limit amount of DOM Storage data stored by Session Restore;r=ttaubert

https://reviewboard.mozilla.org/r/22627/#review21199

::: browser/components/sessionstore/content/content-sessionStore.js:50
(Diff revision 2)
> +const DOM_STORAGE_IS_TOO_LARGE = 10000000; // 10M characters

Maybe DOM_STORAGE_MAX_BYTES or _LIMIT_BYTES?

::: browser/components/sessionstore/content/content-sessionStore.js:568
(Diff revision 2)
> +        // object, and block SessionStorage items that are too large. As a
> +        // we also don't want to cause an OOM here, we use a quick and memory-

"As a we also..." probably just remove the "a"?

::: browser/components/sessionstore/content/content-sessionStore.js:572
(Diff revision 2)
> +        let size = 0;
> +        for (let host of Object.keys(collected)) {
> +          size += host.length;
> +          let perHost = collected[host];
> +          for (let key of Object.keys(perHost)) {
> +            size += key.length;
> +            let perKey = perHost[key];
> +            size += perKey.length;
> +          }
> +        }

I think this should move into a separate function, probably together with the comment.

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8676203 [details]
MozReview Request: Bug 1216250 - Limit amount of DOM Storage data stored by Session Restore;r=ttaubert

Bug 1216250 - Limit amount of DOM Storage data stored by Session Restore;r?ttaubert

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8676203 [details]
MozReview Request: Bug 1216250 - Limit amount of DOM Storage data stored by Session Restore;r=ttaubert

https://reviewboard.mozilla.org/r/22627/#review21231

::: browser/components/sessionstore/content/content-sessionStore.js:564
(Diff revision 3)
> +    if (!storage) {
> +      return 0;
> +    }

Looks like |storage| would always be true-ish.

::: browser/components/sessionstore/content/content-sessionStore.js:587
(Diff revision 3)
> +        if (collected == null) {
> +          return collected;
> +        }

We can remove the check above due to this, OTOH do we want to omit reporting zeros? Do we want to know how many pages have no DOMSessionStorage data?

Comment 9

[David Teller [:Yoric] - still alive but not very active]

https://reviewboard.mozilla.org/r/22627/#review21231

> Looks like |storage| would always be true-ish.

Yes, I wanted to be on the safe side.

> We can remove the check above due to this, OTOH do we want to omit reporting zeros? Do we want to know how many pages have no DOMSessionStorage data?

I suspect that we don't care all that much about pages with no DOMSessionStorage data. This histogram is about how much memory it takes when there is some data, so no need to feed 99.9% of 0s to the server.

I have documented this in the histogram.

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8676203 [details]
MozReview Request: Bug 1216250 - Limit amount of DOM Storage data stored by Session Restore;r=ttaubert

Bug 1216250 - Limit amount of DOM Storage data stored by Session Restore;r=ttaubert

DOM Storage is a pretty inefficient and memory-hungry storage mechanism. Session Store attempts to record DOM Storage for each tab, which leads to (possibly very large) objects being serialized once to be sent from frame/content to parent and once to be sent from the main thread to the I/O thread. This is a suspect behind a number of crashes (see bug 1106264 for a discussion on the topic).

This patch limits the amount of DOM Storage that Session Restore attempts to store. We perform a quick estimate on the amount of memory needed to serialize DOM Storage and prevent storage larger than ~10M chars being sent from frame/content to the parent. Once this patch has landed, we will need to watch FX_SESSION_RESTORE_DOM_STORAGE_SIZE_ESTIMATE_CHARS to find out whether our threshold is meaningful.

Comment 11

[David Teller [:Yoric] - still alive but not very active]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd774cadb1e7

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f09f524621de

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8676203 [details]
MozReview Request: Bug 1216250 - Limit amount of DOM Storage data stored by Session Restore;r=ttaubert

Bug 1216250 - Limit amount of DOM Storage data stored by Session Restore;r=ttaubert

DOM Storage is a pretty inefficient and memory-hungry storage mechanism. Session Store attempts to record DOM Storage for each tab, which leads to (possibly very large) objects being serialized once to be sent from frame/content to parent and once to be sent from the main thread to the I/O thread. This is a suspect behind a number of crashes (see bug 1106264 for a discussion on the topic).

This patch limits the amount of DOM Storage that Session Restore attempts to store. We perform a quick estimate on the amount of memory needed to serialize DOM Storage and prevent storage larger than ~10M chars being sent from frame/content to the parent. Once this patch has landed, we will need to watch FX_SESSION_RESTORE_DOM_STORAGE_SIZE_ESTIMATE_CHARS to find out whether our threshold is meaningful.

Comment 14

[David Teller [:Yoric] - still alive but not very active]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f09f524621de

Comment 15

[Carsten Book [:Tomcat]]

Hi, this failed to apply:

applying 7756fb39915c
patching file browser/components/sessionstore/content/content-sessionStore.js
Hunk #4 FAILED at 718
1 out of 4 hunks FAILED -- saving rejects to file browser/components/sessionstore/content/content-sessionStore.js.rej
patching file toolkit/components/telemetry/Histograms.json
Hunk #1 FAILED at 4346
1 out of 1 hunks FAILED -- saving rejects to file toolkit/components/telemetry/Histograms.json.rej

Comment 16

[David Teller [:Yoric] - still alive but not very active]

Most likely bitrotten by bug 1214408. I'll unbitrot once that one has finished landing.

Comment 17

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8676203 [details]
MozReview Request: Bug 1216250 - Limit amount of DOM Storage data stored by Session Restore;r=ttaubert

Bug 1216250 - Limit amount of DOM Storage data stored by Session Restore;r=ttaubert

DOM Storage is a pretty inefficient and memory-hungry storage mechanism. Session Store attempts to record DOM Storage for each tab, which leads to (possibly very large) objects being serialized once to be sent from frame/content to parent and once to be sent from the main thread to the I/O thread. This is a suspect behind a number of crashes (see bug 1106264 for a discussion on the topic).

This patch limits the amount of DOM Storage that Session Restore attempts to store. We perform a quick estimate on the amount of memory needed to serialize DOM Storage and prevent storage larger than ~10M chars being sent from frame/content to the parent. Once this patch has landed, we will need to watch FX_SESSION_RESTORE_DOM_STORAGE_SIZE_ESTIMATE_CHARS to find out whether our threshold is meaningful.

Comment 18

[David Teller [:Yoric] - still alive but not very active]

rebased

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/bb09cf018056

Comment 21

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

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

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

Hi Yoric, if this looks good on m-c and will help with bug 1106264, want to request uplift?

Comment 23

[David Teller [:Yoric] - still alive but not very active]

I'd like to wait a few days and check if this breaks anything.

Comment 24

[Kohei Yoshino]

Added the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/session-storage-data-size-is-now-limited-to-10-mb/

Comment 25

[Liz Henry (:lizzard) (relman/hg->git project)]

Hi Yoric, what do you think? Here's the latest session restore bugs from when this patch landed until today:
https://bugzilla.mozilla.org/buglist.cgi?list_id=12689962&chfieldto=2015-11-18&query_format=advanced&chfield=[Bug%20creation]&chfieldfrom=2015-11-05&component=Session%20Restore&product=Firefox

Comment 26

[David Teller [:Yoric] - still alive but not very active]

Hi Liz, I'm afraid I don't understand the question.

Comment 27

[David Teller [:Yoric] - still alive but not very active]

(In reply to Kohei Yoshino [:kohei] from comment #24)
> Added the site compatibility doc:
> https://www.fxsitecompat.com/en-US/docs/2015/session-storage-data-size-is-
> now-limited-to-10-mb/

Actually, that's not exactly the case. It's the amount of data that is recorded in case of crash/restart. The amount of data that can be stored by a webpage hasn't changed.

Comment 28

[Kohei Yoshino]

Ah okay, will delete the compat doc then. Thanks!

Comment 29

[Liz Henry (:lizzard) (relman/hg->git project)]

Do you want to uplift this to aurora or beta, was the original question.... then you mentioned waiting a few days to see if this breaks anything. 

I checked the list of newly reported session restore bugs to see if there were any obvious regressions. While there are some new bugs, I'm not sure how to tell if they're related. 
 
It's getting late to uplift this to beta,  but you could still request uplift to 44 if that seems like a good idea to you.

Comment 30

[Kohei Yoshino]

Deleted the doc, clearing the keywords.

Comment 31

[David Teller [:Yoric] - still alive but not very active]

@lizzard Ah, good point.

Comment 32

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8676203 [details]
MozReview Request: Bug 1216250 - Limit amount of DOM Storage data stored by Session Restore;r=ttaubert

Approval Request Comment
[Feature/regressing bug #]: Making Session Restore compatible with e10s, a long time ago.
[User impact if declined]: OOM Crashes.
[Describe test coverage new/current, TreeHerder]: This has now been on m-c for a few weeks.
[Risks and why]: In case of crash or restart, some webpages that consume too much memory may not be restored properly. We don't know of any such page yet.
[String/UUID change made/needed]:

Comment 33

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8676203 [details]
MozReview Request: Bug 1216250 - Limit amount of DOM Storage data stored by Session Restore;r=ttaubert

This fixes possible OOM crashes and definitely the kind of fix (once stable) seem worthy of uplifting to Aurora44. It was on Nightly for almost 3 weeks, approved.

Comment 34

[Carsten Book [:Tomcat]]

this cause problems uplifting to aurora:

grafting 313277:abbcb9ec539b "Bug 1221356 - Don't package mozharness for Thunderbird builds. r=gps"
Tomcats-MacBook-Pro-2:mozilla-central Tomcat$ hg graft --edit -r bb09cf018056
grafting 313196:bb09cf018056 "Bug 1216250 - Limit amount of DOM Storage data stored by Session Restore. r=ttaubert"
merging browser/components/sessionstore/content/content-sessionStore.js
merging toolkit/components/telemetry/Histograms.json
warning: conflicts during merge.
merging toolkit/components/telemetry/Histograms.json incomplete! (edit conflicts, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)

could you take a look, thanks!

Comment 35

[David Teller [:Yoric] - still alive but not very active]

Attachment: Aurora version of the patch

Comment 36

[David Teller [:Yoric] - still alive but not very active]

Does this work better?

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/17f3d2ac13a4

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/17f3d2ac13a4

Comment 39

[Carsten Book [:Tomcat]]

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #36)
> Does this work better?

yeah thanks!

Comment 40

[Chris Mills [:cmills]]

I thought this should perhaps be recorded somewhere in the docs, just in case — have a look at the top note in the first section of this doc:

https://developer.mozilla.org/en-US/docs/Web/API/Web_Storage_API

Does this wording read ok?

Comment 41

[David Teller [:Yoric] - still alive but not very active]

Thanks. I have changed one word.