438925 - localized URL prefs in distribution.ini not being interpreted properly

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Kev Needham [:kev]]

Attachment: distribution.ini for testcase

When localized pref URLs are used in the distribution.ini file, the value of the pref is reformatted  as "data:text/plain,pref.name=pref.value". As an example, if 

startup.homepage_welcome_url="http://%locale%.www.mozilla.com/%locale%/add-ons/ebay/extension/firstrun" 

is specified in the localized prefs section, the value of the pref startup.homepage_welcome appears in the en-US build of fx3 as data:text/plain,startup.homepage_welcome_url=http://en-US.www.mozilla.com/en-US/add-ons/ebay/extension/firstrun

On startup, fx loads the entire pref value as the URL, not just the URL. Gavin mentioned that bug 386696 may be responsible for this.

Steps to reproduce:

- Create a "distribution" directory off the application directory
- Copy the "distribution.ini for testcase" attachment to the distribution
directory
- Start Firefox3 with a new profile

Expected results: Firefox 3 appearing with a customized firstrun page and start page

Actual results: Firefox 3 appears with a tab that tries to load data:text/plain,startup.homepage_welcome_url=http://en-US.www.mozilla.com/en-US/add-ons/ebay/extension/firstrun as a URL and the start page (see attached fx3_startup image)

Comment 1

[Kev Needham [:kev]]

Attachment: fx3_startup image

Comment 2

[Dan Mills [:thunder]]

Note: bug 386696 may be related.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

How do you decide whether to use the data: hack for a given pref? Shouldn't you just avoid using it for that pref, given that it is retrieved using nsIURLFormatter::formatURLPref (which prefers getCharPref) rather than being retrieved using getComplexValue(..., nsIPrefLocalizedString)?

Comment 4

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #3)
> (which prefers getCharPref)

Post-bug 386696, of course - I see why that patch broke this, but if I'm not mistaken the solution can just be "stop using the data: URI hack for that pref".

Comment 5

[Dan Mills [:thunder]]

There are two "flavors" of prefs in the distribution.ini format, "Global" and "Localizable" prefs.  Global ones can't be overridden per-locale and do not get the data: hack.  Localizable ones do.

Not adding the data: hack to this pref seems like the way to go, yes.  We will need to either hardcode this pref into the repack tool (since we want it to be localizable, but not get the data: hack), or we'll need to redesign how prefs work in the INI to deal with this problem in a more general way.

It would be really nice if the distribution code could know at runtime how the pref would be accessed and set it in the correct fashion.  But I guess that's a pipe dream at this point.

Comment 6

[Rob Campbell [:rc] (:robcee)]

Attachment: removed data: hack from distribution.js

tested with included testcase.

Comment 7

[Dan Mills [:thunder]]

Perhaps I didn't understand the problem correctly.  I was under the impression that the data: hack was still required for *some* localizable preferences.  Is that not true?

Comment 8

[Rob Campbell [:rc] (:robcee)]

yeah, it appears so. I was talking with Gavin and Kev in IRC after I posted this and it looks like this won't cut it. It's possible if the prefs that get created by the repack tool are limited to a certain set, but I don't know what that set is yet. Withdrawing the review for now.

Comment 9

[Dan Mills [:thunder]]

I think the right way to go is to change the distribution.ini format to separate out somehow which prefs require the data: hack and which ones don't.

