1908925 - storage.session quota is not enforced

Status: RESOLVED FIXED

(WebExtensions :: Storage, defect, P2)

Description

[Rob Wu [:robwu]]

The current implementation of storage.session in Firefox does not enforce any limit on the size of the data stored. There is cross-browser consensus on limiting the size to 10 MB (https://github.com/w3c/webextensions/issues/350).

In bug 1906755, a severe performance degradation was observed when an extension stored too much data. It maintained a list of values in storage.session, by using storage.session.get, updating the list and using storage.session.set to update the list again. It lead to massive memory usage (and CPU consumption) because any update to the list does not only result in 3 copies of the data each way (detailed in https://bugzilla.mozilla.org/show_bug.cgi?id=1906755#c7), but also because the storage.session.onChanged event is fired, which includes the modified data (changes.keyname.oldValue + changes.keyname.newValue).

Interestingly, the documentation on MDN does not mention any limits in the content itself, but the Browser compatibility table notes in the Chromium browsers that "Before version 112, storage quota limits were 1MB.". When we fix this bug, we should document the limits in a clearer way.

As part of enforcing the limits, we should also consider implementing the getBytesInUse method on storage.session (and storage.local - bug 1385832).

Comment 1

[Tomislav Jovanovic :zombie]

Attachment: Bug 1908925 - Enforce storage.session quota in Nightly builds r?robwu

Comment 2

[Rob Wu [:robwu]]

As seen in bug 1913018, there are add-ons that do not account for a quota. We should announce the intent to enforce the quota, e.g. in a blog post. To maximize Nightly coverage, let's aim at landing this patch at the start of the next (132) cycle.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/346d8277bee9
Enforce storage.session quota in Nightly builds r=robwu,mccr8

Comment 4

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/346d8277bee9

Comment 5

[Richard Bloor]

Could I get some clarification on the documentation requirements given the comment about a blog post and landing in 132.

Comment 6

[Tomislav Jovanovic :zombie]

We don't need to wait for the blog post (which might need to wait until we figure exact plan for storage.sync), we can document the storage.session quota as being unintentionally not enforced until Nightly 130 (and later update that to the exact release, like 132 when we let it ride the trains).

Comment 7

[Richard Bloor]

That seems bit vague - what release am I documenting this for and is the change limited to only the enforcement of the limit?

Comment 8

[Rob Wu [:robwu]]

The above patch landed in 131 (not 130). Here is an example of the message to be conveyed in the release notes:

The amount of data stored in the storage.session API is documented to be limited to 10 MB. Firefox did erroneously not enforce this quota. Firefox Nightly 131 enforces the quota, to allow extensions that relied on the previous behavior to detect this issue. This enforcement will be rolled out to release versions of Firefox, from version 134.

Version 134 has not been decided yet, but once the PR with the documentation proposal appears we can mention the right version. We should also have a bugzilla ticket that tracks enforcing the storage.session quota on release.

Comment 9

[Richard Bloor]

Release note added in storage.session quota enforcement release note #35629

Comment 10

[Rob Wu [:robwu]]

I filed bug 1915688 about enforcing the quota on release.

I also asked for the storage.session.getBytesInUse method and the storage.session.QUOTA_BYTES constant to be documented: https://github.com/mdn/content/pull/35629#pullrequestreview-2269550055

Comment 11

[Richard Bloor]

However, and why I asked in https://bugzilla.mozilla.org/show_bug.cgi?id=1908925#c7 about the scope:

• toolkit/components/extensions/schemas/storage.json indicates that getBytesInUse is unsupported.
• QUOTA_BYTES would become the only property of 5 documented, should we document the others at the same time?

Comment 12

[Rob Wu [:robwu]]

(In reply to Richard Bloor from comment #11)

However, and why I asked in https://bugzilla.mozilla.org/show_bug.cgi?id=1908925#c7 about the scope:

• toolkit/components/extensions/schemas/storage.json indicates that getBytesInUse is unsupported.

The storage.sync and storage.session APIs support getBytesInUse. .local and .managed do not.

• definition for storage.session and .sync: https://searchfox.org/mozilla-central/rev/cb5faf5dd5176494302068c553da97b4d08aa339/toolkit/components/extensions/schemas/storage.json#209
• (unsupported) definition for .local and .managed: https://searchfox.org/mozilla-central/rev/cb5faf5dd5176494302068c553da97b4d08aa339/toolkit/components/extensions/schemas/storage.json#65

• QUOTA_BYTES would become the only property of 5 documented, should we document the others at the same time?

What are the other 5 properties? QUOTA_BYTES_PER_ITEM etc? These are only supported in storage.sync, not in .local, .session or .managed.

Comment 13

[Richard Bloor]

What are the other 5 properties? QUOTA_BYTES_PER_ITEM etc? These are only supported in storage.sync, not in .local, .session or .managed.

Indeed, however, we don't document them - should we document them all or none of them? They are all documented in the Chrome docs, https://developer.chrome.com/docs/extensions/reference/api/storage#property

Comment 14

[Rob Wu [:robwu]]

I suppose that we could document them, but not as part of this bug.

These constants are currently enforced on desktop Firefox only, and not on Firefox for Android yet. We are going to enforce them "soon" and at that point it would make sense to add documentation for these extra constants. This enforcement happens when the mobile implementation switches to the same backend as the desktop one, which is tracked at bug 1625257.

Comment 15

[Richard Bloor]

Current documentation lists getBytesInUse under all the storage types.
Documentation for getBytesInUse states "This function only exists in browser.storage.sync It does not exist in browser.storage.local See https://bugzil.la/1385832".
So, should I remove reference to storage.session.getBytesInUse() from:

• https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/storage/local
• https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/storage/managed'
  Or will it be implemented for those eventually? (e.g. https://bugzil.la/1385832)

Or should I leave the documentation alone but modify the BCD to exemplify all of the methods, events, and properties (including QUOTA_BYTES, QUOTA_BYTES_PER_ITEM etc.) for each of the storage areas (e.g. to note that getBytesInUse isn't supported in local or managed or that QUOTA_BYTES only applies to local and session etc. ).

The latter seems the correct approach to me.

Comment 16

[Richard Bloor]

See my new comment

Comment 17

[Rob Wu [:robwu]]

(In reply to Richard Bloor from comment #15)

Current documentation lists getBytesInUse under all the storage types.
Documentation for getBytesInUse states "This function only exists in browser.storage.sync It does not exist in browser.storage.local See https://bugzil.la/1385832".
So, should I remove reference to storage.session.getBytesInUse() from:

• https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/storage/local
• https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/storage/managed'
  Or will it be implemented for those eventually? (e.g. https://bugzil.la/1385832)

These should be kept because some implementations support it.

Or should I leave the documentation alone but modify the BCD to exemplify all of the methods, events

Yes.

, and properties (including QUOTA_BYTES,

Yes.

QUOTA_BYTES_PER_ITEM etc.)

The other constants are specific to storage.sync and should not be mentioned elsewhere.

To minimize confusion and allow for more readable documentation, I am willing to duplicate the relevant parts of the current documentation under StorageArea/, to /storage/local/, managed/, session/ and sync/. This would enable the documentation to spell out API examples under the concrete namespaces instead of storage.<storageType>.set. If you agree, let's file an issue on mdn/content and continue the discussion there.

Comment 18

[Richard Bloor]

Created https://github.com/mdn/content/issues/35657