[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..29dcd1372f06cc3b2ee045d4c7c6657d9e293daf 100644
--- a/components/autofill/core/common/autofill_l10n_util.cc
+++ b/components/autofill/core/common/autofill_l10n_util.cc
@@ -6,15 +6,32 @@
 
 #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.
+  // See http://crbug.com/558625.
+  bool success = !!U_SUCCESS(error);
+  if (success) {
+    collator_->setStrength(icu::Collator::PRIMARY);
+  } else {
+    // NOTE(mathp): Developers hitting this should either report their device
+    // information in the bug linked above, or contact me directly.
+    LOG(ERROR) << "Failed to initialize the ICU Collator for "
+               << "CaseInsensitiveCompare.";

[Ilya Sherman, 2015/11/20 22:33:13]

As long as you're logging a string here, can you log the locale that was
unsupported?

[Mathieu, 2015/11/22 15:17:27]

Done. 

+  }
+
+  UMA_HISTOGRAM_BOOLEAN("Autofill.IcuCollatorCreationSuccess", success);
 }
 
 CaseInsensitiveCompare::~CaseInsensitiveCompare() {
@@ -22,8 +39,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_.get()) {

[Evan Stade, 2015/11/20 21:57:20]

don't need .get()

[Mathieu, 2015/11/22 15:17:26]

Done.

+    return base::i18n::CompareString16WithCollator(*collator_, lhs, rhs) ==
+           UCOL_EQUAL;
+  }
+  return lhs == rhs;

[Evan Stade, 2015/11/20 21:57:20]

this doesn't seem like a great fallback, arbitrary things will just seem
mysteriously broken. Seems like it would be marginally better to fall back to an
english collator.

On these systems with a broken locale, what does the rest of Chrome do? e.g. how
does it choose which translations to use?

[Mathieu, 2015/11/22 15:17:27]

On the contrary I thought this was quite a good fallback on systems where the
default locale is broken since it will do a string16 compare. It appears to be
what other devs have chosen to do, notably in the translate case which you were
asking about (they've hit this NULL collator bug a few weeks ago):
https://code.google.com/p/chromium/issues/detail?id=548355

[Evan Stade, 2015/11/24 23:45:43]

The translate case is a bit different because it's just sorting a list for
display, it's not being used for data de-duping or anything particularly
important. And to be clear, I wasn't asking about the page translation feature.
I was asking what language/locale Chrome decides to display in when the given
locale is missing or unrecognized.

This does make me wonder whether there's generally a better way to de-dupe data.
For example, assume I have two addresses which are written in language Y, but my
default locale is language Z (I'm in some country with mixed languages).
Shouldn't the two addresses be de-duped based on language Y and not language Z?

 }
 
 }  // namespace l10n