I looked through the different input element types but I didn't find any that appear to leak locale information. It seems these input types may display (at least in theory) some sort of locale-specific information, but when JS accesses the .value property, it receives non-localized data. Please correct me if I'm missing something!
There are a couple issues with this patch. You shouldn't need to store the current locale just to have something to do in DefaultJSLocaleSetter::Run() when the pref is empty. If the pref is empty, just do nothing. This eliminates the need to export JS_GetDefaultLocale() as well.
But beyond this, there's actually two bugs in the storage of this locale information. In the case of DefaultJSLocaleSetter::jsLocale, you leak it on XPCOM shutdown. In the case of DefaultJSLocaleSetting::systemLocale, you are keeping a pointer to a static buffer, so that subsequent calls to setlocale may cause this memory to get replaced with something else. It probably will always contain the actual current locale, but this seems a bit sloppy to rely on.
There are a couple issues with this patch. You shouldn't need to store the current locale just to have something to do in DefaultJSLocaleSetter::Run() when the pref is empty. If the pref is empty, just do nothing. This eliminates the need to export JS_GetDefaultLocale() as well.
My thinking is that it is nice to be able to set this pref at runtime. So if the user sets the pref to empty at some point, then the original locale will be restored. That's why we need to store the original locale on startup. Otherwise we would have to require restarting the browser whenever the pref is changed.
But beyond this, there's actually two bugs in the storage of this locale information. In the case of DefaultJSLocaleSetter::jsLocale, you leak it on XPCOM shutdown. In the case of DefaultJSLocaleSetting::systemLocale, you are keeping a pointer to a static buffer, so that subsequent calls to setlocale may cause this memory to get replaced with something else. It probably will always contain the actual current locale, but this seems a bit sloppy to rely on.
Thanks for catching these two bad mistakes. I believe I've fixed them in the new patch version, added above. I also changed the implementation to use Preferences::RegisterCallback/UnRegisterCallback, which is more concise than the nsIObserver approach.
Ok. Is there a reason why there is a 5926+ branch in your repo, but not this patch? I was looking at that branch for a while, and it seemed to fix these problems but did not use the RegisterCallback/UnregisterCallback, as the attachment does.
I'm a little nervous about the use of free() to free stuff allocated with the JS-runtime allocators. They appear to all boil down to the same js_free right now, but that may change? Not sure if we should put this as an XXX, or what. The JS_Free() calls do not seem to match JS_Strdup used by JS_GetDefaultLocale, in that they don't take a JSRuntime as an argument...
We also still need to set this new pref in the Torbutton code that sets extensions.torbutton.spoof_*, too.
I am thinking for both of these reasons this might not make it into the FF31 release on Tuesday, but we should perhaps aim for the next release after that for this.
Ok. Is there a reason why there is a 5926+ branch in your repo, but not this patch? I was looking at that branch for a while, and it seemed to fix these problems but did not use the RegisterCallback/UnregisterCallback, as the attachment does.
Sorry for the confusion -- this was an old branch.
I'm a little nervous about the use of free() to free stuff allocated with the JS-runtime allocators. They appear to all boil down to the same js_free right now, but that may change? Not sure if we should put this as an XXX, or what. The JS_Free() calls do not seem to match JS_Strdup used by JS_GetDefaultLocale, in that they don't take a JSRuntime as an argument...
I've updated the patch to use the JS_free() call instead. You're right that it doesn't match, but I think it's OK to use nullptr for the JSContext here.
We also still need to set this new pref in the Torbutton code that sets extensions.torbutton.spoof_*, too.
I've added a patch for torbutton that does this.
(Please see two patches attached today, 2014-10-10.)