Closed Bug 1005640 Opened 11 years ago Closed 8 years ago

Wrong Accept-Language if using a language pack that changes the default value of the intl.accept_language pref

Categories

(Core :: Networking: HTTP, defect)

Product:
Core
Component:
Networking: HTTP
Version:
29 Branch
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: o1645908, Assigned: zbraniecki)

References

Details

(Whiteboard: [necko-backlog])

Attachments

(3 files, 3 obsolete files)

screen-shot
1.83 MB, image/png
Details
accept-lang.patch
1.52 KB, patch
rhelmer
: feedback+
Pike
: feedback-
Details | Diff | Splinter Review
Bug 1005640 - Flush StringBundle cache when app-locales change.
59 bytes, text/x-review-board-request
valentin
: review+
Details
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:29.0) Gecko/20100101 Firefox/29.0 (Beta/Release) Build ID: 20140428193813 Steps to reproduce: I upgraded Firefox to 29: With the Linux version of Firefox 29, with the package firefox-locale-pt installed and LANG=pt_BR environment (I guess it will happen with any other than english environment) open Firefox and browse to any phpinfo page you find. Actual results: Look for the Accept-Language header, the value is "en-US,en;q=0.5" Expected results: The value should be "pt-BR,pt;q=0.8,en-US;q=0.5,en;q=0.3", as it was in previous version.
Please check the intl.accept_languages in about:config, its status and value is ...?
Flags: needinfo?(o1645908)
(In reply to YF (Yang) from comment #2) > Please check the intl.accept_languages in about:config, its status and value > is ...? The value is "pt-BR, pt, en-US, en", the status is default, but if I change it to anything and restore the default the error is fixed, but just last till I exit Firefox, if I open it again I need to do all over again, I explained it here: http://unix.stackexchange.com/questions/127803/firefox-29-default-accept-language-bug
Flags: needinfo?(o1645908)
Attached image Screen Shot of about:config (obsolete) —
Screen-shot of about:config
Sorry for not carefully read. but I tested just now, and not reproduce, through from http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/29.0/linux-x86_64/pt-BR/ on Lubuntu with clean profile. Can you reproduce with clean profile? or may network factors? Try DevTools - Network to check the http header by browser send.
Flags: needinfo?(o1645908)
Attached image clear profile (obsolete) —
The error happens even with clean profile, I am on Ubuntu 14.04, but I tested with 12.04 too, no third part ppa repository, Firefox from official canonical repository, here I have localhost apache, but it is the same for a remote one from anyone you get on Google.
Flags: needinfo?(o1645908)
Attached image screen-shot (obsolete) —
Here from google.com header using dev tools
Attached image screen-shot —
It's fuzzy, with the Mozilla FTP version the problem do not happen, only with Canonical one, and if I create a new profile with the Mozilla FTP version and open with Canonical seems to be ok too, the problem happens only with new and legacy profiles...
Attachment #8417045 - Attachment is obsolete: true
Attachment #8417055 - Attachment is obsolete: true
Attachment #8417056 - Attachment is obsolete: true
(In reply to o1645908 from comment #8) > Created attachment 8417060 [details] > screen-shot > > It's fuzzy, with the Mozilla FTP version the problem do not happen, only > with Canonical one, and if I create a new profile with the Mozilla FTP > version and open with Canonical seems to be ok too, the problem happens only > with new and legacy profiles... In this case, it sounds like it's a problem with the Canonical builds, which doesn't sound like something we can fix. You should report it to the maintainers of the official Canonical builds.
Status: UNCONFIRMED → RESOLVED
Closed: 11 years ago
Resolution: --- → INVALID
same problem with mint linux. Workaround: Install plugin to chance Accepted Languages like https://addons.mozilla.org/de/firefox/addon/quick-accept-language-switc/?src=api
Hrmmm... this bug is still present... Using Firefox canonical in Ubuntu, everything up to date, Ubuntu x64.
(In reply to b1566451 from comment #11) > Hrmmm... this bug is still present... > Using Firefox canonical in Ubuntu, everything up to date, Ubuntu x64. See earlier comments on this bug. It seems to be specific to Ubuntu's family of builds. You should file a bug there.
This bug is also happening for me, with Firefox 45 on Fedora 23. I filled https://bugzilla.redhat.com/show_bug.cgi?id=1329855 in their Bugzilla.
Same problem with Firefox 46 on Ubuntu 16.04. The launchpad bug I found is https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1527663 If it is actually a problem with Ubuntu's build, what I don't understand is why Firefox displays the correct value in about:config but still sends the wrong value in HTTP request headers. Shouldn't Firefox be consistent with itself?
Reading some of the code, I actually suspect that while this happens with: Ubuntu's official build + language pack (e.g. firefox-locale-br / firefox-locale-fr / ...) installed and doesn't happen with: official localized build from mozilla.org ( e.g. http://ftp.mozilla.org/pub/firefox/releases/46.0/linux-x86_64/fr/ ) I suspect it might happen if you tried e.g.: http://ftp.mozilla.org/pub/firefox/releases/46.0/linux-x86_64/en-US/ *with a relevant language pack from*: https://ftp.mozilla.org/pub/firefox/releases/46.0/linux-x86_64/xpi/ Can someone confirm my suspicion? For reasoning: I *think* the issue is that the http handler implementation reads the default prefs ( https://dxr.mozilla.org/mozilla-central/rev/4d63dde701b47b8661ab7990f197b6b60e543839/netwerk/protocol/http/nsHttpHandler.cpp#297-313 ), and then reacts to pref changes that get reported. Unfortunately, the lang pack being installed doesn't change the pref but just overrides the default pref (if that sounds confusing, that's because it is - but it's how our preference system works) and so the http handler never updates to the 'new' default value, if it is first initialized before the language pack registers (which seems plausible... but not 100% certain, which is why I'm asking for confirmation). This also explains why you can "kick it to work" by changing the value in about:config manually, which triggers the pref change handler in network, after which all is well with the world...
Status: RESOLVED → REOPENED
Ever confirmed: true
Flags: needinfo?(moz)
Flags: needinfo?(cesarb)
Resolution: INVALID → ---
Installed from FTP and created a new profile (Ubuntu 16.04). intl.accept_languages = en-US, en Accept Language HEADER: en-US,en;q=0.5 Installed it.xpi, switched general.useragent.locale to 'it' and restarted intl.accept_languages = it-IT, it, en-US, en Accept Language HEADER: en-US,en;q=0.5 Touching the pref, e.g. adding an empty space at the end, generates the correct header. Accept Language HEADER: it-IT,it;q=0.8,en-US;q=0.5,en;q=0.3
yeah, this is one of our many bugs around "launching en-US and then doing stuff, and then installing language pack" at startup. Bug 1236347 is another example.
Flags: needinfo?(moz)
Flags: needinfo?(cesarb)
See Also: → 1236347
Summary: Wrong Accept-Language on Linux version 29 → Wrong Accept-Language if using a language pack that changes the default value of the intl.accept_language pref
Axel, could you please advise on which component should this bug go to? My first choice would be Core/Internalization, but since it involves network, add-on and localization, not so sure that it's the best place to start with.
Flags: needinfo?(l10n)
Networking is the call site, moving there. It might be possible to fix it there based on the current infra, or maybe redo the code with l20n in the back.
Component: Untriaged → Networking: HTTP
Flags: needinfo?(l10n)
Product: Firefox → Core
Whiteboard: [necko-backlog]
This also happens for me on Debian. I have filed this to their bugtracker: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=835640
Yes, we also see it on Fedora [1]. Regarding to comment 15 it seems that the reason the nsHttpHandler::PrefsChanged is not notified is that technically the value of 'intl.accept_languages' does not change, because it remains set to 'chrome://global/locale/intl.properties' [2] but the content of chrome://global/locale/intl.properties is changed by loading language pack. I'm not sure how we can update mAcceptLanguages beside ugly obtaining the value from prefs everytime we call nsHttpHandler::AddStandardRequestHeaders. [1] https://bugzilla.redhat.com/show_bug.cgi?id=1329855 [2] http://searchfox.org/mozilla-central/rev/6536590412ea212c291719d1ed226fae65a0f917/modules/libpref/init/all.js#1991
I've finally managed to debug this issue. Assumptions I've made in comment 24 are not right. The problem is in XPIProvider.jsm and with flushing nsStringBundleService cache. 1. The caches are flushed by notifying observers by chrome-flush-caches topic there [1]. Comment in source clearly explains why - to take locales and stuff full effect. 2. Subsequently the bootstrap method is called for bootstraped addons. In case any of them require chrome://global/locale/intl.properties then the cache entry is made when calling nsStringBundleService::getStringBundle [2]. But since extensions from .mozilla/extensions (where the langpack is located) are not yet loaded the cache entry points to intl.properties from en-US. 3. Then the nsHttpHandler::SetAcceptLanguages [4] is called and it use cached intl.properties from en-US. I think we could fix that by flushing the cache (maybe again) after the the boostrap methods (it works for my case), ie. on this location [3] [1] http://searchfox.org/mozilla-central/rev/bcc9ea6947878ca6378e5b7d6f08c1c090ed9eb7/toolkit/mozapps/extensions/internal/XPIProvider.jsm#2799 [2] http://searchfox.org/mozilla-central/rev/bcc9ea6947878ca6378e5b7d6f08c1c090ed9eb7/intl/strres/nsStringBundle.cpp#581 [3] http://searchfox.org/mozilla-central/rev/bcc9ea6947878ca6378e5b7d6f08c1c090ed9eb7/toolkit/mozapps/extensions/internal/XPIProvider.jsm#2851 [4] http://searchfox.org/mozilla-central/rev/bcc9ea6947878ca6378e5b7d6f08c1c090ed9eb7/netwerk/protocol/http/nsHttpHandler.cpp#1854
(In reply to Jan Horak from comment #26) > I've finally managed to debug this issue. Assumptions I've made in comment > 24 are not right. The problem is in XPIProvider.jsm and with flushing > nsStringBundleService cache. > > 1. The caches are flushed by notifying observers by chrome-flush-caches > topic there [1]. Comment in source clearly explains why - to take locales > and stuff full effect. > 2. Subsequently the bootstrap method is called for bootstraped addons. In > case any of them require chrome://global/locale/intl.properties then the > cache entry is made when calling nsStringBundleService::getStringBundle [2]. > But since extensions from .mozilla/extensions (where the langpack is > located) are not yet loaded the cache entry points to intl.properties from > en-US. > 3. Then the nsHttpHandler::SetAcceptLanguages [4] is called and it use > cached intl.properties from en-US. > > I think we could fix that by flushing the cache (maybe again) after the the > boostrap methods (it works for my case), ie. on this location [3] > > [1] > http://searchfox.org/mozilla-central/rev/ > bcc9ea6947878ca6378e5b7d6f08c1c090ed9eb7/toolkit/mozapps/extensions/internal/ > XPIProvider.jsm#2799 > [2] > http://searchfox.org/mozilla-central/rev/ > bcc9ea6947878ca6378e5b7d6f08c1c090ed9eb7/intl/strres/nsStringBundle.cpp#581 > [3] > http://searchfox.org/mozilla-central/rev/ > bcc9ea6947878ca6378e5b7d6f08c1c090ed9eb7/toolkit/mozapps/extensions/internal/ > XPIProvider.jsm#2851 > [4] > http://searchfox.org/mozilla-central/rev/ > bcc9ea6947878ca6378e5b7d6f08c1c090ed9eb7/netwerk/protocol/http/nsHttpHandler. > cpp#1854 This seems like a sane idea, though I think we should only do it if the preferred locale changed (ie check: Cc["@mozilla.org/chrome/chrome-registry;1"].getService(Ci.nsIXULChromeRegistry).getSelectedLocale("toolkit") before and after) by loading the add-ons, so as not to needlessly flush these caches several times over when the locale doesn't change (as langpack users are comparatively a very small minority and the flush isn't exactly free. :rhelmer, does Jan's idea with/without the above modification sound like a sane way to fix this and other problems associated with how we load langpacks?
Flags: needinfo?(rhelmer)
Thanks, Jan, for drilling down to the offender. We'll need to check the "global" package, there's no "toolkit". I wonder if we should just wait for the point where we'd install language packs, and if we change locale at that point, fake a pref change notification for "general.useragent.locale", and add that to the prefs for nsHttpHandler/nsStringBundle to watch for. That way we get to a consistent pattern of "this is what happens when locale changes" and "you listen to locale changes and react accordingly".
(In reply to Axel Hecht [:Pike] from comment #28) > Thanks, Jan, for drilling down to the offender. > > We'll need to check the "global" package, there's no "toolkit". > > I wonder if we should just wait for the point where we'd install language > packs, and if we change locale at that point, fake a pref change > notification for "general.useragent.locale", and add that to the prefs for > nsHttpHandler/nsStringBundle to watch for. You mean that in nsHttpHandler the observer would invalidate nsStringBundleService cache? Hm, could try that. > That way we get to a consistent pattern of "this is what happens when locale > changes" and "you listen to locale changes and react accordingly".
Just recognizing "race" in my own proposal. We cache localized assets in a variety of places, string bundles and nshttphandler just being two. Now just watching for locale changes might still trigger the nshttphandler before the stringbundle cache, and then we're back to where we are. Things that are intermediate caches probably need their own signals, i.e., stringbundle service needs to listen for locale changes, and for that (and profile change), emit a notification (observerservice?) for customers to reload their own cached values, which nshttphandler would then have to listen to. Bug 1236347 would then be about making the browser.js code listen to that notification, too.
(In reply to Jan Horak from comment #29) > (In reply to Axel Hecht [:Pike] from comment #28) > > Thanks, Jan, for drilling down to the offender. > > > > We'll need to check the "global" package, there's no "toolkit". > > > > I wonder if we should just wait for the point where we'd install language > > packs, and if we change locale at that point, fake a pref change > > notification for "general.useragent.locale", and add that to the prefs for > > nsHttpHandler/nsStringBundle to watch for. > You mean that in nsHttpHandler the observer would invalidate > nsStringBundleService cache? Hm, could try that. > > > That way we get to a consistent pattern of "this is what happens when locale > > changes" and "you listen to locale changes and react accordingly". I like this approach better than working around it in the XPIProvider module. If it doesn't work out I'd be happy to revisit, but invalidating the cache on pref change sounds more like the way this was intended to work.
Flags: needinfo?(rhelmer)
(In reply to Robert Helmer [:rhelmer] from comment #31) > (In reply to Jan Horak from comment #29) > > (In reply to Axel Hecht [:Pike] from comment #28) > > > Thanks, Jan, for drilling down to the offender. > > > > > > We'll need to check the "global" package, there's no "toolkit". > > > > > > I wonder if we should just wait for the point where we'd install language > > > packs, and if we change locale at that point, fake a pref change > > > notification for "general.useragent.locale", and add that to the prefs for > > > nsHttpHandler/nsStringBundle to watch for. > > You mean that in nsHttpHandler the observer would invalidate > > nsStringBundleService cache? Hm, could try that. > > > > > That way we get to a consistent pattern of "this is what happens when locale > > > changes" and "you listen to locale changes and react accordingly". > > I like this approach better than working around it in the XPIProvider > module. If it doesn't work out I'd be happy to revisit, but invalidating the > cache on pref change sounds more like the way this was intended to work. Well, adding: FlushBundles there: http://searchfox.org/mozilla-central/rev/515bca4fca5e2e8233ed30e6b6d48fcab8497dbf/netwerk/protocol/http/nsHttpHandler.cpp#1475 "fix" the problem. But the nsHttpHandler::PrefsChanged is called during initialization of nsHttpHandler which happen after extensions are already loaded, while the nsStringBundle cache is still dirty. So that's why FlushBundles fix the problem. The nsHttpHandler observer which would watch for "general.useragent.locale" changes is registered after the change of "general.useragent.locale" has been made by extension, so watching this change does not help. Maybe we could FlushBundles before PrefsChanged call, there: http://searchfox.org/mozilla-central/rev/515bca4fca5e2e8233ed30e6b6d48fcab8497dbf/netwerk/protocol/http/nsHttpHandler.cpp#310 What do you think about it?
I don't think the http code should be directly calling flushbundles on the l10n code. That dependency shouldn't exist in that way. I think this is righteous: (In reply to Axel Hecht [:Pike] from comment #30) > stringbundle service needs to listen for locale changes, and for that (and > profile change), emit a notification (observerservice?) for customers to > reload their own cached values, which nshttphandler would then have to > listen to. So to comment 32: (In reply to Jan Horak from comment #32) > The nsHttpHandler observer which would watch for "general.useragent.locale" > changes is registered after the change of "general.useragent.locale" has > been made by extension, so watching this change does not help. I think the idea is that the string bundle service should itself listen for whatever changes trigger this (general.useragent.locale, profile change) and *first* flush caches if necessary, and *then* emit a second notification. In this case, either the http stuff is inited before the string cache is flushed, and then it gets this "new" notification, or it's inited after the new notification, at which point the string cache should be flushed so we should be OK.
(In reply to :Gijs Kruitbosch from comment #33) > I don't think the http code should be directly calling flushbundles on the > l10n code. That dependency shouldn't exist in that way. > > I think this is righteous: > > (In reply to Axel Hecht [:Pike] from comment #30) > > stringbundle service needs to listen for locale changes, and for that (and > > profile change), emit a notification (observerservice?) for customers to > > reload their own cached values, which nshttphandler would then have to > > listen to. > > So to comment 32: > > (In reply to Jan Horak from comment #32) > > The nsHttpHandler observer which would watch for "general.useragent.locale" > > changes is registered after the change of "general.useragent.locale" has > > been made by extension, so watching this change does not help. > > I think the idea is that the string bundle service should itself listen for > whatever changes trigger this (general.useragent.locale, profile change) and > *first* flush caches if necessary, and *then* emit a second notification. > > In this case, either the http stuff is inited before the string cache is > flushed, and then it gets this "new" notification, or it's inited after the > new notification, at which point the string cache should be flushed so we > should be OK. I see the point now and I like it, but the problem is that my debugger does not show that nsPrefBranch::NotifyObserver("general.useragent.locale", ...) is called during startup, only when I manually change "general.useragent.locale" preference in about:config, then the breakpoint is reached. The reason is most likely that overlayed preferences does not notify observers. So I'm still to the manual flushing caches in nsHttpHandler (since I lack any better solution right now). We could do that with lesser coupling by notifying observers by "chrome-flush-caches" topic if you'd like. And thanks for the feedback!
(In reply to Jan Horak from comment #34) > I see the point now and I like it, but the problem is that my debugger does > not show that nsPrefBranch::NotifyObserver("general.useragent.locale", ...) > is called during startup, only when I manually change > "general.useragent.locale" preference in about:config, then the breakpoint > is reached. The reason is most likely that overlayed preferences does not > notify observers. That's why Gijs and I talk about checking the selected locale before and after registring add-ons in comment 27 and comment 28. There's absolutely nothing notifying anything right now, and that's the first thing to fix. > So I'm still to the manual flushing caches in nsHttpHandler (since I lack > any better solution right now). We could do that with lesser coupling by > notifying observers by "chrome-flush-caches" topic if you'd like. And thanks > for the feedback! What triggers your code in nsHttpHandler, though?
(In reply to Axel Hecht [:Pike] from comment #35) > (In reply to Jan Horak from comment #34) > > I see the point now and I like it, but the problem is that my debugger does > > not show that nsPrefBranch::NotifyObserver("general.useragent.locale", ...) > > is called during startup, only when I manually change > > "general.useragent.locale" preference in about:config, then the breakpoint > > is reached. The reason is most likely that overlayed preferences does not > > notify observers. > > That's why Gijs and I talk about checking the selected locale before and > after registring add-ons in comment 27 and comment 28. > There's absolutely nothing notifying anything right now, and that's the > first thing to fix. Hm, you guys need to help me out there, are these changes required in XPIProvider.jsm, some new sort of observer's topic? > > So I'm still to the manual flushing caches in nsHttpHandler (since I lack > > any better solution right now). We could do that with lesser coupling by > > notifying observers by "chrome-flush-caches" topic if you'd like. And thanks > > for the feedback! > > What triggers your code in nsHttpHandler, though? It's not triggered, it's called in nsHttpHandler::Init, ie. if the cache is flushed before calling PrefsChanged (this line: http://searchfox.org/mozilla-central/source/netwerk/protocol/http/nsHttpHandler.cpp#310) then it's working fine (obviously).
(In reply to Axel Hecht [:Pike] from comment #35) > (In reply to Jan Horak from comment #34) > > I see the point now and I like it, but the problem is that my debugger does > > not show that nsPrefBranch::NotifyObserver("general.useragent.locale", ...) > > is called during startup, only when I manually change > > "general.useragent.locale" preference in about:config, then the breakpoint > > is reached. The reason is most likely that overlayed preferences does not > > notify observers. > > That's why Gijs and I talk about checking the selected locale before and > after registring add-ons in comment 27 and comment 28. > > There's absolutely nothing notifying anything right now, and that's the > first thing to fix. Well, checking the change of useragent after calling bootstrap methods does not help, because the "general.useragent.locale" is also in the same file as "intl.accept_languages", therefore cached in the StringBundle. I seems to be moving in circles.
Which is why comment 27/28 talk about asking the chrome registry for the selected locale, and not going through the prefs service. Cc["@mozilla.org/chrome/chrome-registry;1"].getService(Ci.nsIXULChromeRegistry).getSelectedLocale("global")
^^ works for me now. Thank you. It took me a while, but here's the patch... Do you rather prefer a bit more complicated fix by: 1. Notifying observers with some new topic, like "locale-changed" instead of "chrome-flush-caches" 2. which stringbundles will be listening to and flush caches 3. stringbundles notify observers with some new topic, like "string-bundle-cache-invalid" as proposed by you guys before?
Attachment #8793332 - Flags: feedback?(rhelmer)
Comment on attachment 8793332 [details] [diff] [review] accept-lang.patch >+ var previousLocale = Cc["@mozilla.org/chrome/chrome-registry;1"] >+ .getService(Ci.nsIXULChromeRegistry).getSelectedLocale("global"); >+ (...) >+ var currentLocale = Cc["@mozilla.org/chrome/chrome-registry;1"] >+ .getService(Ci.nsIXULChromeRegistry).getSelectedLocale("global"); There are several places in the tree that do this by either saving the result of the `.getService` call into a variable (`let registry = Cc[...].getService(...);`) or have a utility function (TelemetryEnvironment.jsm has `getBrowserLocale()`): https://dxr.mozilla.org/mozilla-central/search?q=nsIXULChromeRegistry+ext%3Ajsm I think it's worth considering refactoring this something shared, maybe a lazy service getter in XPCOMUtils.jsm or something. That should be done in a followup though, choosing one of the above to do in XPIProvider.jsm is fine with me for the current bug. I defer to Axel and Gijs in terms of whether this is the right place globally to make this change, and if this is the change they suggested. I'm a relative newb to the way l10n is done in the browser. Assuming the above is fine, I am happy to do the final review - there should be a test too to make sure this doesn't regress. Let me know if you need help with that bit, pretty sure we could exercise this with a simple unit test (aka xpcshell).
Attachment #8793332 - Flags: feedback?(rhelmer)
Attachment #8793332 - Flags: feedback?(gijskruitbosch+bugs)
Attachment #8793332 - Flags: feedback?(axel)
Attachment #8793332 - Flags: feedback+
Attachment #8793332 - Flags: feedback?(axel) → feedback?(l10n)
Comment on attachment 8793332 [details] [diff] [review] accept-lang.patch Review of attachment 8793332 [details] [diff] [review]: ----------------------------------------------------------------- Leaving Axel to comment on the stringbundle thing. I'm not sure I understand why this works - how is the HTTP code realizing the pref has changed, just by the virtue of the stringbundle flush? ::: firefox-48.0.1/toolkit/mozapps/extensions/internal/XPIProvider.jsm.mydebug @@ +2756,5 @@ > AddonManagerPrivate.recordException("XPI-BOOTSTRAP", "startup failed", e); > } > > + var currentLocale = Cc["@mozilla.org/chrome/chrome-registry;1"] > + .getService(Ci.nsIXULChromeRegistry).getSelectedLocale("global"); Nit: rhelmer is right that the service ref should probably be stored. Also, s/var/let/ here and above. @@ +2760,5 @@ > + .getService(Ci.nsIXULChromeRegistry).getSelectedLocale("global"); > + if (currentLocale != previousLocale) { > + // We have to flush string cache if the locale was changed during loading > + // of addons > + Services.obs.notifyObservers(null, "chrome-flush-caches", null); Axel will know more about whether this is the best way to trigger a stringbundle flush... With this code, how is the http handler finding out about the new value for general.useragent.locale? You & I have both previously concluded that the pref change doesn't notify the http handler.
Attachment #8793332 - Flags: feedback?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #41) > Comment on attachment 8793332 [details] [diff] [review] > accept-lang.patch > > Review of attachment 8793332 [details] [diff] [review]: > ----------------------------------------------------------------- > > Leaving Axel to comment on the stringbundle thing. I'm not sure I understand > why this works - how is the HTTP code realizing the pref has changed, just > by the virtue of the stringbundle flush? > > ::: > firefox-48.0.1/toolkit/mozapps/extensions/internal/XPIProvider.jsm.mydebug > @@ +2756,5 @@ > > AddonManagerPrivate.recordException("XPI-BOOTSTRAP", "startup failed", e); > > } > > > > + var currentLocale = Cc["@mozilla.org/chrome/chrome-registry;1"] > > + .getService(Ci.nsIXULChromeRegistry).getSelectedLocale("global"); > > Nit: rhelmer is right that the service ref should probably be stored. Also, > s/var/let/ here and above. > > @@ +2760,5 @@ > > + .getService(Ci.nsIXULChromeRegistry).getSelectedLocale("global"); > > + if (currentLocale != previousLocale) { > > + // We have to flush string cache if the locale was changed during loading > > + // of addons > > + Services.obs.notifyObservers(null, "chrome-flush-caches", null); > > Axel will know more about whether this is the best way to trigger a > stringbundle flush... > > With this code, how is the http handler finding out about the new value for > general.useragent.locale? You & I have both previously concluded that the > pref change doesn't notify the http handler. The nsHttpHandler is not notified, it is initialized after this code, but since StringBundle has cache flushed, it won't get the wrong value. Without the patch the wrong value for intl.accept_language is cached by bootstrap method of some addon here [1], the bootstrap method for system-installed addon (read langpack) is called later, it's near the end of XPIProvider.sortBootstrappedAddons().reverse() array. [1] http://searchfox.org/mozilla-central/rev/bcc9ea6947878ca6378e5b7d6f08c1c090ed9eb7/toolkit/mozapps/extensions/internal/XPIProvider.jsm#2871
(In reply to Jan Horak from comment #42) > (In reply to :Gijs Kruitbosch from comment #41) > > Comment on attachment 8793332 [details] [diff] [review] > > accept-lang.patch > > > > Review of attachment 8793332 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Leaving Axel to comment on the stringbundle thing. I'm not sure I understand > > why this works - how is the HTTP code realizing the pref has changed, just > > by the virtue of the stringbundle flush? > > > > ::: > > firefox-48.0.1/toolkit/mozapps/extensions/internal/XPIProvider.jsm.mydebug > > @@ +2756,5 @@ > > > AddonManagerPrivate.recordException("XPI-BOOTSTRAP", "startup failed", e); > > > } > > > > > > + var currentLocale = Cc["@mozilla.org/chrome/chrome-registry;1"] > > > + .getService(Ci.nsIXULChromeRegistry).getSelectedLocale("global"); > > > > Nit: rhelmer is right that the service ref should probably be stored. Also, > > s/var/let/ here and above. > > > > @@ +2760,5 @@ > > > + .getService(Ci.nsIXULChromeRegistry).getSelectedLocale("global"); > > > + if (currentLocale != previousLocale) { > > > + // We have to flush string cache if the locale was changed during loading > > > + // of addons > > > + Services.obs.notifyObservers(null, "chrome-flush-caches", null); > > > > Axel will know more about whether this is the best way to trigger a > > stringbundle flush... > > > > With this code, how is the http handler finding out about the new value for > > general.useragent.locale? You & I have both previously concluded that the > > pref change doesn't notify the http handler. > The nsHttpHandler is not notified, it is initialized after this code, but > since StringBundle has cache flushed, it won't get the wrong value. > > Without the patch the wrong value for intl.accept_language is cached by > bootstrap method of some addon here [1], the bootstrap method for > system-installed addon (read langpack) is called later, it's near the end of > XPIProvider.sortBootstrappedAddons().reverse() array. > > [1] > http://searchfox.org/mozilla-central/rev/ > bcc9ea6947878ca6378e5b7d6f08c1c090ed9eb7/toolkit/mozapps/extensions/internal/ > XPIProvider.jsm#2871 This is still racy, then, I suspect. Some bootstrapped add-ons might cause the http handler to be initialized sooner (e.g. by doing an XHR somewhere). I also don't think the langpack is guaranteed to be a system-installed add-on, even if that's likely the most common case because of linux distros.
Comment on attachment 8793332 [details] [diff] [review] accept-lang.patch Review of attachment 8793332 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, but as much as this might fix the behaviour your seeing in your setup, I don't think this is the right fix. What's worse, I can't say what's right just yet. But at least I know what to expect: When the locale changes during startup, we should trigger code paths like https://dxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#11562 Might be that that code there isn't good enough either in what it currently does. I'm sorry, but this is a bit of a rat hole. .... train of thought travels around town .... It dawns on me that we need to actively define the behavior, and then audit things to make them confirm to that definition. That defined behavior also needs to be forwards compatible with what we're doing with l20n (which won't go through chrome registry for localized content no more). rhelmer, thread on .platform sounds like a good next step?
Attachment #8793332 - Flags: feedback?(l10n) → feedback-
(In reply to Axel Hecht [:Pike] from comment #44) > Comment on attachment 8793332 [details] [diff] [review] > accept-lang.patch > > Review of attachment 8793332 [details] [diff] [review]: > ----------------------------------------------------------------- > > Sorry, but as much as this might fix the behaviour your seeing in your > setup, I don't think this is the right fix. > > What's worse, I can't say what's right just yet. But at least I know what to > expect: > > When the locale changes during startup, we should trigger code paths like > https://dxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow. > cpp#11562 So implementing something like I've summarized in comment 39 is also not the option? About the .platform, is it dev-platform list? I'd like to track the discussion, if possible.
Flags: needinfo?(gandalf)
This is a simple fix using the new LocaleService's intl:app-locales-changed event.
Flags: needinfo?(gandalf)
Comment on attachment 8859056 [details] Bug 1005640 - Flush StringBundle cache when app-locales change. https://reviewboard.mozilla.org/r/131080/#review133658 https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpHandler.cpp#2061 would need to listen, too, and probably set mUserAgentIsDirty. Not sure if this is safe for race conditions? I'll mark this as an r- to figure this stuff out, though I don't find anything wrong ad-hoc in the patch we have so far.
Attachment #8859056 - Flags: review?(l10n) → review-
Ah, yes, I tested only `navigator.languages` value. I'll check that when I have a moment, but it does look racy.
Flags: needinfo?(gandalf)
So, I'm a bit confused. I instrumented nsHttpHandler.cpp to printf when SetAcceptLanguages is fired[0] and when SetHeader for accepted languages is fired [1]. Then I installed 'pl' langpack on top of en-US build and launched: SetAcceptLanguages: en-US, en Setting AcceptLanguages header: en-US,en;q=0.5 Setting AcceptLanguages header: en-US,en;q=0.5 SetAcceptLanguages: pl, en-US, en Setting AcceptLanguages header: pl,en-US;q=0.7,en;q=0.3 Setting AcceptLanguages header: en-US,en;q=0.5 Setting AcceptLanguages header: pl,en-US;q=0.7,en;q=0.3 Setting AcceptLanguages header: en-US,en;q=0.5 Setting AcceptLanguages header: en-US,en;q=0.5 Setting AcceptLanguages header: en-US,en;q=0.5 Setting AcceptLanguages header: pl,en-US;q=0.7,en;q=0.3 Setting AcceptLanguages header: pl,en-US;q=0.7,en;q=0.3 Setting AcceptLanguages header: pl,en-US;q=0.7,en;q=0.3 Setting AcceptLanguages header: pl,en-US;q=0.7,en;q=0.3 Setting AcceptLanguages header: pl,en-US;q=0.7,en;q=0.3 Setting AcceptLanguages header: en-US,en;q=0.5 Setting AcceptLanguages header: en-US,en;q=0.5 Setting AcceptLanguages header: en-US,en;q=0.5 It does look like it starts with en-US, then pl is picked up and mAcceptLanguages is updated, but then half of the requests go with old and half with new. Not sure why is that. Is it e10s processes with content starting after langpack updates and parent staying in pre-langpack values? [0] http://searchfox.org/mozilla-central/rev/4bd7a206dea5382c97a8a0c30beef668cc449f5b/netwerk/protocol/http/nsHttpHandler.cpp#1819 [1] http://searchfox.org/mozilla-central/rev/4bd7a206dea5382c97a8a0c30beef668cc449f5b/netwerk/protocol/http/nsHttpHandler.cpp#479
Assignee: nobody → gandalf
Status: REOPENED → ASSIGNED
Flags: needinfo?(gandalf)
I reshuffled the cache invalidation a bit and it seems to work well now!
Attachment #8859056 - Flags: review?(mcmanus)
Patrick, I'd like the http channel piece to be reviewed by someone from the necko module. Can you redirect that review if you're not a good pick?
Comment on attachment 8859056 [details] Bug 1005640 - Flush StringBundle cache when app-locales change. https://reviewboard.mozilla.org/r/131080/#review133990 ::: netwerk/protocol/http/nsHttpHandler.cpp:481 (Diff revision 2) > // service worker. > - if (!mAcceptLanguages.IsEmpty()) { > + if (mAcceptLanguages.IsEmpty()) { > + SetAcceptLanguages(); > + } > - // Add the "Accept-Language" header > + // Add the "Accept-Language" header > - rv = request->SetHeader(nsHttp::Accept_Language, mAcceptLanguages, > + rv = request->SetHeader(nsHttp::Accept_Language, mAcceptLanguages, does the setHeader() still need to be conditional on !IsEmpty() in case SetAcceptLanguages() did not put anything in that stirng?
I don't know if that ever happens, but sure, I readded it :)
Attachment #8859056 - Flags: review?(valentin.gosu)
Comment on attachment 8859056 [details] Bug 1005640 - Flush StringBundle cache when app-locales change. https://reviewboard.mozilla.org/r/131080/#review134122 ::: netwerk/protocol/http/nsHttpHandler.cpp:478 (Diff revision 3) > if (NS_FAILED(rv)) return rv; > > // Add the "Accept-Language" header. This header is also exposed to the > // service worker. > - if (!mAcceptLanguages.IsEmpty()) { > + if (mAcceptLanguages.IsEmpty()) { > + SetAcceptLanguages(); If SetAcceptLanguages fails, or the pref is simply empty, then mAcceptLanguages will always be empty, but we still query the pref for every request. Maybe instead set a flag when INTL_ACCEPT_LANGUAGES changes - and reset the flag when calling SetAcceptLanguages regardless if it succeeds, or if mAcceptLanguages is empty. ::: netwerk/protocol/http/nsHttpHandler.cpp:1487 (Diff revision 3) > // > // INTL options > // > > if (PREF_CHANGED(INTL_ACCEPT_LANGUAGES)) { > - nsCOMPtr<nsIPrefLocalizedString> pls; > + mAcceptLanguages.AssignLiteral(""); Can you add comments mentioning why we can't just call SetAcceptLanguages here?
Attachment #8859056 - Flags: review?(valentin.gosu)
Updated, thanks :valentin! Also, this is a temporary solution. Since this pref is not really a regular pref, it is used in multiple places (httphandler, global object etc.) and it's part of the future language selection update (see bug 1325870), I will eventually move the logic to LocaleService. I imagine that it will end up as LocaleService::GetContentLanguages LocaleService::SetContentLanguages and `intl:content-languages-changed`. But for now this should fix the bug :)
Comment on attachment 8859056 [details] Bug 1005640 - Flush StringBundle cache when app-locales change. https://reviewboard.mozilla.org/r/131080/#review134196 Just one small issue left. Looks good. Thanks! ::: netwerk/protocol/http/nsHttpHandler.cpp:480 (Diff revision 4) > > // Add the "Accept-Language" header. This header is also exposed to the > // service worker. > - if (!mAcceptLanguages.IsEmpty()) { > + if (mAcceptLanguagesIsDirty) { > + rv = SetAcceptLanguages(); > + if (NS_FAILED(rv)) return rv; The old code didn't fail in AddStandardRequestHeaders if there was an issue with the pref, rather than returning rv, just MOZ_ASSERT(NS_SUCCEEDED(rv))
Attachment #8859056 - Flags: review?(valentin.gosu) → review+
Pushed by zbraniecki@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4e489e84adfd Flush StringBundle cache when app-locales change. r=valentin
https://hg.mozilla.org/mozilla-central/rev/4e489e84adfd
Status: ASSIGNED → RESOLVED
Closed: 11 years ago → 8 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Dear developers Can this fix be ported to current Firefox ESR? If we can, this fix will be propagated to users of Linux distros (At least Debian and derivatives) earlier than with the next ESR version Thanks!
So this was supposed to be fixed in Firefox 55? I still experience the same issue on my firefox-55.0.1-1.fc26.x86_64 with a Polish langpack: en-US,en;q=0.5.
Flags: needinfo?(gandalf)
Piotr, can you check in nightly? I tried to reproduce it by: 1) Open Firefox Nightly (on Arch Linux) en-US 2) Install polish langpack 3) Switch locale using `Services.locale.setRequestedLocales(['pl']);` (but you can also just switch it in about:config - general.useragent.locale) 3) Restart the browser 4) Open https://duckduckgo.com/?q=user+agent&t=ffab&ia=answer 5) I see "Accept-Language: pl,en-US;q=0.7,en;q=0.3" If you'll see different outcome in Nightly, can you please open a new bug and NI me in it? Thank you!
Flags: needinfo?(gandalf) → needinfo?(piotrdrag)
Using instructions you provided results in proper Accept-Language being sent. I guess it might be something specific to Fedora RPMs, but I can’t test 57 as RPM until it’s stable and gets packaged.
Flags: needinfo?(piotrdrag)
I reported it downstream, in case you’re interested: https://bugzilla.redhat.com/show_bug.cgi?id=1514536
I think it's not fixed. It seems i got the same bug (Firefox -Quantum- 60 / 64b on Ubuntu 16 LTS) intl.accept_languages is : fr, fr-fr, en-us, en accept-Language is : en-GB,en;q=0.5
(In reply to Francesco Lodolo [:flod][PTO May 17-20] from comment #72) > (In reply to Silex from comment #71) > > I think it's not fixed. > > > > It seems i got the same bug (Firefox -Quantum- 60 / 64b on Ubuntu 16 LTS) > > > > intl.accept_languages is : fr, fr-fr, en-us, en > > accept-Language is : en-GB,en;q=0.5 > > intl.accept_language is gone, you need intl.locale.requested. > > See also > https://support.mozilla.org/en-US/kb/use-firefox-interface-other-languages- > language-pack Ehm, forget my answer, it's completely unrelated to your point. Please file a new bug if you're still seeing the issue, providing info on your system (e.g. how the language pack is installed)
See Also: → 1464025
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: