Last Comment Bug 783184 - navigator.language is not defined for OOP apps, which breaks localization
: navigator.language is not defined for OOP apps, which breaks localization
Status: RESOLVED FIXED
[LOE:S]
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla17
Assigned To: Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
:
Mentors:
Depends on:
Blocks: b2g-e10s-work
  Show dependency treegraph
 
Reported: 2012-08-16 00:03 PDT by David Flanagan [:djf]
Modified: 2012-08-23 03:48 PDT (History)
9 users (show)
cjones.bugs: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
Serialize whether the pref is user or default because that can affect semantics (6.59 KB, patch)
2012-08-16 02:07 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
benjamin: review+
Details | Diff | Splinter Review
Test (3.65 KB, patch)
2012-08-17 11:41 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
josh: review+
Details | Diff | Splinter Review
Ensure that child-process pref state always is the same as its parent's (25.98 KB, patch)
2012-08-18 19:38 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
benjamin: review+
Details | Diff | Splinter Review

Description David Flanagan [:djf] 2012-08-16 00:03:13 PDT
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 2012-08-16 00:19:51 PDT
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 2012-08-16 00:34:01 PDT
nsPrefBranch::GetComplexValue is failing at

      rv = GetDefaultFromPropertiesFile(pref, getter_Copies(utf16String));
Comment 3 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-16 00:38:43 PDT
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 2012-08-16 00:46:37 PDT
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 2012-08-16 00:50:48 PDT
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 2012-08-16 02:07:33 PDT
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 AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2012-08-17 09:41:29 PDT
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 2012-08-17 11:41:57 PDT
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] 2012-08-17 13:41:08 PDT
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 2012-08-18 17:28:55 PDT
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 2012-08-18 18:52:51 PDT
On tryserver, too.
Comment 12 Josh Matthews [:jdm] 2012-08-18 18:58:23 PDT
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 2012-08-18 19:34:02 PDT
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 2012-08-18 19:38:29 PDT
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 2012-08-22 04:15:32 PDT
Green on tryserver, awaiting review to land.
Comment 16 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-22 12:23:26 PDT
Thanks for stealing!

Note You need to log in before you can comment on or make changes to this bug.