Fix variations locale logic broken in M71.

There were two issues:

1. The new language::GetApplicationLocale() function that was
introduced as part of M71 changes to variations service did not
match the previous behavior - it had an early return when the
locale pref didn't exist - which would be hit when the user didn't
explicitly set their language in Chrome. But that's not sufficient
since l10n_util::GetApplicationLocale() actually does logic beyond
what's provided to it via the pref. On Mac, it gets the language
from the system. The changes to components/language were sufficient
to fix things on Mac.

2. On non-Mac platforms, the call to l10n_util::GetApplicationLocale()
also had a dependency on ResourceBundle being initialized, so this part
did not work correctly either. IsLocaleAvailable() was returning false
when there was no ResourceBundle. There was also a TODO to make the
LocaleDataPakExists() API static - which then could be used without
ResourceBundle being initialized. This was mostly straight-forward
except for use of delegate_->GetPathForLocalePack(). This delegate
API was only overridden (outside of tests) by CastResourceDelegate.
Given cast doesn't use the code path in question (variations), the
code is changed to only use that delegate if the resource bundle has
been initialized.

Also adds a check that the locale determined after resource bundle
has been loaded matches what was used to initialize variations.

Bug: 908114
Change-Id: Ief6bf3e370bf2773187387c1fd12fc4517b31a69
Reviewed-on: https://chromium-review.googlesource.com/c/1349770
Commit-Queue: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Reviewed-by: anthonyvd <anthonyvd@chromium.org>
Reviewed-by: Xi Han <hanxi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610954}

chrome/browser/metrics/chrome_feature_list_creator.cc

@@ -79,6 +79,11 @@ void ChromeFeatureListCreator::CreateFeatureList() {
   SetupFieldTrials();
 }
 
