Closed Bug 1908925 Opened 7 months ago Closed 6 months ago

storage.session quota is not enforced

Categories

(WebExtensions :: Storage, defect, P2)

Product:
WebExtensions
Component:
Storage
Type:
defect

Tracking

(firefox131 fixed)

Status:
RESOLVED FIXED
Milestone:
131 Branch
Tracking Flags:
Tracking Status
firefox131 --- fixed

People

(Reporter: robwu, Assigned: zombie)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [wecg][addons-jira])

Attachments

(1 file)

Bug 1908925 - Enforce storage.session quota in Nightly builds r?robwu
48 bytes, text/x-phabricator-request
Details | Review

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).

Assignee: nobody → tomica
Severity: -- → S3
Priority: -- → P2
See Also: → 1913018

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.

Keywords: dev-doc-needed
Pushed by tjovanovic@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/346d8277bee9 Enforce storage.session quota in Nightly builds r=robwu,mccr8

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

Status: NEW → RESOLVED
Closed: 6 months ago
status-firefox131: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 131 Branch

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

Flags: needinfo?(rob)

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).

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

Flags: needinfo?(tomica)

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.

Flags: needinfo?(tomica)
Flags: needinfo?(rob)
Blocks: 1915688

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

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?
Flags: needinfo?(rob)

(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.

  • 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.

Flags: needinfo?(rob)

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

Flags: needinfo?(rob)

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.

Flags: needinfo?(rob)

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:

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.

See my new comment

Flags: needinfo?(rob)

(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:

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.

Flags: needinfo?(rob)
Blocks: 1916276
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: