947838 - Autoconfig's defaultPref() has no effect when distribution.ini exists

Status: VERIFIED FIXED

(Firefox :: Distributions, defect)

Description

[Michael Weghorn]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux i686; rv:17.0) Gecko/20100101 Firefox/17.0 (Beta/Release)
Build ID: 20130116094752

Steps to reproduce:

I'm using Autoconfig to set values for preferences.
The Autoconfig file is included by creating the file "custom.js" in the directory "defaults/pref" in Firefox's installation directory with the following content:

// use Autoconfig
pref("general.config.obscure_value", 0);
pref("general.config.filename", "firefox.cfg");

The Autoconfig file "firefox.cfg" has the following content:

// without this setting, default value for "general.smoothScroll" is 'true'
defaultPref("general.smoothScroll", false);

Firefox's installation directory contains the directory "distribution". This directory contains a file "distribution.ini". (This is default in Ubuntu.)




Actual results:

When starting Firefox, the default value for "general.smoothScroll" is still 'true'.


Expected results:

The default value for "general.smoothScroll" should be set to 'false'.

If the file "distribution.ini" is deleted, the behaviour is as expected (the default value is set to 'false').
The preference "general.smoothScroll" is just an example. Any of the predefined preferences can be used.

If a new preference is defined in the Autoconfig file (e.g. using "defaultPref("test.pref", false);"), the default value is set to the given value.

lockPref() and pref() in the configuration file work fine.

The behaviour can be reproduced with the Firefox version provided on the Mozilla website when you create the "distribution" directory and a file "distribution.ini" (can be an empty file) in it.

Comment 1

[Loic]

C'ing Mike whos is aware of that.

Comment 2

[Mike Kaply [:mkaply]]

This is weird. And apparently it has been this way for a while (I checked Firefox 17 ESR).

Comment 3

[Mike Kaply [:mkaply]]

It's only a problem with defaultPref

pref and lockPref work fine.

And it is the simple presence of a distribution.ini, regardless of the content.

Comment 4

[Mike Kaply [:mkaply]]

This autoconfig;

//
Components.utils.import("resource://gre/modules/Services.jsm");
Services.prompt.alert(null, "Title", getPref("general.smoothScroll"));
defaultPref("general.smoothScroll", false);
Services.prompt.alert(null, "Title", getPref("general.smoothScroll"));

Shows that the pref is changed to false, but when you go to about:config, general.smoothScroll is set to true.

Comment 5

[Michael Weghorn]

I do not know whether they are related with each other, but bug #712789 sounds similar to this one.

Comment 6

[Mike Kaply [:mkaply]]

Yes, that is definitely helpful.

I'm moving this to major because this finally answers a question I've had for a while which is people constantly reporting that defaultPref doesn't work on Linux. Since most Linux distribution have a distribution.ini, this is a major problem there.

Mossop: Would love your input here.

Comment 7

[Michael Weghorn]

Another issue: Using "unlockPref()" in the Autoconfig file on settings which were locked in the "defaults/pref/custom.js" file does not work if the "distribution/distribution.ini" file is present (but works, when the file is not present).
This is independent of whether an already existing property (e.g. "general.smoothScroll") or a newly defined property (e.g. "test.property") is used.

example:

// content of "defaults/pref/custom.js"
lockPref("general.smoothScroll", false);
lockPref("test.property", false);

// content in "firefox.cfg"
unlockPref("general.smoothScroll")
unlockPref("test.property");

When the "distribution.ini" file is present, the properties are still locked. When the file is not present, the properties are unlocked.

Comment 8

[Mike Kaply [:mkaply]]

Come to find out, this isn't just related to autoconfig. It happens when any app messages with default preferences at startup.

Comment 9

[Mike Kaply [:mkaply]]

Sorry, that last comment wasn't clear.

This happens without AutoConfig.

If you listen to say "profile-after-change" and set default preferences in response to that, they are not applied if you have a distribution.ini.

Comment 10

[Mike Kaply [:mkaply]]

I'm taking this. I really need to get this figured out.

Comment 11

[Mike Kaply [:mkaply]]

Distribution code reinitializes the preference code which causes things to get lost.

Comment 12

[Jan Horak [:jhorak]]

Would you accept some patch for that? I would make something if you'd like.

Comment 13

[Mike Kaply [:mkaply]]

> Would you accept some patch for that? I would make something if you'd like.

