[Autofill] Guard against the initialization failure of ICU Collator.

components/autofill/core/common/autofill_l10n_util.cc

Index: components/autofill/core/common/autofill_l10n_util.cc
diff --git a/components/autofill/core/common/autofill_l10n_util.cc b/components/autofill/core/common/autofill_l10n_util.cc
index 6f063a85f4f7821d0e962b3d26680b26d075d151..afe7a96f1dae90b4dcbabdbadd2a5b2bd6c96b74 100644
--- a/components/autofill/core/common/autofill_l10n_util.cc
+++ b/components/autofill/core/common/autofill_l10n_util.cc
@@ -6,15 +6,42 @@
 
 #include "base/i18n/string_compare.h"
 #include "base/logging.h"
+#include "base/metrics/histogram_macros.h"
 
 namespace autofill {
 namespace l10n {
 
-CaseInsensitiveCompare::CaseInsensitiveCompare() {
+CaseInsensitiveCompare::CaseInsensitiveCompare()
+    : CaseInsensitiveCompare(icu::Locale::getDefault()) {}
+
+CaseInsensitiveCompare::CaseInsensitiveCompare(const icu::Locale& locale) {
   UErrorCode error = U_ZERO_ERROR;
-  collator_.reset(icu::Collator::createInstance(error));
-  DCHECK(U_SUCCESS(error));
-  collator_->setStrength(icu::Collator::PRIMARY);
+  collator_.reset(icu::Collator::createInstance(locale, error));
+  // On some systems, the default locale is invalid to the eyes of the ICU
+  // library. This could be due to a device-specific issue (has been seen in the
+  // wild on Android devices). In the failure case, |collator_| will be null.

[Evan Stade, 2015/11/25 20:05:45]

if collator_ is null for failure it seems easier to ignore |error|

[Mathieu, 2015/11/27 01:05:28]

Acknowledged.

+  // See http://crbug.com/558625.
+  bool success = !!U_SUCCESS(error);

[Evan Stade, 2015/11/25 20:05:45]

nit: this might be clearer and give better logging:

if (!collator_) {
  // [some comment]
  icu_54::UnicodeString name;
  std::string locale_name;
  locale.getDisplayName(name).toUTF8String(locale_name);
  LOG(ERROR) << "Failed to initialize the ICU Collator for "
             << "CaseInsensitiveCompare with locale" << locale_name;
  // Attempt to load the English locale.
  collator_.reset(icu::Collator::createInstance(icu::Locale::getEnglish(),
                  error));
}

if (collator_) {
  collator_->setStrength(icu::Collator::PRIMARY);
} else {
  LOG(ERROR) << "Failed to initialize the ICU Collator for "
             << "CaseInsensitiveCompare with the English locale."  
}
UMA_HISTOGRAM_BOOLEAN("Autofill.IcuCollatorCreationSuccess", !!collator_);

[Mathieu, 2015/11/27 01:05:28]

Thanks

+  if (success) {
+    collator_->setStrength(icu::Collator::PRIMARY);
+  } else {
+    // Attempt to load the English locale.
+    collator_.reset(icu::Collator::createInstance(icu::Locale::getEnglish(),

[Evan Stade, 2015/11/25 20:05:45]

I think you forgot to setStrength in the success case here?

[Mathieu, 2015/11/27 01:05:28]

Done.

+                    error));
+    success = !!U_SUCCESS(error);
+    if (!success) {
+      // NOTE(mathp): Developers hitting this should either report their device
+      // information in the bug linked above, or contact me directly.
+      icu_54::UnicodeString name;

[Evan Stade, 2015/11/25 20:06:59]

can you inline |name|?

[Mathieu, 2015/11/27 01:05:28]

They have a strange API where you pass the ref and it's returned as well. Not
sure what's possible other than what I've done here.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/icu/so...

+      std::string locale_name;
+      locale.getDisplayName(name).toUTF8String(locale_name);
+      LOG(ERROR) << "Failed to initialize the ICU Collator for "
+                 << "CaseInsensitiveCompare with locale: "
+                 << locale_name << ", as well as the English locale.";
+    }
+  }
+
+  UMA_HISTOGRAM_BOOLEAN("Autofill.IcuCollatorCreationSuccess", success);
 }
 
 CaseInsensitiveCompare::~CaseInsensitiveCompare() {
@@ -22,8 +49,11 @@ CaseInsensitiveCompare::~CaseInsensitiveCompare() {
 
 bool CaseInsensitiveCompare::StringsEqual(const base::string16& lhs,
                                           const base::string16& rhs) const {
-  return base::i18n::CompareString16WithCollator(*collator_, lhs, rhs) ==
-         UCOL_EQUAL;
+  if (collator_) {
+    return base::i18n::CompareString16WithCollator(*collator_, lhs, rhs) ==
+           UCOL_EQUAL;
+  }
+  return lhs == rhs;
 }
 
 }  // namespace l10n