Closed Bug 872981 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Preferences: Backend, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: Yoric, Assigned: Yoric)

References

(Blocks 1 open bug)

Details

(Keywords: addon-compat, dev-doc-needed, Whiteboard: [Snappy][mentor=Yoric][lang=c++])

Attachments

(1 file, 3 obsolete files)

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.
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.
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++]
Attached patch a text file containing the fix (obsolete) — Splinter Review
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
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)
There's a typo in the warning message: shoudl.
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?
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!
Just a warning: removing addon-compat.
We still want to track this for developer outreach.
Keywords: addon-compat
(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/mozilla-central/rev/5777dd1cbd32
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
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.