localizable general.useragent.locale pref shipped by ubufox breaks mozilla OS usage stats (ubuntu not counted)

sorry ... the result url had a line break:

https://addons.mozilla.org/blocklist/2/%7Bec8030f7-c20a-464f-9b0e-13a3a9e97384%7D/3.0.4/Firefox/2008111319/Linux_x86_64-gcc3/chrome://global/locale/intl.properties/default/Linux%202.6.27-7-generic%20(GTK%202.14.4)/canonical/1.0/

fwiw, its an ubuntu only issue. we ship a special pref since i took over the package ... we can drop that if you dont want to fix it in blocklist code.

Why is general.useragent.locale a localized pref in your builds? browser/app/profile/firefox.js and browser/locales/en-US/firefox-l10n.js explicitly set it to a non-localized value, and this isn't the only piece of code that assumes it's a normal charpref. In fact all.js seems to be the only one who thinks it can end up localized, so the easiest fix is probably to just fix it to use @AB_CD@ as well.

Ah, sorry, missed your comment. I think you should just remove that pref, yeah (and we should fix all.js too).

remove blocking from stable branches in turn of comment 2

(In reply to comment #4)
> Ah, sorry, missed your comment. I think you should just remove that pref, yeah
> (and we should fix all.js too).

ok, will prepare a patch for that.

I would expect this to take a patch like in bug 446527 to really work independent of user settings.

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/urlformatter/src/nsURLFormatter.js is worth a look, too. Sadly, one can't pass in extra variables. Not sure if some of the blocklist variables would be worth adding.

(In reply to comment #7)
> I would expect this to take a patch like in bug 446527 to really work
> independent of user settings.
I believe that the difference between this and bug 446527 is that we want the currently running locale vs. the installation locale.

I wonder if we really mind in the end. In which scenario would be blacklist an extension for just the currently selected locale?

Maybe just making sure that the actual char prefs are properly escaped so that "/" chars don't make it through would be already good enough.

(In reply to comment #9)
> I wonder if we really mind in the end. In which scenario would be blacklist an
> extension for just the currently selected locale?
>

I agree that its not obvious as of why to pass a %LOCALE% to the blocklist server at all ...

If there is no use-case for that we can just drop that component from the blocklist URL completely. Not sure how many backends/services rely on that component for parsing though.

> Maybe just making sure that the actual char prefs are properly escaped so that
> "/" chars don't make it through would be already good enough.

Agreed, properly escaping the components would make this more fail-safe ... not sure if its really worth the effort though. Thoughts?

I don't see any particular reason to block 3.1 on this, given that it works for the standard case. I don't see why we wouldn't take a patch though.

(In reply to comment #10)
> (In reply to comment #9)
> > I wonder if we really mind in the end. In which scenario would be blacklist an
> > extension for just the currently selected locale?
> >
>
> I agree that its not obvious as of why to pass a %LOCALE% to the blocklist
> server at all ...
>
> If there is no use-case for that we can just drop that component from the
> blocklist URL completely. Not sure how many backends/services rely on that
> component for parsing though.

bug 430120

> > Maybe just making sure that the actual char prefs are properly escaped so that
> > "/" chars don't make it through would be already good enough.
>
> Agreed, properly escaping the components would make this more fail-safe ... not
> sure if its really worth the effort though. Thoughts?

bug 468527

As the rationale for bug 430120 is to retrieve the same data as with update pings, we should feed the same locale info, i.e., the installed locale code, as it determines in http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/src/nsUpdateService.js.in#519.

Not sure what info linux distro's have there, and what we do about that.

Confirming that a patch for this problem should interpolate the installation locale to match the spec. As comment #12 mentions, bug 430120 contains the rational for having these extra fields in the URL.

The parser expects a legitimate @AB_CD@ code, but it special cases the chrome:// value using a gawd awful complicated regex. That workaround would break if you tried to do something like adding escaping to the URL instead, so if we do end up adding any new escaping to the URL, we need to open a separate bug for Metrics to track that.

(In reply to comment #13)
> Confirming that a patch for this problem should interpolate the installation
> locale to match the spec. As comment #12 mentions, bug 430120 contains the
> rational for having these extra fields in the URL.
>
> The parser expects a legitimate @AB_CD@ code, but it special cases the
> chrome:// value using a gawd awful complicated regex. That workaround would
> break if you tried to do something like adding escaping to the URL instead, so
> if we do end up adding any new escaping to the URL, we need to open a separate
> bug for Metrics to track that.

Presumably if we take a patch for this bug (getting rid of the chrome url from the locale and using a proper locale code) then adding escaping shouldn't actually affect anything.

... right, escaping should just kick in when you'd get garbage anyway. I'd suggest to drop the locale to something "None" or "undefined" in the metrics backend whenever you get a non-good locale code. No idea what you currently do if the regexp isn't matched.

(In reply to comment #7)
> I would expect this to take a patch like in bug 446527 to really work
> independent of user settings.
>
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/urlformatter/src/nsURLFormatter.js
> is worth a look, too. Sadly, one can't pass in extra variables. Not sure if
> some of the blocklist variables would be worth adding.

Ideally we should just get bug 430235 fixed

replacing the chrome URL with a properly formatted locale would be transparent on metrics.

I just went back and looked at the regex. It would properly handle an escaped bogus value as well without a problem, but I'd still very much like to get mindset that any time a change happens with the structure of one of these URLs, a bug gets opened with metrics to review it for potential impact. I've opened Bug 469806 to that effect.

FYI, here is the piece of the regex that deals with the locale segment:

   /(?> # locale raw_string
     (?>
        chrome://(?:.{0,108})\.properties
          # Note: some Linuxes are messed up and report chrome URL instead of locale
     )|(?>
       ( # locale code
        (?>[a-z]{2,3}) # language
        (?>-[A-Z]{2})? # region
        (?>-[\w]{2,8})? # dialect
       )
     )|(?>
        [^/]*
         # Failsafe to eat unacceptable values
     )
   )

(In reply to comment #17)
> I just went back and looked at the regex. It would properly handle an escaped
> bogus value as well without a problem, but I'd still very much like to get
> mindset that any time a change happens with the structure of one of these URLs,
> a bug gets opened with metrics to review it for potential impact. I've opened
> Bug 469806 to that effect.

I'm not entirely sure what the benefit is in opening a bug to evaluate the potential impact of another bug. Surely it would be better for all the discussion to go in the single bug?

(In reply to comment #17):

There are more variants to locale codes than that regexp matches. Like script, at least. We don't have any of those right now, but the real monty looks more complex still. Not this bug, though.

(In reply to comment #17)
> FYI, here is the piece of the regex that deals with the locale segment:
>
> /(?> # locale raw_string
> (?>
> chrome://(?:.{0,108})\.properties
> # Note: some Linuxes are messed up and report chrome URL instead of
> locale
> )|(?>

Thanks for the insight! So I guess this means that the url i posted in the bug description is properly matched as of now? can you double check that by running your complete regexp against what i posted? Its quite important for us to know, so we can assign a proper prio to this issue in ubuntu.

Thanks again!

Yes, the "expected result" URL you pasted in the description is properly matched and the locale is captured.

@Axel Hecht :Pike -- Regarding comment #19, I replied via e-mail asking for more details. Did you get that? I figured that tangent wasn't very related to the actual bug so I didn't want to junk up the comments here.

Yeah, got that. I should update the docs to say that http://tools.ietf.org/html/rfc4646 is what we use, while trying to be as short as possible conforming to that spec.