Introduce String::fromUTF8Lenient() and use it for cache_name in CacheStorage API.

Introduce String::fromUTF8Lenient() and use it for cache_name in CacheStorage API.

LenientUTF8Conversion mode of WTF::String::utf8() converts a unpaired surrogate to the value of [EDA080-EDAFBF] and [EDB080-EDBFBF].
But current WTF::String::fromUTF8() doesn't support those values.

This cl introduce fromUTF8Lenient() which supports the unpaired surrogates.
And use WebString::fromUTF8Lenient() in CacheStorage API to fix https://crbug.com/432746

BUG=432746

[horo, 2016-03-07 08:36:37]

Description was changed from

==========
Introduce WebString::fromUTF8Lenient() and use for cache_name in CacheStorage
API.

BUG=
==========

to

==========
Introduce WebString::fromUTF8Lenient() and use for cache_name in CacheStorage
API.

LenientUTF8Conversion mode of WTF::String::utf8() converts a unpaired surrogate
to the value of  [EDA080 - EDAFBF] and [EDB080-EDBFBF].
But current WTF::String::fromUTF8() doesn't support those values.

This cl introduce fromUTF8Lenient() which support the unpaired surrogates.
And use WebString::fromUTF8Lenient() in CacheStorage API to fix
https://crbug.com/432746

BUG=432746
==========

[horo, 2016-03-07 08:36:59]

Description was changed from

==========
Introduce WebString::fromUTF8Lenient() and use for cache_name in CacheStorage
API.

LenientUTF8Conversion mode of WTF::String::utf8() converts a unpaired surrogate
to the value of  [EDA080 - EDAFBF] and [EDB080-EDBFBF].
But current WTF::String::fromUTF8() doesn't support those values.

This cl introduce fromUTF8Lenient() which support the unpaired surrogates.
And use WebString::fromUTF8Lenient() in CacheStorage API to fix
https://crbug.com/432746

BUG=432746
==========

to

==========
Introduce WebString::fromUTF8Lenient() and use for cache_name in CacheStorage
API.

LenientUTF8Conversion mode of WTF::String::utf8() converts a unpaired surrogate
to the value of [EDA080 - EDAFBF] and [EDB080-EDBFBF].
But current WTF::String::fromUTF8() doesn't support those values.

This cl introduce fromUTF8Lenient() which support the unpaired surrogates.
And use WebString::fromUTF8Lenient() in CacheStorage API to fix
https://crbug.com/432746

BUG=432746
==========

[horo, 2016-03-07 08:37:26]

Description was changed from

==========
Introduce WebString::fromUTF8Lenient() and use for cache_name in CacheStorage
API.

LenientUTF8Conversion mode of WTF::String::utf8() converts a unpaired surrogate
to the value of [EDA080 - EDAFBF] and [EDB080-EDBFBF].
But current WTF::String::fromUTF8() doesn't support those values.

This cl introduce fromUTF8Lenient() which support the unpaired surrogates.
And use WebString::fromUTF8Lenient() in CacheStorage API to fix
https://crbug.com/432746

BUG=432746
==========

to

==========
Introduce WebString::fromUTF8Lenient() and use for cache_name in CacheStorage
API.

LenientUTF8Conversion mode of WTF::String::utf8() converts a unpaired surrogate
to the value of [EDA080 - EDAFBF] and [EDB080-EDBFBF].
But current WTF::String::fromUTF8() doesn't support those values.

This cl introduce fromUTF8Lenient() which supports the unpaired surrogates.
And use WebString::fromUTF8Lenient() in CacheStorage API to fix
https://crbug.com/432746

BUG=432746
==========

[horo, 2016-03-07 08:38:38]

Description was changed from

==========
Introduce WebString::fromUTF8Lenient() and use for cache_name in CacheStorage
API.

LenientUTF8Conversion mode of WTF::String::utf8() converts a unpaired surrogate
to the value of [EDA080 - EDAFBF] and [EDB080-EDBFBF].
But current WTF::String::fromUTF8() doesn't support those values.

This cl introduce fromUTF8Lenient() which supports the unpaired surrogates.
And use WebString::fromUTF8Lenient() in CacheStorage API to fix
https://crbug.com/432746

BUG=432746
==========

to

==========
Introduce WebString::fromUTF8Lenient() and use for cache_name in CacheStorage
API.

LenientUTF8Conversion mode of WTF::String::utf8() converts a unpaired surrogate
to the value of [EDA080-EDAFBF] and [EDB080-EDBFBF].
But current WTF::String::fromUTF8() doesn't support those values.

