[Tracing] Minor bug fixes related to trace serialization

Escape sequences may now be written to a trace file (previously,
any string with a valid escapable character would fail a check).

Also, string properties are now surrounded with quotes.

BUG=v8:4561

Review-Url: https://codereview.chromium.org/2309943005
Cr-Commit-Position: refs/heads/master@{#39319}

src/libplatform/tracing/trace-writer.cc

@@ -13,17 +13,45 @@ namespace v8 {
 namespace platform {
 namespace tracing {
 
-// Currently we do not support JSON-escaping strings in trace arguments.
-// Thus we perform an IsJSONString() check before writing any string argument.
-// In particular, this means strings cannot have control characters or \.
-V8_INLINE static bool IsJSONString(const char* str) {
+// Writes the given string to a stream, taking care to escape characters
+// when necessary.
+V8_INLINE static void WriteJSONStringToStream(const char* str,
+                                              std::ostream& stream) {
   size_t len = strlen(str);
+  stream << "\"";
   for (size_t i = 0; i < len; ++i) {
-    if (iscntrl(str[i]) || str[i] == '\\') {
-      return false;
+    // All of the permitted escape sequences in JSON strings, as per
+    // https://mathiasbynens.be/notes/javascript-escapes
+    switch (str[i]) {
+      case '\b':
+        stream << "\\b";
+        break;
+      case '\f':
+        stream << "\\f";
+        break;
+      case '\n':
+        stream << "\\n";
+        break;
+      case '\r':
+        stream << "\\r";
+        break;
+      case '\t':
+        stream << "\\t";
+        break;
+      case '\"':
+        stream << "\\\"";
+        break;
+      case '\\':
+        stream << "\\\\";
+        break;
+      // Note that because we use double quotes for JSON strings,
+      // we don't need to escape single quotes.
+      default:
+        stream << str[i];
+        break;
     }
   }
-  return true;
+  stream << "\"";
 }
 
 void JSONTraceWriter::AppendArgValue(uint8_t type,
@@ -72,10 +100,11 @@ void JSONTraceWriter::AppendArgValue(uint8_t type,
       break;
     case TRACE_VALUE_TYPE_STRING:
     case TRACE_VALUE_TYPE_COPY_STRING:
-      // Strings are currently not JSON-escaped, so we need to perform a check
-      // to see if they are valid JSON strings.
-      CHECK(value.as_string != nullptr && IsJSONString(value.as_string));
-      stream_ << (value.as_string ? value.as_string : "NULL");
+      if (value.as_string == nullptr) {
+        stream_ << "\"NULL\"";
+      } else {
+        WriteJSONStringToStream(value.as_string, stream_);
+      }
       break;
     default:
       UNREACHABLE();

test/cctest/libplatform/test-tracing.cc

@@ -217,7 +217,7 @@ TEST(TestTracingControllerMultipleArgsAndCopy) {
   double jj4 = std::numeric_limits<double>::infinity();
   double jj5 = -std::numeric_limits<double>::infinity();
   void* kk = &aa;
-  const char* ll = "\"100\"";
+  const char* ll = "100";
   std::string mm = "INIT";
   std::string mmm = "\"INIT\"";
 
@@ -254,15 +254,13 @@ TEST(TestTracingControllerMultipleArgsAndCopy) {
     TRACE_EVENT1("v8", "v8.Test.mm", "mm", TRACE_STR_COPY(mmm.c_str()));
 
     TRACE_EVENT2("v8", "v8.Test2.1", "aa", aa, "ll", ll);
-    TRACE_EVENT2("v8", "v8.Test2.2", "mm1", TRACE_STR_COPY(mmm.c_str()), "mm2",
+    TRACE_EVENT2("v8", "v8.Test2.2", "mm1", TRACE_STR_COPY(mm.c_str()), "mm2",
                  TRACE_STR_COPY(mmm.c_str()));
 
     // Check copies are correct.
     TRACE_EVENT_COPY_INSTANT0("v8", mm.c_str(), TRACE_EVENT_SCOPE_THREAD);
-    TRACE_EVENT_COPY_INSTANT1("v8", mm.c_str(), TRACE_EVENT_SCOPE_THREAD,
-                              mm.c_str(), mmm.c_str());
-    TRACE_EVENT_COPY_INSTANT2("v8", mm.c_str(), TRACE_EVENT_SCOPE_THREAD,
-                              mm.c_str(), mmm.c_str(), mm.c_str(), mmm.c_str());
+    TRACE_EVENT_COPY_INSTANT2("v8", mm.c_str(), TRACE_EVENT_SCOPE_THREAD, "mm1",
+                              mm.c_str(), "mm2", mmm.c_str());
     mm = "CHANGED";
     mmm = "CHANGED";
 
@@ -276,7 +274,7 @@ TEST(TestTracingControllerMultipleArgsAndCopy) {
   GetJSONStrings(all_names, trace_str, "\"name\"", "\"", "\"");
   GetJSONStrings(all_cats, trace_str, "\"cat\"", "\"", "\"");
 
-  CHECK_EQ(all_args.size(), 23);
+  CHECK_EQ(all_args.size(), 22);
   CHECK_EQ(all_args[0], "\"aa\":11");
   CHECK_EQ(all_args[1], "\"bb\":22");
   CHECK_EQ(all_args[2], "\"cc\":33");
@@ -296,17 +294,15 @@ TEST(TestTracingControllerMultipleArgsAndCopy) {
   pointer_stream << "\"kk\":\"" << &aa << "\"";
   CHECK_EQ(all_args[15], pointer_stream.str());
   CHECK_EQ(all_args[16], "\"ll\":\"100\"");
-  CHECK_EQ(all_args[17], "\"mm\":\"INIT\"");
+  CHECK_EQ(all_args[17], "\"mm\":\"\\\"INIT\\\"\"");
 
   CHECK_EQ(all_names[18], "v8.Test2.1");
   CHECK_EQ(all_args[18], "\"aa\":11,\"ll\":\"100\"");
-  CHECK_EQ(all_args[19], "\"mm1\":\"INIT\",\"mm2\":\"INIT\"");
+  CHECK_EQ(all_args[19], "\"mm1\":\"INIT\",\"mm2\":\"\\\"INIT\\\"\"");
 
   CHECK_EQ(all_names[20], "INIT");
   CHECK_EQ(all_names[21], "INIT");
-  CHECK_EQ(all_args[21], "\"INIT\":\"INIT\"");
-  CHECK_EQ(all_names[22], "INIT");
-  CHECK_EQ(all_args[22], "\"INIT\":\"INIT\",\"INIT\":\"INIT\"");
+  CHECK_EQ(all_args[21], "\"mm1\":\"INIT\",\"mm2\":\"\\\"INIT\\\"\"");
 
   i::V8::SetPlatformForTesting(old_platform);
 }