+void ChromeFeatureListCreator::SetApplicationLocale(const std::string& locale) {
+  actual_locale_ = locale;
+  metrics_services_manager_->GetVariationsService()->EnsureLocaleEquals(locale);
+}
+
 metrics_services_manager::MetricsServicesManagerClient*
 ChromeFeatureListCreator::GetMetricsServicesManagerClient() {
   return metrics_services_manager_client_;

chrome/browser/metrics/chrome_feature_list_creator.h

@@ -29,6 +29,10 @@ class ChromeFeatureListCreator {
   // base::FeatureList::SetInstance() to set the global instance.
   void CreateFeatureList();
 
+  // Sets the application locale and verifies (via a CHECK) that it matches
+  // what was used when creating field trials.
+  void SetApplicationLocale(const std::string& locale);
+
   // Gets the MetricsServicesManagerClient* used in this class.
   metrics_services_manager::MetricsServicesManagerClient*
   GetMetricsServicesManagerClient();
@@ -53,10 +57,6 @@ class ChromeFeatureListCreator {
   }
   const std::string& actual_locale() { return actual_locale_; }
 
-  void SetApplicationLocale(const std::string& locale) {
-    actual_locale_ = locale;
-  }
-
  private:
   void CreatePrefService();
   void ConvertFlagsToSwitches();

components/language/core/browser/locale_util.cc

@@ -4,6 +4,7 @@
 
 #include "components/language/core/browser/locale_util.h"
 
+#include "build/build_config.h"
 #include "components/language/core/browser/pref_names.h"
 #include "components/prefs/pref_service.h"
 #include "ui/base/l10n/l10n_util.h"
@@ -11,10 +12,20 @@
 namespace language {
 
 std::string GetApplicationLocale(PrefService* local_state) {
-  if (!local_state->HasPrefPath(prefs::kApplicationLocale))
-    return std::string();
-  std::string locale = local_state->GetString(prefs::kApplicationLocale);
-  return l10n_util::GetApplicationLocale(locale);
+  std::string preferred_locale;
+  // Note: This logic should match InitResourceBundleAndDetermineLocale() and
+  // LoadLocaleResources(), which is how the global locale is set.
+  // TODO(asvitkine): We should try to refactor things so that the logic is not
+  // duplicated in multiple files.
+#if !defined(OS_MACOSX)
+  // The pref isn't always registered in unit tests.
+  if (local_state->HasPrefPath(prefs::kApplicationLocale))
+    preferred_locale = local_state->GetString(prefs::kApplicationLocale);
+#endif
+  // Note: The call below is necessary even if |preferred_locale| is empty, as
+  // it will get the locale that should be used potentially from other sources,
+  // depending on the platform (e.g. the OS locale on Mac).
+  return l10n_util::GetApplicationLocale(preferred_locale);
 }
 
 }  // namespace language

components/variations/service/variations_field_trial_creator.h

@@ -100,6 +100,9 @@ class VariationsFieldTrialCreator {
   // Returns whether the map of the cached UI strings to override is empty.
   bool IsOverrideResourceMapEmpty();
 
+  // Returns the locale that was used for evaluating trials.
+  const std::string& application_locale() const { return application_locale_; }
+
   // Returns the short hardware class value used to evaluate variations hardware
   // class filters. Only implemented on CrOS - returns empty string on other
   // platforms.

components/variations/service/variations_service.cc

@@ -419,6 +419,13 @@ GURL VariationsService::GetVariationsServerURL(HttpOptions http_options) {
   return server_url;
 }
 
+void VariationsService::EnsureLocaleEquals(const std::string& locale) {
+  // Uses a CHECK rather than a DCHECK to ensure that issues are caught since
+  // problems in this area may only appear in the wild due to official builds
+  // and end user machines.
+  CHECK_EQ(locale, field_trial_creator_.application_locale());
+}
+
 // static
 std::string VariationsService::GetDefaultVariationsServerURLForTesting() {
   return kDefaultServerUrl;

components/variations/service/variations_service.h

@@ -128,6 +128,12 @@ class VariationsService
   // empty if it is not available.
   std::string GetLatestCountry() const;
 
+  // Ensures the locale that was used for evaluating variations matches the
+  // passed |locale|. This is used to ensure that the locale determined after
+  // loading the resource bundle (which is passed here) corresponds to what
+  // was used for variations during an earlier stage of start up.
+  void EnsureLocaleEquals(const std::string& locale);
+
   // Exposed for testing.
   static std::string GetDefaultVariationsServerURLForTesting();
 

ui/base/l10n/l10n_util.cc

@@ -268,15 +268,7 @@ bool IsLocaleAvailable(const std::string& locale) {
   if (!l10n_util::IsLocaleSupportedByOS(locale))
     return false;
 
-  // If the ResourceBundle is not yet initialized, return false to avoid the
-  // CHECK failure in ResourceBundle::GetSharedInstance().
-  if (!ui::ResourceBundle::HasSharedInstance())
-    return false;
-
-  // TODO(hshi): make ResourceBundle::LocaleDataPakExists() a static function
-  // so that this can be invoked without initializing the global instance.
-  // See crbug.com/230432: CHECK failure in GetUserDataDir().
-  return ui::ResourceBundle::GetSharedInstance().LocaleDataPakExists(locale);
+  return ui::ResourceBundle::LocaleDataPakExists(locale);
 }
 #endif
 

ui/base/resource/resource_bundle.cc

@@ -242,6 +242,7 @@ void ResourceBundle::LoadSecondaryLocaleDataWithPakFileRegion(
 }
 
 #if !defined(OS_ANDROID)
+// static
 bool ResourceBundle::LocaleDataPakExists(const std::string& locale) {
   return !GetLocaleFilePath(locale, true).empty();
 }
@@ -288,6 +289,7 @@ void ResourceBundle::AddDataPackFromFileRegion(
 }
 
 #if !defined(OS_MACOSX)
+// static
 base::FilePath ResourceBundle::GetLocaleFilePath(const std::string& app_locale,
                                                  bool test_file_exists) {
   if (app_locale.empty())
@@ -316,9 +318,14 @@ base::FilePath ResourceBundle::GetLocaleFilePath(const std::string& app_locale,
 #endif
   }
 
-  if (delegate_) {
-    locale_file_path =
-        delegate_->GetPathForLocalePack(locale_file_path, app_locale);
+  // Note: The delegate GetPathForLocalePack() override is currently only used
+  // by CastResourceDelegate, which does not call this function prior to
+  // initializing the ResourceBundle. This called earlier than that by the
+  // variations code which also has a CHECK that an inconsistent value does not
+  // get returned via VariationsService::EnsureLocaleEquals().
+  if (HasSharedInstance() && GetSharedInstance().delegate_) {
+    locale_file_path = GetSharedInstance().delegate_->GetPathForLocalePack(
+        locale_file_path, app_locale);
   }
 
   // Don't try to load empty values or values that are not absolute paths.

ui/base/resource/resource_bundle.h

@@ -156,7 +156,7 @@ class UI_BASE_EXPORT ResourceBundle {
       const base::MemoryMappedFile::Region& region);
 
   // Check if the .pak for the given locale exists.
-  bool LocaleDataPakExists(const std::string& locale);
+  static bool LocaleDataPakExists(const std::string& locale);
 
   // Registers additional data pack files with this ResourceBundle.  When
   // looking for a DataResource, we will search these files after searching the
@@ -283,8 +283,8 @@ class UI_BASE_EXPORT ResourceBundle {
   // string if no locale data files are found and |test_file_exists| is true.
   // Used on Android to load the local file in the browser process and pass it
   // to the sandboxed renderer process.
-  base::FilePath GetLocaleFilePath(const std::string& app_locale,
-                                   bool test_file_exists);
+  static base::FilePath GetLocaleFilePath(const std::string& app_locale,
+                                          bool test_file_exists);
 
   // Returns the maximum scale factor currently loaded.
   // Returns SCALE_FACTOR_100P if no resource is loaded.

ui/base/resource/resource_bundle_android.cc

@@ -92,6 +92,7 @@ void ResourceBundle::LoadCommonResources() {
   }
 }
 
+// static
 bool ResourceBundle::LocaleDataPakExists(const std::string& locale) {
   if (g_locale_paks_in_apk) {
     return !GetPathForAndroidLocalePakWithinApk(locale).empty();

ui/base/resource/resource_bundle_ios.mm

@@ -59,6 +59,7 @@ void ResourceBundle::LoadCommonResources() {
   }
 }
 
+// static
 base::FilePath ResourceBundle::GetLocaleFilePath(const std::string& app_locale,
                                                  bool test_file_exists) {
   NSString* mac_locale = base::SysUTF8ToNSString(app_locale);
@@ -74,9 +75,9 @@ base::FilePath ResourceBundle::GetLocaleFilePath(const std::string& app_locale,
   base::FilePath locale_file_path =
       GetResourcesPakFilePath(@"locale", mac_locale);
 
-  if (delegate_) {
-    locale_file_path =
-        delegate_->GetPathForLocalePack(locale_file_path, app_locale);
+  if (HasSharedInstance() && GetSharedInstance().delegate_) {
+    locale_file_path = GetSharedInstance().delegate_->GetPathForLocalePack(
+        locale_file_path, app_locale);
   }
 
   // Don't try to load empty values or values that are not absolute paths.

ui/base/resource/resource_bundle_mac.mm

@@ -59,6 +59,7 @@ void ResourceBundle::LoadCommonResources() {
   }
 }
 
+// static
 base::FilePath ResourceBundle::GetLocaleFilePath(const std::string& app_locale,
                                                  bool test_file_exists) {
   NSString* mac_locale = base::SysUTF8ToNSString(app_locale);
@@ -74,9 +75,9 @@ base::FilePath ResourceBundle::GetLocaleFilePath(const std::string& app_locale,
   base::FilePath locale_file_path =
       GetResourcesPakFilePath(@"locale", mac_locale);
 
-  if (delegate_) {
-    locale_file_path =
-        delegate_->GetPathForLocalePack(locale_file_path, app_locale);
+  if (HasSharedInstance() && GetSharedInstance().delegate_) {
+    locale_file_path = GetSharedInstance().delegate_->GetPathForLocalePack(
+        locale_file_path, app_locale);
   }
 
   // Don't try to load empty values or values that are not absolute paths.

ui/base/resource/resource_bundle_unittest.cc

@@ -176,27 +176,31 @@ TEST_F(ResourceBundleTest, DelegateGetPathForResourcePack) {
 }
 
 TEST_F(ResourceBundleTest, DelegateGetPathForLocalePack) {
+  ResourceBundle::CleanupSharedInstance();
+
   MockResourceBundleDelegate delegate;
-  ResourceBundle* resource_bundle = CreateResourceBundle(&delegate);
+  ResourceBundle::InitSharedInstance(&delegate);
 
   std::string locale = "en-US";
 
   // Cancel the load.
-  EXPECT_CALL(delegate, GetPathForLocalePack(_, locale))
-      .Times(2)
+  EXPECT_CALL(delegate, GetPathForLocalePack(_, _))
       .WillRepeatedly(Return(base::FilePath()))
       .RetiresOnSaturation();
 
-  EXPECT_FALSE(resource_bundle->LocaleDataPakExists(locale));
-  EXPECT_EQ("", resource_bundle->LoadLocaleResources(locale));
+  EXPECT_FALSE(ResourceBundle::LocaleDataPakExists(locale));
+  EXPECT_EQ("",
+            ResourceBundle::GetSharedInstance().LoadLocaleResources(locale));
 
   // Allow the load to proceed.
-  EXPECT_CALL(delegate, GetPathForLocalePack(_, locale))
-      .Times(2)
+  EXPECT_CALL(delegate, GetPathForLocalePack(_, _))
       .WillRepeatedly(ReturnArg<0>());
 
-  EXPECT_TRUE(resource_bundle->LocaleDataPakExists(locale));
-  EXPECT_EQ(locale, resource_bundle->LoadLocaleResources(locale));
+  EXPECT_TRUE(ResourceBundle::LocaleDataPakExists(locale));
+  EXPECT_EQ(locale,
+            ResourceBundle::GetSharedInstance().LoadLocaleResources(locale));
+
+  ResourceBundle::CleanupSharedInstance();
 }
 
 TEST_F(ResourceBundleTest, DelegateGetImageNamed) {
@@ -336,11 +340,9 @@ TEST_F(ResourceBundleTest, DelegateGetLocalizedStringWithOverride) {
 }
 
 TEST_F(ResourceBundleTest, LocaleDataPakExists) {
-  ResourceBundle* resource_bundle = CreateResourceBundle(nullptr);
-
   // Check that ResourceBundle::LocaleDataPakExists returns the correct results.
-  EXPECT_TRUE(resource_bundle->LocaleDataPakExists("en-US"));
-  EXPECT_FALSE(resource_bundle->LocaleDataPakExists("not_a_real_locale"));
+  EXPECT_TRUE(ResourceBundle::LocaleDataPakExists("en-US"));
+  EXPECT_FALSE(ResourceBundle::LocaleDataPakExists("not_a_real_locale"));
 }
 
 class ResourceBundleImageTest : public ResourceBundleTest {