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

Bug #308397 reported by Alexander Sack
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Mozilla Firefox
Won't Fix
High
Ubufox Extension
Fix Committed
Critical
Chris Coulson
ubufox (Ubuntu)
Triaged
Low
Chris Coulson
Gutsy
Won't Fix
Critical
Alexander Sack
Hardy
Won't Fix
High
Alexander Sack
Intrepid
Won't Fix
Critical
Unassigned
Jaunty
Won't Fix
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.

Revision history for this message
In , Alexander Sack (asac) wrote :
Revision history for this message
In , Alexander Sack (asac) wrote :

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.

Revision history for this message
In , Gavin Sharp (gavin-sharp) wrote :

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.

Revision history for this message
In , Gavin Sharp (gavin-sharp) wrote :

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

Revision history for this message
In , Alexander Sack (asac) wrote :

remove blocking from stable branches in turn of comment 2

Revision history for this message
In , Alexander Sack (asac) wrote :

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

Revision history for this message
In , L10n-mozilla (l10n-mozilla) wrote :

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.

Revision history for this message
In , Robert-bugzilla (robert-bugzilla) wrote :

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

Revision history for this message
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
Revision history for this message
Alexander Sack (asac) wrote :

fix committed on upstream ubufox branch (rev 149)

Changed in ubufox:
status: Triaged → Fix Committed
Revision history for this message
In , L10n-mozilla (l10n-mozilla) wrote :

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.

Revision history for this message
In , Alexander Sack (asac) wrote :

(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
Revision history for this message
In , Dtownsend (dtownsend) wrote :

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

Revision history for this message
In , L10n-mozilla (l10n-mozilla) wrote :

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.

Revision history for this message
In , Deinspanjer (deinspanjer) wrote :

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.

Revision history for this message
In , Dtownsend (dtownsend) wrote :

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

Revision history for this message
In , L10n-mozilla (l10n-mozilla) wrote :

... 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.

Revision history for this message
In , Dtownsend (dtownsend) wrote :

(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

Revision history for this message
In , Deinspanjer (deinspanjer) wrote :

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
     )
   )

Revision history for this message
In , Dtownsend (dtownsend) wrote :

(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?

Revision history for this message
In , L10n-mozilla (l10n-mozilla) wrote :

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

Revision history for this message
In , Alexander Sack (asac) wrote :

(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!

Revision history for this message
In , Deinspanjer (deinspanjer) wrote :

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.

Revision history for this message
In , L10n-mozilla (l10n-mozilla) wrote :

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)
Changed in ubufox:
assignee: nobody → asac
Revision history for this message
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)
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)
Changed in ubufox:
importance: Critical → Medium
milestone: jaunty-alpha-4 → none
Revision history for this message
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
Revision history for this message
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)
Changed in ubufox (Ubuntu):
status: Fix Committed → Triaged
Changed in firefox:
importance: Unknown → High
Alexander Sack (asac)
Changed in ubufox (Ubuntu):
assignee: Alexander Sack (asac) → Chris Coulson (chrisccoulson)
Changed in ubufox:
assignee: Alexander Sack (asac) → Chris Coulson (chrisccoulson)
Revision history for this message
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  
Everyone can see this information.

Other bug subscribers

Remote bug watches

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