712789 - If distribution.ini is present then default preferences from packed extensions won't be used (reloading default prefs doesn't load from packed extensions)

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[Sander Plas]

The version of Firefox shipped with Ubuntu & Mint "ignores" preferences set by add-ons. 

The problem seems to be caused by the file "distribution/distribution.ini" in the firefox directory. If i remove that file from Ubuntu Firefox directory, the problem disappears. 

If i add that file to the generic Firefox from mozilla.com, the problem also appears in that version. The file can even be empty - just its existence seems to cause the problem.  

I am not 100% sure what distribution.ini is supposed to do, but i suspect that this is not the intended behaviour? 

To reproduce: 

- Start with a "clean" firefox installation from mozilla.com

- go to "about:config" and look at the "browser.tabs.insertRelatedAfterCurrent" setting. If you haven't modified this, it should be set to "true". 

- install the Classic Opera add-on from https://addons.mozilla.org/en-US/firefox/addon/classic-opera/ & restart firefox

- look at browser.tabs.insertRelatedAfterCurrent again - it should be set to "false" now. 

- now create a directory called "distribution" in the firefox directory an create an empty file called "distribution.ini" in that directory

- restart firefox

- look at browser.tabs.insertRelatedAfterCurrent again - it's suddenly back on "true", which is the default value from before the add-on was installed.

Comment 1

[Dave Townsend [:mossop]]

This will have regressed from the packed XPI landing for Firefox 4. Distribution customizations work by recreating the default preferences shortly after startup (by signalling the pref service with "reload-default-prefs") and applying some new defaults while that is happening. Unfortunately the pref service doesn't know how to load default preferences from packed XPIs directly, instead we have to tell it to do so in nsXREDirProvider at the time we scan extensions.ini, so all those prefs get lost by this reload.

This is likely breaking most extensions installed in partner builds.

Comment 2

[Michael Wu [:mwu]]

Attachment: Put packed xpi extensions in the extension lists

Pushing to try, results should show up here.

Comment 3

[Mozilla RelEng Bot]

Try run for bf8158aa8be5 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=bf8158aa8be5
Results (out of 43 total builds):
    exception: 30
    success: 10
    warnings: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-bf8158aa8be5

Comment 4

[Michael Wu [:mwu]]

This try run result and the next one upcoming one are bad since I used the tip of m-c and assumed that m-c is generally green. Which it's not, since it's the train is about to leave.

Comment 5

[Mozilla RelEng Bot]

Try run for e8b40afb7e4d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=e8b40afb7e4d
Results (out of 14 total builds):
    exception: 10
    failure: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-e8b40afb7e4d

Comment 6

[Mozilla RelEng Bot]

Try run for 408d68c3ef5d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=408d68c3ef5d
Results (out of 209 total builds):
    exception: 1
    success: 185
    warnings: 23
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-408d68c3ef5d
 Timed out after 06 hours without completing.

Comment 7

[Michael Wu [:mwu]]

Attachment: Put packed xpi extensions in the extension lists, v2

This one seems to work for me. I've pushed it to try and results will show up here.

I don't have any good ideas for how to write an automated test for this though. Getting the directory service to reread the extensions list appears to require an actual restart.

Comment 8

[Mozilla RelEng Bot]

Try run for 5cb53a75dd07 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5cb53a75dd07
Results (out of 207 total builds):
    success: 187
    warnings: 19
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-5cb53a75dd07

Comment 9

[Dave Townsend [:mossop]]

Comment on attachment 593684 [details] [diff] [review]
Put packed xpi extensions in the extension lists, v2

This looks good to me but I think bsmedberg probably needs to do a pass on it.

Comment 10

[Benjamin Smedberg]

Comment on attachment 593684 [details] [diff] [review]
Put packed xpi extensions in the extension lists, v2

In LoadDirsIntoArray, why add the IsDirectory check? We have in the past said that it's more expensive to check whether something is a directory (which involves a stat) than to just try and open it later later.

Also I presume you intend to remove the printf.

In LoadExtensionDirectories, there is no `aType` check to make sure that we're only loading default pref files for extensions and not themes (that codepath is shared). That seems like a serious preexisting bug, but you should fix it while you're here (and perhaps make an ESR patch for just that issue).

The net effect of this change is to change the meaning of XRE_EXTENSIONS_DIR_LIST. You need to update its documentation. Did you audit the clients of that entry to make sure they all are expecting .xpi files and not just directories?

Comment 11

[Michael Wu [:mwu]]

Attachment: Put packed xpi extensions in the extension lists, v3

Comment 12

[Michael Wu [:mwu]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #10)
> Comment on attachment 593684 [details] [diff] [review]
> Put packed xpi extensions in the extension lists, v2
> 
> In LoadDirsIntoArray, why add the IsDirectory check? We have in the past
> said that it's more expensive to check whether something is a directory
> (which involves a stat) than to just try and open it later later.
> 
All IsDirectory calls removed.

> Also I presume you intend to remove the printf.
> 
Oops. Removed.

> In LoadExtensionDirectories, there is no `aType` check to make sure that
> we're only loading default pref files for extensions and not themes (that
> codepath is shared). That seems like a serious preexisting bug, but you
> should fix it while you're here (and perhaps make an ESR patch for just that
> issue).
> 

Well, this patch changes pref loading so that it isn't done in LoadExtensionDirectories anyway so no check is necessary.

> The net effect of this change is to change the meaning of
> XRE_EXTENSIONS_DIR_LIST. You need to update its documentation. Did you audit
> the clients of that entry to make sure they all are expecting .xpi files and
> not just directories?

I've checked all the affected users and it looks like there shouldn't be any regressions in their behavior if they hit xpi files. Also, the documentation for XRE_EXTENSIONS_DIR_LIST has been updated.

Comment 13

[Michael Wu [:mwu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ffcf24db0f74

Comment 14

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/ffcf24db0f74

Comment 15

[Mike Kaply [:mkaply]]

See bug 907321 which is a similar problem for AutoConfig.