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

RESOLVED FIXED in mozilla13

Status

()

RESOLVED FIXED
7 years ago
9 months ago

People

(Reporter: sander, Assigned: mwu)

Tracking

({regression})

9 Branch
mozilla13
x86
Linux
regression
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

7 years ago
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.
Component: Preferences → Add-ons Manager
Product: Firefox → Toolkit
QA Contact: preferences → add-ons.manager
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.
Blocks: 533038
Status: UNCONFIRMED → NEW
Component: Add-ons Manager → Startup and Profile System
Ever confirmed: true
Keywords: regression
QA Contact: add-ons.manager → startup
Summary: existence of distribution/distribution.ini makes firefox ignore preferences set by add-ons → If distribution.ini is present then default preferences from packed extensions won't be used (reloading default prefs doesn't load from packed extensions)
(Assignee)

Comment 2

7 years ago
Created attachment 592842 [details] [diff] [review]
Put packed xpi extensions in the extension lists

Pushing to try, results should show up here.
Assignee: nobody → mwu

Comment 3

7 years ago
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/mwu@mozilla.com-bf8158aa8be5
(Assignee)

Comment 4

7 years ago
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

7 years ago
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/mwu@mozilla.com-e8b40afb7e4d

Comment 6

7 years ago
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/mwu@mozilla.com-408d68c3ef5d
 Timed out after 06 hours without completing.
(Assignee)

Updated

7 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 7

7 years ago
Created attachment 593684 [details] [diff] [review]
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.
Attachment #592842 - Attachment is obsolete: true
Attachment #593684 - Flags: review?(dtownsend+bugmail)

Comment 8

7 years ago
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/mwu@mozilla.com-5cb53a75dd07
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.
Attachment #593684 - Flags: review?(dtownsend+bugmail)
Attachment #593684 - Flags: review?(benjamin)
Attachment #593684 - Flags: review+

Comment 10

7 years ago
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?
Attachment #593684 - Flags: review?(benjamin) → review-
(Assignee)

Comment 11

7 years ago
Created attachment 595879 [details] [diff] [review]
Put packed xpi extensions in the extension lists, v3
Attachment #593684 - Attachment is obsolete: true
Attachment #595879 - Flags: review?(benjamin)
(Assignee)

Comment 12

7 years ago
(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.

Updated

7 years ago
Attachment #595879 - Flags: review?(benjamin) → review+
(Assignee)

Comment 13

7 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ffcf24db0f74
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/ffcf24db0f74
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
See bug 907321 which is a similar problem for AutoConfig.
You need to log in before you can comment on or make changes to this bug.