Fix crash OverrideUIString() is called before resource bundle is initialized.

The crash is caused by the callback UIStringOverrider::OverrideUIString() in
ApplyUIStringOverrides() is called before resource bundle is initialized. In
this CL, we checks whether the resource bundle has been initialized or not. If
not, we cache the UI strings to be override in a map owned by the
VariationsFieldTrialCreator, and override these UI strings once the full browser
starts.

Bug: 893707
Change-Id: Ia23bc46a5e13a20ab784ff48d565ce1438a6aa0b
Reviewed-on: https://chromium-review.googlesource.com/c/1274001
Commit-Queue: Xi Han <hanxi@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600564}

chrome/browser/browser_process_impl.cc

@@ -115,6 +115,7 @@
 #include "components/translate/core/browser/translate_download_manager.h"
 #include "components/ukm/ukm_service.h"
 #include "components/update_client/update_query_params.h"
+#include "components/variations/service/variations_service.h"
 #include "components/web_resource/web_resource_pref_names.h"
 #include "content/public/browser/browser_task_traits.h"
 #include "content/public/browser/browser_thread.h"
@@ -474,6 +475,7 @@ void BrowserProcessImpl::SetMetricsServices(
   metrics_services_manager_ = std::move(manager);
   metrics_services_manager_client_ =
       static_cast<ChromeMetricsServicesManagerClient*>(client);
+  metrics_services_manager_->GetVariationsService()->OverrideCachedUIStrings();
 }
 
 namespace {

components/variations/service/ui_string_overrider.cc

@@ -38,14 +38,4 @@ int UIStringOverrider::GetResourceIndex(uint32_t hash) {
   return resource_indices_[element - resource_hashes_];
 }
 
-void UIStringOverrider::OverrideUIString(uint32_t hash,
-                                         const base::string16& string) {
-  int resource_id = GetResourceIndex(hash);
-  if (resource_id == -1)
-    return;
-
-  ui::ResourceBundle::GetSharedInstance().OverrideLocaleStringResource(
-      resource_id, string);
-}
-
 }  // namespace variations

components/variations/service/ui_string_overrider.h