This cl introduce fromUTF8Lenient() which supports the unpaired surrogates.
And use WebString::fromUTF8Lenient() in CacheStorage API to fix
https://crbug.com/432746

BUG=432746
==========

[horo, 2016-03-07 08:41:08]

Patchset #1 (id:1) has been deleted

[horo, 2016-03-07 08:55:25]

Description was changed from

==========
Introduce WebString::fromUTF8Lenient() and use for cache_name in CacheStorage
API.

LenientUTF8Conversion mode of WTF::String::utf8() converts a unpaired surrogate
to the value of [EDA080-EDAFBF] and [EDB080-EDBFBF].
But current WTF::String::fromUTF8() doesn't support those values.

This cl introduce fromUTF8Lenient() which supports the unpaired surrogates.
And use WebString::fromUTF8Lenient() in CacheStorage API to fix
https://crbug.com/432746

BUG=432746
==========

to

==========
Introduce String::fromUTF8Lenient() and use it for cache_name in CacheStorage
API.

LenientUTF8Conversion mode of WTF::String::utf8() converts a unpaired surrogate
to the value of [EDA080-EDAFBF] and [EDB080-EDBFBF].
But current WTF::String::fromUTF8() doesn't support those values.

This cl introduce fromUTF8Lenient() which supports the unpaired surrogates.
And use WebString::fromUTF8Lenient() in CacheStorage API to fix
https://crbug.com/432746

BUG=432746
==========

[horo, 2016-03-07 08:57:07]

horo@chromium.org changed reviewers:
+ jkarlin@chromium.org

[horo, 2016-03-07 08:57:07]

jkarlin@
Could you please review this?

[jkarlin, 2016-03-07 14:37:43]

Code looks good, thanks for doing this.

We're going to lose existing caches that contain \uFFFD characters. Do we have a
sense for how many of those there are?

When trying to open a cache we could check if converting the utf8 cache name to
16 and strict-mode back to 8 results in an already existing cache name. If so we
could migrate that cache to the lenient utf8 name though there is risk of error.

[horo, 2016-03-08 01:44:52]

On 2016/03/07 14:37:43, jkarlin wrote:
> Code looks good, thanks for doing this.
> 
> We're going to lose existing caches that contain \uFFFD characters. Do we have
a
> sense for how many of those there are?
> 
> When trying to open a cache we could check if converting the utf8 cache name
to
> 16 and strict-mode back to 8 results in an already existing cache name. If so
we
> could migrate that cache to the lenient utf8 name though there is risk of
error.

I don't think there is a web developer who intentionally use the unpaired
surrogate characters for cache_name.
And there is already a bug that Chrome can't handle those characters in
cache_name now.
All those characters are mapped to the same \uFFFD.

So I think we don't need to take care of this edge case.

[jkarlin, 2016-03-08 12:16:45]

On 2016/03/08 01:44:52, horo wrote:
> On 2016/03/07 14:37:43, jkarlin wrote:
> > Code looks good, thanks for doing this.
> > 
> > We're going to lose existing caches that contain \uFFFD characters. Do we
have
> a
> > sense for how many of those there are?
> > 
> > When trying to open a cache we could check if converting the utf8 cache name
> to
> > 16 and strict-mode back to 8 results in an already existing cache name. If
so
> we
> > could migrate that cache to the lenient utf8 name though there is risk of
> error.
> 
> I don't think there is a web developer who intentionally use the unpaired
> surrogate characters for cache_name.
> And there is already a bug that Chrome can't handle those characters in
> cache_name now.
> All those characters are mapped to the same \uFFFD.
> 
> So I think we don't need to take care of this edge case.

Fair enough. cache_storage/ and layout tests lgtm

[horo, 2016-03-08 13:11:21]

horo@chromium.org changed reviewers:
+ jochen@chromium.org

[horo, 2016-03-08 13:11:21]

