Closed Bug 1216250 Opened 4 years ago Closed 4 years ago

Limit amount of DOM Storage data stored by Session Restore

Categories

(Firefox :: Session Restore, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 45
Tracking Status
firefox44 + fixed
firefox45 --- fixed

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

(Keywords: dev-doc-complete)

Attachments

(2 files)

No description provided.
Bug 1216250 - Limit amount of DOM Storage data stored by Session Restore;r?ttaubert
Attachment #8676203 - Flags: review?(ttaubert)
Attachment #8676203 - Flags: review?(ttaubert)
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.
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.
Assignee: nobody → dteller
Flags: needinfo?(ttaubert)
(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.
Flags: needinfo?(ttaubert)
Attachment #8676203 - Flags: review?(ttaubert)
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
Attachment #8676203 - Flags: review?(ttaubert)
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 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
Attachment #8676203 - Flags: review?(ttaubert)
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?
Attachment #8676203 - Flags: review?(ttaubert) → review+
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.
Attachment #8676203 - Attachment description: MozReview Request: Bug 1216250 - Limit amount of DOM Storage data stored by Session Restore;r?ttaubert → MozReview Request: Bug 1216250 - Limit amount of DOM Storage data stored by Session Restore;r=ttaubert
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 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.
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
Flags: needinfo?(dteller)
Keywords: checkin-needed
Most likely bitrotten by bug 1214408. I'll unbitrot once that one has finished landing.
Flags: needinfo?(dteller)
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.
Duplicate of this bug: 1216251
https://hg.mozilla.org/mozilla-central/rev/bb09cf018056
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Hi Yoric, if this looks good on m-c and will help with bug 1106264, want to request uplift?
Flags: needinfo?(dteller)
I'd like to wait a few days and check if this breaks anything.
Flags: needinfo?(dteller)
Depends on: 1223082
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
Flags: needinfo?(dteller)
Hi Liz, I'm afraid I don't understand the question.
Flags: needinfo?(dteller) → needinfo?(lhenry)
(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.
Flags: needinfo?(kohei.yoshino)
Ah okay, will delete the compat doc then. Thanks!
Flags: needinfo?(kohei.yoshino)
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.
Flags: needinfo?(lhenry)
Deleted the doc, clearing the keywords.
@lizzard Ah, good point.
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]:
Attachment #8676203 - Flags: approval-mozilla-aurora?
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.
Attachment #8676203 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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!
Flags: needinfo?(dteller)
Flags: needinfo?(dteller)
Does this work better?
Flags: needinfo?(cbook)
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #36)
> Does this work better?

yeah thanks!
Flags: needinfo?(cbook)
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?
Thanks. I have changed one word.
You need to log in before you can comment on or make changes to this bug.