Escape invalid code points

Escape invalid code points

Make base::EscapeJSONString() to escape strings like "\xF3\xBF\xBF\xBF"
(containing invalid Unicode code points) to be escaped as "\xEF\xBF\xBD".

BUG=725778
Review-Url: https://codereview.chromium.org/2903773003
Cr-Commit-Position: refs/heads/master@{#478900}
Committed: https://chromium.googlesource.com/chromium/src/+/51d82b24af89da60880835f3032e2d508613f558

[sabbakumov, 2017-05-24 05:21:41]

Description was changed from

==========
Escape invalid code points

Make base::EscapeJSONString() to escape strings like "\xF3\xBF\xBF\xBF"
(containing invalid Unicode code points) to be escaped as "\xEF\xBF\xBD".

BUG=725778
==========

to

==========
Escape invalid code points

Make base::EscapeJSONString() to escape strings like "\xF3\xBF\xBF\xBF"
(containing invalid Unicode code points) to be escaped as "\xEF\xBF\xBD".

BUG=725778
==========

[sabbakumov, 2017-05-24 05:21:41]

sabbakumov@yandex-team.ru changed reviewers:
+ mark@chromium.org

[Mark Mentovai, 2017-05-24 19:02:48]

https://codereview.chromium.org/2903773003/diff/1/base/json/string_escape.cc
File base/json/string_escape.cc (right):

https://codereview.chromium.org/2903773003/diff/1/base/json/string_escape.cc#...
base/json/string_escape.cc:95: code_point < 0 || !IsValidCharacter(code_point))
{
How can code_point, a uint32_t, ever be < 0?

https://codereview.chromium.org/2903773003/diff/1/base/json/string_escape.cc#...
base/json/string_escape.cc:95: code_point < 0 || !IsValidCharacter(code_point))
{
More importantly, you’re confusing characters with codepoints. The documentation
says “Invalid code units will be replaced.” It doesn’t say that invalid
characters will be.

UTF-8 f3 bf bf bf = U+fffff may well be in invalid as a Unicode character, but
it’s valid and unambiguous as a code point.

[sabbakumov, 2017-05-25 03:26:14]

https://codereview.chromium.org/2903773003/diff/1/base/json/string_escape.cc
File base/json/string_escape.cc (right):

https://codereview.chromium.org/2903773003/diff/1/base/json/string_escape.cc#...
base/json/string_escape.cc:95: code_point < 0 || !IsValidCharacter(code_point))
{
On 2017/05/24 19:02:48, Mark Mentovai wrote:
> How can code_point, a uint32_t, ever be < 0?

Thank you. I've mistakenly copy-pasted it from here:
https://cs.chromium.org/chromium/src/base/json/json_parser.cc?sq&l=487

https://codereview.chromium.org/2903773003/diff/1/base/json/string_escape.cc#...
base/json/string_escape.cc:95: code_point < 0 || !IsValidCharacter(code_point))
{
On 2017/05/24 19:02:48, Mark Mentovai wrote:
> How can code_point, a uint32_t, ever be < 0?

Done.

https://codereview.chromium.org/2903773003/diff/1/base/json/string_escape.cc#...
base/json/string_escape.cc:95: code_point < 0 || !IsValidCharacter(code_point))
{
On 2017/05/24 19:02:48, Mark Mentovai wrote:
> More importantly, you’re confusing characters with codepoints. The
documentation
> says “Invalid code units will be replaced.” It doesn’t say that invalid
> characters will be.
> 
> UTF-8 f3 bf bf bf = U+fffff may well be in invalid as a Unicode character, but
> it’s valid and unambiguous as a code point.

I've updated the documentation.
IsValidCharacter() check is still in the CL because I'm sure we don't want to
write a JSON that we're unable to read using JSONParser.

https://codereview.chromium.org/2903773003/diff/1/base/json/string_escape.cc#...
base/json/string_escape.cc:95: code_point < 0 || !IsValidCharacter(code_point))
{
On 2017/05/24 19:02:48, Mark Mentovai wrote:
> More importantly, you’re confusing characters with codepoints. The
documentation
> says “Invalid code units will be replaced.” It doesn’t say that invalid
> characters will be.
> 
> UTF-8 f3 bf bf bf = U+fffff may well be in invalid as a Unicode character, but
> it’s valid and unambiguous as a code point.

Done.

[Mark Mentovai, 2017-06-01 16:57:46]

mark@chromium.org changed reviewers:
+ rsesek@chromium.org

[Mark Mentovai, 2017-06-01 16:57:46]

+rsesek

I don’t know if this is totally reasonable. Since invalid characters can be
unambiguously encoded, I’m not sure if we should be in the business of rejecting
them here. I think it depends on how this is being used elsewhere.

[Robert Sesek, 2017-06-01 17:23:09]

What's the rationale for this change? There's a linked bug, but it's not clear
why this is actually an issue. I take Mark's point that this is not necessarily
the right layer to be doing this at, since invalid characters can be properly
encoded and escaped. This may not round-trip through the JSONParser cleanly, but
fixing that at this layer I'm not sure makes sense.

[sabbakumov, 2017-06-02 02:14:57]

On 2017/06/01 17:23:09, Robert Sesek wrote:
> What's the rationale for this change? There's a linked bug, but it's not clear
> why this is actually an issue. I take Mark's point that this is not
necessarily
> the right layer to be doing this at, since invalid characters can be properly
> encoded and escaped. This may not round-trip through the JSONParser cleanly,
but
> fixing that at this layer I'm not sure makes sense.

There was a bug about the similar issue:
https://bugs.chromium.org/p/chromium/issues/detail?id=656115. If the site had
invalid Unicode characters then adding it to the bookmarks led to the situation
when JSONParser could not parse bookmarks and failed by default. I don't think
it's optimal behavior.
Instead, I propose to replace those invalid characters.

The string serialization happens here:
https://cs.chromium.org/chromium/src/base/json/json_writer.cc?q=json_writer+p....
This is the only place where we iterate over the whole string which gives O(n).
I don't think we should add the second iteration (O(n)) just for replacing
invalid characters. So, in my opinion, replacing can be done in
EscapeJSONString() at that layer.

[Mark Mentovai, 2017-06-02 16:42:21]

What about making this function return a tri-state value:

success
invalid Unicode character, losslessly encoded
invalid Unicode codepoint, lossily encoded to replacement character

[Robert Sesek, 2017-06-02 20:21:58]

rsesek@chromium.org changed reviewers:
+ brettw@chromium.org

[Robert Sesek, 2017-06-02 20:21:59]

Brett, do you have an opinion on this JSON string handling change? I think this
LGTM.

On 2017/06/02 16:42:21, Mark Mentovai wrote:
> What about making this function return a tri-state value:
> 
> success
> invalid Unicode character, losslessly encoded
> invalid Unicode codepoint, lossily encoded to replacement character

I'm not sure if this would be useful to any caller in practice.

[sabbakumov, 2017-06-07 01:46:05]

brettw@chromium.org: Brett, could you review this CL, please?

[brettw, 2017-06-12 18:01:59]

lgtm

[sabbakumov, 2017-06-13 02:55:51]

The CQ bit was checked by sabbakumov@yandex-team.ru

[commit-bot: I haz the power, 2017-06-13 02:56:13]

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, 2017-06-13 04:28:16]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1497322551491480,
"parent_rev": "47c391ce0d0d65703c290d7fbc6e3739ed14bc21", "commit_rev":
"51d82b24af89da60880835f3032e2d508613f558"}

[commit-bot: I haz the power, 2017-06-13 04:28:27]

Description was changed from

==========
Escape invalid code points

Make base::EscapeJSONString() to escape strings like "\xF3\xBF\xBF\xBF"
(containing invalid Unicode code points) to be escaped as "\xEF\xBF\xBD".

BUG=725778
==========

to

==========
Escape invalid code points

Make base::EscapeJSONString() to escape strings like "\xF3\xBF\xBF\xBF"
(containing invalid Unicode code points) to be escaped as "\xEF\xBF\xBD".

BUG=725778

Review-Url: https://codereview.chromium.org/2903773003
Cr-Commit-Position: refs/heads/master@{#478900}
Committed:
https://chromium.googlesource.com/chromium/src/+/51d82b24af89da60880835f3032e...
==========

[commit-bot: I haz the power, 2017-06-13 04:28:30]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/51d82b24af89da60880835f3032e...