We would, but in my research on it, it wasn't as simple as it seemed. We need some tests to make sure it doesn't break anything.

Comment 14

[R P Herrold]

(In reply to Mike Kaply [:mkaply] from comment #13)
> > Would you accept some patch for that? I would make something if you'd like.
> 
> We would, but in my research on it, it wasn't as simple as it seemed. We
> need some tests to make sure it doesn't break anything.

I have been reading bugs on AutoConfig most of a week, and note that there is a difference of parsing paths 
when a ./distribution/distribution.ini is present and when absent

This may be being reflected in bug:
    https://bugzilla.mozilla.org/show_bug.cgi?id=1191860
"Errors in Firefox AutoConfig no longer show an error popup"

and particularly comment #6 there, which notes a different result between local controlled builds (likely omitting the 'distribution.ini') and 'official ones, assumedly containing such a path and file


I happen to work in a distribution which omits this, so I cannot confirm or deny locally:

[herrold@centos-7 ~]$ strace /usr/lib64/firefox/firefox-bin 2>&1 | \
        grep "distribution.ini"
access("/usr/lib64/firefox/distribution/distribution.ini", F_OK) = -1 
        ENOENT (No such file or directory)
open("/usr/lib64/firefox/distribution/distribution.ini", O_RDONLY) = -1 
        ENOENT (No such file or directory)

============

My vendor has tested adding a distribution.ini and noticed unexpected results:
    https://bugzilla.mozilla.org/show_bug.cgi?id=1364455

============


The second 'axis' of difference' is the main thread / lazy thread behaviour possibly introducing timing indeterminancy as to when a change to a preference is expressed.  see Comment #3 on
    https://bugzilla.mozilla.org/show_bug.cgi?id=802022


This is not a formal 'race' condition per se, but rather how a 'lazy' parsing and application of settings sequence 
would be expressed

It sure looks like some 'barrier synchronization' is needed, to get deterministic application of preferences, 
possibly both as to applying: directory.ini scripting, and also the other components enumerated in bug #802022

also: I think this Bug's Subject line over-states the scope of effect of the presence of an distribution.ini ; it is not: "no" effect, but providing an unanticipated avenue for rather unexpected ones to enter analysis

Comment 15

[Thimo Emmerich]

I am not sure if I should prepare a new bug report on this or not since it seems tightly coupled.

Actually the presence - or more precisely: the readability - of distribution.ini also breaks the policies introduced in FF60 in case they are not locked.

Environment: Ubuntu 16.04.4, Firefox 60.0.2

How to confirm:
echo '{ "policies": { "Proxy": { "Mode": "autoDetect", "Locked" : false } } }' >/usr/lib/firefox/distribution/policies.json
touch /usr/lib/firefox/distribution/distribution.ini
chmod 644 /usr/lib/firefox/distribution/distribution.ini

start firefox and check value in about:preferences --> Network Proxy --> Setting... 
Actual Result:
"Use system proxy settings"
Expected Result:
"Auto-detect proxy ..."

Cross-check:
close firefox
chmod 000 /usr/lib/firefox/distribution/distribution.ini
start firefox
Actual & Expected Result:
"Auto-detect proxy ..."

Another way to enforce this is using "Locked" : true.

Since this is a highlighted feature of FF60 and the distribution.ini seems somehow common, this issue might affect several people.

Best regards,
 Thimo Emmerich

Comment 16

[Mike Kaply [:mkaply]]

I'm working on this one since it impacts policies as well.

Comment 17

[Mike Kaply [:mkaply]]

There's some weird stuff here.

So the preferences are supposed to be set via _onAppDefaults here:

https://searchfox.org/mozilla-central/source/browser/components/nsBrowserGlue.js#711

which is called when nsBrowserGlue.js receives the prefservice:after-app-defaults notification from the pref service.

https://searchfox.org/mozilla-central/source/browser/components/nsBrowserGlue.js#428

But that notification never happens.

Instead, we end up going down the applyCustomizations route here:

https://searchfox.org/mozilla-central/source/browser/components/nsBrowserGlue.js#728

which ends up forcing a reload of default prefs here:

https://searchfox.org/mozilla-central/source/browser/components/distribution.js#250

which is what causes the problems.

I tried simply calling applyPrefDefaults instead of reloading default prefs and that seemed to work, but I need to do more research into what it could break, in particular the original bug that was the reason the second load was added - bug 516444.

I'm still trying to figure out why the after-app-defaults notification never happens. I've contacted Nicholas Nethercote about that one.

Comment 18

[Mike Kaply [:mkaply]]

The after-app-defaults notifications happens before nsBrowserGlue even registers it's observers.

So the way this is architected won't work at all.

Comment 19

[Mike Kaply [:mkaply]]

Attachment: Bug 947838 - Don't reload distro prefs, just load at the same time as customizations.

Review commit: https://reviewboard.mozilla.org/r/253560/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/253560/

Comment 20

[Mike Kaply [:mkaply]]

Florian:

I picked you to review this because you understand some of the Firefox internals here.

I tested this patch with our existing distribution.ini files and everything worked fine.

And it fixes the existing bugs with autoconfig and policy coexistence.

Comment 21

[Mike Kaply [:mkaply]]

Comment on attachment 8988297 [details]
Bug 947838 - Don't reload distro prefs, just load at the same time as customizations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253560/diff/1-2/

Comment 22

[Code Review Bot [:reviewbot]]

Comment on attachment 8988297 [details]
Bug 947838 - Don't reload distro prefs, just load at the same time as customizations.

https://reviewboard.mozilla.org/r/253560/#review260136


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/nsBrowserGlue.js:428
(Diff revision 1)
>      this.pingCentre.sendPing(payload, options);
>    },
>  
>    // nsIObserver implementation
>    observe: async function BG_observe(subject, topic, data) {
> +    Components.utils.reportError(topic);

Error: Use cu rather than components.utils [eslint: mozilla/use-cc-etc]

Comment 24

[Florian Quèze [:florian]]

Comment on attachment 8988297 [details]
Bug 947838 - Don't reload distro prefs, just load at the same time as customizations.

https://reviewboard.mozilla.org/r/253560/#review260640

Looks reasonable.

In comment 17 you were still wondering why the second load was introduced in bug 516444; have you figured it out?

::: browser/components/distribution.js:221
(Diff revision 2)
>      }
>    },
>  
>    _newProfile: false,
>    _customizationsApplied: false,
>    applyCustomizations: function DIST_applyCustomizations() {

It seems we could just call this.applyPrefDefaults(); here, as both calls to applyCustomizations are now preceded by a call to applyPrefDefaults.

Comment 25

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] from comment #24)

