Handle odd data lengths in UTF-16 decoder

Handle odd data lengths in UTF-16 decoder

The UTF-16 decoder was silently dropping ignoring odd byte lengths.
Per the Encoding standard, it should emit U+FFFD (replacement char),
and set the error flag for interested callers.

This makes TextDecoder match the behavior in Firefox.

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

[jsbell, 2013-08-30 00:11:29]

Uploading for the try bots, but not ready for landing yet.

This needs an additional test of odd-length UTF-16 decoding when there are
streams involved, e.g.

decode(odd, {stream: true}); decode(odd); // no error
decode(even, {stream: true}); decode(odd); // error
decode(odd, {stream: true}); decode(even); // error
decode(even, {stream: true}); decode(even); // no error

[jsbell, 2013-08-30 17:21:41]

Heh, the test-unicode-characters-in-attribute-name.html file is UTF-16 encoded
but had a lone trailing byte, so the output was getting a U+FFFD tacked on,
which is why the layout tests were failing. Removed that byte, and added
fast/encoding/utf-16-odd-byte.html to exercise that case.

[jsbell, 2013-08-30 18:15:49]

On 2013/08/30 17:21:41, jsbell wrote:
> Heh, the test-unicode-characters-in-attribute-name.html file is UTF-16 encoded
> but had a lone trailing byte, so the output was getting a U+FFFD tacked on,
> which is why the layout tests were failing. Removed that byte, and added
> fast/encoding/utf-16-odd-byte.html to exercise that case.