Personally, I think this should be metadata about the pref itself, which anyone could query from the prefs manager (this is what I was getting at in the last paragraph of comment #5).  Unfortunately, the reality is that some prefs look and smell the same, but need to be set differently.  So we have to expose that fact to distribution.ini authors.

Exactly how we expose that is not clear to me.

Comment 10

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #9)
> I think the right way to go is to change the distribution.ini format to
> separate out somehow which prefs require the data: hack and which ones don't.

After talking with Gavin and with Kev, this was my assessment too. I asked for a canonical list of prefs that could be generated from the distribution tool, but have yet to receive it. It's a hard problem without knowing which prefs are localized and which are not and would require some data-mining to discover. Also, I think hard-coding a matrix into the distribution.js code would be problematic and require updating everytime a pref was changed or localized. IOW, we'd have to update packaging code to do some code generation.

> Personally, I think this should be metadata about the pref itself, which anyone
> could query from the prefs manager (this is what I was getting at in the last
> paragraph of comment #5).  Unfortunately, the reality is that some prefs look
> and smell the same, but need to be set differently.  So we have to expose that
> fact to distribution.ini authors.

That could be another way of doing it. Provide some descriptor for the prefs so we can do a lookup at runtime.

> Exactly how we expose that is not clear to me.

Me neither. This bug is going to require some further research to complete. Throwing it back in the pool for now.

Comment 11

[Kev Needham [:kev]]

part of the problem is there is no list of definitive variables.

I thought the approach we would take would be more along the lines of 

[LocalizablePrefs]
browser.startup.homepage=datahack:"<prefvalue">

or something similar. basically anything with the datahack: predicate gets interpreted with the fix. because it's only in the distribution.ini file that this would ever be checked, and would only be applied to prefs listed in the LocalizablePrefs section, I'm hoping any risk/confusion would be mitigated by documentation and established syntax.

the other method thunder suggested was to create a second entry for every localizable pref such as:

[LocalizablePrefs]
browser.startup.homepage="<prefvalue">
browser.startup.homepage.datahack=true

Either/or works, and isn't constrained by a list of prefs.

I'll try and get the prefs list put together today. 

Comment 12

[Dan Mills [:thunder]]

(In reply to comment #10)

> After talking with Gavin and with Kev, this was my assessment too. I asked for
> a canonical list of prefs that could be generated from the distribution tool,
> but have yet to receive it.

distribution.ini can (and needs to be able to) set any pref, existing or currently unset.  There is no set list of prefs.

> It's a hard problem without knowing which prefs are
> localized and which are not and would require some data-mining to discover.

We already expose the localizable/non-localizable difference to the distribution.ini author (they muse use prior knowledge to know where to put the pref, in [GlobalPrefs] or [LocalizablePrefs]).

The problem is that (as I understand it) localizable prefs are now further classified into those that are always localizable, and those that are only localizable when the non-localized value of the pref is a chrome uri to a properties file (see bug 386696).  Since distribution.js never generates a properties file, these new "sometimes localizable" prefs are effectively only settable in the [GlobalPrefs] section of distribution.ini.

> Also, I think hard-coding a matrix into the distribution.js code would be
> problematic and require updating everytime a pref was changed or localized.
> IOW, we'd have to update packaging code to do some code generation.

Yes, hard-coding a matrix is a bad idea.  That's why I think we should expose this decision to the distribution.ini author somehow.

There are the options that Kev described in comment #11, plus one:

[GlobalPrefs]
foo=bar

[LocalizablePrefs]
bar=baz

[LocalizablePrefs-es_VE]
foo=fred
bar=barney

That is, allow 'global' prefs to be overridden in LocalizablePrefs locale sections.  This makes them ... not global.  But basically it means we'll dynamically set that pref on a per-locale basis _without_ assuming it will be interpreted as a localizable pref (that is, we won't use the data: hack).

It might be best to rename GlobalPrefs to NonLocalizablePrefs, perhaps.  Or something else, I'm open to suggestions.

Comment 13

[Dan Mills [:thunder]]

Here's another option:  modify the regex in the patch from bug 386696 to something like:

/^(data:text\/plain,.+=.+|chrome:\/\/.+\/locale\/.+\.properties)$/

But I don't really fully understand what possible side-effects that might have.

Comment 14

[Samuel Sidler (old account; do not CC)]

Can we get an owner here? This bug blocks the next release.

Comment 15

[Mike Kaply [:mkaply]]

The fix in bug 386696 should have been to fix the problem in Thunderbird, not to change the way complex prefs work (and have worked for many years).

I'd rather see a backout of 386696 and a fix for the thunderbird problem.

Note that I believe the fix for 386696 also broke some stuff on Red Hat as well and they put some ugly hacks in their Firefox to work around the 386696 bustage.

Comment 16

[Mike Kaply [:mkaply]]

And incidentally, bug 386698 removed the need for the patch in bug 386696.

Bug 386698 fixed the problem the correct way - in Thunderbird.

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

So is the expedient fix here to simply backout bug 386696? Can we be assured that fix isn't being relied upon by something else?

Comment 18

[Mike Kaply [:mkaply]]

(In reply to comment #17)
> So is the expedient fix here to simply backout bug 386696?

Yes


> Can we be assured
> that fix isn't being relied upon by something else?

Not assured, but I doubt it. Complex pref URLs have worked that way for years, and I can't picture someone discovering that new behavior and deciding to use it that late in the game.

Easiest way to check is to go through about:config and check for URLs as default values and verify that they are not using complex pref code.

I say easiest but mxr is having a lot of trouble with pref searches for me right now...

Comment 19

[Dan Mills [:thunder]]

Attachment: back out bug 386696's patch

Backing out bug 386696's patch seems like the correct thing to do.  Here's a patch (-p1) to do that.

Requesting review from Benjamin since he's the toolkit owner, please let me know if there's a more appropriate person.

Comment 20

[:Gavin Sharp [email: [e-mail]]]

Backing out that patch would regress bug 378210, I think. What about adding a check for data: URIs to the urlformatter code?

Comment 21

[Mike Kaply [:mkaply]]

Bug 378210 is a different problem, and I don't think the other bug affected it.

Looking in mxr

http://mxr.mozilla.org/firefox/search?string=startup.homepage_welcome_url

startup.homepage_welcome_url is never a complex pref. 

The only way that other bug would have affected this is if startup.homepage_welcome_url was a complex pref.

The bug in 378210 is that browserContentHandler.js should not be using formatURLPref since startup.homepage_welcome_url is not a complex pref.

Comment 22

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 332236 [details] [diff] [review]
back out bug 386696's patch

It doesn't matter whether or not the pref is complex, what matters is how it is retrieved, and formatURLPref used to try to retrieve it as a localized pref first, which triggered the load of the http:// URI. Bug 378210 was about that http:// URI load hanging the app. The patch for bug 386696 made us avoid trying to load the http:// URI as a stringbundle unless the pref looked "localized" (i.e., fixing bug 378210), and kept formatURLPref usable regardless of the underlying pref type. I don't think we want to regress either of those, so that's why I think adding a check for data: is a better option than backing out the patch for bug 386696.

Comment 23

[Mike Kaply [:mkaply]]

Sorry, reading the code wrong. You're right about the 378210 fix.

Since this is a branch only fix, adding the data: check seems like the right thing to me.


In the long run, I think we should query the preference and call formatURL (we already have a pref service in this case)

It makes no sense to go through the trouble of formatURLPref when we know it's not a complex pref (and never will be).

formatURLPref is designed for things that could be complex prefs. If we know it's not, we shouldn't go through it.

Looking through the code, it appears we have very few complex prefs even left.

Maybe we need a bug to determine how much complex pref code is even needed anymore, since we don't expect locales to override these anymore.

Something like the CCK can override them all in a .js file, it doesn't need to use the complex prefs.

Comment 24

[Dan Mills [:thunder]]

Attachment: Modify URL formatter's complex pref detection regex

Comment 25

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 332592 [details] [diff] [review]
Modify URL formatter's complex pref detection regex

>diff -r 5fa7d9ddf7bf toolkit/components/urlformatter/src/nsURLFormatter.js

>     if (!PS.prefHasUserValue(aPref) &&
>-        /^chrome:\/\/.+\/locale\/.+\.properties$/.test(format)) {
>+        /^(data:text\/plain,.+=.+|chrome:\/\/.+\/locale\/.+\.properties)$/.test(format)) {

You can make this non-capturing ("?:" before "data:"). Please add a test to http://mxr.mozilla.org/seamonkey/source/toolkit/components/urlformatter/tests/unit/test_urlformatter.js as well?

Comment 26

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Is this ready for checkin? If not, what is needed?

Comment 27

[Dan Mills [:thunder]]

The test Gavin mentioned in comment #25 needs to be written.  This is on my TODO list... I'll bump up the priority.  Maybe tonight.

Comment 28

[Dan Mills [:thunder]]

Attachment: Modify URL formatter's complex pref detection regex v2

Updated patch with gavin's suggestion + test.  I'm building/testing now.

Comment 29

[Dan Mills [:thunder]]

Attachment: Modify URL formatter's complex pref detection regex v3

Fixes the test.  Should be ready to commit now.

Comment 30

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Dan: did this land?  It needs to. :)

Comment 31

[Dan Mills [:thunder]]

No, I didn't want to land and then promptly sleep... and now I'm blocking on bug 450637.

Comment 32

[Dan Mills [:thunder]]

Checking in src/nsURLFormatter.js;
/cvsroot/mozilla/toolkit/components/urlformatter/src/nsURLFormatter.js,v  <--  nsURLFormatter.js
new revision: 1.10; previous revision: 1.9
done
Checking in tests/unit/test_urlformatter.js;
/cvsroot/mozilla/toolkit/components/urlformatter/tests/unit/test_urlformatter.js,v  <--  test_urlformatter.js
new revision: 1.5; previous revision: 1.4

Comment 33

[Samuel Sidler (old account; do not CC)]

Comment on attachment 333720 [details] [diff] [review]
Modify URL formatter's complex pref detection regex v3

This technically needed approval from the 1.9.0.x drivers before landing. Retroactively approving...

Comment 34

[Dan Mills [:thunder]]

Oh? I asked Shaver over IM, and he told me if it was blocking+ I didn't need to request approval.  Sorry.

Comment 35

[Dan Mills [:thunder]]

I'm reopening this bug, since it hasn't been fixed on trunk yet.

Comment 36

[Dan Mills [:thunder]]

Pushed to mozilla-central now:

changeset:   18392:6837b9f4d0cc
user:        Dan Mills <[e-mail]>
date:        Mon Aug 25 14:21:10 2008 -0700
summary:     Bug 438925: recognize data: hack as localized prefs in formatURLPref. r=gavin