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

base/json/string_escape.h

Index: base/json/string_escape.h
diff --git a/base/json/string_escape.h b/base/json/string_escape.h
index 0f16f59d4fd8bc3d427b751385272b08f175b212..de198ee550c2efbda069e7d721c2a0e83d1ecfc7 100644
--- a/base/json/string_escape.h
+++ b/base/json/string_escape.h
@@ -1,8 +1,8 @@
 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
-//
-// This file defines utility functions for escaping strings.
+
+// This file defines utility functions for escaping strings suitable for JSON.
 
 #ifndef BASE_JSON_STRING_ESCAPE_H_
 #define BASE_JSON_STRING_ESCAPE_H_
@@ -14,24 +14,46 @@
 
 namespace base {
 
-// Escape |str| appropriately for a JSON string literal, _appending_ the
-// result to |dst|. This will create unicode escape sequences (\uXXXX).
-// If |put_in_quotes| is true, the result will be surrounded in double quotes.
-// The outputted literal, when interpreted by the browser, should result in a
-// javascript string that is identical and the same length as the input |str|.
-BASE_EXPORT void JsonDoubleQuote(const StringPiece& str,
-                                 bool put_in_quotes,
-                                 std::string* dst);
-
-// Same as above, but always returns the result double quoted.
-BASE_EXPORT std::string GetDoubleQuotedJson(const StringPiece& str);
-
-BASE_EXPORT void JsonDoubleQuote(const StringPiece16& str,
-                                 bool put_in_quotes,
-                                 std::string* dst);
-
-// Same as above, but always returns the result double quoted.
-BASE_EXPORT std::string GetDoubleQuotedJson(const StringPiece16& str);
+// Appends to |dest| an escaped version of |str|. Valid UTF-8 code units will
+// pass through from the input to the output. Invalid code units will be
+// replaced with the U+FFFD replacement character. On return, |dest| will

[Mark Mentovai, 2013/12/06 15:35:12]

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.

[Avi (use Gerrit), 2013/12/06 15:45:25]

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?

[jungshik at Google, 2013/12/07 13:16:59]

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') 

[Mark Mentovai, 2013/12/07 22:22:49]

Jungshik Shin wrote:
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.

[Robert Sesek, 2013/12/09 19:52:09]

Done.
I don't know what use this would be.
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.

+// contain a valid UTF-8 JSON string.
+//
+// Non-printing control characters will be escaped as \uXXXX sequences for
+// readability.
+//
+// If |put_in_quotes| is true, then a leading and trailing double-quote mark
+// will be appended to |dest| as well.
+BASE_EXPORT void EscapeJSONString(const StringPiece& str,
+                                  bool put_in_quotes,
+                                  std::string* dest);
+
+// Performs a similar function to the UTF-8 StringPiece version above, but
+// instead operates on UTF-16 code units. Unlike UTF-8 code units, the
+// UTF-16 units will be escaped into \uXXXX sequences. Invalid code units

[Mark Mentovai, 2013/12/06 15:35:12]

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.

[jungshik at Google, 2013/12/07 13:16:59]

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

[Robert Sesek, 2013/12/09 19:52:09]

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

+// will be replaced with \uFFFD. On return, |dest| will contain a valid
+// UTF-8 JSON string.
+BASE_EXPORT void EscapeJSONString(const StringPiece16& str,
+                                  bool put_in_quotes,
+                                  std::string* dest);
+
+// Helper functions that wrap the above two functions but return the value
+// instead of appending. |put_in_quotes| is always true.
+BASE_EXPORT std::string GetQuotedJSONString(const StringPiece& str);
+BASE_EXPORT std::string GetQuotedJSONString(const StringPiece16& str);
+
+// Given an arbitrary byte string |str|, this will escape all non-ASCII bytes
+// as \uXXXX escape sequences. This function is *NOT* meant to be used with
+// Unicode strings and does not validate |str| as one.
+//
+// CAVEAT CALLER: The output of this function may not be valid JSON, since
+// JSON requires escape sequences to be valid UTF-16 code units. This output
+// will be rejcted via a parser error if passed to to the base::JSONReader.

[jungshik at Google, 2013/12/07 13:16:59]

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:49]

Jungshik Shin wrote:
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.

[Mark Mentovai, 2013/12/07 22:22:49]

rejcted → rejected

[Robert Sesek, 2013/12/09 19:52:09]

Done.

+//
+// The output of this function takes the *appearance* of JSON but is not in
+// fact valid according to RFC 4627.
+BASE_EXPORT std::string EscapeBytesAsInvalidJSONString(const StringPiece& str,

[Mark Mentovai, 2013/12/06 15:35:12]

Good name.

+                                                       bool put_in_quotes);
 
 }  // namespace base