Make UTF-8 encoding of unpaired surrogates match Encoding standard

Make UTF-8 encoding of unpaired surrogates match Encoding standard

The Encoding standard says that unpaired UTF-16 surrogates in JS
strings should be converted into U+FFFD (replacement character)
during encode operations. This is (optionally) done already in
WTFString::utf8() but not handled in TextCodecUTF8.

BUG=277035
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=157459

[jsbell, 2013-08-29 21:44:20]

This is a pretty scary change, just throwing it up for the bots to party on for
now.

[jungshik at Google, 2013-08-30 00:39:26]

https://codereview.chromium.org/23601010/diff/1/LayoutTests/fast/encoding/api...
File LayoutTests/fast/encoding/api/surrogate-pairs.html (right):

https://codereview.chromium.org/23601010/diff/1/LayoutTests/fast/encoding/api...
LayoutTests/fast/encoding/api/surrogate-pairs.html:8: { input: "'abc123'",
expected: [97, 98, 99, 49, 50, 51] }, // Sanity check.
any reason to use decimal instead of hexadecimal? I can kinda decode a UTF-8
byte sequence in hexadecimal to a Unicode code point, but can't do that with
decimal. :-)

https://codereview.chromium.org/23601010/diff/1/Source/wtf/text/TextCodecUTF8...
File Source/wtf/text/TextCodecUTF8.cpp (right):

https://codereview.chromium.org/23601010/diff/1/Source/wtf/text/TextCodecUTF8...
Source/wtf/text/TextCodecUTF8.cpp:440: character = 0xFFFD;
For a future code archaeologist, could you add a comment to the effect that
"U16_NEXT" will set |character| to a surrogate code point (U+D800 .. U+DFFF)
when coming across an unmatched surrogate code point" and we have to convert it
to a replacement character.

[jsbell, 2013-08-30 00:43:56]

https://codereview.chromium.org/23601010/diff/1/LayoutTests/fast/encoding/api...
File LayoutTests/fast/encoding/api/surrogate-pairs.html (right):

https://codereview.chromium.org/23601010/diff/1/LayoutTests/fast/encoding/api...
LayoutTests/fast/encoding/api/surrogate-pairs.html:8: { input: "'abc123'",
expected: [97, 98, 99, 49, 50, 51] }, // Sanity check.
On 2013/08/30 00:39:27, Jungshik Shin wrote:
> any reason to use decimal instead of hexadecimal? I can kinda decode a UTF-8
> byte sequence in hexadecimal to a Unicode code point, but can't do that with
> decimal. :-) 

Agreed - I was copying/pasting from a JS console. Will fix.

https://codereview.chromium.org/23601010/diff/1/Source/wtf/text/TextCodecUTF8...
File Source/wtf/text/TextCodecUTF8.cpp (right):

https://codereview.chromium.org/23601010/diff/1/Source/wtf/text/TextCodecUTF8...
Source/wtf/text/TextCodecUTF8.cpp:440: character = 0xFFFD;
On 2013/08/30 00:39:27, Jungshik Shin wrote:
> For a future code archaeologist, could you add a comment to the effect that
> "U16_NEXT" will set |character| to a surrogate code point (U+D800 .. U+DFFF)
> when coming across an unmatched surrogate code point" and we have to convert
it
> to a replacement character. 

Will do. I'll also use WTF::Unicode::replacementCharacter which I discovered
after uploading this.

[jungshik at Google, 2013-08-30 18:09:02]

LGTM !

[Ken Russell (switch to Gerrit), 2013-08-30 18:11:24]

lgtm

[jsbell, 2013-09-03 18:20:10]

jamesr@ - OWNERS review for Source/wtf?

[jamesr, 2013-09-05 01:50:21]

Same comment as on the other - could you test in other browsers and put the
results of that testing in the patch description?

[jsbell, 2013-09-05 20:28:55]

+tkent@

On 2013/09/05 01:50:21, jamesr wrote:
> Same comment as on the other - could you test in other browsers and put the
> results of that testing in the patch description?

So this is interesting. As far as the Encoding API goes, Firefox passes.

I added the char-encoding.html test which exercises encoding via forms. Firefox
also passes, i.e:

PASS encode('UTF-8', '0xD800') is '%EF%BF%BD'
PASS encode('UTF-8', '0xDC00') is '%EF%BF%BD'

Without this patch, we fail both. With this patch, we pass only one:

FAIL encode('UTF-8', '0xD800') should be %EF%BF%BD. Was .
PASS encode('UTF-8', '0xDC00') is '%EF%BF%BD'

The reason for the failure is a recently added clause in
TextFieldInputType.cpp's limitLength() added in
https://chromiumcodereview.appspot.com/22590003 - 

+    if (newLength > 0 && U16_IS_LEAD(string[newLength - 1]))
+        --newLength;

... which basically removes a dangling surrogate lead. That function doesn't do
any other UTF-16 surrogate pair cleanup, though - it would not remove a
surrogate tail or replace lone surrogates elsewhere in the string. Since the
dangling surrogate half is removed, no replacement character is inserted by the
new clause in this CL, hence the FAIL. 

I need to ponder more what to suggest here. The limitLength() function seems to
be assuming things about the downstream encoding; I'm not entirely sure without
more experimenting if the function as written does the right thing for other
unpaired surrogate cases.

[tkent, 2013-09-05 21:49:06]

limitLength() has a bug.  I filed crbug.com/285997.

[jsbell, 2013-09-05 22:32:54]

On 2013/09/05 21:49:06, tkent wrote:
> limitLength() has a bug.  I filed crbug.com/285997.

Thanks - that new CL (crrev.com/23526022) clarifies what crrev.com/22590003 was
trying to do. I believe after crrev.com/23526022 lands, this patch would make us
pass both cases i.e. the \uD800 would be replaced with U+FFFD in the encoded
data, matching Firefox's behavior.

So - waiting until that CL lands, then I'll rebase this CL.

[jsbell, 2013-09-06 16:34:25]

Rebased on tkent's fix. As predicted, we pass the char-encoding.html test cases
now.

Encoding API:
* Firefox - passes test
* Chrome (w/ this CL) - passes tests
* IE - NYI

Form encoding (new char-encoding.html test cases):
* Firefox - passes test
* Chrome (w/ this CL) - passes test
* IE10 - fails, emitting � and � (i.e. � and �)

For reference, Chrome's previous behavior (prior to tkent's first patch) was to
emit an (illegal?) UTF-8 sequence that decodes to 0xD800/0xDC00.

[tkent, 2013-09-08 21:27:07]

lgtm

[commit-bot: I haz the power, 2013-09-09 15:47:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jsbell@chromium.org/23601010/28001

[commit-bot: I haz the power, 2013-09-09 16:58:00]

Change committed as 157459