386696 - nsIURLFormatter::formatURLPref causes chrome files to be parsed as property files

Status: RESOLVED FIXED

(Toolkit :: Preferences, defect)

Description

[Scott MacGregor]

I couldn't find a component in toolkit for General or for the url formatter service so I picked a random component :)

In nsIURLFormatter::formatURLPref:
http://mxr.mozilla.org/seamonkey/source/toolkit/components/urlformatter/src/nsURLFormatter.js#120

we first try to get the pref as a localized string and if that fails, we try to get the pef as a nsISupportsString.  But the attempt to get the pref as a localized string  has an unintended side effect. The pref service takes the value of the passed in pref, loads it as a string bundle and then looks up the string in the bundle. this doesn't work so well when the passed in pref name isn't a localized pref string.

I noticed this because in Thunderbird, I've got a pref with the following value:
DEBUG builds:
pref("mailnews.start_page.url","chrome://messenger/content/start.xhtml");
branded builds:
pref("mailnews.start_page.url", "http://%LOCALE%.www.mozilla.com/%LOCALE%/%APP%/%VERSION%/start/");

In the first case, we end up treating start.xhtml as a string property file and I can see a bunch of warnings on the console where the string bundle service is struggling to load it. 

I wonder if it's possible the string bundle service would try to load a remote http page as a string bundle if we pass a http url to formatURLPref. I haven't tested that out yet thought.

Comment 1

[Phil Ringnalda (:philor)]

Your random General (see bug 350204) is even worse than mine: at least I make it down to the X's. Though in this case, Prefs seems closer.

Comment 3

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

(In reply to comment #0)
> I wonder if it's possible the string bundle service would try to load a remote
> http page as a string bundle if we pass a http url to formatURLPref. I haven't
> tested that out yet thought.

Indeed, that's essentially the cause of bug 378210 (the comments there describe essentially the same problem you're describing). 

Comment 4

[[e-mail]]

Attachment: Proposed patch

This is based on the code in config.js but I added a $ to the regexp ;-)
(my fault for leaving it out of config.js, sigh...)

Comment 5

[Dietrich Ayala (:dietrich)]

Comment on attachment 295915 [details] [diff] [review]
Proposed patch

>Index: toolkit/components/urlformatter/src/nsURLFormatter.js
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/urlformatter/src/nsURLFormatter.js,v
>retrieving revision 1.6
>diff -u -p -r1.6 nsURLFormatter.js
>--- toolkit/components/urlformatter/src/nsURLFormatter.js	7 Jan 2008 10:44:54 -0000	1.6
>+++ toolkit/components/urlformatter/src/nsURLFormatter.js	8 Jan 2008 10:26:22 -0000
>@@ -94,17 +94,20 @@ formatURLPref: function uf_formatURLPref(aPref) {
>              getService(Ci.nsIPrefBranch);
> 
>     try {
>-      format = PS.getComplexValue(aPref, Ci.nsIPrefLocalizedString).data;
>-    } catch(ex) {}
>+      format = PS.getComplexValue(aPref, Ci.nsISupportsString).data;
>+    } catch(ex) {
>+      Cu.reportError("formatURLPref: Couldn't get pref: " + aPref);
>+      return "about:blank";
>+    }
> 
>-    if (!format) {
>+    if (!PS.prefHasUserValue(aPref) &&
>+        /^chrome:\/\/.+\/locale\/.+\.properties$/.test(format)) {
>+      // This looks as if it might be a localised preference

nit: s/localised/localized/

unless that's the proper spelling in your locale :)

r=me.

Comment 6

[[e-mail]]

(In reply to comment #5)
> nit: s/localised/localized/
> 
> unless that's the proper spelling in your locale :)
Yeah, it's localised ;-)

Comment 7

[Mike Connor [:mconnor]]

Comment on attachment 295915 [details] [diff] [review]
Proposed patch

a=mconnor on behalf of drivers for 1.9 checkin

Comment 8

[[e-mail]]

Fix checked in.

Comment 9

[Dan Mills [:thunder]]

There is some discussion that this checkin might be the cause for bug 438925.  We localize pref values *directly* for partner builds (we do so dynamically at startup), so for example the value for the 'startup.homepage_welcome_url' might end up looking like this:

data:text/plain,startup.homepage_welcome_url=http://en-US.www.mozilla.com/en-US/add-ons/ebay/extension/firstrun

If I understand the patch above correctly, since this value doesn't look like a chrome properties file we will attempt to load the whole thing as the URL, which is exactly what we are seeing in bug 438925.