506224 - locked preferences values shouldn't be stored in user preferences

Status: RESOLVED FIXED

(Core :: Preferences: Backend, defect)

Description

[Mike Hommey [:glandium]]

Attachment: patch

One of the consequences of this is debian bug #512111 ( http://bugs.debian.org/512111 ).

What can happen in other contexts, is an administrator replacing a locked preference with an unlocked pref with a different value. Users would then not get the modified value.

The attached patch doesn't save locked preferences at all when it was not already in the prefs file, but will keep any user value if it was set before the pref was locked. Which means in the case the administrator replaces the lockes pref with an unlocked pref with a different value, the user won't get it, as he had a custom value in the first place. I think this is a reasonable drawback, if that be called so.

Note that bug 330590 might be related to this one, though I haven't verified.

PS: No owner listed on http://www.mozilla.org/owners.html, so I'm requesting review to the last person who reviewed last commit to libpref.

Comment 1

[Mike Hommey [:glandium]]

(In reply to comment #0)
> Note that bug 330590 might be related to this one, though I haven't verified.

Except that the behaviour described in comment #2 out there would still happen with the patch from here applied.

Comment 2

[Benjamin Smedberg]

Comment on attachment 390442 [details] [diff] [review]
patch

I can't accept this without a test; I'm a little confused about the scenario to begin with: the administrator is setting a locked preference via what means? Is it a default pref or a userpref?

Comment 3

[Mike Hommey [:glandium]]

(In reply to comment #2)
> (From update of attachment 390442 [details] [diff] [review])
> I can't accept this without a test; I'm a little confused about the scenario to
> begin with: the administrator is setting a locked preference via what means? Is
> it a default pref or a userpref?

On Debian and Ubuntu (iirc), the administrator would be setting a locked preference in the standard preferences, cf. bug 440908. On other cases, he/she would be setting the locked preference in the general.config.filename pointed file. cf. https://developer.mozilla.org/en/Automatic_Mozilla_Configurator/Locked_config_settings

For a test, I have absolutely no idea how this can be tested through the test framework...

Comment 4

[Mike Hommey [:glandium]]

Attachment: Same patch, with a test

The test is derived from modules/libpref/test/unit/test_bug345529.js so I took the license text (public domain) from there.

Comment 5

[Benjamin Smedberg]

Comment on attachment 437278 [details] [diff] [review]
Same patch, with a test

Woohoo, somebody other than me willing to review libpref stuff.

Comment 6

[[e-mail]]

Comment on attachment 437278 [details] [diff] [review]
Same patch, with a test

Looks OK. I'm really not sure how this will interact with existing uses, but the change in behavior seems sane.

r=dwitte

Comment 7

[Mike Hommey [:glandium]]

http://hg.mozilla.org/mozilla-central/rev/b723f72611ea