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

components/autofill/core/common/autofill_l10n_util.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/autofill/core/common/autofill_l10n_util.h"
 
 #include "base/i18n/string_compare.h"
 #include "base/logging.h"
+#include "base/metrics/histogram_macros.h"
 
 namespace autofill {
 namespace l10n {
 
 CaseInsensitiveCompare::CaseInsensitiveCompare() {
   UErrorCode error = U_ZERO_ERROR;
   collator_.reset(icu::Collator::createInstance(error));
-  DCHECK(U_SUCCESS(error));
-  collator_->setStrength(icu::Collator::PRIMARY);
+  // 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).

[Ilya Sherman, 2015/11/19 21:04:06]

Please add a crbug reference to this comment.

[Mathieu, 2015/11/19 21:53:02]

Done.

+  bool success = U_SUCCESS(error);
+  DCHECK(success);

[Ilya Sherman, 2015/11/19 21:04:06]

nit: It's not appropriate to DCHECK a statement that might fail.  Please remove
this check.

[Mathieu, 2015/11/19 21:53:01]

My idea there was that a developer with a faulty device would be inclined to
investigate this.

[Ilya Sherman, 2015/11/19 22:31:20]

If you think it's useful for random developers to investigate this, then you can
leave the DCHECK; but in that case, please also add a comment indicating what
they should do if they hit this DCHECK.  For example, should they contact you
with some debug info?

[Mathieu, 2015/11/20 16:19:38]

It hits me that I can't test the failure codepath if there is a DCHECK in the
middle of it. Instead, I've changed it to a LOG(ERROR) in the failure case only,
which seems to be common in the codebase for something failing to be created.
https://code.google.com/p/chromium/codesearch#search/&q=%22LOG(ERROR)%22

+  if (success)
+    collator_->setStrength(icu::Collator::PRIMARY);
+
+  UMA_HISTOGRAM_BOOLEAN("Autofill.IcuCollatorCreationSuccess", success);

[Mathieu, 2015/11/19 20:50:51]

I thought it was simpler to have the macro here, let me know if you would like
me to incorporate to AutofillMetrics.

[Ilya Sherman, 2015/11/19 21:04:06]

I think it's been helpful in the past to funnel all Autofill metrics through the
AutofillMetrics interface -- in part because it reminds us to write tests for
the metrics.

[Mathieu, 2015/11/19 21:53:02]

Done. Created an explicit constructor for testing, it seemed like the most
straightforward way to test a bogus locale, let me know.

[Mathieu, 2015/11/20 16:19:38]

It turns out that there is no dependency path between autofill/core/common and
autofill/core/browser, and other code in core/common calls UMA macros directly,
possibly because of this. I've moved the tests to their own file in core/common.

 }
 
 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()) {

[Ilya Sherman, 2015/11/19 21:04:06]

Hmm, when might the collator be unset?  The code on line 16 still
unconditionally creates the collator.

[Mathieu, 2015/11/19 21:53:01]

On line 16, the constructor can return NULL on error cases (this is was caused
the original crash). 

Therefore the scoped_ptr can have NULL within in, which this line would check?

[Ilya Sherman, 2015/11/19 22:31:19]

I see -- I'd misunderstood what the failure case was, exactly.  Thanks for
clarifying.

[Mathieu, 2015/11/20 16:19:38]

Acknowledged.

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