Closed
Bug 783184
Opened 13 years ago
Closed 13 years ago
navigator.language is not defined for OOP apps, which breaks localization
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
People
(Reporter: djf, Assigned: cjones)
References
Details
(Whiteboard: [LOE:S])
Attachments
(2 files, 1 obsolete file)
3.65 KB,
patch
|
jdm
:
review+
|
Details | Diff | Splinter Review |
25.98 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
When B2G apps are moved out-of-process, localization breaks. This is apparently because navigator.language is not set when the app is OOP. Normally (for an non-OOP app) navigator.language has a value like "en-US". For OOP, the value is either the empty string or undefined (something that prints as the empty string in console.log, anyway).
Assignee | ||
Comment 1•13 years ago
|
||
Bad! Especially because I distinctly remember fixing this in the past :(.
If memory serves, we send this value to content processes when they start up. Perhaps we're overriding the normal source of the locale somehow for b2g?
Blocks: b2g-e10s-work
blocking-basecamp: --- → +
Assignee | ||
Comment 2•13 years ago
|
||
nsPrefBranch::GetComplexValue is failing at
rv = GetDefaultFromPropertiesFile(pref, getter_Copies(utf16String));
Assignee | ||
Comment 3•13 years ago
|
||
Because nsStringBundle::GetStringFromName(const PRUnichar *aName, PRUnichar **aResult) is failing at
rv = LoadProperties();
Assignee | ||
Comment 4•13 years ago
|
||
Which is failing at
rv = NS_NewURI(getter_AddRefs(uri), mPropertiesURL);
with
(gdb) p mPropertiesURL
$1 = {
<nsACString_internal> = {
mData = 0xea96d8 "en-US",
mLength = 5,
mFlags = 5
}, <No data fields>}
I don't have the foggiest clue what this code is trying to do ...
Assignee | ||
Comment 5•13 years ago
|
||
We're bailing out of NS_NewURI() at
nsresult rv = ExtractScheme(aSpec, scheme);
if (NS_FAILED(rv)) {
// then aSpec is relative
if (!aBaseURI)
return NS_ERROR_MALFORMED_URI;
which is (finally!) what I would expect. This may be a case of something being lost in translation when we convert prefs for content processes.
Assignee | ||
Comment 6•13 years ago
|
||
Whee, this was a fun one! ;)
navigator.languages is based on the value of the pref intl.accept_languages. This defaults to
pref("intl.accept_languages", "chrome://global/locale/intl.properties");
in all.js. Navigator.cpp reads this as a SpecialValue(), of type localized string. This kicks in special-case logic to
- if the value is default, look up the key in a .properties file specified by the default value
- otherwise, return the (user) value
When we serialize the pref DB, we forget whether the value is user or default. B2G currently updates intl.accept_languages to match the user-selected locale, so the pref is normally user set (though can still be default in certain race conditions).
However, the content process always thinks the value is default, in all circumstances. So, in this bug, we tried to read the value from an "en-US" properties URI. Whups!
This patch is quite straightforward but rather gross. By my reading of prefapi.cpp, it should still respect default/user semantics correctly, though a bit subtly.
This code is quite dated and vastly overcomplex, but rewriting is not worth the time at the moment. We need to overhaul prefs to prevent information leaks first, anyway.
Assignee: nobody → jones.chris.g
Attachment #652369 -
Flags: review?(benjamin)
Comment 7•13 years ago
|
||
Comment on attachment 652369 [details] [diff] [review]
Serialize whether the pref is user or default because that can affect semantics
I believe that this is correct. As noted on IRC, I'd really like a test for this, probably an xpcshell test. You can check that values are default or user with prefs.prefHasUserValue(name);
Attachment #652369 -
Flags: review?(benjamin) → review+
Updated•13 years ago
|
Flags: in-testsuite?
Assignee | ||
Comment 8•13 years ago
|
||
Fails without, passes with. If prelaunch is enabled and this fix regresses, then this test will go intermittent orange.
Attachment #652851 -
Flags: review?(josh)
Comment 9•13 years ago
|
||
Comment on attachment 652851 [details] [diff] [review]
Test
This is good, but your use of do_test_pending and do_test_finished is really confusing. Could you just surround the continuation call with do_test_pending and do_test_finished to achieve the same effect?
Attachment #652851 -
Flags: review?(josh) → review+
Assignee | ||
Comment 10•13 years ago
|
||
The test as written passes, but with the do_test_pending/do_test_finished change, fails. wtf
Assignee | ||
Comment 11•13 years ago
|
||
On tryserver, too.
Comment 12•13 years ago
|
||
Oh, I forgot that run_test_in_child performs a do_test_pending but not do_test_finished, so we need to account for that.
Assignee | ||
Comment 13•13 years ago
|
||
I'm not using that (AFAIK), but it turns out that I had an extra do_test_finished(). Whups!
Assignee | ||
Comment 14•13 years ago
|
||
There were some strange-looking reftest-ipc failures on try with the previous patch. Probably some kind of subtle bug.
I decided it was easier to rewrite the impl to something I knew would work than debug the subtlety. And as a bonus, this is a net reduction in code.
Attachment #652369 -
Attachment is obsolete: true
Attachment #653125 -
Flags: review?(josh)
Updated•13 years ago
|
Attachment #653125 -
Flags: review?(josh) → review+
Assignee | ||
Comment 16•13 years ago
|
||
Thanks for stealing!
Assignee | ||
Comment 17•13 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/525302117187
https://hg.mozilla.org/integration/mozilla-inbound/rev/739eef9dbbdd
Flags: in-testsuite? → in-testsuite+
Comment 18•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/525302117187
https://hg.mozilla.org/mozilla-central/rev/739eef9dbbdd
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•