1632279 - Default preferences should not be settable via prefs.js in the user profile directory

Status: RESOLVED FIXED

(Core :: Preferences: Backend, defect)

Description

[Mike Kaply [:mkaply]]

If you add this line:

pref("a.b.c", "d.e.f");

to prefs.js in the profile directory, you end up with a default preference.

prefs.js should only be able to set user prefs via user_pref.

It should not allow the setting of default prefs.

Comment 1

[Mike Kaply [:mkaply]]

That same should be true of user.js. It should not support pref(), only user_pref

Comment 2

[:Gijs (out until Jan 5th; he/him)]

Unless you take bugs, please do not assign P1, because we never see these bugs in triage.

Have you tried running with mozregression? Do the prefs persist in prefs.js after a restart?

Comment 3

[Mike Kaply [:mkaply]]

(In reply to :Gijs (he/him) from comment #2)

Unless you take bugs, please do not assign P1, because we never see these bugs in triage.

Sorry about that.

Have you tried running with mozregression? Do the prefs persist in prefs.js after a restart?

Pref does not persist. I'll run mozregression.

Comment 4

[Mike Kaply [:mkaply]]

You can also set default prefs with user.js which I don't think used to be a thing. I'm checking into that with mozregression as well

Comment 5

[Mike Kaply [:mkaply]]

Not new at all. Works this way in Firefox 45. I'm shocked someone has just now figured out this works considering it's been around forever.

Comment 6

[:Gijs (out until Jan 5th; he/him)]

Pretty sure this is actually something to do with the pref parser.

Comment 7

[Nicholas Nethercote [inactive]]

The prefs won't persist in prefs.js because prefs.js is frequently overwritten.

More generally, the behaviour described has been like that forever, as far as I know. I rewrote the prefs parser in Firefox 60, but AFAIK the old parser didn't have any concept of "default pref syntax" vs. "user pref syntax". Fortunately, the new parser does have such a concept, and it should be pretty easy to change things so that pref and sticky_pref are rejected in user pref files, i.e. prefs.js and user.js.

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: Bug 1632279 - Disallow default pref definitions in user pref files. r=KrisWright

We distinguish between two kinds of pref syntax.

• "Default pref files" are the ones that come with Firefox, constructed from
  all.js and similar files.
• "User pref files" are the ones that get created in the user's profile.
  prefs.js is the one that Firefox creates and overwrites every time a pref
  changes. user.js is the one that users can create themselves.

We also have two basic kinds of pref.

• Default: pref(...) and the unfortunate sticky_pref(...).
• User: user_pref(...), which override but don't replace the default.

It only makes sense for user pref files to contain user prefs; users shouldn't
be able to create default prefs or change default pref values.

But it turns out that user pref files have been able to define default prefs
pretty much forever. This appears to be an oversight, and this commit restricts
things so that user pref files cannot contain default prefs.

The commit also fixes an incorrect comment in testParser.js.

Comment 9

[Nicholas Nethercote [inactive]]

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2b0f0e3226785e3490317030090915b6adf9dcf

Comment 10

[Mike Kaply [:mkaply]]

Thanks for doing this so quick. I had a feeling it would be easy with your new architecture.

And this definitely only affects user.js and prefs.js? Not defaults/prefs/*.js?

Comment 11

[Nicholas Nethercote [inactive]]

And this definitely only affects user.js and prefs.js? Not defaults/prefs/*.js?

As far as I can tell, yes. There are more code paths in to the file loading stuff than I'd like, but I checked all the ones I could find. It's conceivable I missed an obscure one, but if any of the main default pref files were affected the try push would have lit up like a Christmas tree.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/305747d9db4c
Disallow default pref definitions in user pref files. r=KrisWright

Comment 13

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/305747d9db4c

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d607997b4a52
adjust comm/mailnews/addrbook/test/unit/test_bug534822.js to use user_pref not pref. rs=test-bustage-fix DONTBUILD

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b608f0bbdacd
followup to fix linting . rs=eslint