Stop doing unnecessary UTF-8 to UTF-16 conversions in JSONWriter.

Stop doing unnecessary UTF-8 to UTF-16 conversions in JSONWriter.

The JSONReader only accepts UTF-8 input strings and converts \uXXXX sequences
back into UTF-8. However, the JSONWriter converts all non-ASCII characters to
UTF-16 escape sequences. This round-tripping is sub-optimal, as noted in a
TODO from r54359.

One reason for this may be that JsonDoubleQuote(), used by JSONWriter, does not
handle UTF-8 bytes correctly, interpreting them as code points and writing them
out as \u00XX sequences. If this were read back through a RFC-compliant JSON
parser, the result would be an invalid encoding error. JsonDoubleQuote() does
handle UTF-16 correctly, though.

This rewrites the base/json/string_escape.h API and fixes the above UTF-8 issue
by dividing callers up into three groups:

1. Those that pass valid UTF-8 to be escaped. Prior to this change, very few
   callers used this variant. Those that did were likely using ASCII, otherwise
   the output would be mangled due to the above issue. Now, valid UTF-8 will be
   passed through to the output unescaped. Invalid UTF-8 sequences are replaced
   with U+FFFD.

2. Those that pass valid UTF-16 to be escaped. This function now validates that
   the input is valid UTF-16, and then converts it to unescaped UTF-8 sequences
   for the output.

3. Those that pass arbitrary byte arrays as std::string and expect a non-RFC-
   compliant encoding of the binary data using \uXXXX escapes. This behavior is
   now in the EscapeBytesAsInvalidJSONString() function. It is only used by
   callers who want a "debug string" but do not expect to actually parse the
   output as valid JSON, since it is not.

Additionally, this removes the JSONWriter::OPTIONS_DO_NOT_ESCAPE flag, since
the writer can now handle UTF-8 appropriately.

BUG=15466
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=239800
Reverted: https://src.chromium.org/viewvc/chrome?view=rev&revision=240082

R=asanka@chromium.org, bauerb@chromium.org, mark@chromium.org, thakis@chromium.org, zea@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=240190

[Mark Mentovai, 2013-12-06 15:35:11]

https://codereview.chromium.org/100823007/diff/20001/base/debug/trace_event_i...
File base/debug/trace_event_impl.cc (right):

https://codereview.chromium.org/100823007/diff/20001/base/debug/trace_event_i...
base/debug/trace_event_impl.cc:1: // Copyright (c) 2012 The Chromium Authors.
All rights reserved.
In your CL description, for the three cases, you should say explicitly what the
“before” and “after” cases are. You only did so for case 2. For case 3, you
don’t say what the main JSONWriter entry point does when it encounters bogus
arbitrary non-UTF-8 binary data.

https://codereview.chromium.org/100823007/diff/20001/base/json/json_writer.h
File base/json/json_writer.h (right):

https://codereview.chromium.org/100823007/diff/20001/base/json/json_writer.h#...
base/json/json_writer.h:20: // Deprecated. Has no effect.
Can you remove it totally, and fix the CL description to say “removes” instead
of “obsoletes”? codesearch shows only three callers using it. That’s less stuff
to fix up than  the name changes and function split from the work in
string_escape.h.

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