Looks like I'll need to upload those UTF-16 files manually, I guess, and do the
TextExpectations dance for the new one. :(

[jungshik at Google, 2013-08-30 18:22:04]

I guess  "LayoutTests/fast/encoding/utf-16-odd-byte.html" has a string in the
corresponding expected file in UTF-16 (+ 1 extra byte), but codereview just
showed it as empty.

With that confirmed, LGTM !

[jungshik at Google, 2013-08-30 18:23:37]

On 2013/08/30 18:22:04, Jungshik Shin wrote:
> I guess  "LayoutTests/fast/encoding/utf-16-odd-byte.html" has a string in the
> corresponding expected file in UTF-16 (+ 1 extra byte), but codereview just
> showed it as empty.

I meant: "has the same string as the corresponding result file but in UTF-16
with one extra byte at the end."  :-) 

> 
> With that confirmed, LGTM !

[jsbell, 2013-08-30 18:40:23]

On 2013/08/30 18:23:37, Jungshik Shin wrote:
> On 2013/08/30 18:22:04, Jungshik Shin wrote:
> > I guess  "LayoutTests/fast/encoding/utf-16-odd-byte.html" has a string in
the
> > corresponding expected file in UTF-16 (+ 1 extra byte), but codereview just
> > showed it as empty.
> 
> I meant: "has the same string as the corresponding result file but in UTF-16
> with one extra byte at the end."  :-) 

Correct. I'll land it manually (disabled), and you can byte-inspect it from a
checkout.

[Ken Russell (switch to Gerrit), 2013-08-30 21:26:23]

lgtm

[jsbell, 2013-09-03 17:54:27]

The http/tests/appcache/online-whitelist.html failure looks unrelated... appears
to be flaky in other runs.

[jsbell, 2013-09-03 18:17:27]

On 2013/09/03 17:54:27, jsbell wrote:
> The http/tests/appcache/online-whitelist.html failure looks unrelated...
appears
> to be flaky in other runs.

On linux at least the appcache tests don't go through this code at all, so
pretty sure it's not related.

[jsbell, 2013-09-03 18:18:11]

jamesr@ - OWNERS review for Source/wtf?

[jamesr, 2013-09-04 22:31:31]

Have you run this test case on other browsers?  I'm a little worried about
corner cases like this lacking interoperability, especially if there are no
standard tests.  Documenting the current state of the world would help a bit.

[jsbell, 2013-09-04 23:45:16]

On 2013/09/04 22:31:31, jamesr wrote:
> Have you run this test case on other browsers?  I'm a little worried about
> corner cases like this lacking interoperability, especially if there are no
> standard tests.  Documenting the current state of the world would help a bit.

FF implements the Encoding API and passes these tests.

It does not appear to emit a trailing replacement character for the
utf-16-odd-byte.html test, which is interesting. We might have to special-case
the codec to match. I'll ping FF devs to see if this is intentional.

[jamesr, 2013-09-04 23:46:48]

There's a recent thread on WHATWG titled "Encoding: lone surrogates and utf-8,
utf-16be, and utf-16le encoders" - is that about this case?

[jsbell, 2013-09-05 00:03:19]

On 2013/09/04 23:46:48, jamesr wrote:
> There's a recent thread on WHATWG titled "Encoding: lone surrogates and utf-8,
> utf-16be, and utf-16le encoders" - is that about this case?

*checks his email*

Nope. This is a *decoder* issue - what happens when content (in this case, the
page itself) has a lone surrogate.

[abarth-chromium, 2013-09-05 05:57:51]

Seems like reasonable behavior to me, but I haven't read the spec.

https://codereview.chromium.org/23532016/diff/19001/Source/wtf/text/TextCodec...
File Source/wtf/text/TextCodecUTF16.cpp (right):

https://codereview.chromium.org/23532016/diff/19001/Source/wtf/text/TextCodec...
Source/wtf/text/TextCodecUTF16.cpp:77: s.append(Unicode::replacementCharacter);
Never call String::append.  It's a terrible, no good, awful function.  The idiom
you want here is:

return String(&Unicode::replacementCharacter, 1);

[jsbell, 2013-09-05 15:45:10]

On 2013/09/05 05:57:51, abarth wrote:
> The idiom you want here is:
> 
> return String(&Unicode::replacementCharacter, 1);

Excellent, thanks.

Re: Firefox behavior - it's a bug in their decoder that they don't flush
properly:

https://bugzilla.mozilla.org/show_bug.cgi?id=562590

I'll check IE. If none of us are flushing on doc load this could be a compat
issue. :(

[jsbell, 2013-09-05 15:52:26]

On 2013/09/05 15:45:10, jsbell wrote:
> 
> I'll check IE. If none of us are flushing on doc load this could be a compat
> issue. :(

Bummer - IE10 doesn't show a replacement character either.

[jungshik at Google, 2013-09-05 19:50:07]

On 2013/09/05 15:52:26, jsbell wrote:
> On 2013/09/05 15:45:10, jsbell wrote:
> > 
> > I'll check IE. If none of us are flushing on doc load this could be a compat
> > issue. :(
> 
> Bummer - IE10 doesn't show a replacement character either.

If Firefox regards it as a bug (which it is) and is going to fix it, shouldn't
we go ahead to flush it properly? Hopefully, IE 10 will follow  suit?

[abarth-chromium, 2013-09-06 05:09:35]

On 2013/09/05 19:50:07, Jungshik Shin wrote:
> If Firefox regards it as a bug (which it is) and is going to fix it, shouldn't
> we go ahead to flush it properly? Hopefully, IE 10 will follow  suit?

Why not just have everyone keep the current behavior and remain perfectly
interoperable?

[jsbell, 2013-09-06 15:50:48]

On 2013/09/06 05:09:35, abarth wrote:
> On 2013/09/05 19:50:07, Jungshik Shin wrote:
> > If Firefox regards it as a bug (which it is) and is going to fix it,
shouldn't
> > we go ahead to flush it properly? Hopefully, IE 10 will follow  suit?
> 
> Why not just have everyone keep the current behavior and remain perfectly
> interoperable?

The pedantic answer is: because it requires a special case in the decoder
algorithm just for the document body. Elsewhere[1] browsers[2] flush at the end
of a stream and a stray byte in UTF-16 results in a replacement character in the
output code point stream.

[1] I'm not sure where, we should nail this down. XHR? I'm not sure.
[2] And not sure which browsers do what, where. It's possible the spec is a
platonic ideal and we all do have identically buggy decoders in all cases.

I will try to answer those, and/or push back on the Encoding spec here.

[jsbell, 2013-10-08 21:45:36]

On 2013/09/06 15:50:48, jsbell wrote:
> On 2013/09/06 05:09:35, abarth wrote:
> > On 2013/09/05 19:50:07, Jungshik Shin wrote:
> > > If Firefox regards it as a bug (which it is) and is going to fix it,
> shouldn't
> > > we go ahead to flush it properly? Hopefully, IE 10 will follow  suit?
> > 
> > Why not just have everyone keep the current behavior and remain perfectly
> > interoperable?
> 
> The pedantic answer is: because it requires a special case in the decoder
> algorithm just for the document body. Elsewhere[1] browsers[2] flush at the
end
> of a stream and a stray byte in UTF-16 results in a replacement character in
the
> output code point stream.
> 
> [1] I'm not sure where, we should nail this down. XHR? I'm not sure.
> [2] And not sure which browsers do what, where. It's possible the spec is a
> platonic ideal and we all do have identically buggy decoders in all cases.
> 
> I will try to answer those, and/or push back on the Encoding spec here.

Just one more bit of data here: the call into the codec w/ flush set is coming
from core/fetch (TextResourceDecoder::flush) so presumably XHR would suffer the
same problem.

I gave Firefox a try, and indeed with XHR (and xhr.overrideMimeType('text/plain;
charset=utf-16le');) they are not emitting the replacement character either.

From conversation with Moz folks, this will likely affect all codecs in FF since
they don't plumb the "flush" through on fetch. Blink does, it was just unhandled
in the UTF-16 codec. I haven't tested other codecs yet, though.

[jsbell, 2013-12-06 22:43:33]

Throwing this out for review again. Since it was last reviewed by jshin@ and
abarth@:

* No longer flush codec at the end of resource fetch. In the case of truncated
multibyte sequences, the incomplete character is dropped rather than producing
U+FFFD REPLACEMENT CHARACTER. This is consistent with our previously (buggy)
UTF-16 decoder, and new behavior for other decoders. 

This matches Firefox, with one exception: in UTF-16 cases where it's an
incomplete surrogate pair. For example with UTF-16BE with D8 00 DC or D8 00

Chrome: the lead surrogate passes through, i.e. the decode emits \uD800

FF: the lead surrogate is dropped, i.e. the decode emits nothing

This UTF-16 case is no change from Chrome's previous behavior, and is captured
in an existing FIXME in TextCodecUTF16: "This should generate an error if there
is an unpaired surrogate."

Given truncated data, the script API still produces U+FFFD. This matches Firefox
(and the Encoding spec).

jshin@ - what do you think?

abarth@ - is this reasonable?

[jsbell, 2013-12-20 18:25:24]

Pinging again - see previous comment on the CL:

jshin@ - what do you think?
abarth@ - is this reasonable?

[jungshik at Google, 2014-01-02 19:07:48]

On 2013/12/20 18:25:24, jsbell wrote:
> Pinging again - see previous comment on the CL:
> 
> jshin@ - what do you think?
> abarth@ - is this reasonable?

Sorry for the late reply. I'm rather confused as to what we do when for what
reason. 

> * No longer flush codec at the end of resource fetch. 

Is this to be compatible with Firefox? What's their plan about flushing? Is
there any provision in the encoding spec about this issue if it falls into its
realm (maybe not)? 
If this is mainly due to the compatibility with Firefox, why do you keep UTF-16
behavior (a lone surrogate) different from Firefox? Does FIXME mean that it'll
match Firefox' behavior eventually?  

> Given truncated data, the script API still produces U+FFFD. 
> This matches Firefox (and the Encoding spec).

This part looks good.

[jsbell, 2014-01-02 19:19:30]

On 2014/01/02 19:07:48, Jungshik Shin wrote:
> On 2013/12/20 18:25:24, jsbell wrote:
> > Pinging again - see previous comment on the CL:
> > 
> > jshin@ - what do you think?
> > abarth@ - is this reasonable?
> 
> Sorry for the late reply. I'm rather confused as to what we do when for what
> reason. 

No worries. It's not urgent for me, either.

And to be clear: I'd rather we fixed the underlying bug (not flushing for
UTF-16) by flushed for all codecs rather than matching FF's bug and flushing for
no codecs, but wanted to present this option.

> > * No longer flush codec at the end of resource fetch. 
> 
> Is this to be compatible with Firefox? What's their plan about flushing? 

Yes. Their bug is: https://bugzilla.mozilla.org/show_bug.cgi?id=562590 -
untouched since 2010.

> Is
> there any provision in the encoding spec about this issue if it falls into its
> realm (maybe not)? 

If we decide to standardize on the "don't flush results of fetch" it would
probably require an addition to the Encoding spec (and possibly Fetch spec?).

I'll poke the bug to see what the latest Moz thinking is.

> If this is mainly due to the compatibility with Firefox, why do you keep
UTF-16
> behavior (a lone surrogate) different from Firefox? Does FIXME mean that it'll
> match Firefox' behavior eventually?  

We can tackle that as a separate patch. FF and the spec agree there.
 
> > Given truncated data, the script API still produces U+FFFD. 
> > This matches Firefox (and the Encoding spec).
> 
> This part looks good.

[jungshik at Google, 2014-01-02 23:28:08]

On 2014/01/02 19:19:30, jsbell wrote:
> On 2014/01/02 19:07:48, Jungshik Shin wrote:
> > On 2013/12/20 18:25:24, jsbell wrote:
> > > Pinging again - see previous comment on the CL:
> > > 
> > > jshin@ - what do you think?
> > > abarth@ - is this reasonable?
> > 
> > Sorry for the late reply. I'm rather confused as to what we do when for what
> > reason. 
> 
> No worries. It's not urgent for me, either.
> 
> And to be clear: I'd rather we fixed the underlying bug (not flushing for
> UTF-16) by flushed for all codecs rather than matching FF's bug and flushing
for
> no codecs, but wanted to present this option.
> 

Thank you for the clarification. It's my preference, too, to flush for all
codecs. So, let's see what you hear from Mozilla guys about their bug 562590.
I'll add myself to the bug.  

> > > * No longer flush codec at the end of resource fetch. 
> > 
> > Is this to be compatible with Firefox? What's their plan about flushing? 
> 
> Yes. Their bug is: https://bugzilla.mozilla.org/show_bug.cgi?id=562590 -
> untouched since 2010.
> 
> > Is
> > there any provision in the encoding spec about this issue if it falls into
its
> > realm (maybe not)? 
> 
> If we decide to standardize on the "don't flush results of fetch" it would
> probably require an addition to the Encoding spec (and possibly Fetch spec?).
> 
> I'll poke the bug to see what the latest Moz thinking is.
> 
> > If this is mainly due to the compatibility with Firefox, why do you keep
> UTF-16
> > behavior (a lone surrogate) different from Firefox? Does FIXME mean that
it'll
> > match Firefox' behavior eventually?  
> 
> We can tackle that as a separate patch. FF and the spec agree there.
>  
> > > Given truncated data, the script API still produces U+FFFD. 
> > > This matches Firefox (and the Encoding spec).
> > 
> > This part looks good.

[abarth-chromium, 2014-01-04 05:26:41]

lgtm

[abarth-chromium, 2014-01-04 05:42:00]

On 2014/01/04 05:26:41, abarth wrote:
> lgtm

Sorry, I missed the rest of this thread because it was split up by gmail.

[jsbell, 2014-02-28 22:29:24]

I wasn't satisfied with the last patch since it made us *less* standard, even if
bug-compatible with Firefox. 

This new patch introduces no behavior changes by making "flush" a 3 value enum:
`DoNotFlush`, `FetchEOF` (codecs may introduce compatibility behavior, like not
emitting replacement characters), and `DataEOF` (per-spec). I don't like the
names, so suggestions welcome.

Only TextCodecUTF16 behaves differently between FetchEOF and DataEOF, to
preserve the previous behavior. Newfangled places can pass DataEOF (e.g.
Encoding API) to get true flushing, old call sites can pass FetchEOF to get
"compat" behavior.

WDYT?

[Ken Russell (switch to Gerrit), 2014-02-28 23:21:43]

On 2014/02/28 22:29:24, jsbell wrote:
> I wasn't satisfied with the last patch since it made us *less* standard, even
if
> bug-compatible with Firefox. 
> 
> This new patch introduces no behavior changes by making "flush" a 3 value
enum:
> `DoNotFlush`, `FetchEOF` (codecs may introduce compatibility behavior, like
not
> emitting replacement characters), and `DataEOF` (per-spec). I don't like the
> names, so suggestions welcome.
> 
> Only TextCodecUTF16 behaves differently between FetchEOF and DataEOF, to
> preserve the previous behavior. Newfangled places can pass DataEOF (e.g.
> Encoding API) to get true flushing, old call sites can pass FetchEOF to get
> "compat" behavior.
> 
> WDYT?

As a non-expert in the code, this sounds good. I'd like to see this change land
so that progress on the TextEncoder/Decoder can be made.

[jsbell, 2014-03-06 18:39:31]

Friendly ping - the patch has been substantially reworked so I'm discounting the
above l*g*t*ms.

abarth@, jshin@ - PTAL?

[abarth-chromium, 2014-03-06 22:02:54]

Still LGTM.  Thanks!

[jsbell, 2014-03-07 18:22:15]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2014-03-07 18:22:28]

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

[commit-bot: I haz the power, 2014-03-07 18:58:34]

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

[commit-bot: I haz the power, 2014-03-07 18:58:35]

Try jobs failed on following builders: linux_blink_rel

[jsbell, 2014-03-07 21:31:40]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2014-03-07 21:31:48]

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

[commit-bot: I haz the power, 2014-03-08 03:53:02]

Change committed as 168763