Also, please fix the typo in the commit message.

Comment 26

[Mike Kaply [:mkaply]]

Comment on attachment 8988297 [details]
Bug 947838 - Don't reload distro prefs, just load at the same time as customizations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253560/diff/2-3/

Comment 27

[Mike Kaply [:mkaply]]

> It seems we could just call this.applyPrefDefaults(); here, as both calls to applyCustomizations are now preceded by a call to applyPrefDefaults.

That's what I've done and it fixed the other issue I was seeing where the "showPersonalToolbar" pref didn't work.

Also, I've verified the original scenario in bug 516444 - all bookmarks are added correctly.

Comment 28

[Florian Quèze [:florian]]

(In reply to Mike Kaply [:mkaply] from comment #27)
> > It seems we could just call this.applyPrefDefaults(); here, as both calls to applyCustomizations are now preceded by a call to applyPrefDefaults.
> 
> That's what I've done and it fixed the other issue I was seeing where the
> "showPersonalToolbar" pref didn't work.

Can you explain a bit how it fixed that showPersonalToolbar pref? How was it misbehaving before, and how is it working better now? Or what's the expected behavior.

To match the previous behavior I expected the applyPrefDefaults call to be at the beginning of the applyCustomizations method rather than at the end. Especially, is the interaction with these lines fine? https://searchfox.org/mozilla-central/rev/ed2763bea882619ccb48b0aecc54e523d2bdd2ae/browser/components/distribution.js#432-434,438-440,448

Comment 29

[Mike Kaply [:mkaply]]

> Can you explain a bit how it fixed that showPersonalToolbar pref? How was it misbehaving before, and how is it working better now? Or what's the expected behavior.

checkCustomizationsComplete depends on the value of newProfile which is set at the beginning of applyCustomizations:

https://searchfox.org/mozilla-central/source/browser/components/distribution.js#224

checkCustomizationsComplete will then get called at the end of applyPrefDefaults:

https://searchfox.org/mozilla-central/source/browser/components/distribution.js#422

(It also gets called in one other places - https://searchfox.org/mozilla-central/source/browser/components/distribution.js#241)

The problem with the code before is that applyPrefDefaults was being called before applyCustomizations, so newProfile wasn't set.

It needs to be called at the end of applyCustomizations.

So the previous behavior was wrong, but only worked because we were reloading all of the prefs.

Comment 30

[Florian Quèze [:florian]]

Comment on attachment 8988297 [details]
Bug 947838 - Don't reload distro prefs, just load at the same time as customizations.

https://reviewboard.mozilla.org/r/253560/#review261260

Comment 31

[Mozilla Autoland]

hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 2 changes to 2 files
remote: 
remote: 
remote: ************************** ERROR ****************************
remote: Rev 0c8d2ce33e17 needs "Bug N" or "No bug" in the commit message.
remote: Michael Kaply <[e-mail]>
remote: Bug #947838 - Don't reload distro prefs, just load at the same time as customizations. r=florian
remote: 
remote: MozReview-Commit-ID: 6GlC9xIwY9h
remote: *************************************************************
remote: 
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.c_commitmessage hook failed
abort: push failed on remote

Comment 32

[Mike Kaply [:mkaply]]

Comment on attachment 8988297 [details]
Bug 947838 - Don't reload distro prefs, just load at the same time as customizations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253560/diff/3-4/

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7b54fb09d5f9
Don't reload distro prefs, just load at the same time as customizations. r=florian

Comment 34

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/7b54fb09d5f9

Comment 35

[Martin Stránský [:stransky] (ni? me)]

Mike, do you mind to nominate it for ESR60?

Comment 36

[Mike Kaply [:mkaply]]

Comment on attachment 8988297 [details]
Bug 947838 - Don't reload distro prefs, just load at the same time as customizations.

Approval Request Comment
[Feature/Bug causing the regression]: Distribution.ini can't coexist with policies
[User impact if declined]:
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Yes. Steps are in the bug
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Low
[Why is the change risky/not risky?]: Patch only affects partner builds if any. Has been thoroughly tested.
[String changes made/needed]: None.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined:  Distribution.ini can't coexist with policies (bug problem on Linux)
Fix Landed on Version: 63
Risk to taking this patch (and alternatives if risky): Risk is primarily around partner builds which we don't do on the ESR.
String or UUID changes made by this patch: 

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 37

[Liz Henry (:lizzard) (relman/hg->git project)]

This sounds like a really good thing to fix!  I wonder if various distros have created workarounds for some of the issues you mention.

While you say this has been thoroughly tested, I worry that the developer and the tester should not be the same. 

Is there some way we can better plan to test partner builds (since in the past, we have done last minute RC week uplifts and I believe also dot releases, for partner build issues)   Or, is part of the idea here that the faster we get this fix into beta, the more likely partner QE is to detect potential problems before release?

I see STR for the original issue in comment 0.  It looks like there are other bugs and issues mentioned here that might be worth testing / verifying as well since the underlying issue turned out to be complex.  What do you think about putting in a PI request here?

Comment 38

[Mike Kaply [:mkaply]]

> I wonder if various distros have created workarounds for some of the issues you mention.

What usually happens is enterprises have to delete the distribution.ini file in order to get their stuff to work. We didn't realize it was affecting the new policies code as well.

> Or, is part of the idea here that the faster we get this fix into beta, the more likely partner QE is to detect potential problems before release?

That's defintitely the idea. We build betas of partner builds, so once the fix is in, we can start testing partner builds with the next beta.

And I agree with the test statement. I'll put in a PI request.

Comment 39

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8988297 [details]
Bug 947838 - Don't reload distro prefs, just load at the same time as customizations.

Let's try this for beta 9 and see how the partner testing goes.

Comment 40

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/95d66abccf63

Comment 41

[Cristian Craciun]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:62.0) Gecko/20100101 Firefox/62.0
Build ID: 20180713213322

Verified as fixed on Nightly build 63.0a1 (2018-07-16) and Beta 62.0b9 on Ubuntu 16.04 LTS.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

Has there been any partner testing done with Beta builds that you're aware of, Mike?

Comment 43

[Mike Kaply [:mkaply]]

Not that I'm aware of, no. I'll ping some partners to get them to test.

I'd rather not do a PI request at this point as we have enough going on with them.

Comment 44

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8988297 [details]
Bug 947838 - Don't reload distro prefs, just load at the same time as customizations.

From talking to Mike, it sounds like this has gotten about as much feedback as it's going to get at this point. Taking it for ESR 60.2 so it's in the builds I send to the enterprise list for wider feedback.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/dff964b7a5e6