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

Status: RESOLVED FIXED

(Core :: Preferences: Backend, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

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] - still alive but not very active]

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]]

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] - still alive but not very active]

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]

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] (he/him)]

(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] - still alive but not very active]

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]

Attachment: a text file containing the fix

Comment 8

[Aritra Sasmal]

Hopefully it is fixed now. Please verify it.

Comment 9

[David Teller [:Yoric] - still alive but not very active]

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] - still alive but not very active]

Ah, well, I have nothing better to do while waiting for reviews. I'll try and produce a patch.

Comment 11

[David Teller [:Yoric] - still alive but not very active]

Attachment: 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 Hoffmann]

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

Comment 13

[Benjamin Smedberg]

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] (he/him)]

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] - still alive but not very active]

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]

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] - still alive but not very active]

Attachment: Display a warning when setting a preference larger than 4kb.

Try: https://tbpl.mozilla.org/?tree=Try&rev=31f074a73030

Comment 18

[David Teller [:Yoric] - still alive but not very active]

Just a warning: removing addon-compat.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ca1abed8c9a

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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

Comment 21

[Jorge Villalobos [:jorgev] (he/him)]

We still want to track this for developer outreach.

Comment 22

[David Teller [:Yoric] - still alive but not very active]

Attachment: 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] - still alive but not very active]

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/5777dd1cbd32

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5777dd1cbd32