The default bug view has changed. See this FAQ.

navigator.language is not defined for OOP apps, which breaks localization

RESOLVED FIXED in mozilla17

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: djf, Assigned: cjones)

Tracking

Trunk
mozilla17
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-basecamp:+)

Details

(Whiteboard: [LOE:S])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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).
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: 745143
blocking-basecamp: --- → +
nsPrefBranch::GetComplexValue is failing at

      rv = GetDefaultFromPropertiesFile(pref, getter_Copies(utf16String));
Because nsStringBundle::GetStringFromName(const PRUnichar *aName, PRUnichar **aResult) is failing at

  rv = LoadProperties();
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 ...
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.
Created attachment 652369 [details] [diff] [review]
Serialize whether the pref is user or default because that can affect semantics

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 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+
Flags: in-testsuite?
Created attachment 652851 [details] [diff] [review]
Test

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

5 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+
The test as written passes, but with the do_test_pending/do_test_finished change, fails.  wtf
On tryserver, too.
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.
I'm not using that (AFAIK), but it turns out that I had an extra do_test_finished().  Whups!
Created attachment 653125 [details] [diff] [review]
Ensure that child-process pref state always is the same as its parent's

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)
Green on tryserver, awaiting review to land.
Whiteboard: [LOE:S]
Attachment #653125 - Flags: review?(josh) → review+
Thanks for stealing!
https://hg.mozilla.org/integration/mozilla-inbound/rev/525302117187
https://hg.mozilla.org/integration/mozilla-inbound/rev/739eef9dbbdd
Flags: in-testsuite? → in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/525302117187
https://hg.mozilla.org/mozilla-central/rev/739eef9dbbdd
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.