Adds option for JSON reader to allow invalid utf characters

Adds option for JSON reader to allow invalid utf characters

This way reading in bookmarks files won't throw out everything if a
bogus utf8 character was written.

BUG=656115
TEST=covered by test
R=brettw@chromium.org, stevenjb@chromium.org
TBR=stevenjb@chromium.org

Committed: https://crrev.com/cc7f72dcbc3953587ffe9706601f8d09e8d81481
Cr-Commit-Position: refs/heads/master@{#431433}

[sky, 2016-11-03 00:01:37]

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

[commit-bot: I haz the power, 2016-11-03 00:03:00]

Dry run: 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-11-03 00:20:00]

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

[commit-bot: I haz the power, 2016-11-03 00:20:01]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[sky, 2016-11-03 03:06:55]

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

[commit-bot: I haz the power, 2016-11-03 03:07:14]

Dry run: 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-11-03 05:07:44]

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

[commit-bot: I haz the power, 2016-11-03 05:07:45]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[sky, 2016-11-03 18:03:19]

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

[commit-bot: I haz the power, 2016-11-03 18:04:02]

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

[sky, 2016-11-03 19:14:14]

https://codereview.chromium.org/2475583002/diff/60001/base/json/json_parser.cc
File base/json/json_parser.cc (right):

https://codereview.chromium.org/2475583002/diff/60001/base/json/json_parser.c...
base/json/json_parser.cc:629: const int start_index = index_;
It may be possible to backtrack for the first valid character, but I'm keeping
it simple.

https://codereview.chromium.org/2475583002/diff/60001/base/values.cc
File base/values.cc (right):

https://codereview.chromium.org/2475583002/diff/60001/base/values.cc#newcode256
base/values.cc:256: // NOTE: no DCHECK that |in_value| is valid utf-8. We
explicitly allow invalid
My rationale here for removing the DCHECK is that UTF16ToUTF8 below basically
continues on with invalid input.

[brettw, 2016-11-04 23:17:13]

https://codereview.chromium.org/2475583002/diff/60001/base/json/json_parser.cc
File base/json/json_parser.cc (right):

https://codereview.chromium.org/2475583002/diff/60001/base/json/json_parser.c...
base/json/json_parser.cc:639: memcpy(&invalid_string.front(), pos_, index_ -
start_index);
Personally, I would prefer substituting it with the "Unicode replacement
character" than keeping it as-is. Various Chrome code assumes strings are
encoded properly, and I think this will be safer.

The replacement character = U+FFFD.

[sky, 2016-11-08 01:01:46]

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

[sky, 2016-11-08 01:02:20]

https://codereview.chromium.org/2475583002/diff/60001/base/json/json_parser.cc
File base/json/json_parser.cc (right):

https://codereview.chromium.org/2475583002/diff/60001/base/json/json_parser.c...
base/json/json_parser.cc:639: memcpy(&invalid_string.front(), pos_, index_ -
start_index);
On 2016/11/04 23:17:13, brettw (ping on IM after 24h) wrote:
> Personally, I would prefer substituting it with the "Unicode replacement
> character" than keeping it as-is. Various Chrome code assumes strings are
> encoded properly, and I think this will be safer.
> 
> The replacement character = U+FFFD.

Done.

[commit-bot: I haz the power, 2016-11-08 01:02:49]

Dry run: 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-11-08 02:32:48]

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

[commit-bot: I haz the power, 2016-11-08 02:32:49]

Dry run: This issue passed the CQ dry run.

[brettw, 2016-11-08 19:21:19]

https://codereview.chromium.org/2475583002/diff/80001/base/json/json_parser.cc
File base/json/json_parser.cc (right):

https://codereview.chromium.org/2475583002/diff/80001/base/json/json_parser.c...
base/json/json_parser.cc:191: const char kUnicodeReplacementString[] =
"\xFF\xDF";
I'm not sure where you got this string. U+FFFD in UTF-8 is "\EF\BF\BD". Is that
what you meant?

[brettw, 2016-11-08 20:26:56]

https://codereview.chromium.org/2475583002/diff/80001/base/json/json_parser.cc
File base/json/json_parser.cc (right):