jochen@
Could you please review third_party/WebKit/Source/*?

[horo, 2016-03-08 13:11:47]

On 2016/03/08 13:11:21, horo wrote:
> jochen@
> Could you please review third_party/WebKit/Source/*?

And third_party/WebKit/public/platform/WebString.h.

[johnme, 2016-03-08 14:17:50]

johnme@chromium.org changed reviewers:
+ johnme@chromium.org

[johnme, 2016-03-08 14:17:51]

Drive-by: it's not clear this fully fixes https://crbug.com/432746.
String::utf8() and fromUTF8Lenient() are backed by convertUTF8ToUTF16 and
convertUTF16ToUTF8 in third_party/WebKit/Source/wtf/text/UTF8.h, which are
documented as returning an error for illegal sequences such as <F4 90 80 80>,
<C0 80>, or <A0> in UTF-8.

So these conversions still don't seem suitable for DOMString, which IIRC needs
to be binary-safe.

[jkarlin, 2016-03-08 15:29:22]

On 2016/03/08 14:17:51, johnme wrote:
> Drive-by: it's not clear this fully fixes https://crbug.com/432746.
> String::utf8() and fromUTF8Lenient() are backed by convertUTF8ToUTF16 and
> convertUTF16ToUTF8 in third_party/WebKit/Source/wtf/text/UTF8.h, which are
> documented as returning an error for illegal sequences such as <F4 90 80 80>,
> <C0 80>, or <A0> in UTF-8.
> 
> So these conversions still don't seem suitable for DOMString, which IIRC needs
> to be binary-safe.

It returns an error but still converts so long as it's not strict mode. We
should add tests for illegal sequences.

[jkarlin, 2016-03-08 15:34:08]

On 2016/03/08 15:29:22, jkarlin wrote:
> On 2016/03/08 14:17:51, johnme wrote:
> > Drive-by: it's not clear this fully fixes https://crbug.com/432746.
> > String::utf8() and fromUTF8Lenient() are backed by convertUTF8ToUTF16 and
> > convertUTF16ToUTF8 in third_party/WebKit/Source/wtf/text/UTF8.h, which are
> > documented as returning an error for illegal sequences such as <F4 90 80
80>,
> > <C0 80>, or <A0> in UTF-8.
> > 
> > So these conversions still don't seem suitable for DOMString, which IIRC
needs
> > to be binary-safe.
> 
> It returns an error but still converts so long as it's not strict mode. We
> should add tests for illegal sequences.

Sorry, you're right. It does bail out on error in conversion.

[jkarlin, 2016-03-08 17:48:41]

On 2016/03/08 15:34:08, jkarlin wrote:
> On 2016/03/08 15:29:22, jkarlin wrote:
> > On 2016/03/08 14:17:51, johnme wrote:
> > > Drive-by: it's not clear this fully fixes https://crbug.com/432746.
> > > String::utf8() and fromUTF8Lenient() are backed by convertUTF8ToUTF16 and
> > > convertUTF16ToUTF8 in third_party/WebKit/Source/wtf/text/UTF8.h, which are
> > > documented as returning an error for illegal sequences such as <F4 90 80
> 80>,
> > > <C0 80>, or <A0> in UTF-8.
> > > 
> > > So these conversions still don't seem suitable for DOMString, which IIRC
> needs
> > > to be binary-safe.
> > 
> > It returns an error but still converts so long as it's not strict mode. We
> > should add tests for illegal sequences.
> 
> Sorry, you're right. It does bail out on error in conversion.

A DOMString is a sequence of code points, so it does not allow arbitrary binary
data, such as values above 0x10FFFF.

The unpaired surrogates should pass through safely (in lenient mode) as no UTF
is allowed to use U+D800 to U+DFFF as a code point.

The other code points should all have valid conversions.

[johnme, 2016-03-08 18:13:48]

On 2016/03/08 17:48:41, jkarlin wrote:
> On 2016/03/08 15:34:08, jkarlin wrote:
> > On 2016/03/08 15:29:22, jkarlin wrote:
> > > On 2016/03/08 14:17:51, johnme wrote:
> > > > Drive-by: it's not clear this fully fixes https://crbug.com/432746.
> > > > String::utf8() and fromUTF8Lenient() are backed by convertUTF8ToUTF16
and
> > > > convertUTF16ToUTF8 in third_party/WebKit/Source/wtf/text/UTF8.h, which
are
> > > > documented as returning an error for illegal sequences such as <F4 90 80
> > 80>,
> > > > <C0 80>, or <A0> in UTF-8.
> > > > 
> > > > So these conversions still don't seem suitable for DOMString, which IIRC
> > needs
> > > > to be binary-safe.
> > > 
> > > It returns an error but still converts so long as it's not strict mode. We
> > > should add tests for illegal sequences.
> > 
> > Sorry, you're right. It does bail out on error in conversion.
> 
> A DOMString is a sequence of code points, so it does not allow arbitrary
binary
> data, such as values above 0x10FFFF.
> 
> The unpaired surrogates should pass through safely (in lenient mode) as no UTF
> is allowed to use U+D800 to U+DFFF as a code point.
> 
> The other code points should all have valid conversions.

http://heycam.github.io/webidl/#idl-DOMString defines a DOMString as any
sequence of code units.
http://heycam.github.io/webidl/#dfn-code-unit defines a code unit as a 16 bit
unsigned integer

So it seems binary data would be in the range 0 to 0xFFFF (I'm not sure where
0x10FFFF comes from?).

> The other code points should all have valid conversions.

Are you saying that lenient conversions UTF-8 > UTF-16 > UTF-8 would fail to
round trip, but UTF-16 > UTF-8 > UTF-16 would succeed, because the illegal UTF-8
sequences can never be generated? Are there any corresponding illegal sequences
in UTF-16?

[jkarlin, 2016-03-08 19:33:12]

On 2016/03/08 18:13:48, johnme wrote:
> On 2016/03/08 17:48:41, jkarlin wrote:
> > On 2016/03/08 15:34:08, jkarlin wrote:
> > > On 2016/03/08 15:29:22, jkarlin wrote:
> > > > On 2016/03/08 14:17:51, johnme wrote:
> > > > > Drive-by: it's not clear this fully fixes https://crbug.com/432746.
> > > > > String::utf8() and fromUTF8Lenient() are backed by convertUTF8ToUTF16
> and
> > > > > convertUTF16ToUTF8 in third_party/WebKit/Source/wtf/text/UTF8.h, which
> are
> > > > > documented as returning an error for illegal sequences such as <F4 90
80
> > > 80>,
> > > > > <C0 80>, or <A0> in UTF-8.
> > > > > 
> > > > > So these conversions still don't seem suitable for DOMString, which
IIRC
> > > needs
> > > > > to be binary-safe.
> > > > 
> > > > It returns an error but still converts so long as it's not strict mode.
We
> > > > should add tests for illegal sequences.
> > > 
> > > Sorry, you're right. It does bail out on error in conversion.
> > 
> > A DOMString is a sequence of code points, so it does not allow arbitrary
> binary
> > data, such as values above 0x10FFFF.
> > 
> > The unpaired surrogates should pass through safely (in lenient mode) as no
UTF
> > is allowed to use U+D800 to U+DFFF as a code point.
> > 
> > The other code points should all have valid conversions.
> 
> http://heycam.github.io/webidl/#idl-DOMString defines a DOMString as any
> sequence of code units.

Thanks for the correction, I saw code unit and thought code point. Yes,
arbitrary binary is allowed.
 
> Are you saying that lenient conversions UTF-8 > UTF-16 > UTF-8 would fail to
> round trip,but UTF-16 > UTF-8 > UTF-16 would succeed, because the illegal
UTF-8
> sequences can never be generated? 

Yes. Though I'm not an expert. Adding dominicc@ who I believe knows this well.

[jkarlin, 2016-03-08 19:35:02]

jkarlin@chromium.org changed reviewers:
+ dominicc@chromium.org

[jkarlin, 2016-03-08 19:35:02]

dominicc: Can you chime in on this discussion? Is it safe to go from DOMString
-> UTF16 --(lenient)--> UTF8 --(lenient)--> UTF16?

[jsbell, 2016-03-08 20:12:10]

On 2016/03/08 19:35:02, jkarlin wrote:
> dominicc: Can you chime in on this discussion? Is it safe to go from DOMString
> -> UTF16 --(lenient)--> UTF8 --(lenient)--> UTF16?

[jsbell, 2016-03-08 20:15:57]

On 2016/03/08 19:35:02, jkarlin wrote:
> dominicc: Can you chime in on this discussion? Is it safe to go from DOMString
> -> UTF16 --(lenient)--> UTF8 --(lenient)--> UTF16?

(sorry about the empty reply)

drive-by:

As long as any sequence of 16-bit code units round-trips cleanly, this approach
is acceptable for passing DOMStrings around. It'd be fine with a unit test that
exercises all the code units, e.g. on their own (only 64k of them) and maybe as
one big string (only 128kB).

(I haven't looked at the code; will do so now...)

[jsbell, 2016-03-08 20:28:14]

jsbell@chromium.org changed reviewers:
+ jsbell@chromium.org

[jsbell, 2016-03-08 20:28:15]

https://codereview.chromium.org/1768063002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/text/WTFString.h (right):

https://codereview.chromium.org/1768063002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/WTFString.h:433: static String fromUTF8(const
LChar*, size_t, bool strict = true);
Please update the documentation.

https://codereview.chromium.org/1768063002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/WTFString.h:444: static String
fromUTF8Lenient(const LChar* s, size_t length)  { return fromUTF8(s, length,
false); }
Please add documentation.

https://codereview.chromium.org/1768063002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/text/WTFStringTest.cpp (right):

https://codereview.chromium.org/1768063002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/WTFStringTest.cpp:404: TEST(StringTest,
fromUTF8Lenient)
Nit: test name capitalized?

(I know this matches the method name, but that'll be changing soon and this is
really testing round-tripping, not just that method)

https://codereview.chromium.org/1768063002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/WTFStringTest.cpp:412: String utf16NewString
= String::fromUTF8Lenient(utf8String.data(), utf8String.length());
In addition to testing that fromUTF8Lenient lets these through whereas fromUTF8
returns empty string, we might as well go ahead and test that all 64k code units
make it through cleanly.

I'd also like to see test cases for:

* NUL (U+0000)
* non-ASCII 
* unpaired high surrogate (0xDC00)
* word-swapped surrogates (0xDC00 0xD800)
* a correct surrogate pair (e.g. U+1F4A9 - 0xD83D 0xDCA9)
* non-characters (U+FFFF, etc)

(The multi-unit sequences above wouldn't be exercised by just doing all 64k code
units.)

[horo, 2016-03-09 01:58:41]

https://codereview.chromium.org/1768063002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/text/WTFString.h (right):

https://codereview.chromium.org/1768063002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/WTFString.h:433: static String fromUTF8(const
LChar*, size_t, bool strict = true);
On 2016/03/08 20:28:15, jsbell wrote:
> Please update the documentation.

Done.

https://codereview.chromium.org/1768063002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/WTFString.h:444: static String
fromUTF8Lenient(const LChar* s, size_t length)  { return fromUTF8(s, length,
false); }
On 2016/03/08 20:28:15, jsbell wrote:
> Please add documentation.

Done.

https://codereview.chromium.org/1768063002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/text/WTFStringTest.cpp (right):

https://codereview.chromium.org/1768063002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/WTFStringTest.cpp:404: TEST(StringTest,
fromUTF8Lenient)
On 2016/03/08 20:28:15, jsbell wrote:
> Nit: test name capitalized?
> 
> (I know this matches the method name, but that'll be changing soon and this is
> really testing round-tripping, not just that method)

Done.

https://codereview.chromium.org/1768063002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/WTFStringTest.cpp:412: String utf16NewString
= String::fromUTF8Lenient(utf8String.data(), utf8String.length());
On 2016/03/08 20:28:15, jsbell wrote:
> In addition to testing that fromUTF8Lenient lets these through whereas
fromUTF8
> returns empty string, we might as well go ahead and test that all 64k code
units
> make it through cleanly.
> 
> I'd also like to see test cases for:
> 
> * NUL (U+0000)
> * non-ASCII 
> * unpaired high surrogate (0xDC00)
> * word-swapped surrogates (0xDC00 0xD800)
> * a correct surrogate pair (e.g. U+1F4A9 - 0xD83D 0xDCA9)
> * non-characters (U+FFFF, etc)
> 
> (The multi-unit sequences above wouldn't be exercised by just doing all 64k
code
> units.)

Done.

[jochen (gone - plz use gerrit), 2016-03-09 15:13:27]

unpaired surrogates are not valid utf8, they should be replaced by 0xFFFD

[jochen (gone - plz use gerrit), 2016-03-09 15:21:28]

i see that there's already some discussion about roundtrips going on.

However, I don't think we should strive for being able to roundtrip invalid
utf8, esp. since the utf8 spec does not allow that.

If we want to have a roundtrip, there should not only be an api
"fromUTF8Lenient" but also toUTF8Lenient.

String::utf8() should return valid utf8

[jsbell, 2016-03-09 16:47:41]

Updated tests lgtm, but we should satisfy jochen's concern. Given that
WebString's conversion is lenient by default, that may mean renaming it,
converting all callers, and then auditing them?

[jochen (gone - plz use gerrit), 2016-03-10 12:26:15]

also note that all code paths that go through UTF8Encoding().encode() already
replace unmatched surrogate pairs with kBadChar

[jsbell, 2016-03-10 17:16:47]

Hilariously relevant: 

https://simonsapin.github.io/wtf-8/

(and by extension, DOMStrings could be said to be 'WTF-16')

[dominicc (has gone to gerrit), 2016-05-31 22:43:36]

dominicc@chromium.org changed reviewers:
- dominicc@chromium.org