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