Bug 1354055 - Don't cache wrong result in OSPreferences::ReadSystemLocales on Android. r?rnewman draft
authorZibi Braniecki <gandalf@mozilla.com>
Thu, 06 Apr 2017 13:00:02 +0200
changeset 561495 f85ef74277714b5b1ab12577d47d2d52cea041c0
parent 560921 f40e24f40b4c4556944c762d4764eace261297f5
child 623991 cae6818d1e567e169cefb0b8913a99608a0c3286
push id53749
push userzbraniecki@mozilla.com
push dateWed, 12 Apr 2017 19:17:09 +0000
reviewersrnewman
bugs1354055
milestone55.0a1
Bug 1354055 - Don't cache wrong result in OSPreferences::ReadSystemLocales on Android. r?rnewman MozReview-Commit-ID: Li7wUQnC9Gz
intl/locale/OSPreferences.cpp
intl/locale/android/OSPreferences_android.cpp
intl/locale/tests/unit/test_localeService.js
--- a/intl/locale/OSPreferences.cpp
+++ b/intl/locale/OSPreferences.cpp
@@ -33,22 +33,31 @@ OSPreferences::GetInstance()
     ClearOnShutdown(&sInstance);
   }
   return sInstance;
 }
 
 bool
 OSPreferences::GetSystemLocales(nsTArray<nsCString>& aRetVal)
 {
-  bool status = true;
-  if (mSystemLocales.IsEmpty()) {
-    status = ReadSystemLocales(mSystemLocales);
+  if (!mSystemLocales.IsEmpty()) {
+    aRetVal = mSystemLocales;
+    return true;
   }
-  aRetVal = mSystemLocales;
-  return status;
+
+  if (ReadSystemLocales(aRetVal)) {
+    mSystemLocales = aRetVal;
+    return true;
+  }
+
+  // If we failed to get the system locale, we still need
+  // to return something because there are tests out there that
+  // depend on system locale to be set.
+  aRetVal.AppendElement(NS_LITERAL_CSTRING("en-US"));
+  return false;
 }
 
 void
 OSPreferences::Refresh()
 {
   nsTArray<nsCString> newLocales;
   ReadSystemLocales(newLocales);
 
@@ -289,39 +298,48 @@ OSPreferences::GetDateTimeConnectorPatte
 }
 
 /**
  * mozIOSPreferences methods
  */
 NS_IMETHODIMP
 OSPreferences::GetSystemLocales(uint32_t* aCount, char*** aOutArray)
 {
-  if (mSystemLocales.IsEmpty()) {
-    ReadSystemLocales(mSystemLocales);
+  AutoTArray<nsCString,10> tempLocales;
+  nsTArray<nsCString>* systemLocalesPtr;
+
+  if (!mSystemLocales.IsEmpty()) {
+    // use cached value
+    systemLocalesPtr = &mSystemLocales;
+  } else {
+    // get a (perhaps temporary/fallback/hack) value
+    GetSystemLocales(tempLocales);
+    systemLocalesPtr = &tempLocales;
   }
-
-  *aCount = mSystemLocales.Length();
+  *aCount = systemLocalesPtr->Length();
   *aOutArray = static_cast<char**>(moz_xmalloc(*aCount * sizeof(char*)));
 
   for (uint32_t i = 0; i < *aCount; i++) {
-    (*aOutArray)[i] = moz_xstrdup(mSystemLocales[i].get());
+    (*aOutArray)[i] = moz_xstrdup((*systemLocalesPtr)[i].get());
   }
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
 OSPreferences::GetSystemLocale(nsACString& aRetVal)
 {
-  if (mSystemLocales.IsEmpty()) {
-    ReadSystemLocales(mSystemLocales);
-  }
-
   if (!mSystemLocales.IsEmpty()) {
     aRetVal = mSystemLocales[0];
+  } else {
+    AutoTArray<nsCString,10> locales;
+    GetSystemLocales(locales);
+    if (!locales.IsEmpty()) {
+      aRetVal = locales[0];
+    }
   }
   return NS_OK;
 }
 
 static OSPreferences::DateTimeFormatStyle
 ToDateTimeFormatStyle(int32_t aTimeFormat)
 {
   switch (aTimeFormat) {
--- a/intl/locale/android/OSPreferences_android.cpp
+++ b/intl/locale/android/OSPreferences_android.cpp
@@ -10,23 +10,26 @@
 using namespace mozilla::intl;
 
 bool
 OSPreferences::ReadSystemLocales(nsTArray<nsCString>& aLocaleList)
 {
   //XXX: This is a quite sizable hack to work around the fact that we cannot
   //     retrieve OS locale in C++ without reaching out to JNI.
   //     Once we fix this (bug 1337078), this hack should not be necessary.
-  nsAutoCString locale;
-  if (!NS_SUCCEEDED(Preferences::GetCString("intl.locale.os", &locale)) ||
-      locale.IsEmpty()) {
-    locale.AssignLiteral("en-US");
+  //
+  //XXX: Notice, this value may be empty on an early read. In that case
+  //     we won't add anything to the return list so that it doesn't get
+  //     cached in mSystemLocales.
+  nsAdoptingCString locale = Preferences::GetCString("intl.locale.os");
+  if (!locale.IsEmpty()) {
     aLocaleList.AppendElement(locale);
+    return true;
   }
-  return true;
+  return false;
 }
 
 bool
 OSPreferences::ReadDateTimePattern(DateTimeFormatStyle aDateStyle,
                                    DateTimeFormatStyle aTimeStyle,
                                    const nsACString& aLocale, nsAString& aRetVal)
 {
   return false;
--- a/intl/locale/tests/unit/test_localeService.js
+++ b/intl/locale/tests/unit/test_localeService.js
@@ -40,16 +40,17 @@ add_test(function test_getAppLocalesAsLa
   const enUSLocales = appLocales.filter(loc => loc === "en-US");
   do_check_true(enUSLocales.length == 1, "en-US is present exactly one time");
 
   run_next_test();
 });
 
 const PREF_MATCH_OS_LOCALE = "intl.locale.matchOS";
 const PREF_SELECTED_LOCALE = "general.useragent.locale";
+const PREF_OS_LOCALE       = "intl.locale.os";
 const REQ_LOC_CHANGE_EVENT = "intl:requested-locales-changed";
 
 add_test(function test_getRequestedLocales() {
   const requestedLocales = localeService.getRequestedLocales();
   do_check_true(Array.isArray(requestedLocales), "requestedLocales returns an array");
 
   run_next_test();
 });
@@ -62,16 +63,17 @@ add_test(function test_getRequestedLocal
  * Then, we test that when the matchOS is set to true, we will retrieve
  * OS locale from getRequestedLocales.
  */
 add_test(function test_getRequestedLocales_matchOS() {
   do_test_pending();
 
   Services.prefs.setBoolPref(PREF_MATCH_OS_LOCALE, false);
   Services.prefs.setCharPref(PREF_SELECTED_LOCALE, "ar-IR");
+  Services.prefs.setCharPref(PREF_OS_LOCALE, "en-US");
 
   const observer = {
     observe: function (aSubject, aTopic, aData) {
       switch (aTopic) {
         case REQ_LOC_CHANGE_EVENT:
           const reqLocs = localeService.getRequestedLocales();
           do_check_true(reqLocs[0] === osPrefs.systemLocale);
           Services.obs.removeObserver(observer, REQ_LOC_CHANGE_EVENT);
@@ -133,11 +135,12 @@ add_test(function test_setRequestedLocal
 
 add_test(function test_isAppLocaleRTL() {
   do_check_true(typeof localeService.isAppLocaleRTL === 'boolean');
 
   run_next_test();
 });
 
 do_register_cleanup(() => {
-    Services.prefs.clearUserPref(PREF_SELECTED_LOCALE);
-    Services.prefs.clearUserPref(PREF_MATCH_OS_LOCALE);
+  Services.prefs.clearUserPref(PREF_SELECTED_LOCALE);
+  Services.prefs.clearUserPref(PREF_MATCH_OS_LOCALE);
+  Services.prefs.clearUserPref(PREF_OS_LOCALE, "en-US");
 });