UTF-16 Decoder: Convert unpaired surrogates to replacement characters

UTF-16 Decoder: Convert unpaired surrogates to replacement characters

The decoder blithely passed any old 16-bit code unit through, in
violation of the Encoding standard. Surrogate pairs should go through
unscathed:

  [ ... 0xD800 0xDC00 ... ] => [ ... U+D800 U+DC00 ... ]

But cases like these should result in replacement characters:

  [ ... 0xD800 ... ] => [ ... U+FFFD ... ]
  [ ... 0xDC00 ... ] => [ ... U+FFFD ... ]
  [ ... 0xDC00 0xD800 ... ] => [ ... U+FFFD U+FFFD ... ]

This aligns Chrome's behavior with Firefox and Edge.

Note that flushing at the end of a stream remains a special case.
Streams terminating in the above sequences will not get replacements
emitted (current behavior). In addition, a lead surrogate appearing at
the end of a stream will now not be emitted, matching other browsers.

BUG=368904
R=jshin@chromium.org,foolip@chromium.org

Committed: https://crrev.com/9158f6d5f23e54cbce3748539c68cbfdce218bd4
Cr-Commit-Position: refs/heads/master@{#422929}

[jsbell, 2016-09-30 21:07:56]

The CQ bit was checked by jsbell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 21:08:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jsbell, 2016-09-30 21:31:47]

Description was changed from

==========
UTF-16 Decoder: Convert unpaired surrogates to replacement characters

The decoder blithely passed any old 16-bit code unit through, in
violation of the Encoding standard. Surrogate pairs should go through
unscathed:

  [ ... 0xD800 0xDC00 ... ] => [ ... U+D800 U+DC00 ... ]

But cases like these should result in replacement characters:

  [ ... 0xD800 ... ] => [ ... U+FFFD ... ]
  [ ... 0xDC00 ... ] => [ ... U+FFFD ... ]
  [ ... 0xDC00 0xD800 ... ] => [ ... U+FFFD U+FFFD ... ]

This aligns Chrome's behavior with Firefox and Edge.

Note that flushing at the end of a stream remains a special case.
Streams terminating in the above sequences will not get replacements
emitted (current behavior). In addition, a lead surrogate appearing at
the end of a stream will now not be emitted.

BUG=368904
R=jshin@chromium.org,foolip@chromium.org
==========

to

==========
UTF-16 Decoder: Convert unpaired surrogates to replacement characters

The decoder blithely passed any old 16-bit code unit through, in
violation of the Encoding standard. Surrogate pairs should go through
unscathed:

  [ ... 0xD800 0xDC00 ... ] => [ ... U+D800 U+DC00 ... ]

But cases like these should result in replacement characters:

  [ ... 0xD800 ... ] => [ ... U+FFFD ... ]
  [ ... 0xDC00 ... ] => [ ... U+FFFD ... ]
  [ ... 0xDC00 0xD800 ... ] => [ ... U+FFFD U+FFFD ... ]

This aligns Chrome's behavior with Firefox and Edge.

Note that flushing at the end of a stream remains a special case.
Streams terminating in the above sequences will not get replacements
emitted (current behavior). In addition, a lead surrogate appearing at
the end of a stream will now not be emitted, matching other browsers.

BUG=368904
R=jshin@chromium.org,foolip@chromium.org
==========

[jsbell, 2016-09-30 21:35:13]

jshin@, foolip@ - please take a look?

This is less risky than another approach
(https://codereview.chromium.org/1366433005/) since we retain control of the
flushing behavior.

I don't know what the perf impact of this will be; the decode loop can be
slightly optimized (see comments in the code) but just surrogates will
definitely add some cycles. This might be a place to consider UNLIKELY macros.

[commit-bot: I haz the power, 2016-09-30 22:45:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-30 22:45:35]

Dry run: This issue passed the CQ dry run.

[foolip, 2016-09-30 22:59:41]

Will defer to jungshik, but lgtm given my unfamiliarity with the code.

https://codereview.chromium.org/2379333003/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/fast/encoding/utf-16-lone-surrogates.html
(right):

https://codereview.chromium.org/2379333003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/encoding/utf-16-lone-surrogates.html:6:
description("Test encoding behavior for lone UTF-16 surrogates");
Are these testing something in addition to wpt? Can it be written using
testharness.js and then upstreamed?

https://codereview.chromium.org/2379333003/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/text/TextCodecUTF16.cpp (right):

https://codereview.chromium.org/2379333003/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/text/TextCodecUTF16.cpp:133:
DCHECK(!m_haveLeadByte);
I think it's the m_haveLeadByte=false in the loop that makes this hold even if
called with length=2 and m_haveLeadByte=true, but doesn't that hold even if
!willHaveExtraByte, i.e. can this DCHECK be outside the if statement?

[jsbell, 2016-09-30 23:52:13]

https://codereview.chromium.org/2379333003/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/fast/encoding/utf-16-lone-surrogates.html
(right):

https://codereview.chromium.org/2379333003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/encoding/utf-16-lone-surrogates.html:6:
description("Test encoding behavior for lone UTF-16 surrogates");
On 2016/09/30 22:59:41, foolip wrote:
> Are these testing something in addition to wpt? Can it be written using
> testharness.js and then upstreamed?

Copied for consistency with other tests in the same folder. `testDecode` makes
use of things like (ugh) sync XHR. We should convert the whole bunch of them
over and make them async/upstreamable. Was hoping to do that later. :)

https://codereview.chromium.org/2379333003/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/text/TextCodecUTF16.cpp (right):