https://codereview.chromium.org/100823007/diff/20001/base/json/string_escape....
base/json/string_escape.cc:28: bool EscapeCodePoint(uint32 code_point,
std::string* dest) {
This name makes it sound like it can escape any code point, but it really only
operates on a few special characters. EscapeSpecialCodePoint?

https://codereview.chromium.org/100823007/diff/20001/base/json/string_escape....
base/json/string_escape.cc:54: // Escape <, > to prevent script execution.
You really only need < for this, you can save a couple of bytes in the output by
leaving > alone.

https://codereview.chromium.org/100823007/diff/20001/base/json/string_escape....
base/json/string_escape.cc:70: CHECK_LE(str.length(),
static_cast<size_t>(kint32max));
std::numeric_limits<int32>::max() instead of kint32max?

https://codereview.chromium.org/100823007/diff/20001/base/json/string_escape.h
File base/json/string_escape.h (right):

https://codereview.chromium.org/100823007/diff/20001/base/json/string_escape....
base/json/string_escape.h:19: // replaced with the U+FFFD replacement character.
On return, |dest| will
Silently replaced with U+FFFD? Sounds like a recipe for quiet data loss. Maybe
it should DCHECK, in which case the documentation here should say that non-UTF-8
is invalid as opposed to handled in some way. Same for the StringPiece16
variant.

And why’d you choose U+FFFD? I’d use an explicit noncharacter, like U+FFFE, so
that bad non-UTF-8 input results in poisoned output that a proper UTF-8 reader
will detect as invalid.

https://codereview.chromium.org/100823007/diff/20001/base/json/string_escape....
base/json/string_escape.h:33: // UTF-16 units will be escaped into \uXXXX
sequences. Invalid code units
I don’t know why you’d maintain this distinction. Who cares whether the input
was a std::string or a string16? Why don’t you output them both as raw UTF-8?
It’s like the same code as the function above. In fact, if you had them both
behave the same way, you could probably just write the implementation once,
templatized.

https://codereview.chromium.org/100823007/diff/20001/base/json/string_escape....
base/json/string_escape.h:55: BASE_EXPORT std::string
EscapeBytesAsInvalidJSONString(const StringPiece& str,
Good name.

https://codereview.chromium.org/100823007/diff/20001/chrome/browser/extension...
File
chrome/browser/extensions/api/messaging/native_messaging_host_manifest_unittest.cc
(right):

https://codereview.chromium.org/100823007/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_messaging_host_manifest_unittest.cc:38:
base::EscapeJSONString(path, false, &escaped_path);
Why don’t you use the simpler non-out-param-having returns-its-return-value
version of this function, and let it put the quotes on the string for you
(removing the quotes from the string constant)?

I will ask this question several more times.

https://codereview.chromium.org/100823007/diff/20001/chrome/browser/search/if...
File chrome/browser/search/iframe_source.cc (right):

https://codereview.chromium.org/100823007/diff/20001/chrome/browser/search/if...
chrome/browser/search/iframe_source.cc:91: base::EscapeJSONString(origin, false,
&js_escaped_origin);
Here is one place I will ask that question.

In fact, if you take my suggestion, you might get a compiler warning about
js_escaped_origin being unused. Good point, compiler! It IS unused. We don’t
need this EscapeJSONString at all.

https://codereview.chromium.org/100823007/diff/20001/chrome/browser/spellchec...
File chrome/browser/spellchecker/spelling_service_client.cc (right):

https://codereview.chromium.org/100823007/diff/20001/chrome/browser/spellchec...
chrome/browser/spellchecker/spelling_service_client.cc:73:
base::EscapeJSONString(text, false, &encoded_text);
Here.

https://codereview.chromium.org/100823007/diff/20001/chrome/renderer/searchbo...
File chrome/renderer/searchbox/searchbox_extension.cc (right):

https://codereview.chromium.org/100823007/diff/20001/chrome/renderer/searchbo...
chrome/renderer/searchbox/searchbox_extension.cc:464:
base::EscapeJSONString(identity, true, &escaped_identity);
Here. In fact, this time, it wanted the quotes to begin with.

https://codereview.chromium.org/100823007/diff/20001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome/adb_impl.cc (right):

https://codereview.chromium.org/100823007/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/adb_impl.cc:125: base::StringPrintf("echo %s >
%s; echo $?",
Oh my.

https://codereview.chromium.org/100823007/diff/20001/jingle/notifier/listener...
File jingle/notifier/listener/notification_defines.cc (right):

https://codereview.chromium.org/100823007/diff/20001/jingle/notifier/listener...
jingle/notifier/listener/notification_defines.cc:71:
base::EscapeBytesAsInvalidJSONString(channel, true /* put_in_quotes */);
Was this one consumed by some server and used, or is it just for pretty data to
look at?

https://codereview.chromium.org/100823007/diff/20001/sync/syncable/entry.cc
File sync/syncable/entry.cc (right):

https://codereview.chromium.org/100823007/diff/20001/sync/syncable/entry.cc#n...
sync/syncable/entry.cc:150: std::string escaped_str =
base::EscapeBytesAsInvalidJSONString(
How careful were you in determining which of these were OK to use
EscapeJSONString and which had to use the garbage EscapeBytesAsInvalidJSONString
function?

[Avi (use Gerrit), 2013-12-06 15:45:24]

https://codereview.chromium.org/100823007/diff/20001/base/json/string_escape.h
File base/json/string_escape.h (right):

https://codereview.chromium.org/100823007/diff/20001/base/json/string_escape....
base/json/string_escape.h:19: // replaced with the U+FFFD replacement character.
On return, |dest| will
On 2013/12/06 15:35:12, Mark Mentovai wrote:
> And why’d you choose U+FFFD? I’d use an explicit noncharacter, like U+FFFE, so
> that bad non-UTF-8 input results in poisoned output that a proper UTF-8 reader
> will detect as invalid.

U+FFFD is REPLACEMENT CHARACTER, literally designed to be swapped in for invalid
or non-decodable input. Seems like you're talking about questions of behavior.
What is the scenario where poisoning is desired?

[Mark Mentovai, 2013-12-06 15:58:23]

U+FFFD exists to provide a glyph to use to represent an invalid character. It is
itself still a valid codepoint. U+FFFD should be swapped in for invalid data at
the point that the data is rendered for display.

The conversion in question isn’t concerned with rendering and display. It
shouldn’t receive invalid UTF-8 (or UTF-16) input and quietly transform it into
valid (and different) UTF-8 output. If the conversion isn’t going to fail
outright, it should at least produce invalid UTF-8 output. U+FFFE would qualify.

[jungshik at Google, 2013-12-07 13:16:58]

https://codereview.chromium.org/100823007/diff/20001/base/json/json_value_ser...
File base/json/json_value_serializer_unittest.cc (right):

https://codereview.chromium.org/100823007/diff/20001/base/json/json_value_ser...
base/json/json_value_serializer_unittest.cc:274: string16
test(WideToUTF16(L"\x7F51\x9875"));
While you're at it, you may want to expand this test a bit more (or add a new
test) to cover non-BMP cases ( character above U+FFFF) and lone surrogates ?

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

https://codereview.chromium.org/100823007/diff/20001/base/json/string_escape....
base/json/string_escape.cc:28: bool EscapeCodePoint(uint32 code_point,
std::string* dest) {
On 2013/12/06 15:35:12, Mark Mentovai wrote:
> This name makes it sound like it can escape any code point, but it really only
> operates on a few special characters. EscapeSpecialCodePoint?

Agree. I had to come back up here to remind me what this function does while
reading EscapeJSONString() when this function is used.

https://codereview.chromium.org/100823007/diff/20001/base/json/string_escape....
base/json/string_escape.cc:112: code_point = kReplacementCodePoint;
ReadUnicodeCharacter passes througn non-character code points. I think it's all
right (see http://www.unicode.org/faq/private_use.html#noncharacters ).

https://codereview.chromium.org/100823007/diff/20001/base/json/string_escape.h
File base/json/string_escape.h (right):

https://codereview.chromium.org/100823007/diff/20001/base/json/string_escape....
base/json/string_escape.h:19: // replaced with the U+FFFD replacement character.
On return, |dest| will
On 2013/12/06 15:45:25, Avi wrote:
> On 2013/12/06 15:35:12, Mark Mentovai wrote:
> > And why’d you choose U+FFFD? I’d use an explicit noncharacter, like U+FFFE,
so
> > that bad non-UTF-8 input results in poisoned output that a proper UTF-8
reader
> > will detect as invalid.
> 
> U+FFFD is REPLACEMENT CHARACTER, literally designed to be swapped in for
invalid
> or non-decodable input. Seems like you're talking about questions of behavior.
> What is the scenario where poisoning is desired?

I agree with Avi. Putting in U+FFFD for an invalid input sequence is a
wide-spread convention (a  Unicode-standard sanctioned [1]) way to represent
malformed sequences. I haven't seen 'poisoning with a non-character'. What
scenario do you have in mind? 

[1] http://www.unicode.org/versions/Unicode6.2.0/ch05.pdf#G7730 : section 5.22 
http://www.unicode.org/versions/Unicode6.2.0/ch03.pdf : section 3.9 (or search
for 'replacement character')

https://codereview.chromium.org/100823007/diff/20001/base/json/string_escape....
base/json/string_escape.h:33: // UTF-16 units will be escaped into \uXXXX
sequences. Invalid code units
On 2013/12/06 15:35:12, Mark Mentovai wrote:
> I don’t know why you’d maintain this distinction. Who cares whether the input
> was a std::string or a string16? Why don’t you output them both as raw UTF-8?
> It’s like the same code as the function above. In fact, if you had them both
> behave the same way, you could probably just write the implementation once,
> templatized.

I'm  with Mark and  curious as to why you'd make this differ from the UTF-8
version.

https://codereview.chromium.org/100823007/diff/20001/base/json/string_escape....
base/json/string_escape.h:51: // will be rejcted via a parser error if passed to
to the base::JSONReader.
If you're converting 0x80-0xFF to \u0080 - \u00FF, how could that be rejected as
invalid by JSONReader?  (RFC 4627 section 2.5 2nd para).  Am I missing
something?

[Mark Mentovai, 2013-12-07 22:22:48]

https://codereview.chromium.org/100823007/diff/20001/base/json/string_escape.h
File base/json/string_escape.h (right):

https://codereview.chromium.org/100823007/diff/20001/base/json/string_escape....
base/json/string_escape.h:19: // replaced with the U+FFFD replacement character.
On return, |dest| will
Jungshik Shin wrote:
> I agree with Avi. Putting in U+FFFD for an invalid input sequence is a
> wide-spread convention (a  Unicode-standard sanctioned [1]) way to represent
> malformed sequences. I haven't seen 'poisoning with a non-character'. What
> scenario do you have in mind? 

I relent. I thought U+FFFD was for display purposes only, not for generic
conversion. I was wrong.

However, this function may want to “return false” if it had to perform any
conversion that resulted in U+FFFD, and the JSON reader could offer a “strict
mode” via its options argument that causes such conversions to be fatal either
by assertion failure or by signaling the error to the caller.

Seriously, the transparent dropping-in of any character for invalid data seems
like it’ll result in harder-to-trace data loss problems. Dropping in a character
that decodes validly like U+FFFD makes it even more likely that the problem will
go missed.

https://codereview.chromium.org/100823007/diff/20001/base/json/string_escape....
base/json/string_escape.h:51: // will be rejcted via a parser error if passed to
to the base::JSONReader.
Jungshik Shin wrote:
> If you're converting 0x80-0xFF to \u0080 - \u00FF, how could that be rejected
as
> invalid by JSONReader?  (RFC 4627 section 2.5 2nd para).  Am I missing
> something? 

This comment means that the input to this function will not be interpreted as
UTF-8. Thus, this function will produce valid JSON output that, when decoded by
a conformant decoder into UTF-8, will produce a UTF-8 string that does not match
the input.

Input { 0x80, 0x00 } to this function, get "\"\\u0080\"" as output. Feed that to
a JSON decoder that produces proper UTF-8, and get { 0xc2, 0x80, 0x00 } as
output. Thus the output of this function is not valid in terms of equivalence
with the original meaning of its input.

https://codereview.chromium.org/100823007/diff/20001/base/json/string_escape....
base/json/string_escape.h:51: // will be rejcted via a parser error if passed to
to the base::JSONReader.
rejcted → rejected

[Robert Sesek, 2013-12-09 19:52:08]

https://codereview.chromium.org/100823007/diff/20001/base/debug/trace_event_i...
File base/debug/trace_event_impl.cc (right):

https://codereview.chromium.org/100823007/diff/20001/base/debug/trace_event_i...
base/debug/trace_event_impl.cc:1: // Copyright (c) 2012 The Chromium Authors.
All rights reserved.
On 2013/12/06 15:35:12, Mark Mentovai wrote:
> In your CL description, for the three cases, you should say explicitly what
the
> “before” and “after” cases are. You only did so for case 2. For case
3, you
> don’t say what the main JSONWriter entry point does when it encounters bogus
> arbitrary non-UTF-8 binary data.

Done.

https://codereview.chromium.org/100823007/diff/20001/base/json/json_value_ser...
File base/json/json_value_serializer_unittest.cc (right):

https://codereview.chromium.org/100823007/diff/20001/base/json/json_value_ser...
base/json/json_value_serializer_unittest.cc:274: string16
test(WideToUTF16(L"\x7F51\x9875"));
On 2013/12/07 13:16:59, Jungshik Shin wrote:
> While you're at it, you may want to expand this test a bit more (or add a new
> test) to cover non-BMP cases ( character above U+FFFF) and lone surrogates ?  

Done in the string_escape.cc test.

https://codereview.chromium.org/100823007/diff/20001/base/json/json_writer.h
File base/json/json_writer.h (right):

https://codereview.chromium.org/100823007/diff/20001/base/json/json_writer.h#...
base/json/json_writer.h:20: // Deprecated. Has no effect.
On 2013/12/06 15:35:12, Mark Mentovai wrote:
> Can you remove it totally, and fix the CL description to say “removes” instead
> of “obsoletes”? codesearch shows only three callers using it. That’s less
stuff
> to fix up than  the name changes and function split from the work in
> string_escape.h.

Done.

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

https://codereview.chromium.org/100823007/diff/20001/base/json/string_escape....
base/json/string_escape.cc:28: bool EscapeCodePoint(uint32 code_point,
std::string* dest) {
On 2013/12/06 15:35:12, Mark Mentovai wrote:
> This name makes it sound like it can escape any code point, but it really only
> operates on a few special characters. EscapeSpecialCodePoint?

Done.

https://codereview.chromium.org/100823007/diff/20001/base/json/string_escape....
base/json/string_escape.cc:54: // Escape <, > to prevent script execution.
On 2013/12/06 15:35:12, Mark Mentovai wrote:
> You really only need < for this, you can save a couple of bytes in the output
by
> leaving > alone.

Done.

https://codereview.chromium.org/100823007/diff/20001/base/json/string_escape....
base/json/string_escape.cc:70: CHECK_LE(str.length(),
static_cast<size_t>(kint32max));
On 2013/12/06 15:35:12, Mark Mentovai wrote:
> std::numeric_limits<int32>::max() instead of kint32max?

There seems to be a preference for the constant when using the Chromium
basictypes.h integer types.

https://codereview.chromium.org/100823007/diff/20001/base/json/string_escape....
base/json/string_escape.cc:112: code_point = kReplacementCodePoint;
On 2013/12/07 13:16:59, Jungshik Shin wrote:
> ReadUnicodeCharacter passes througn non-character code points. I think it's
all
> right (see http://www.unicode.org/faq/private_use.html#noncharacters ). 

base::StringValue performs a DCHECK(IsStrintUTF8()) when being created, which
ensures that all characters are IsValidCharacter(). Also, considering:

"""
Q: Are noncharacters intended for interchange?
A: No. They are intended explicity for internal use.
"""

and that JSON is an interchange format, I don't think it makes sense to do this.

https://codereview.chromium.org/100823007/diff/20001/base/json/string_escape.h
File base/json/string_escape.h (right):

https://codereview.chromium.org/100823007/diff/20001/base/json/string_escape....
base/json/string_escape.h:19: // replaced with the U+FFFD replacement character.
On return, |dest| will
On 2013/12/07 22:22:49, Mark Mentovai wrote:
> Jungshik Shin wrote:
> > I agree with Avi. Putting in U+FFFD for an invalid input sequence is a
> > wide-spread convention (a  Unicode-standard sanctioned [1]) way to represent
> > malformed sequences. I haven't seen 'poisoning with a non-character'. What
> > scenario do you have in mind? 
> 
> I relent. I thought U+FFFD was for display purposes only, not for generic
> conversion. I was wrong.
> 
> However, this function may want to “return false”&nbsp;if it had to perform
any
> conversion that resulted in U+FFFD

Done.

> and the JSON reader could offer a “strict
> mode”&nbsp;via its options argument that causes such conversions to be fatal
either
> by assertion failure or by signaling the error to the caller.

I don't know what use this would be.

> Seriously, the transparent dropping-in of any character for invalid data seems
> like it’ll result in harder-to-trace data loss problems. Dropping in a
character
> that decodes validly like U+FFFD makes it even more likely that the problem
will
> go missed.

The majority of calls to this function come through serializing
base::StringValue and that already DCHECKs that the string is valid UTF-8. Now
that this function returns bool, I think this is now the caller's problem to
worry about.

https://codereview.chromium.org/100823007/diff/20001/base/json/string_escape....
base/json/string_escape.h:33: // UTF-16 units will be escaped into \uXXXX
sequences. Invalid code units
On 2013/12/07 13:16:59, Jungshik Shin wrote:
> On 2013/12/06 15:35:12, Mark Mentovai wrote:
> > I don’t know why you’d maintain this distinction. Who cares whether the
input
> > was a std::string or a string16? Why don’t you output them both as raw
UTF-8?
> > It’s like the same code as the function above. In fact, if you had them both
> > behave the same way, you could probably just write the implementation once,
> > templatized.
> 
> I'm  with Mark and  curious as to why you'd make this differ from the UTF-8
> version. 

Changed to a templatized impl that results in the same output for UTF-8 and
UTF-16 input.

https://codereview.chromium.org/100823007/diff/20001/base/json/string_escape....
base/json/string_escape.h:51: // will be rejcted via a parser error if passed to
to the base::JSONReader.
On 2013/12/07 22:22:49, Mark Mentovai wrote:
> rejcted → rejected

Done.

https://codereview.chromium.org/100823007/diff/20001/chrome/browser/extension...
File
chrome/browser/extensions/api/messaging/native_messaging_host_manifest_unittest.cc
(right):

https://codereview.chromium.org/100823007/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_messaging_host_manifest_unittest.cc:38:
base::EscapeJSONString(path, false, &escaped_path);
On 2013/12/06 15:35:12, Mark Mentovai wrote:
> Why don’t you use the simpler non-out-param-having returns-its-return-value
> version of this function, and let it put the quotes on the string for you
> (removing the quotes from the string constant)?
> 
> I will ask this question several more times.

Done.

https://codereview.chromium.org/100823007/diff/20001/chrome/browser/search/if...
File chrome/browser/search/iframe_source.cc (right):

https://codereview.chromium.org/100823007/diff/20001/chrome/browser/search/if...
chrome/browser/search/iframe_source.cc:91: base::EscapeJSONString(origin, false,
&js_escaped_origin);
On 2013/12/06 15:35:12, Mark Mentovai wrote:
> Here is one place I will ask that question.
> 
> In fact, if you take my suggestion, you might get a compiler warning about
> js_escaped_origin being unused. Good point, compiler! It IS unused. We don’t
> need this EscapeJSONString at all.

Done.

https://codereview.chromium.org/100823007/diff/20001/chrome/browser/spellchec...
File chrome/browser/spellchecker/spelling_service_client.cc (right):

https://codereview.chromium.org/100823007/diff/20001/chrome/browser/spellchec...
chrome/browser/spellchecker/spelling_service_client.cc:73:
base::EscapeJSONString(text, false, &encoded_text);
On 2013/12/06 15:35:12, Mark Mentovai wrote:
> Here.

Done.

https://codereview.chromium.org/100823007/diff/20001/chrome/renderer/searchbo...
File chrome/renderer/searchbox/searchbox_extension.cc (right):

https://codereview.chromium.org/100823007/diff/20001/chrome/renderer/searchbo...
chrome/renderer/searchbox/searchbox_extension.cc:464:
base::EscapeJSONString(identity, true, &escaped_identity);
On 2013/12/06 15:35:12, Mark Mentovai wrote:
> Here. In fact, this time, it wanted the quotes to begin with.

Done.

https://codereview.chromium.org/100823007/diff/20001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome/adb_impl.cc (right):

https://codereview.chromium.org/100823007/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/adb_impl.cc:125: base::StringPrintf("echo %s >
%s; echo $?",
On 2013/12/06 15:35:12, Mark Mentovai wrote:
> Oh my.

Yes, I didn't even want to go there.

https://codereview.chromium.org/100823007/diff/20001/jingle/notifier/listener...
File jingle/notifier/listener/notification_defines.cc (right):

https://codereview.chromium.org/100823007/diff/20001/jingle/notifier/listener...
jingle/notifier/listener/notification_defines.cc:71:
base::EscapeBytesAsInvalidJSONString(channel, true /* put_in_quotes */);
On 2013/12/06 15:35:12, Mark Mentovai wrote:
> Was this one consumed by some server and used, or is it just for pretty data
to
> look at?

This is only ever called for LOG()ing purposes.

https://codereview.chromium.org/100823007/diff/20001/sync/syncable/entry.cc
File sync/syncable/entry.cc (right):

https://codereview.chromium.org/100823007/diff/20001/sync/syncable/entry.cc#n...
sync/syncable/entry.cc:150: std::string escaped_str =
base::EscapeBytesAsInvalidJSONString(
On 2013/12/06 15:35:12, Mark Mentovai wrote:
> How careful were you in determining which of these were OK to use
> EscapeJSONString and which had to use the garbage
EscapeBytesAsInvalidJSONString
> function?

Very. I just double-checked each one and reverted progress_marker_map.cc.
Everything else is debug-printing-only. I'll ask a sync/ OWNER to triple-check,
though.

[Mark Mentovai, 2013-12-09 20:21:58]

OK, LGTM. Nice work.

https://codereview.chromium.org/100823007/diff/60001/base/json/json_writer.h
File base/json/json_writer.h (right):

https://codereview.chromium.org/100823007/diff/60001/base/json/json_writer.h#...
base/json/json_writer.h:22: OPTIONS_OMIT_BINARY_VALUES = 1 << 1,
You can pack the bits in tightly again now that OPTIONS_DO_NOT_ESCAPE is gone: 1
<< 0, …

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

https://codereview.chromium.org/100823007/diff/60001/base/json/string_escape....
base/json/string_escape.cc:56: dest->append("\\u003C");
0x3c is magic, so COMPILE_ASSERT that '<' == 0x3c. (Unless you want to bring in
StringAppendF here, but I understand not wanting to.)

https://codereview.chromium.org/100823007/diff/60001/base/json/string_escape.h
File base/json/string_escape.h (right):

https://codereview.chromium.org/100823007/diff/60001/base/json/string_escape....
base/json/string_escape.h:50: // will be rejected via a parser error if passed
to to the base::JSONReader.
Will it be a parser error or will it just be bogus UTF-8? The parser should be
able to decode something like "\\u00ff" correctly, it’s just that it’ll turn it
into c2 80 instead of ff.

[Robert Sesek, 2013-12-09 21:14:37]

Thanks for the review. Adding OWNERS:

thakis: chrome/
asanka: net/
zea: jingle/ and sync/ -- please check my changes such that everything using
EscapeBytesAsInvalidJSON() is really just for debugging purposes, and conversely
that ProgressMarkerMapToValue() won't be getting binary data (it doesn't look
like it since it's in a base::StringValue).

https://codereview.chromium.org/100823007/diff/60001/base/json/json_writer.h
File base/json/json_writer.h (right):

https://codereview.chromium.org/100823007/diff/60001/base/json/json_writer.h#...
base/json/json_writer.h:22: OPTIONS_OMIT_BINARY_VALUES = 1 << 1,
On 2013/12/09 20:21:59, Mark Mentovai wrote:
> You can pack the bits in tightly again now that OPTIONS_DO_NOT_ESCAPE is gone:
1
> << 0, …

Done.

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

https://codereview.chromium.org/100823007/diff/60001/base/json/string_escape....
base/json/string_escape.cc:56: dest->append("\\u003C");
On 2013/12/09 20:21:59, Mark Mentovai wrote:
> 0x3c is magic, so COMPILE_ASSERT that '<' == 0x3c. (Unless you want to bring
in
> StringAppendF here, but I understand not wanting to.)

Done.

https://codereview.chromium.org/100823007/diff/60001/base/json/string_escape.h
File base/json/string_escape.h (right):

https://codereview.chromium.org/100823007/diff/60001/base/json/string_escape....
base/json/string_escape.h:50: // will be rejected via a parser error if passed
to to the base::JSONReader.
On 2013/12/09 20:21:59, Mark Mentovai wrote:
> Will it be a parser error or will it just be bogus UTF-8? The parser should be
> able to decode something like "\\u00ff" correctly, it’s just that it’ll turn
it
> into c2 80 instead of ff.

You're right, the reader will accept it. I changed this comment to be about how
it'll mangle the bytes.

[Nico, 2013-12-09 21:20:49]

https://codereview.chromium.org/100823007/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/activity_log/activity_log_unittest.cc (right):

https://codereview.chromium.org/100823007/diff/80001/chrome/browser/extension...
chrome/browser/extensions/activity_log/activity_log_unittest.cc:120:
ASSERT_EQ("[\"POST\",\"\\u003Carg_url>\"]",
Huh, why does < stay escaped but > doesn't?

[Robert Sesek, 2013-12-09 21:21:43]

https://codereview.chromium.org/100823007/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/activity_log/activity_log_unittest.cc (right):

https://codereview.chromium.org/100823007/diff/80001/chrome/browser/extension...
chrome/browser/extensions/activity_log/activity_log_unittest.cc:120:
ASSERT_EQ("[\"POST\",\"\\u003Carg_url>\"]",
On 2013/12/09 21:20:50, Nico wrote:
> Huh, why does < stay escaped but > doesn't?

See earlier review thread. To prevent script execution, only < needs to be
escaped, and we can save a few bytes by not escaping >.

[Nico, 2013-12-09 21:23:59]

On Mon, Dec 9, 2013 at 1:21 PM, <rsesek@chromium.org> wrote:

>
> https://codereview.chromium.org/100823007/diff/80001/
> chrome/browser/extensions/activity_log/activity_log_unittest.cc
> File chrome/browser/extensions/activity_log/activity_log_unittest.cc
> (right):
>
> https://codereview.chromium.org/100823007/diff/80001/
> chrome/browser/extensions/activity_log/activity_log_unittest.cc#newcode120
> chrome/browser/extensions/activity_log/activity_log_unittest.cc:120:
> ASSERT_EQ("[\"POST\",\"\\u003Carg_url>\"]",
> On 2013/12/09 21:20:50, Nico wrote:
>
>> Huh, why does < stay escaped but > doesn't?
>>
>
> See earlier review thread. To prevent script execution, only < needs to
> be escaped, and we can save a few bytes by not escaping >.
>

Might be worth putting that somewhere other than the review thread if it
keeps coming up :-) (e.g. cl description)


>
> https://codereview.chromium.org/100823007/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Robert Sesek, 2013-12-09 21:29:57]

On 2013/12/09 21:23:59, Nico wrote:
> On Mon, Dec 9, 2013 at 1:21 PM, <mailto:rsesek@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.org/100823007/diff/80001/
> > chrome/browser/extensions/activity_log/activity_log_unittest.cc
> > File chrome/browser/extensions/activity_log/activity_log_unittest.cc
> > (right):
> >
> > https://codereview.chromium.org/100823007/diff/80001/
> > chrome/browser/extensions/activity_log/activity_log_unittest.cc#newcode120
> > chrome/browser/extensions/activity_log/activity_log_unittest.cc:120:
> > ASSERT_EQ("[\"POST\",\"\\u003Carg_url>\"]",
> > On 2013/12/09 21:20:50, Nico wrote:
> >
> >> Huh, why does < stay escaped but > doesn't?
> >>
> >
> > See earlier review thread. To prevent script execution, only < needs to
> > be escaped, and we can save a few bytes by not escaping >.
> >
> 
> Might be worth putting that somewhere other than the review thread if it
> keeps coming up :-) (e.g. cl description)

There's a comment where it happens. I'm not sure it's an important enough change
to warrant being in the CL description, at least relative to the other major
changes.

[Nico, 2013-12-09 21:42:22]

chrome lgtm

+inferno fyi who added <> escaping, to shout if not escaping '>' in json might
cause issues ('<' is still escaped). I can't think of anything though.

https://codereview.chromium.org/100823007/diff/80001/base/json/string_escape.h
File base/json/string_escape.h (right):

https://codereview.chromium.org/100823007/diff/80001/base/json/string_escape....
base/json/string_escape.h:29: bool put_in_quotes,
Consider using an enum instead of a bool for this parameter. I was wondering
what the "true" in scapeJSONString(str, true, result) meant in a few places.

(But the new names are much better than the old names.)

[asanka, 2013-12-09 21:51:47]

/net/ lgtm

[Nicolas Zea, 2013-12-09 22:47:47]

jingle/sync lgtm

[Robert Sesek, 2013-12-09 22:50:56]

+bauerb for components/plugins/OWNERS

https://codereview.chromium.org/100823007/diff/80001/base/json/string_escape.h
File base/json/string_escape.h (right):

https://codereview.chromium.org/100823007/diff/80001/base/json/string_escape....
base/json/string_escape.h:29: bool put_in_quotes,
On 2013/12/09 21:42:23, Nico wrote:
> Consider using an enum instead of a bool for this parameter. I was wondering
> what the "true" in scapeJSONString(str, true, result) meant in a few places.
> 
> (But the new names are much better than the old names.)

I did consider it, but it adds a lot of extra characters and IMO would only be
marginally more readable. If this function took a series of bools I'd use enums.
Do you feel strongly?

[Nico, 2013-12-10 00:13:32]

https://codereview.chromium.org/100823007/diff/80001/base/json/string_escape.h
File base/json/string_escape.h (right):

https://codereview.chromium.org/100823007/diff/80001/base/json/string_escape....
base/json/string_escape.h:29: bool put_in_quotes,
On 2013/12/09 22:50:57, rsesek wrote:
> On 2013/12/09 21:42:23, Nico wrote:
> > Consider using an enum instead of a bool for this parameter. I was wondering
> > what the "true" in scapeJSONString(str, true, result) meant in a few places.
> > 
> > (But the new names are much better than the old names.)
> 
> I did consider it, but it adds a lot of extra characters and IMO would only be
> marginally more readable. If this function took a series of bools I'd use
enums.
> Do you feel strongly?

I do think it's a better api. I'm not saying that you need to do this, or that
it needs to happen in this cl.

[Bernhard Bauer, 2013-12-10 09:09:01]

components/plugins LGTM

[commit-bot: I haz the power, 2013-12-10 12:31:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsesek@chromium.org/100823007/100001

[commit-bot: I haz the power, 2013-12-10 14:22:50]

Retried try job too often on win_rel for step(s) net_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-12-10 15:10:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsesek@chromium.org/100823007/120001

[commit-bot: I haz the power, 2013-12-10 17:40:38]

Change committed as 239800

[Denis Kuznetsov (DE-MUC), 2013-12-11 11:52:00]

A revert of this CL has been created in
https://codereview.chromium.org/106793004/ by antrim@chromium.org.

The reason for reverting is: This patchset breaks at least displayment of
Russian localized strings on Chromeos login screen..

[Mark Mentovai, 2013-12-11 14:28:53]

How is it broken? What should have been displayed? What was displayed instead?

You know, kind of like a bug report…

[Robert Sesek, 2013-12-11 22:10:55]

Committed patchset #7 manually as r240190 (presubmit successful).