https://codereview.chromium.org/2475583002/diff/80001/base/json/json_parser.c...
base/json/json_parser.cc:191: const char kUnicodeReplacementString[] =
"\xFF\xDF";
On 2016/11/08 19:21:19, brettw (ping on IM after 24h) wrote:
> I'm not sure where you got this string. U+FFFD in UTF-8 is "\EF\BF\BD". Is
that
> what you meant?

(add x's in there after the backslashes)

[sky, 2016-11-08 23:55:36]

https://codereview.chromium.org/2475583002/diff/80001/base/json/json_parser.cc
File base/json/json_parser.cc (right):

https://codereview.chromium.org/2475583002/diff/80001/base/json/json_parser.c...
base/json/json_parser.cc:191: const char kUnicodeReplacementString[] =
"\xFF\xDF";
On 2016/11/08 20:26:56, brettw (ping on IM after 24h) wrote:
> On 2016/11/08 19:21:19, brettw (ping on IM after 24h) wrote:
> > I'm not sure where you got this string. U+FFFD in UTF-8 is "\EF\BF\BD". Is
> that
> > what you meant?
> 
> (add x's in there after the backslashes)

Done.

[sky, 2016-11-08 23:55:36]

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

[commit-bot: I haz the power, 2016-11-08 23:56:29]

Dry run: 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-11-09 01:15:35]

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

[commit-bot: I haz the power, 2016-11-09 01:15:36]

Dry run: This issue passed the CQ dry run.

[sky, 2016-11-10 19:22:24]

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

[sky, 2016-11-10 19:22:53]

As discussed I removed the DCHECK.

[commit-bot: I haz the power, 2016-11-10 19:22:56]

Dry run: 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-11-10 20:28:14]

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

[commit-bot: I haz the power, 2016-11-10 20:28:15]

Dry run: This issue passed the CQ dry run.

[brettw, 2016-11-10 20:35:40]

lgtm

https://codereview.chromium.org/2475583002/diff/100001/base/json/json_reader.h
File base/json/json_reader.h (right):

https://codereview.chromium.org/2475583002/diff/100001/base/json/json_reader....
base/json/json_reader.h:60: // kUnicodeReplacementString. If not set, invalid
characters trigger a hard
This isn't a global constant so it's weird to refer to it here in the public
API. Maybe just say the "Unicode replacement character (U+FFFD)"

[sky, 2016-11-10 23:01:48]

https://codereview.chromium.org/2475583002/diff/100001/base/json/json_reader.h
File base/json/json_reader.h (right):

https://codereview.chromium.org/2475583002/diff/100001/base/json/json_reader....
base/json/json_reader.h:60: // kUnicodeReplacementString. If not set, invalid
characters trigger a hard
On 2016/11/10 20:35:40, brettw (ping on IM after 24h) wrote:
> This isn't a global constant so it's weird to refer to it here in the public
> API. Maybe just say the "Unicode replacement character (U+FFFD)"

Done.

[sky, 2016-11-10 23:02:26]

The CQ bit was checked by sky@chromium.org

[sky, 2016-11-10 23:02:26]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/2475583002/#ps140001
(title: "comment")

[commit-bot: I haz the power, 2016-11-10 23:03:09]

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-11-10 23:14:49]

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

[commit-bot: I haz the power, 2016-11-10 23:14:50]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[sky, 2016-11-10 23:29:45]

Description was changed from

==========
Adds option for JSON reader to allow invalid utf characters

This way reading in bookmarks files won't throw out everything if a
bogus utf8 character was written.

BUG=656115
TEST=covered by test
R=brettw@chromium.org
==========

to

==========
Adds option for JSON reader to allow invalid utf characters

This way reading in bookmarks files won't throw out everything if a
bogus utf8 character was written.

BUG=656115
TEST=covered by test
R=brettw@chromium.org, stevenjb@chromium.org
TBR=stevenjb@chromium.org
==========

[sky, 2016-11-10 23:29:58]

TBR stevenjb for changes in chromeos

[sky, 2016-11-10 23:30:02]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2016-11-10 23:30:46]

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-11-11 01:13:29]