https://codereview.chromium.org/2379333003/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/text/TextCodecUTF16.cpp:133:
DCHECK(!m_haveLeadByte);
On 2016/09/30 22:59:41, foolip wrote:
> i.e. can this DCHECK be outside the if statement?

Yes.

[jungshik at Google, 2016-10-04 17:32:02]

LGTM !  Thanks

[jsbell, 2016-10-04 19:03:07]

The CQ bit was checked by jsbell@chromium.org

[jsbell, 2016-10-04 19:03:07]

The patchset sent to the CQ was uploaded after l-g-t-m from foolip@chromium.org,
jshin@chromium.org
Link to the patchset: https://codereview.chromium.org/2379333003/#ps40001
(title: "Rebase, switch test to testharness")

[commit-bot: I haz the power, 2016-10-04 19:03:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-04 21:05:44]

Description was changed from

==========
UTF-16 Decoder: Convert unpaired surrogates to replacement characters

The decoder blithely passed any old 16-bit code unit through, in
violation of the Encoding standard. Surrogate pairs should go through
unscathed:

  [ ... 0xD800 0xDC00 ... ] => [ ... U+D800 U+DC00 ... ]

But cases like these should result in replacement characters:

  [ ... 0xD800 ... ] => [ ... U+FFFD ... ]
  [ ... 0xDC00 ... ] => [ ... U+FFFD ... ]
  [ ... 0xDC00 0xD800 ... ] => [ ... U+FFFD U+FFFD ... ]

This aligns Chrome's behavior with Firefox and Edge.

Note that flushing at the end of a stream remains a special case.
Streams terminating in the above sequences will not get replacements
emitted (current behavior). In addition, a lead surrogate appearing at
the end of a stream will now not be emitted, matching other browsers.

BUG=368904
R=jshin@chromium.org,foolip@chromium.org
==========

to

==========
UTF-16 Decoder: Convert unpaired surrogates to replacement characters

The decoder blithely passed any old 16-bit code unit through, in
violation of the Encoding standard. Surrogate pairs should go through
unscathed:

  [ ... 0xD800 0xDC00 ... ] => [ ... U+D800 U+DC00 ... ]

But cases like these should result in replacement characters:

  [ ... 0xD800 ... ] => [ ... U+FFFD ... ]
  [ ... 0xDC00 ... ] => [ ... U+FFFD ... ]
  [ ... 0xDC00 0xD800 ... ] => [ ... U+FFFD U+FFFD ... ]

This aligns Chrome's behavior with Firefox and Edge.

Note that flushing at the end of a stream remains a special case.
Streams terminating in the above sequences will not get replacements
emitted (current behavior). In addition, a lead surrogate appearing at
the end of a stream will now not be emitted, matching other browsers.

BUG=368904
R=jshin@chromium.org,foolip@chromium.org
==========

[commit-bot: I haz the power, 2016-10-04 21:05:47]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-04 21:09:23]

Description was changed from

==========
UTF-16 Decoder: Convert unpaired surrogates to replacement characters

The decoder blithely passed any old 16-bit code unit through, in
violation of the Encoding standard. Surrogate pairs should go through
unscathed:

  [ ... 0xD800 0xDC00 ... ] => [ ... U+D800 U+DC00 ... ]

But cases like these should result in replacement characters:

  [ ... 0xD800 ... ] => [ ... U+FFFD ... ]
  [ ... 0xDC00 ... ] => [ ... U+FFFD ... ]
  [ ... 0xDC00 0xD800 ... ] => [ ... U+FFFD U+FFFD ... ]

This aligns Chrome's behavior with Firefox and Edge.

Note that flushing at the end of a stream remains a special case.
Streams terminating in the above sequences will not get replacements
emitted (current behavior). In addition, a lead surrogate appearing at
the end of a stream will now not be emitted, matching other browsers.

BUG=368904
R=jshin@chromium.org,foolip@chromium.org
==========

to

==========
UTF-16 Decoder: Convert unpaired surrogates to replacement characters

The decoder blithely passed any old 16-bit code unit through, in
violation of the Encoding standard. Surrogate pairs should go through
unscathed:

  [ ... 0xD800 0xDC00 ... ] => [ ... U+D800 U+DC00 ... ]

But cases like these should result in replacement characters:

  [ ... 0xD800 ... ] => [ ... U+FFFD ... ]
  [ ... 0xDC00 ... ] => [ ... U+FFFD ... ]
  [ ... 0xDC00 0xD800 ... ] => [ ... U+FFFD U+FFFD ... ]

This aligns Chrome's behavior with Firefox and Edge.

Note that flushing at the end of a stream remains a special case.
Streams terminating in the above sequences will not get replacements
emitted (current behavior). In addition, a lead surrogate appearing at
the end of a stream will now not be emitted, matching other browsers.

BUG=368904
R=jshin@chromium.org,foolip@chromium.org

Committed: https://crrev.com/9158f6d5f23e54cbce3748539c68cbfdce218bd4
Cr-Commit-Position: refs/heads/master@{#422929}
==========

[commit-bot: I haz the power, 2016-10-04 21:09:24]

Patchset 3 (id:??) landed as
https://crrev.com/9158f6d5f23e54cbce3748539c68cbfdce218bd4
Cr-Commit-Position: refs/heads/master@{#422929}

[jsbell, 2016-10-04 21:37:20]

** Future Perf Sheriffs! **

If this regresses perf, just go ahead and revert.

I've got a few tricks I can try if necessary, but I'd prefer we revert and show
that we've cleanly recovered from the regression rather than iterating.