Last Comment Bug 872981 - Print a warning whenever something attempts to store more than 4kb of preferences
: Print a warning whenever something attempts to store more than 4kb of prefere...
Status: RESOLVED FIXED
[Snappy][mentor=Yoric][lang=c++]
: addon-compat, dev-doc-needed
Product: Core
Classification: Components
Component: Preferences: Backend (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla24
Assigned To: David Teller [:Yoric] (please use "needinfo")
:
:
Mentors:
Depends on:
Blocks: 872980
  Show dependency treegraph
 
Reported: 2013-05-16 03:24 PDT by David Teller [:Yoric] (please use "needinfo")
Modified: 2013-09-24 01:47 PDT (History)
13 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
a text file containing the fix (640 bytes, patch)
2013-05-20 11:46 PDT, Aritra Sasmal
no flags Details | Diff | Splinter Review
Display a warning when setting a preference larger than 16kb. (11.71 KB, patch)
2013-05-28 04:49 PDT, David Teller [:Yoric] (please use "needinfo")
benjamin: review+
Details | Diff | Splinter Review
Display a warning when setting a preference larger than 4kb. (11.65 KB, patch)
2013-06-03 11:33 PDT, David Teller [:Yoric] (please use "needinfo")
dteller: review+
Details | Diff | Splinter Review
Display a warning when setting a preference larger than 4kb, v2 (11.69 KB, patch)
2013-06-03 14:15 PDT, David Teller [:Yoric] (please use "needinfo")
dteller: review+
Details | Diff | Splinter Review

Description David Teller [:Yoric] (please use "needinfo") 2013-05-16 03:24:46 PDT
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.
Comment 1 David Teller [:Yoric] (please use "needinfo") 2013-05-16 05:48:10 PDT
It might be possible to do this here: http://dxr.mozilla.org/mozilla-central/modules/libpref/src/prefapi.cpp#l704
Comment 2 Emanuel Hoogeveen [:ehoogeveen] 2013-05-16 08:29:24 PDT
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.
Comment 3 David Teller [:Yoric] (please use "needinfo") 2013-05-16 08:30:59 PDT
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 Robert Kaiser 2013-05-16 08:58:03 PDT
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).
Comment 5 Jorge Villalobos [:jorgev] 2013-05-16 09:28:07 PDT
(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.
Comment 6 David Teller [:Yoric] (please use "needinfo") 2013-05-18 12:24:30 PDT
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.
Comment 7 Aritra Sasmal 2013-05-20 11:46:36 PDT
Created attachment 751774 [details] [diff] [review]
a text file containing the fix
Comment 8 Aritra Sasmal 2013-05-20 11:57:22 PDT
Hopefully it is fixed now. Please verify it.
Comment 9 David Teller [:Yoric] (please use "needinfo") 2013-05-22 02:57:15 PDT
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.
Comment 10 David Teller [:Yoric] (please use "needinfo") 2013-05-27 10:54:46 PDT
Ah, well, I have nothing better to do while waiting for reviews. I'll try and produce a patch.
Comment 11 David Teller [:Yoric] (please use "needinfo") 2013-05-28 04:49:31 PDT
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.
Comment 12 Michael Brennan 2013-06-01 03:46:05 PDT
There's a typo in the warning message: shoudl.
Comment 13 Benjamin Smedberg [:bsmedberg] 2013-06-03 10:05:57 PDT
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
Comment 14 Jorge Villalobos [:jorgev] 2013-06-03 10:26:24 PDT
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.
Comment 15 David Teller [:Yoric] (please use "needinfo") 2013-06-03 10:42:35 PDT
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 Benjamin Smedberg [:bsmedberg] 2013-06-03 11:12:42 PDT
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!
Comment 17 David Teller [:Yoric] (please use "needinfo") 2013-06-03 11:33:06 PDT
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
Comment 18 David Teller [:Yoric] (please use "needinfo") 2013-06-03 11:46:06 PDT
Just a warning: removing addon-compat.
Comment 19 Ryan VanderMeulen [:RyanVM] 2013-06-03 12:31:14 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ca1abed8c9a
Comment 21 Jorge Villalobos [:jorgev] 2013-06-03 13:56:50 PDT
We still want to track this for developer outreach.
Comment 22 David Teller [:Yoric] (please use "needinfo") 2013-06-03 14:15:26 PDT
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
Comment 23 David Teller [:Yoric] (please use "needinfo") 2013-06-03 22:52:28 PDT
(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.
Comment 24 Ryan VanderMeulen [:RyanVM] 2013-06-04 05:27:28 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/5777dd1cbd32
Comment 25 Ryan VanderMeulen [:RyanVM] 2013-06-04 12:05:31 PDT
https://hg.mozilla.org/mozilla-central/rev/5777dd1cbd32

Note You need to log in before you can comment on or make changes to this bug.