Print a warning whenever something attempts to store more than 4kb of preferences

RESOLVED FIXED in mozilla24

Status

()

Core
Preferences: Backend
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Yoric, Assigned: Yoric)

Tracking

(Blocks: 1 bug, {addon-compat, dev-doc-needed})

unspecified
mozilla24
addon-compat, dev-doc-needed
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Snappy][mentor=Yoric][lang=c++])

Attachments

(1 attachment, 3 obsolete attachments)

As a first step for bug 872980, we should deprecate any attempt to store more than 16kb of preferences in a field (or possibly in a non-root branch) and print a warning whenever code attempts to do this.
It might be possible to do this here: http://dxr.mozilla.org/mozilla-central/modules/libpref/src/prefapi.cpp#l704
Keywords: addon-compat
Is this about the length of a single string type preference, or about the total size of preferences used by an extension? HTTPS-everywhere stores all its website rules as preferences - or at least it used to, I haven't used it in a while. I finally removed them from my prefs.js file manually a few days ago, making it significantly smaller.
If there is a simple way to determine the total size of preferences used by an extension, that's even better. If not, measuring single string type preference would be a good first step.

Comment 4

4 years ago
As a note, the initial patch to do a limit at all (which was set to 1MB for now) was done in bug 836263.

And anyone storing a larger amount of data should probably switch to indexedDB, just like Crossrider did (see the bug I mentioned).
(In reply to David Rajchenbach Teller [:Yoric] from comment #3)
> If there is a simple way to determine the total size of preferences used by
> an extension, that's even better. If not, measuring single string type
> preference would be a good first step.

The bug that inspired this approach is caused by single preferences having too much data, so I think that's what we should be aiming for, rather than setting a quota per-extension. Also, this quote would not be very easy to implement, and very easy to circumvent.
This looks easy enough to do by following the example of
https://bug836263.bugzilla.mozilla.org/attachment.cgi?id=743345

Let's make this a mentored bug.
Whiteboard: [Snappy] → [Snappy][mentor=Yoric][lang=c++]

Comment 7

4 years ago
Created attachment 751774 [details] [diff] [review]
a text file containing the fix

Comment 8

4 years ago
Hopefully it is fixed now. Please verify it.
Aritra, it's a good start, although not exactly what I want.
However, could you please post the fix as a patch?
See https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch for more details.
Ah, well, I have nothing better to do while waiting for reviews. I'll try and produce a patch.
Assignee: nobody → dteller
Created attachment 754763 [details] [diff] [review]
Display a warning when setting a preference larger than 16kb.

Here we go. Somewhere along the way, I have also fixed the patch to bug 836263 to ensure that it works also with setCharPref.
Attachment #751774 - Attachment is obsolete: true
Attachment #754763 - Flags: review?(benjamin)
Attachment #754763 - Flags: review?(dwitte)

Comment 12

4 years ago
There's a typo in the warning message: shoudl.

Comment 13

4 years ago
Comment on attachment 754763 [details] [diff] [review]
Display a warning when setting a preference larger than 16kb.

16k still seems excessive: are there normal prefs that exceed 4k or can we make the warning cutoff 4k?

>diff --git a/modules/libpref/test/unit/test_warnings.js b/modules/libpref/test/unit/test_warnings.js

>\ No newline at end of file

pls fix
Attachment #754763 - Flags: review?(dwitte)
Attachment #754763 - Flags: review?(benjamin)
Attachment #754763 - Flags: review+
That's hard to tell. Some add-ons store lists of URLs in preferences and those can grow pretty quickly. I think we should aim to minimize add-on breakage. As far as I understand, this limit is well below what would cause stability problems, so I think it's a reasonable middle point.
This was a pretty arbitrary value, I am willing to lower it to 4kb. We can also lower it progressively, if we want to.

@jorgev The issue here is not only stability but also performance. Writing anything non-trivial to pref slows down both start-up and further pref writes, so we would like to encourage add-on developers to migrate away from preferences for this kind of uses.

So, any consensus on the cutoff length?

Comment 16

4 years ago
Let's go with 4k for now and we can back it off if we start seeing it too much. This is only a warning, so it isn't going to break anything yet!
Created attachment 757524 [details] [diff] [review]
Display a warning when setting a preference larger than 4kb.

Try: https://tbpl.mozilla.org/?tree=Try&rev=31f074a73030
Attachment #754763 - Attachment is obsolete: true
Attachment #757524 - Flags: review+
Just a warning: removing addon-compat.
Keywords: addon-compat → checkin-needed, dev-doc-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ca1abed8c9a
Keywords: checkin-needed
Backed out for xpcshell crashes.
https://hg.mozilla.org/integration/mozilla-inbound/rev/281b0297de65

https://tbpl.mozilla.org/php/getParsedLog.php?id=23729277&tree=Mozilla-Inbound
We still want to track this for developer outreach.
Keywords: addon-compat
Created attachment 757602 [details] [diff] [review]
Display a warning when setting a preference larger than 4kb, v2

Same one, with a null pointer check.
Try: https://tbpl.mozilla.org/?tree=Try&rev=3486d64f6da9
Attachment #757524 - Attachment is obsolete: true
Attachment #757602 - Flags: review+
(In reply to Jorge Villalobos [:jorgev] from comment #21)
> We still want to track this for developer outreach.

Ah, my bad, I had misinterpreted the use of the keyword.
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/5777dd1cbd32
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5777dd1cbd32
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24

Updated

4 years ago
Summary: Print a warning whenever something attempts to store more than 16kb of preferences → Print a warning whenever something attempts to store more than 4kb of preferences
You need to log in before you can comment on or make changes to this bug.