Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1457393003:
[Autofill] Guard against the initialization failure of ICU Collator. (Closed)
|
Created:
5 years, 1 month ago by Mathieu Modified:
5 years ago CC:
chromium-reviews, rouslan+autofill_chromium.org, browser-components-watch_chromium.org, jdonnelly+autofillwatch_chromium.org, bondd+autofillwatch_chromium.org, asvitkine+watch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org, jshin+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Autofill] Guard against the initialization failure of ICU Collator.
Some crashes have been seen in the wild on Android devices. We now guard against initialization error, and track the failures in a histogram, which allow for locale slicing.
According to the web it is not unseen to have ICU Locales return bogus values, but hopefully we can track it down further.
BUG=548365
Committed: https://crrev.com/2e4d8173e148558f60fe90143613bfeefbcc7896
Cr-Commit-Position: refs/heads/master@{#362212}
Patch Set 1 : Initial #
Total comments: 14
Patch Set 2 : addressed comments #
Total comments: 8
Patch Set 3 : moved tests to their own file #Patch Set 4 : rebase #Patch Set 5 : fix UBool compile error #
Total comments: 3
Patch Set 6 : UBool #
Total comments: 7
Patch Set 7 : addressed comments #Patch Set 8 : English locale as fallback #
Total comments: 8
Patch Set 9 : accepted rewrite #
Messages
Total messages: 34 (9 generated)
Patchset #1 (id:1) has been deleted
mathp@chromium.org changed reviewers: + isherman@chromium.org
Hi Ilya, Please have a look! https://codereview.chromium.org/1457393003/diff/20001/components/autofill/cor... File components/autofill/core/common/autofill_l10n_util.cc (right): https://codereview.chromium.org/1457393003/diff/20001/components/autofill/cor... components/autofill/core/common/autofill_l10n_util.cc:25: UMA_HISTOGRAM_BOOLEAN("Autofill.IcuCollatorCreationSuccess", success); I thought it was simpler to have the macro here, let me know if you would like me to incorporate to AutofillMetrics.
https://codereview.chromium.org/1457393003/diff/20001/components/autofill/cor... File components/autofill/core/common/autofill_l10n_util.cc (right): https://codereview.chromium.org/1457393003/diff/20001/components/autofill/cor... components/autofill/core/common/autofill_l10n_util.cc:19: // wild on Android devices). Please add a crbug reference to this comment. https://codereview.chromium.org/1457393003/diff/20001/components/autofill/cor... components/autofill/core/common/autofill_l10n_util.cc:21: DCHECK(success); nit: It's not appropriate to DCHECK a statement that might fail. Please remove this check. https://codereview.chromium.org/1457393003/diff/20001/components/autofill/cor... components/autofill/core/common/autofill_l10n_util.cc:25: UMA_HISTOGRAM_BOOLEAN("Autofill.IcuCollatorCreationSuccess", success); On 2015/11/19 20:50:51, Mathieu Perreault wrote: > I thought it was simpler to have the macro here, let me know if you would like > me to incorporate to AutofillMetrics. 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. https://codereview.chromium.org/1457393003/diff/20001/components/autofill/cor... components/autofill/core/common/autofill_l10n_util.cc:33: if (collator_.get()) { Hmm, when might the collator be unset? The code on line 16 still unconditionally creates the collator.
Thanks, PTAL. https://codereview.chromium.org/1457393003/diff/20001/components/autofill/cor... File components/autofill/core/common/autofill_l10n_util.cc (right): https://codereview.chromium.org/1457393003/diff/20001/components/autofill/cor... components/autofill/core/common/autofill_l10n_util.cc:19: // wild on Android devices). On 2015/11/19 21:04:06, Ilya Sherman wrote: > Please add a crbug reference to this comment. Done. https://codereview.chromium.org/1457393003/diff/20001/components/autofill/cor... components/autofill/core/common/autofill_l10n_util.cc:21: DCHECK(success); On 2015/11/19 21:04:06, Ilya Sherman wrote: > nit: It's not appropriate to DCHECK a statement that might fail. Please remove > this check. My idea there was that a developer with a faulty device would be inclined to investigate this. https://codereview.chromium.org/1457393003/diff/20001/components/autofill/cor... components/autofill/core/common/autofill_l10n_util.cc:25: UMA_HISTOGRAM_BOOLEAN("Autofill.IcuCollatorCreationSuccess", success); On 2015/11/19 21:04:06, Ilya Sherman wrote: > On 2015/11/19 20:50:51, Mathieu Perreault wrote: > > I thought it was simpler to have the macro here, let me know if you would like > > me to incorporate to AutofillMetrics. > > 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. Done. Created an explicit constructor for testing, it seemed like the most straightforward way to test a bogus locale, let me know. https://codereview.chromium.org/1457393003/diff/20001/components/autofill/cor... components/autofill/core/common/autofill_l10n_util.cc:33: if (collator_.get()) { On 2015/11/19 21:04:06, Ilya Sherman wrote: > Hmm, when might the collator be unset? The code on line 16 still > unconditionally creates the collator. 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?
https://codereview.chromium.org/1457393003/diff/20001/components/autofill/cor... File components/autofill/core/common/autofill_l10n_util.cc (right): https://codereview.chromium.org/1457393003/diff/20001/components/autofill/cor... components/autofill/core/common/autofill_l10n_util.cc:21: DCHECK(success); On 2015/11/19 21:53:01, Mathieu Perreault wrote: > On 2015/11/19 21:04:06, Ilya Sherman wrote: > > nit: It's not appropriate to DCHECK a statement that might fail. Please > remove > > this check. > > My idea there was that a developer with a faulty device would be inclined to > investigate this. 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? https://codereview.chromium.org/1457393003/diff/20001/components/autofill/cor... components/autofill/core/common/autofill_l10n_util.cc:33: if (collator_.get()) { On 2015/11/19 21:53:01, Mathieu Perreault wrote: > On 2015/11/19 21:04:06, Ilya Sherman wrote: > > Hmm, when might the collator be unset? The code on line 16 still > > unconditionally creates the collator. > > 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? I see -- I'd misunderstood what the failure case was, exactly. Thanks for clarifying. https://codereview.chromium.org/1457393003/diff/40001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics_unittest.cc (right): https://codereview.chromium.org/1457393003/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_metrics_unittest.cc:3211: l10n::CaseInsensitiveCompare compare(bogusLocale); Rather than creating a test-only constructor, could you please call setDefault() (and restore the default after the test)? https://codereview.chromium.org/1457393003/diff/40001/components/autofill/cor... File components/autofill/core/common/autofill_l10n_util.cc (right): https://codereview.chromium.org/1457393003/diff/40001/components/autofill/cor... components/autofill/core/common/autofill_l10n_util.cc:22: // wild on Android devices). crbug.com/558625. Please document that in the failure case, collator_ will be null. https://codereview.chromium.org/1457393003/diff/40001/components/autofill/cor... components/autofill/core/common/autofill_l10n_util.cc:22: // wild on Android devices). crbug.com/558625. nit: Please prepend "http://" to the link URL.
https://codereview.chromium.org/1457393003/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1457393003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:2224: +<histogram name="Autofill.IcuCollatorCreationSuccess" enum="BooleanSuccess"> Optional nit: A BooleanCreated with labels "Failed" and "Created" might be a little clearer.
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
PTAL. Having weird issues with mac_chromium_gn_rel ("steps" failing) which
hopefully resolve themselves soon :)
https://codereview.chromium.org/1457393003/diff/20001/components/autofill/cor...
File components/autofill/core/common/autofill_l10n_util.cc (right):
https://codereview.chromium.org/1457393003/diff/20001/components/autofill/cor...
components/autofill/core/common/autofill_l10n_util.cc:21: DCHECK(success);
On 2015/11/19 22:31:20, Ilya Sherman wrote:
> On 2015/11/19 21:53:01, Mathieu Perreault wrote:
> > On 2015/11/19 21:04:06, Ilya Sherman wrote:
> > > nit: It's not appropriate to DCHECK a statement that might fail. Please
> > remove
> > > this check.
> >
> > My idea there was that a developer with a faulty device would be inclined to
> > investigate this.
>
> 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?
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
https://codereview.chromium.org/1457393003/diff/20001/components/autofill/cor...
components/autofill/core/common/autofill_l10n_util.cc:25:
UMA_HISTOGRAM_BOOLEAN("Autofill.IcuCollatorCreationSuccess", success);
On 2015/11/19 21:53:02, Mathieu Perreault wrote:
> On 2015/11/19 21:04:06, Ilya Sherman wrote:
> > On 2015/11/19 20:50:51, Mathieu Perreault wrote:
> > > I thought it was simpler to have the macro here, let me know if you would
> like
> > > me to incorporate to AutofillMetrics.
> >
> > 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.
>
> Done. Created an explicit constructor for testing, it seemed like the most
> straightforward way to test a bogus locale, let me know.
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.
https://codereview.chromium.org/1457393003/diff/20001/components/autofill/cor...
components/autofill/core/common/autofill_l10n_util.cc:33: if (collator_.get()) {
On 2015/11/19 22:31:19, Ilya Sherman wrote:
> On 2015/11/19 21:53:01, Mathieu Perreault wrote:
> > On 2015/11/19 21:04:06, Ilya Sherman wrote:
> > > Hmm, when might the collator be unset? The code on line 16 still
> > > unconditionally creates the collator.
> >
> > 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?
>
> I see -- I'd misunderstood what the failure case was, exactly. Thanks for
> clarifying.
Acknowledged.
https://codereview.chromium.org/1457393003/diff/40001/components/autofill/cor...
File components/autofill/core/browser/autofill_metrics_unittest.cc (right):
https://codereview.chromium.org/1457393003/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_metrics_unittest.cc:3211:
l10n::CaseInsensitiveCompare compare(bogusLocale);
On 2015/11/19 22:31:20, Ilya Sherman wrote:
> Rather than creating a test-only constructor, could you please call
setDefault()
> (and restore the default after the test)?
It was my first thought as well. I tried to call setDefault with a bogus locale,
but unfortunately setDefault appears to guard against this and the next call to
getDefault returns the system default and not the bogus one.
https://codereview.chromium.org/1457393003/diff/40001/components/autofill/cor...
File components/autofill/core/common/autofill_l10n_util.cc (right):
https://codereview.chromium.org/1457393003/diff/40001/components/autofill/cor...
components/autofill/core/common/autofill_l10n_util.cc:22: // wild on Android
devices). crbug.com/558625.
On 2015/11/19 22:31:20, Ilya Sherman wrote:
> nit: Please prepend "http://" to the link URL.
Done.
https://codereview.chromium.org/1457393003/diff/40001/components/autofill/cor...
components/autofill/core/common/autofill_l10n_util.cc:22: // wild on Android
devices). crbug.com/558625.
On 2015/11/19 22:31:20, Ilya Sherman wrote:
> nit: Please prepend "http://" to the link URL.
Done.
https://codereview.chromium.org/1457393003/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):
https://codereview.chromium.org/1457393003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:2224: +<histogram
name="Autofill.IcuCollatorCreationSuccess" enum="BooleanSuccess">
On 2015/11/19 22:31:54, Ilya Sherman wrote:
> Optional nit: A BooleanCreated with labels "Failed" and "Created" might be a
> little clearer.
I find it would be slightly redundant because there is "Creation" in the
histogram name. Creation success and Creation failure is straightforward to me.
Patchset #5 (id:140001) has been deleted
https://codereview.chromium.org/1457393003/diff/160001/components/autofill/co... File components/autofill/core/common/autofill_l10n_util.cc (right): https://codereview.chromium.org/1457393003/diff/160001/components/autofill/co... components/autofill/core/common/autofill_l10n_util.cc:24: bool success = U_SUCCESS(error) ? true : false; This is not elegant, but I had to do it to fix a compile error (it wouldn't assign a bool from the return value of U_SUCCESS). Example here: http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...
estade@chromium.org changed reviewers: + estade@chromium.org
https://codereview.chromium.org/1457393003/diff/160001/components/autofill/co... File components/autofill/core/common/autofill_l10n_util.cc (right): https://codereview.chromium.org/1457393003/diff/160001/components/autofill/co... components/autofill/core/common/autofill_l10n_util.cc:24: bool success = U_SUCCESS(error) ? true : false; On 2015/11/20 18:40:46, Mathieu Perreault wrote: > This is not elegant, but I had to do it to fix a compile error (it wouldn't > assign a bool from the return value of U_SUCCESS). Example here: > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_... normally I use !! for this, or you can probably do U_SUCCESS(error) == TRUE or static_cast to bool
Thanks, I used !! and sent it to the bots https://codereview.chromium.org/1457393003/diff/160001/components/autofill/co... File components/autofill/core/common/autofill_l10n_util.cc (right): https://codereview.chromium.org/1457393003/diff/160001/components/autofill/co... components/autofill/core/common/autofill_l10n_util.cc:24: bool success = U_SUCCESS(error) ? true : false; On 2015/11/20 19:49:47, Evan Stade wrote: > On 2015/11/20 18:40:46, Mathieu Perreault wrote: > > This is not elegant, but I had to do it to fix a compile error (it wouldn't > > assign a bool from the return value of U_SUCCESS). Example here: > > > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_... > > normally I use !! for this, or you can probably do U_SUCCESS(error) == TRUE or > static_cast to bool Done.
https://codereview.chromium.org/1457393003/diff/170001/components/autofill/co... File components/autofill/core/common/autofill_l10n_util.cc (right): https://codereview.chromium.org/1457393003/diff/170001/components/autofill/co... components/autofill/core/common/autofill_l10n_util.cc:42: if (collator_.get()) { don't need .get() https://codereview.chromium.org/1457393003/diff/170001/components/autofill/co... components/autofill/core/common/autofill_l10n_util.cc:46: return lhs == rhs; 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?
LGTM % the comment below, and Evan's comments. https://codereview.chromium.org/1457393003/diff/170001/components/autofill/co... File components/autofill/core/common/autofill_l10n_util.cc (right): https://codereview.chromium.org/1457393003/diff/170001/components/autofill/co... components/autofill/core/common/autofill_l10n_util.cc:31: << "CaseInsensitiveCompare."; As long as you're logging a string here, can you log the locale that was unsupported?
Thanks, PTAL https://codereview.chromium.org/1457393003/diff/170001/components/autofill/co... File components/autofill/core/common/autofill_l10n_util.cc (right): https://codereview.chromium.org/1457393003/diff/170001/components/autofill/co... components/autofill/core/common/autofill_l10n_util.cc:31: << "CaseInsensitiveCompare."; On 2015/11/20 22:33:13, Ilya Sherman wrote: > As long as you're logging a string here, can you log the locale that was > unsupported? Done. https://codereview.chromium.org/1457393003/diff/170001/components/autofill/co... components/autofill/core/common/autofill_l10n_util.cc:42: if (collator_.get()) { On 2015/11/20 21:57:20, Evan Stade wrote: > don't need .get() Done. https://codereview.chromium.org/1457393003/diff/170001/components/autofill/co... components/autofill/core/common/autofill_l10n_util.cc:46: return lhs == rhs; On 2015/11/20 21:57:20, Evan Stade wrote: > 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? 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
On 2015/11/22 15:17:27, Mathieu Perreault wrote: > Thanks, PTAL > > https://codereview.chromium.org/1457393003/diff/170001/components/autofill/co... > File components/autofill/core/common/autofill_l10n_util.cc (right): > > https://codereview.chromium.org/1457393003/diff/170001/components/autofill/co... > components/autofill/core/common/autofill_l10n_util.cc:31: << > "CaseInsensitiveCompare."; > On 2015/11/20 22:33:13, Ilya Sherman wrote: > > As long as you're logging a string here, can you log the locale that was > > unsupported? > > Done. > > https://codereview.chromium.org/1457393003/diff/170001/components/autofill/co... > components/autofill/core/common/autofill_l10n_util.cc:42: if (collator_.get()) { > On 2015/11/20 21:57:20, Evan Stade wrote: > > don't need .get() > > Done. > > https://codereview.chromium.org/1457393003/diff/170001/components/autofill/co... > components/autofill/core/common/autofill_l10n_util.cc:46: return lhs == rhs; > On 2015/11/20 21:57:20, Evan Stade wrote: > > 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? > > 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 friendly ping.
On 2015/11/22 15:17:27, Mathieu Perreault wrote: > Thanks, PTAL > > https://codereview.chromium.org/1457393003/diff/170001/components/autofill/co... > File components/autofill/core/common/autofill_l10n_util.cc (right): > > https://codereview.chromium.org/1457393003/diff/170001/components/autofill/co... > components/autofill/core/common/autofill_l10n_util.cc:31: << > "CaseInsensitiveCompare."; > On 2015/11/20 22:33:13, Ilya Sherman wrote: > > As long as you're logging a string here, can you log the locale that was > > unsupported? > > Done. > > https://codereview.chromium.org/1457393003/diff/170001/components/autofill/co... > components/autofill/core/common/autofill_l10n_util.cc:42: if (collator_.get()) { > On 2015/11/20 21:57:20, Evan Stade wrote: > > don't need .get() > > Done. > > https://codereview.chromium.org/1457393003/diff/170001/components/autofill/co... > components/autofill/core/common/autofill_l10n_util.cc:46: return lhs == rhs; > On 2015/11/20 21:57:20, Evan Stade wrote: > > 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? > > 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 friendly ping.
https://codereview.chromium.org/1457393003/diff/170001/components/autofill/co... File components/autofill/core/common/autofill_l10n_util.cc (right): https://codereview.chromium.org/1457393003/diff/170001/components/autofill/co... components/autofill/core/common/autofill_l10n_util.cc:46: return lhs == rhs; On 2015/11/22 15:17:27, Mathieu Perreault wrote: > On 2015/11/20 21:57:20, Evan Stade wrote: > > 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? > > 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 > 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?
On 2015/11/24 23:45:43, Evan Stade wrote: > https://codereview.chromium.org/1457393003/diff/170001/components/autofill/co... > File components/autofill/core/common/autofill_l10n_util.cc (right): > > https://codereview.chromium.org/1457393003/diff/170001/components/autofill/co... > components/autofill/core/common/autofill_l10n_util.cc:46: return lhs == rhs; > On 2015/11/22 15:17:27, Mathieu Perreault wrote: > > On 2015/11/20 21:57:20, Evan Stade wrote: > > > 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? > > > > 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 > > > > 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? In your example, how would you determine that the address was entered in language Y? I'm not sure we have this information. We have |app_locale| in the AutofillManager, and this is set from g_browser_process->GetApplicationLocale(). I often enter fr-CA addresses while using Chrome in en_US locale. Chrome doesn't have a way to know the language of the address (perhaps it could remember what was the page locale when the address was saved? Seems hacky). In all cases, this CL handles the crash only and I intend to merge it in M48. I'll open a bug for prospective work around different locales (this question also comes up when dealing with libaddressinput which needs to be passed the exact input locale for address canonicalization).
On 2015/11/25 00:44:26, Mathieu Perreault wrote: > On 2015/11/24 23:45:43, Evan Stade wrote: > > > https://codereview.chromium.org/1457393003/diff/170001/components/autofill/co... > > File components/autofill/core/common/autofill_l10n_util.cc (right): > > > > > https://codereview.chromium.org/1457393003/diff/170001/components/autofill/co... > > components/autofill/core/common/autofill_l10n_util.cc:46: return lhs == rhs; > > On 2015/11/22 15:17:27, Mathieu Perreault wrote: > > > On 2015/11/20 21:57:20, Evan Stade wrote: > > > > 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? > > > > > > 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 > > > > > > > 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? > > In your example, how would you determine that the address was entered in > language Y? I'm not sure we have this information. We have |app_locale| in the > AutofillManager, and this is set from g_browser_process->GetApplicationLocale(). > I often enter fr-CA addresses while using Chrome in en_US locale. Chrome doesn't > have a way to know the language of the address (perhaps it could remember what > was the page locale when the address was saved? Seems hacky). Admittedly I don't know. I don't even know why lower-casing is locale specific in the first place. Is it the case that capital letter X lowercases to y in one language and z in another? > > In all cases, this CL handles the crash only and I intend to merge it in M48. > I'll open a bug for prospective work around different locales (this question > also comes up when dealing with libaddressinput which needs to be passed the > exact input locale for address canonicalization). if you're looking for a small and relatively useful change, why not fall back to "en" before falling back to ASCII?
Evan please have a look
https://codereview.chromium.org/1457393003/diff/210001/components/autofill/co... File components/autofill/core/common/autofill_l10n_util.cc (right): https://codereview.chromium.org/1457393003/diff/210001/components/autofill/co... components/autofill/core/common/autofill_l10n_util.cc:22: // wild on Android devices). In the failure case, |collator_| will be null. if collator_ is null for failure it seems easier to ignore |error| https://codereview.chromium.org/1457393003/diff/210001/components/autofill/co... components/autofill/core/common/autofill_l10n_util.cc:24: bool success = !!U_SUCCESS(error); 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_); https://codereview.chromium.org/1457393003/diff/210001/components/autofill/co... components/autofill/core/common/autofill_l10n_util.cc:29: collator_.reset(icu::Collator::createInstance(icu::Locale::getEnglish(), I think you forgot to setStrength in the success case here?
https://codereview.chromium.org/1457393003/diff/210001/components/autofill/co... File components/autofill/core/common/autofill_l10n_util.cc (right): https://codereview.chromium.org/1457393003/diff/210001/components/autofill/co... components/autofill/core/common/autofill_l10n_util.cc:35: icu_54::UnicodeString name; can you inline |name|?
ptal https://codereview.chromium.org/1457393003/diff/210001/components/autofill/co... File components/autofill/core/common/autofill_l10n_util.cc (right): https://codereview.chromium.org/1457393003/diff/210001/components/autofill/co... components/autofill/core/common/autofill_l10n_util.cc:22: // wild on Android devices). In the failure case, |collator_| will be null. On 2015/11/25 20:05:45, Evan Stade wrote: > if collator_ is null for failure it seems easier to ignore |error| Acknowledged. https://codereview.chromium.org/1457393003/diff/210001/components/autofill/co... components/autofill/core/common/autofill_l10n_util.cc:24: bool success = !!U_SUCCESS(error); On 2015/11/25 20:05:45, Evan Stade wrote: > 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_); Thanks https://codereview.chromium.org/1457393003/diff/210001/components/autofill/co... components/autofill/core/common/autofill_l10n_util.cc:29: collator_.reset(icu::Collator::createInstance(icu::Locale::getEnglish(), On 2015/11/25 20:05:45, Evan Stade wrote: > I think you forgot to setStrength in the success case here? Done. https://codereview.chromium.org/1457393003/diff/210001/components/autofill/co... components/autofill/core/common/autofill_l10n_util.cc:35: icu_54::UnicodeString name; On 2015/11/25 20:06:59, Evan Stade wrote: > can you inline |name|? 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...
lgtm
The CQ bit was checked by mathp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1457393003/#ps230001 (title: "accepted rewrite")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1457393003/230001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1457393003/230001
Message was sent while issue was closed.
Committed patchset #9 (id:230001)
Message was sent while issue was closed.
Description was changed from ========== [Autofill] Guard against the initialization failure of ICU Collator. Some crashes have been seen in the wild on Android devices. We now guard against initialization error, and track the failures in a histogram, which allow for locale slicing. According to the web it is not unseen to have ICU Locales return bogus values, but hopefully we can track it down further. BUG=548365 ========== to ========== [Autofill] Guard against the initialization failure of ICU Collator. Some crashes have been seen in the wild on Android devices. We now guard against initialization error, and track the failures in a histogram, which allow for locale slicing. According to the web it is not unseen to have ICU Locales return bogus values, but hopefully we can track it down further. BUG=548365 Committed: https://crrev.com/2e4d8173e148558f60fe90143613bfeefbcc7896 Cr-Commit-Position: refs/heads/master@{#362212} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/2e4d8173e148558f60fe90143613bfeefbcc7896 Cr-Commit-Position: refs/heads/master@{#362212} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
