Switch language detection to use CLD2's DetectLanguageCheckUTF8 method.

Switch language detection to use CLD2's DetectLanguageCheckUTF8 method.

There is a long-standing bug in issue 444258 describing a crash in
CLD2::QuadHashV3Lookup4. A hypothesis for the crashes is that invalid UTF-8 is
reaching the CLD2 code layer, and this is confirmed in bug 444258. We must use
the "safe" (CheckUTF8) variant of language detection instead.

The existing code uses DetectLanguageSummary, but there is no "safe" variant of
this method in CLD2. However, the existing code doesn't consider the extra data
returned by DetectLanguageSummary (i.e., multiple language guesses with
accompanying probabilities) so there's no reason to stick with it. Using the
simpler and safer DetectLanguageCheckUTF8 should produce the same results, will
be safer, and should have comparable performance for Chromium's use cases.

BUG=444258
Committed: https://crrev.com/90bb2b934366c595c1c979e0b2363f0a822e1b92
Cr-Commit-Position: refs/heads/master@{#328947}

[Andrew Hayden (chromium.org), 2015-05-07 11:47:07]

andrewhayden@chromium.org changed reviewers:
+ droger@chromium.org, hajimehoshi@chromium.org, mad@chromium.org,
toyoshim@chromium.org

[Andrew Hayden (chromium.org), 2015-05-07 11:47:54]

Can I get an owner to take a look, please. Hajime, how about you? :)

[droger, 2015-05-07 11:56:57]

I'm not an expert of CLD, but this LGTM.

https://codereview.chromium.org/1125403004/diff/1/components/translate/core/l...
File components/translate/core/language_detection/language_detection_util.cc
(right):

https://codereview.chromium.org/1125403004/diff/1/components/translate/core/l...
components/translate/core/language_detection/language_detection_util.cc:115:
utf8_text.c_str(), (int)utf8_text.size(), true /* is_plain_text */,
Could you use static_cast here instead of C-style cast?

https://codereview.chromium.org/1125403004/diff/1/components/translate/core/l...
components/translate/core/language_detection/language_detection_util.cc:117: if
(text_bytes < (int)utf8_text.size() &&
same here.

https://codereview.chromium.org/1125403004/diff/1/components/translate/core/l...
components/translate/core/language_detection/language_detection_util.cc:120: //
In practice this shouldn't happen, as Chromium should sanitize the
Do you want to add a NOTREACHED here?

[Andrew Hayden (chromium.org), 2015-05-07 12:24:05]

Patchset 2 addresses comments from droger@ and fixes a bug in the
check-for-invalid-utf8 code.

https://codereview.chromium.org/1125403004/diff/1/components/translate/core/l...
File components/translate/core/language_detection/language_detection_util.cc
(right):

https://codereview.chromium.org/1125403004/diff/1/components/translate/core/l...
components/translate/core/language_detection/language_detection_util.cc:115:
utf8_text.c_str(), (int)utf8_text.size(), true /* is_plain_text */,
On 2015/05/07 11:56:57, droger wrote:
> Could you use static_cast here instead of C-style cast?

Done.

https://codereview.chromium.org/1125403004/diff/1/components/translate/core/l...
components/translate/core/language_detection/language_detection_util.cc:117: if
(text_bytes < (int)utf8_text.size() &&
On 2015/05/07 11:56:56, droger wrote:
> same here.

Done.

https://codereview.chromium.org/1125403004/diff/1/components/translate/core/l...
components/translate/core/language_detection/language_detection_util.cc:118:
cld_language != CLD2::UNKNOWN_LANGUAGE) {
That should be a ==, not a !=.

https://codereview.chromium.org/1125403004/diff/1/components/translate/core/l...
components/translate/core/language_detection/language_detection_util.cc:120: //
In practice this shouldn't happen, as Chromium should sanitize the
On 2015/05/07 11:56:56, droger wrote:
> Do you want to add a NOTREACHED here?

Done.

[Andrew Hayden (chromium.org), 2015-05-07 13:37:31]

Fascinating. AutomationApiTest.Find is failing the new NOTREACHED(). I'll see if
I can hunt this down.

[Andrew Hayden (chromium.org), 2015-05-07 14:40:51]

On 2015/05/07 13:37:31, Andrew Hayden wrote:
> Fascinating. AutomationApiTest.Find is failing the new NOTREACHED(). I'll see
if
> I can hunt this down.

I've captured the broken UTF8 from the test case and will upload to the
associated issue, we'll triage the underlying bug there. We'll be blocked on the
resolution of the bug, since we can't break the tests but it isn't clear that
it's the test that is at fault here :)

[Andrew Hayden (chromium.org), 2015-05-08 09:13:47]

OK, new patchset and trying this again based on the outcome of bug discussion
and a chat with chrome-security.

[Andrew Hayden (chromium.org), 2015-05-08 09:16:30]

Updated the change description with the new learnings from the bug.

[Takashi Toyoshima, 2015-05-08 09:31:01]

Is this a tentative fix? If not, UMA on the retry counts will be useful to
monitor failed cases.

[Andrew Hayden (chromium.org), 2015-05-08 09:36:33]

On 2015/05/08 09:31:01, Takashi Toyoshima (chromium) wrote:
> Is this a tentative fix? If not, UMA on the retry counts will be useful to
> monitor failed cases.

No, it's not tentative. We know for certain (see linked bug) that malformed UTF8
does reach CLD2 - the browser tests in this very CL prove it true, see the
failures on patchset 2. This is a permanent fix.

[Takashi Toyoshima, 2015-05-08 12:06:23]

LGTM to fix the current issue, but can you keep a bug opened for investigating
why invalid utf-8 is generated by UTF16ToUTF8?

If the previous failures were deterministic, we can easily get the exact data
that fired the assertion.

[Andrew Hayden (chromium.org), 2015-05-08 12:15:00]

On 2015/05/08 12:06:23, Takashi Toyoshima (chromium) wrote:
> LGTM to fix the current issue, but can you keep a bug opened for investigating
> why invalid utf-8 is generated by UTF16ToUTF8?
> 
> If the previous failures were deterministic, we can easily get the exact data
> that fired the assertion.

Sounds great, thanks Takashi.

I've updated the related bug with exactly that data, and I'm speaking with the
author of CLD2 right now about whether it's Chromium's UTF16->UTF8 that's got an
error or CLD2's UTF8 detection that's wrong. I think we cna chase that down in
444258 or spawn a new one, depending on the outcome.

Since you're good with this as-is for now, I'll get it submitted so we can fix
the crashes. Thank you for the review.

[Andrew Hayden (chromium.org), 2015-05-08 12:15:08]

The CQ bit was checked by andrewhayden@chromium.org

[Andrew Hayden (chromium.org), 2015-05-08 12:15:10]

The patchset sent to the CQ was uploaded after l-g-t-m from droger@chromium.org
Link to the patchset: https://codereview.chromium.org/1125403004/#ps80001
(title: "git cl format")

[commit-bot: I haz the power, 2015-05-08 12:15:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1125403004/80001

[commit-bot: I haz the power, 2015-05-08 12:48:05]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-05-08 12:49:07]

Patchset 5 (id:??) landed as
https://crrev.com/90bb2b934366c595c1c979e0b2363f0a822e1b92
Cr-Commit-Position: refs/heads/master@{#328947}