Description was changed from

==========
Adds option for JSON reader to allow invalid utf characters

This way reading in bookmarks files won't throw out everything if a
bogus utf8 character was written.

BUG=656115
TEST=covered by test
R=brettw@chromium.org, stevenjb@chromium.org
TBR=stevenjb@chromium.org
==========

to

==========
Adds option for JSON reader to allow invalid utf characters

This way reading in bookmarks files won't throw out everything if a
bogus utf8 character was written.

BUG=656115
TEST=covered by test
R=brettw@chromium.org, stevenjb@chromium.org
TBR=stevenjb@chromium.org
==========

[commit-bot: I haz the power, 2016-11-11 01:13:31]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-11-11 01:18:25]

Description was changed from

==========
Adds option for JSON reader to allow invalid utf characters

This way reading in bookmarks files won't throw out everything if a
bogus utf8 character was written.

BUG=656115
TEST=covered by test
R=brettw@chromium.org, stevenjb@chromium.org
TBR=stevenjb@chromium.org
==========

to

==========
Adds option for JSON reader to allow invalid utf characters

This way reading in bookmarks files won't throw out everything if a
bogus utf8 character was written.

BUG=656115
TEST=covered by test
R=brettw@chromium.org, stevenjb@chromium.org
TBR=stevenjb@chromium.org

Committed: https://crrev.com/cc7f72dcbc3953587ffe9706601f8d09e8d81481
Cr-Commit-Position: refs/heads/master@{#431433}
==========

[commit-bot: I haz the power, 2016-11-11 01:18:26]

Patchset 8 (id:??) landed as
https://crrev.com/cc7f72dcbc3953587ffe9706601f8d09e8d81481
Cr-Commit-Position: refs/heads/master@{#431433}

[sabbakumov, 2017-05-23 13:47:56]

sabbakumov@yandex-team.ru changed reviewers:
+ sabbakumov@yandex-team.ru

[sabbakumov, 2017-05-23 13:47:56]

It's stated
https://cs.chromium.org/chromium/src/base/json/string_escape.h?sq&l=17 that the
JSONWriter should already escape bad unicode symbols. Why does that fix work?

[sky, 2017-05-23 16:44:27]

On 2017/05/23 13:47:56, sabbakumov wrote:
> It's stated
> https://cs.chromium.org/chromium/src/base/json/string_escape.h?sq&l=17 that
the
> JSONWriter should already escape bad unicode symbols. Why does that fix work?

Maybe that was changing after this patch?

[sabbakumov, 2017-05-24 05:32:07]

On 2017/05/23 16:44:27, sky wrote:
> On 2017/05/23 13:47:56, sabbakumov wrote:
> > It's stated
> > https://cs.chromium.org/chromium/src/base/json/string_escape.h?sq&l=17 that
> the
> > JSONWriter should already escape bad unicode symbols. Why does that fix
work?
> 
> Maybe that was changing after this patch?

I've made a patch https://codereview.chromium.org/2903773003/. It replaces
invalid
Unicode code points when you use JSONWriter, so probably this patch is no longer
needed except cases where JSON to be loaded isn't made by us.

[sky, 2017-05-24 16:51:03]

At this point folks could have the bogus utf in their bookmarks file. So,
we need to ensure we don't drop all the bookmarks in that case.

On Tue, May 23, 2017 at 10:32 PM, <sabbakumov@yandex-team.ru> wrote:

> On 2017/05/23 16:44:27, sky wrote:
> > On 2017/05/23 13:47:56, sabbakumov wrote:
> > > It's stated
> > > https://cs.chromium.org/chromium/src/base/json/string_escape.h?sq&l=17
> that
> > the
> > > JSONWriter should already escape bad unicode symbols. Why does that fix
> work?
> >
> > Maybe that was changing after this patch?
>
> I've made a patch https://codereview.chromium.org/2903773003/. It replaces
> invalid
> Unicode code points when you use JSONWriter, so probably this patch is no
> longer
> needed except cases where JSON to be loaded isn't made by us.
>
> https://codereview.chromium.org/2475583002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.