@@ -42,10 +42,6 @@ class UIStringOverrider {
   // resources is found. Visible for testing.
   int GetResourceIndex(uint32_t hash);
 
-  // Overrides the string resource specified by |hash| with |string| in the
-  // resource bundle.
-  void OverrideUIString(uint32_t hash, const base::string16& string);
-
  private:
   const uint32_t* const resource_hashes_;
   const int* const resource_indices_;

components/variations/service/variations_field_trial_creator.cc

@@ -37,6 +37,7 @@
 #include "components/variations/variations_switches.h"
 #include "ui/base/device_form_factor.h"
 #include "ui/base/l10n/l10n_util.h"
+#include "ui/base/resource/resource_bundle.h"
 
 namespace variations {
 namespace {
@@ -210,14 +211,15 @@ bool VariationsFieldTrialCreator::CreateTrialsFromSeed(
   UMA_HISTOGRAM_BOOLEAN("Variations.SafeMode.FellBackToSafeMode2",
                         run_in_safe_mode);
 
-  // Note that passing |&ui_string_overrider_| via base::Unretained below is
-  // safe because the callback is executed synchronously. It is not possible to
-  // pass UIStringOverrider directly to VariationSeedProcessor as the variations
-  // component should not depend on //ui/base.
+  // Note that passing base::Unretained(this) below is safe because the callback
+  // is executed synchronously. It is not possible to pass UIStringOverrider
+  // directly to VariationsSeedProcessor (which is in components/variations and
+  // not components/variations/service) as the variations component should not
+  // depend on //ui/base.
   VariationsSeedProcessor().CreateTrialsFromSeed(
       seed, *client_filterable_state,
-      base::Bind(&UIStringOverrider::OverrideUIString,
-                 base::Unretained(&ui_string_overrider_)),
+      base::BindRepeating(&VariationsFieldTrialCreator::OverrideUIString,
+                          base::Unretained(this)),
       low_entropy_provider.get(), feature_list);
 
   // Store into the |safe_seed_manager| the combined server and client data used
@@ -353,6 +355,18 @@ void VariationsFieldTrialCreator::OverrideVariationsPlatform(
   platform_override_ = platform_override;
 }
 
+void VariationsFieldTrialCreator::OverrideCachedUIStrings() {
+  DCHECK(ui::ResourceBundle::HasSharedInstance());
+
+  ui::ResourceBundle* bundle = &ui::ResourceBundle::GetSharedInstance();
+  bundle->CheckCanOverrideStringResources();
+
+  for (auto const& it : overridden_strings_map_)
+    bundle->OverrideLocaleStringResource(it.first, it.second);
+
+  overridden_strings_map_.clear();
+}
+
 // static
 std::string VariationsFieldTrialCreator::GetShortHardwareClass() {
 #if defined(OS_CHROMEOS)
@@ -510,6 +524,27 @@ bool VariationsFieldTrialCreator::SetupFieldTrials(
   return used_seed;
 }
 
+void VariationsFieldTrialCreator::OverrideUIString(uint32_t resource_hash,
+                                                   const base::string16& str) {
+  int resource_id = ui_string_overrider_.GetResourceIndex(resource_hash);
+  if (resource_id == -1)
+    return;
+
+  // This function may be called before the resource bundle is initialized. So
+  // we cache the UI strings and override them after the full browser starts.
+  if (!ui::ResourceBundle::HasSharedInstance()) {
+    overridden_strings_map_[resource_id] = str;
+    return;
+  }
+
+  ui::ResourceBundle::GetSharedInstance().OverrideLocaleStringResource(
+      resource_id, str);
+}
+
+bool VariationsFieldTrialCreator::IsOverrideResourceMapEmpty() {
+  return overridden_strings_map_.empty();
+}
+
 VariationsSeedStore* VariationsFieldTrialCreator::GetSeedStore() {
   return seed_store_.get();
 }

components/variations/service/variations_field_trial_creator.h

@@ -11,6 +11,7 @@
 #include <vector>
 
 #include "base/compiler_specific.h"
+#include "base/containers/hash_tables.h"
 #include "base/macros.h"
 #include "base/metrics/field_trial.h"
 #include "components/variations/client_filterable_state.h"
@@ -93,6 +94,12 @@ class VariationsFieldTrialCreator {
   // overridden.
   void OverrideVariationsPlatform(Study::Platform platform_override);
 
+  // Overrides cached UI strings on the resource bundle once it is initialized.
+  void OverrideCachedUIStrings();
+
+  // Returns whether the map of the cached UI strings to override is empty.
+  bool IsOverrideResourceMapEmpty();
+
   // Returns the short hardware class value used to evaluate variations hardware
   // class filters. Only implemented on CrOS - returns empty string on other
   // platforms.
@@ -126,6 +133,10 @@ class VariationsFieldTrialCreator {
       base::FeatureList* feature_list,
       SafeSeedManager* safe_seed_manager);
 
+  // Overrides the string resource specified by |hash| with |str| in the
+  // resource bundle.
+  void OverrideUIString(uint32_t hash, const base::string16& str);
+
   // Returns the seed store. Virtual for testing.
   virtual VariationsSeedStore* GetSeedStore();
 
@@ -158,6 +169,10 @@ class VariationsFieldTrialCreator {
   // platform.
   Study::Platform platform_override_;
 
+  // Caches the UI strings which need to be overridden in the resource bundle.
+  // These strings are cached before the resource bundle is initialized.
+  base::hash_map<int, base::string16> overridden_strings_map_;
+
   SEQUENCE_CHECKER(sequence_checker_);
 
   DISALLOW_COPY_AND_ASSIGN(VariationsFieldTrialCreator);

components/variations/service/variations_service.cc

@@ -299,6 +299,7 @@ VariationsService::~VariationsService() {
 
 void VariationsService::PerformPreMainMessageLoopStartup() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  DCHECK(field_trial_creator_.IsOverrideResourceMapEmpty());
 
   InitResourceRequestedAllowedNotifier();
 
@@ -824,6 +825,10 @@ bool VariationsService::SetupFieldTrials(
       std::move(feature_list), platform_field_trials, &safe_seed_manager_);
 }
 
+void VariationsService::OverrideCachedUIStrings() {
+  field_trial_creator_.OverrideCachedUIStrings();
+}
+
 std::string VariationsService::GetStoredPermanentCountry() {
   const base::ListValue* list_value =
       local_state_->GetList(prefs::kVariationsPermanentConsistencyCountry);

components/variations/service/variations_service.h

@@ -175,6 +175,9 @@ class VariationsService
                         std::unique_ptr<base::FeatureList> feature_list,
                         variations::PlatformFieldTrials* platform_field_trials);
 
+  // Overrides cached UI strings on the resource bundle once it is initialized.
+  void OverrideCachedUIStrings();
+
   int request_count() const { return request_count_; }
 
  protected:

ui/base/resource/resource_bundle.cc

@@ -402,9 +402,9 @@ void ResourceBundle::OverrideLocalePakForTest(const base::FilePath& pak_path) {
 }
 
 void ResourceBundle::OverrideLocaleStringResource(
-    int message_id,
+    int resource_id,
     const base::string16& string) {
-  overridden_locale_strings_[message_id] = string;
+  overridden_locale_strings_[resource_id] = string;
 }
 
 const base::FilePath& ResourceBundle::GetOverriddenPakPath() {
@@ -556,62 +556,16 @@ base::StringPiece ResourceBundle::GetRawDataResourceForScale(
   return base::StringPiece();
 }
 
-base::string16 ResourceBundle::GetLocalizedString(int message_id) {
-  base::string16 string;
-  if (delegate_ && delegate_->GetLocalizedString(message_id, &string))
-    return MaybeMangleLocalizedString(string);
-
-  // Ensure that ReloadLocaleResources() doesn't drop the resources while
-  // we're using them.
-  base::AutoLock lock_scope(*locale_resources_data_lock_);
-
-  IdToStringMap::const_iterator it =
-      overridden_locale_strings_.find(message_id);
-  if (it != overridden_locale_strings_.end())
-    return MaybeMangleLocalizedString(it->second);
-
-  // If for some reason we were unable to load the resources , return an empty
-  // string (better than crashing).
-  if (!locale_resources_data_.get()) {
-    LOG(WARNING) << "locale resources are not loaded";
-    return base::string16();
-  }
-
-  base::StringPiece data;
-  ResourceHandle::TextEncodingType encoding =
-      locale_resources_data_->GetTextEncodingType();
-  if (!locale_resources_data_->GetStringPiece(static_cast<uint16_t>(message_id),
-                                              &data)) {
-    if (secondary_locale_resources_data_.get() &&
-        secondary_locale_resources_data_->GetStringPiece(
-            static_cast<uint16_t>(message_id), &data)) {
-      // Fall back on the secondary locale pak if it exists.
-      encoding = secondary_locale_resources_data_->GetTextEncodingType();
-    } else {
-      // Fall back on the main data pack (shouldn't be any strings here except
-      // in unittests).
-      data = GetRawDataResource(message_id);
-      if (data.empty()) {
-        LOG(WARNING) << "unable to find resource: " << message_id;
-        NOTREACHED();
-        return base::string16();
-      }
-    }
-  }
-
-  // Strings should not be loaded from a data pack that contains binary data.
-  DCHECK(encoding == ResourceHandle::UTF16 || encoding == ResourceHandle::UTF8)
-      << "requested localized string from binary pack file";
-
-  // Data pack encodes strings as either UTF8 or UTF16.
-  base::string16 msg;
-  if (encoding == ResourceHandle::UTF16) {
-    msg = base::string16(reinterpret_cast<const base::char16*>(data.data()),
-                         data.length() / 2);
-  } else if (encoding == ResourceHandle::UTF8) {
-    msg = base::UTF8ToUTF16(data);
+base::string16 ResourceBundle::GetLocalizedString(int resource_id) {
+#if DCHECK_IS_ON()
+  {
+    base::AutoLock lock_scope(*locale_resources_data_lock_);
+    // Overriding locale strings isn't supported if the first string resource
+    // has already been queried.
+    can_override_locale_string_resources_ = false;
   }
-  return MaybeMangleLocalizedString(msg);
+#endif
+  return GetLocalizedStringImpl(resource_id);
 }
 
 base::RefCountedMemory* ResourceBundle::LoadLocalizedResourceBytes(
@@ -733,6 +687,13 @@ bool ResourceBundle::IsScaleFactorSupported(ScaleFactor scale_factor) {
   return base::ContainsValue(supported_scale_factors, scale_factor);
 }
 
+void ResourceBundle::CheckCanOverrideStringResources() {
+#if DCHECK_IS_ON()
+  base::AutoLock lock_scope(*locale_resources_data_lock_);
+  DCHECK(can_override_locale_string_resources_);
+#endif
+}
+
 ResourceBundle::ResourceBundle(Delegate* delegate)
     : delegate_(delegate),
       locale_resources_data_lock_(new base::Lock),
@@ -819,8 +780,12 @@ void ResourceBundle::AddDataPackFromPathInternal(
 
 void ResourceBundle::InitDefaultFontList() {
 #if defined(OS_CHROMEOS)
-  std::string font_family = base::UTF16ToUTF8(
-      GetLocalizedString(IDS_UI_FONT_FAMILY_CROS));
+  // InitDefaultFontList() is called earlier than overriding the locale strings.
+  // So we call the |GetLocalizedStringImpl()| which doesn't set the flag
+  // |can_override_locale_string_resources_| to false. This is okay, because the
+  // font list doesn't need to be overridden by variations.
+  std::string font_family =
+      base::UTF16ToUTF8(GetLocalizedStringImpl(IDS_UI_FONT_FAMILY_CROS));
   gfx::FontList::SetDefaultFontDescription(font_family);
 
   // TODO(yukishiino): Remove SetDefaultFontDescription() once the migration to
@@ -907,6 +872,64 @@ gfx::Image& ResourceBundle::GetEmptyImage() {
   return empty_image_;
 }
 
+base::string16 ResourceBundle::GetLocalizedStringImpl(int resource_id) {
+  base::string16 string;
+  if (delegate_ && delegate_->GetLocalizedString(resource_id, &string))
+    return MaybeMangleLocalizedString(string);
+
+  // Ensure that ReloadLocaleResources() doesn't drop the resources while
+  // we're using them.
+  base::AutoLock lock_scope(*locale_resources_data_lock_);
+
+  IdToStringMap::const_iterator it =
+      overridden_locale_strings_.find(resource_id);
+  if (it != overridden_locale_strings_.end())
+    return MaybeMangleLocalizedString(it->second);
+
+  // If for some reason we were unable to load the resources , return an empty
+  // string (better than crashing).
+  if (!locale_resources_data_.get()) {
+    LOG(WARNING) << "locale resources are not loaded";
+    return base::string16();
+  }
+
+  base::StringPiece data;
+  ResourceHandle::TextEncodingType encoding =
+      locale_resources_data_->GetTextEncodingType();
+  if (!locale_resources_data_->GetStringPiece(
+          static_cast<uint16_t>(resource_id), &data)) {
+    if (secondary_locale_resources_data_.get() &&
+        secondary_locale_resources_data_->GetStringPiece(
+            static_cast<uint16_t>(resource_id), &data)) {
+      // Fall back on the secondary locale pak if it exists.
+      encoding = secondary_locale_resources_data_->GetTextEncodingType();
+    } else {
+      // Fall back on the main data pack (shouldn't be any strings here except
+      // in unittests).
+      data = GetRawDataResource(resource_id);
+      if (data.empty()) {
+        LOG(WARNING) << "unable to find resource: " << resource_id;
+        NOTREACHED();
+        return base::string16();
+      }
+    }
+  }
+
+  // Strings should not be loaded from a data pack that contains binary data.
+  DCHECK(encoding == ResourceHandle::UTF16 || encoding == ResourceHandle::UTF8)
+      << "requested localized string from binary pack file";
+
+  // Data pack encodes strings as either UTF8 or UTF16.
+  base::string16 msg;
+  if (encoding == ResourceHandle::UTF16) {
+    msg = base::string16(reinterpret_cast<const base::char16*>(data.data()),
+                         data.length() / 2);
+  } else if (encoding == ResourceHandle::UTF8) {
+    msg = base::UTF8ToUTF16(data);
+  }
+  return MaybeMangleLocalizedString(msg);
+}
+
 // static
 bool ResourceBundle::PNGContainsFallbackMarker(const unsigned char* buf,
                                                size_t size) {

ui/base/resource/resource_bundle.h

@@ -241,9 +241,9 @@ class UI_BASE_EXPORT ResourceBundle {
   base::StringPiece GetRawDataResourceForScale(int resource_id,
                                                ScaleFactor scale_factor) const;
 
-  // Get a localized string given a message id.  Returns an empty
-  // string if the message_id is not found.
-  base::string16 GetLocalizedString(int message_id);
+  // Get a localized string given a message id.  Returns an empty string if the
+  // resource_id is not found.
+  base::string16 GetLocalizedString(int resource_id);
 
   // Get a localized resource (for example, localized image logo) given a
   // resource id.
@@ -276,11 +276,11 @@ class UI_BASE_EXPORT ResourceBundle {
 
   // Overrides a localized string resource with the given string. If no delegate
   // is present, the |string| will be returned when getting the localized string
-  // |message_id|. If |ReloadLocaleResources| is called, all overrides are
+  // |resource_id|. If |ReloadLocaleResources| is called, all overrides are
   // cleared. This is intended to be used in conjunction with field trials and
   // the variations service to experiment with different UI strings. This method
   // is not thread safe!
-  void OverrideLocaleStringResource(int message_id,
+  void OverrideLocaleStringResource(int resource_id,
                                     const base::string16& string);
 
   // Returns the full pathname of the locale file to load.  May return an empty
@@ -297,11 +297,22 @@ class UI_BASE_EXPORT ResourceBundle {
   // Returns true if |scale_factor| is supported by this platform.
   static bool IsScaleFactorSupported(ScaleFactor scale_factor);
 
+  // Checks whether overriding locale strings is supported. This will fail with
+  // a DCHECK if the first string resource has already been queried.
+  void CheckCanOverrideStringResources();
+
   // Sets whether this ResourceBundle should mangle localized strings or not.
   void set_mangle_localized_strings_for_test(bool mangle) {
     mangle_localized_strings_ = mangle;
   }
 
+#if DCHECK_IS_ON()
+  // Gets whether overriding locale strings is supported.
+  bool get_can_override_locale_string_resources_for_test() {
+    return can_override_locale_string_resources_;
+  }
+#endif
+
  private:
   FRIEND_TEST_ALL_PREFIXES(ResourceBundleTest, DelegateGetPathForLocalePack);
   FRIEND_TEST_ALL_PREFIXES(ResourceBundleTest, DelegateGetImageNamed);
@@ -404,6 +415,13 @@ class UI_BASE_EXPORT ResourceBundle {
   // visible and returns the mangled string. If not, returns |str|.
   base::string16 MaybeMangleLocalizedString(const base::string16& str);
 
+  // An internal implementation of |GetLocalizedString()| without setting the
+  // flag of whether overriding locale strings is supported to false. We don't
+  // update this flag only in |InitDefaultFontList()| which is called earlier
+  // than the overriding. This is okay, because the font list doesn't need to be
+  // overridden by variations.
+  base::string16 GetLocalizedStringImpl(int resource_id);
+
   // This pointer is guaranteed to outlive the ResourceBundle instance and may
   // be NULL.
   Delegate* delegate_;
@@ -436,6 +454,10 @@ class UI_BASE_EXPORT ResourceBundle {
 
   IdToStringMap overridden_locale_strings_;
 
+#if DCHECK_IS_ON()
+  bool can_override_locale_string_resources_ = true;
+#endif
+
   bool is_test_resources_ = false;
   bool mangle_localized_strings_ = false;
 

ui/base/resource/resource_bundle_unittest.cc

@@ -134,8 +134,7 @@ void CreateDataPackWithSingleBitmap(const base::FilePath& path,
 
 class ResourceBundleTest : public testing::Test {
  public:
-  ResourceBundleTest() : resource_bundle_(NULL) {
-  }
+  ResourceBundleTest() : resource_bundle_(nullptr) {}
 
   ~ResourceBundleTest() override {}
 
@@ -297,7 +296,7 @@ TEST_F(ResourceBundleTest, DelegateGetLocalizedString) {
 }
 
 TEST_F(ResourceBundleTest, OverrideStringResource) {
-  ResourceBundle* resource_bundle = CreateResourceBundle(NULL);
+  ResourceBundle* resource_bundle = CreateResourceBundle(nullptr);
 
   base::string16 data = base::ASCIIToUTF16("My test data");
   int resource_id = 5;
@@ -311,6 +310,21 @@ TEST_F(ResourceBundleTest, OverrideStringResource) {
   EXPECT_EQ(data, result);
 }
 
+#if DCHECK_IS_ON()
+TEST_F(ResourceBundleTest, CanOverrideStringResources) {
+  ResourceBundle* resource_bundle = CreateResourceBundle(nullptr);
+
+  base::string16 data = base::ASCIIToUTF16("My test data");
+  int resource_id = 5;
+
+  EXPECT_TRUE(
+      resource_bundle->get_can_override_locale_string_resources_for_test());
+  resource_bundle->GetLocalizedString(resource_id);
+  EXPECT_FALSE(
+      resource_bundle->get_can_override_locale_string_resources_for_test());
+}
+#endif
+
 TEST_F(ResourceBundleTest, DelegateGetLocalizedStringWithOverride) {
   MockResourceBundleDelegate delegate;
   ResourceBundle* resource_bundle = CreateResourceBundle(&delegate);
@@ -328,7 +342,7 @@ TEST_F(ResourceBundleTest, DelegateGetLocalizedStringWithOverride) {
 }
 
 TEST_F(ResourceBundleTest, LocaleDataPakExists) {
-  ResourceBundle* resource_bundle = CreateResourceBundle(NULL);
+  ResourceBundle* resource_bundle = CreateResourceBundle(nullptr);
 
   // Check that ResourceBundle::LocaleDataPakExists returns the correct results.
   EXPECT_TRUE(resource_bundle->LocaleDataPakExists("en-US"));
@@ -354,7 +368,7 @@ class ResourceBundleImageTest : public ResourceBundleTest {
     EXPECT_EQ(base::WriteFile(locale_path, kEmptyPakContents, kEmptyPakSize),
               static_cast<int>(kEmptyPakSize));
 
-    ui::ResourceBundle* resource_bundle = CreateResourceBundle(NULL);
+    ui::ResourceBundle* resource_bundle = CreateResourceBundle(nullptr);
 
     // Load the empty locale data pak.
     resource_bundle->LoadTestResources(base::FilePath(), locale_path);
@@ -391,15 +405,15 @@ TEST_F(ResourceBundleImageTest, LoadDataResourceBytes) {
   resource_bundle->AddDataPackFromPath(data_path, SCALE_FACTOR_100P);
 
   const int kUnfoundResourceId = 10000;
-  EXPECT_EQ(NULL, resource_bundle->LoadDataResourceBytes(
-      kUnfoundResourceId));
+  EXPECT_EQ(nullptr,
+            resource_bundle->LoadDataResourceBytes(kUnfoundResourceId));
 
   // Give a .pak file that doesn't exist so we will fail to load it.
   resource_bundle->AddDataPackFromPath(
       base::FilePath(FILE_PATH_LITERAL("non-existant-file.pak")),
       ui::SCALE_FACTOR_NONE);
-  EXPECT_EQ(NULL, resource_bundle->LoadDataResourceBytes(
-      kUnfoundResourceId));
+  EXPECT_EQ(nullptr,
+            resource_bundle->LoadDataResourceBytes(kUnfoundResourceId));
 }
 
 TEST_F(ResourceBundleImageTest, GetRawDataResource) {