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

Bug #308397 reported by Alexander Sack on 2008-12-16
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Mozilla Firefox
Won't Fix
High
Ubufox Extension
Critical
Chris Coulson
ubufox (Ubuntu)
Low
Chris Coulson
Gutsy
Critical
Alexander Sack
Hardy
High
Alexander Sack
Intrepid
Critical
Unassigned
Jaunty
Low
Alexander Sack

Bug Description

Binary package hint: ubufox

See: https://bugzilla.mozilla.org/show_bug.cgi?id=469760

this is major and should be addressed by dropping the localized pref.

From bmo bug:

1. start http live headers
2. reset app.update.lastUpdateTime.blocklist-background-update-timer
3. open error console
4. force blocklist update by running:

Components.classes['@mozilla.org/extensions/blocklist;1'].getService(Components.interfaces.nsITimerCallback).notify(null)
5. check the GET url invoked

result:

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

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

difference: %LOCALE% is replaced by chrome://global/locale/intl.properties ...
but should be en-US

Note: this probably busts "OS" stats if they are based on blocklist runs.

Evaluation: PREF_GENERAL_USERAGENT_LOCALE seems to be a complex pref; fix would
first check for complex pref and then fall back to not complex pref.

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.

Alexander Sack (asac) wrote :

Binary package hint: ubufox

See: https://bugzilla.mozilla.org/show_bug.cgi?id=469760

this is major and should be addressed by dropping the localized pref.

From bmo bug:

1. start http live headers
2. reset app.update.lastUpdateTime.blocklist-background-update-timer
3. open error console
4. force blocklist update by running:

Components.classes['@mozilla.org/extensions/blocklist;1'].getService(Components.interfaces.nsITimerCallback).notify(null)
5. check the GET url invoked

result:

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

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

difference: %LOCALE% is replaced by chrome://global/locale/intl.properties ...
but should be en-US

Note: this probably busts "OS" stats if they are based on blocklist runs.

Evaluation: PREF_GENERAL_USERAGENT_LOCALE seems to be a complex pref; fix would
first check for complex pref and then fall back to not complex pref.

Changed in ubufox:
assignee: nobody → asac
importance: Undecided → Critical
status: New → Confirmed
assignee: nobody → asac
importance: Undecided → Critical
milestone: none → jaunty-alpha-4
status: New → Triaged
assignee: nobody → asac
importance: Undecided → Critical
milestone: none → gutsy-updates
status: New → Triaged
importance: Undecided → Critical
milestone: none → ubuntu-8.04.2
status: New → Triaged
importance: Undecided → Critical
milestone: none → intrepid-updates
status: New → Triaged
status: Confirmed → Triaged
Alexander Sack (asac) wrote :

fix committed on upstream ubufox branch (rev 149)

Changed in ubufox:
status: Triaged → Fix Committed

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?

Changed in firefox:
status: Unknown → In Progress

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.

Steve Langasek (vorlon) on 2009-01-07
Changed in ubufox:
assignee: nobody → asac
Steve Langasek (vorlon) wrote :

<asac> slangasek: an update on ubufox. upstream says we get counted. so its not as bad as it seems and can wait until after .2 i think

So it sounds lke this is less-than-critical.

Changed in ubufox:
importance: Critical → High
milestone: ubuntu-8.04.2 → ubuntu-8.04.3
Alexander Sack (asac) on 2009-01-30
Changed in ubufox:
status: Triaged → Won't Fix
milestone: ubuntu-8.04.3 → none
status: Triaged → Won't Fix
milestone: gutsy-updates → none
milestone: intrepid-updates → none
status: Triaged → Won't Fix
Martin Pitt (pitti) on 2009-01-30
Changed in ubufox:
importance: Critical → Medium
milestone: jaunty-alpha-4 → none
Alexander Sack (asac) wrote :

upstream confirmed that they already had a workaround for our strange "locale" encoded in the blacklist requests and that we are properly counted for their user stats. This means that importance of this bug is rather Low.

Changed in ubufox:
importance: Medium → Low
Martin Pitt (pitti) wrote :

Not a jaunty blocker.

Changed in ubufox:
status: Triaged → Won't Fix
Changed in ubufox (Ubuntu):
status: Triaged → Fix Committed
Micah Gersten (micahg) on 2009-08-13
Changed in ubufox (Ubuntu):
status: Fix Committed → Triaged
Changed in firefox:
importance: Unknown → High
Alexander Sack (asac) on 2011-01-06
Changed in ubufox (Ubuntu):
assignee: Alexander Sack (asac) → Chris Coulson (chrisccoulson)
Changed in ubufox:
assignee: Alexander Sack (asac) → Chris Coulson (chrisccoulson)
Markitox (marcos-aruj) wrote :

Hi all,

I just experienced this issue with Ubuntu 11.10 and Firefox 7. Disabling Ubufox now correctly return en-US. I got the incorrect chrome://global/locale/intl.properties when using getCharPref inside an extension.

Ubufox 1.0 according to the addon manager. Hope this helps. Thanks!

Changed in firefox:
status: In Progress → Won't Fix
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.