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

Status: RESOLVED FIXED

Description

[David Flanagan [:djf]]

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

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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?

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

nsPrefBranch::GetComplexValue is failing at

      rv = GetDefaultFromPropertiesFile(pref, getter_Copies(utf16String));

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Because nsStringBundle::GetStringFromName(const PRUnichar *aName, PRUnichar **aResult) is failing at

  rv = LoadProperties();

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 7

[Benjamin Smedberg]

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

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 9

[Josh Matthews [:jdm]]

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?

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The test as written passes, but with the do_test_pending/do_test_finished change, fails.  wtf

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

On tryserver, too.

Comment 12

[Josh Matthews [:jdm]]

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.

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I'm not using that (AFAIK), but it turns out that I had an extra do_test_finished().  Whups!

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Green on tryserver, awaiting review to land.

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Thanks for stealing!

Comment 17

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://hg.mozilla.org/integration/mozilla-inbound/rev/525302117187
https://hg.mozilla.org/integration/mozilla-inbound/rev/739eef9dbbdd

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/525302117187
https://hg.mozilla.org/mozilla-central/rev/739eef9dbbdd