Make VM service id handling JSON-RPC 2 compliant.

* Numeric ids will not be converted to strings.

* String ids will be escaped, so an id of '"' doesn't produce invalid
  JSON.

* List or map ids will be rejected.

* Null ids will produce no response.

R=johnmccutchan@google.com

Review URL: https://codereview.chromium.org//1255003003 .

CHANGELOG.md

@@ -78,6 +78,13 @@
 
 ### VM Service Protocol Changes
 
+* The service protocol now includes a `"jsonrpc"` property in its responses, as
+  opposed to `"json-rpc"`.
+
+* The service protocol now properly handles requests with non-string ids.
+  Numeric ids are no longer converted to strings, and null ids now don't produce
+  a response.
+
 * Some RPCs that didn't include a `"jsonrpc"` property in their responses now
   include one.
 

runtime/bin/vmservice/server.dart

@@ -8,6 +8,7 @@ class WebSocketClient extends Client {
   static const int PARSE_ERROR_CODE = 4000;
   static const int BINARY_MESSAGE_ERROR_CODE = 4001;
   static const int NOT_MAP_ERROR_CODE = 4002;
+  static const int ID_ERROR_CODE = 4003;
   final WebSocket socket;
 
   WebSocketClient(this.socket, VMService service) : super(service) {
@@ -29,6 +30,9 @@ class WebSocketClient extends Client {
         return;
       }
       var serial = map['id'];
+      if (serial != null && serial is! num && serial is! String) {
+        socket.close(ID_ERROR_CODE, '"id" must be a number, string, or null.');
+      }
       onMessage(serial, new Message.fromJsonRpc(this, map));
     } else {
       socket.close(BINARY_MESSAGE_ERROR_CODE, 'Message must be a string.');

runtime/vm/json_stream.cc

@@ -27,7 +27,7 @@ JSONStream::JSONStream(intptr_t buf_size)
                        ObjectIdRing::kAllocateId),
       id_zone_(&default_id_zone_),
       reply_port_(ILLEGAL_PORT),
-      seq_(""),
+      seq_(Instance::Handle(Instance::null())),
       method_(""),
       param_keys_(NULL),
       param_values_(NULL),
@@ -41,12 +41,12 @@ JSONStream::~JSONStream() {
 
 void JSONStream::Setup(Zone* zone,
                        Dart_Port reply_port,
-                       const String& seq,
+                       const Instance& seq,
                        const String& method,
                        const Array& param_keys,
                        const Array& param_values) {
   set_reply_port(reply_port);
-  seq_ = seq.ToCString();
+  seq_ ^= seq.raw();
   method_ = method.ToCString();
 
   String& string_iterator = String::Handle();
@@ -162,8 +162,22 @@ void JSONStream::PostReply() {
   if (FLAG_trace_service) {
     process_delta_micros = OS::GetCurrentTimeMicros() - setup_time_micros_;
   }
-  // TODO(turnidge): Handle non-string sequence numbers.
-  buffer_.Printf(", \"id\":\"%s\"}", seq());
+
+  if (seq_.IsString()) {
+    const String& str = String::Cast(seq_);
+    PrintProperty("id", str.ToCString());
+  } else if (seq_.IsInteger()) {
+    const Integer& integer = Integer::Cast(seq_);
+    PrintProperty64("id", integer.AsInt64Value());
+  } else if (seq_.IsDouble()) {
+    const Double& dbl = Double::Cast(seq_);
+    PrintProperty("id", dbl.value());
+  } else if (seq_.IsNull()) {
+    // JSON-RPC 2.0 says that a request with a null ID shouldn't get a reply.
+    return;
+  }
+  buffer_.AddChar('}');
+
   const String& reply = String::Handle(String::New(ToCString()));
   ASSERT(!reply.IsNull());
 

runtime/vm/json_stream.h

@@ -57,7 +57,7 @@ class JSONStream : ValueObject {
 
   void Setup(Zone* zone,
              Dart_Port reply_port,
-             const String& seq,
+             const Instance& seq,
              const String& method,
              const Array& param_keys,
              const Array& param_values);
@@ -100,7 +100,6 @@ class JSONStream : ValueObject {
   // otherwise.
   bool ParamIs(const char* key, const char* value) const;
 
-  const char* seq() const { return seq_; }
   const char* method() const { return method_; }
   const char** param_keys() const { return param_keys_; }
   const char** param_values() const { return param_values_; }
@@ -171,7 +170,7 @@ class JSONStream : ValueObject {
   RingServiceIdZone default_id_zone_;
   ServiceIdZone* id_zone_;
   Dart_Port reply_port_;
-  const char* seq_;
+  Instance& seq_;
   const char* method_;
   const char** param_keys_;
   const char** param_values_;

runtime/vm/service.cc

@@ -509,7 +509,7 @@ void Service::InvokeMethod(Isolate* isolate, const Array& msg) {
     HANDLESCOPE(isolate);
 
     Instance& reply_port = Instance::Handle(isolate);
-    String& seq = String::Handle(isolate);
+    Instance& seq = String::Handle(isolate);
     String& method_name = String::Handle(isolate);
     Array& param_keys = Array::Handle(isolate);
     Array& param_values = Array::Handle(isolate);
@@ -520,7 +520,7 @@ void Service::InvokeMethod(Isolate* isolate, const Array& msg) {
     param_values ^= msg.At(5);
 
     ASSERT(!method_name.IsNull());
-    ASSERT(!seq.IsNull());
+    ASSERT(seq.IsNull() || seq.IsString() || seq.IsNumber());
     ASSERT(!param_keys.IsNull());
     ASSERT(!param_values.IsNull());
     ASSERT(param_keys.Length() == param_values.Length());

runtime/vm/service/message.dart

@@ -102,7 +102,7 @@ class Message {
     var request = new List(6)
         ..[0] = 0  // Make room for OOB message type.
         ..[1] = receivePort.sendPort
-        ..[2] = serial.toString()
+        ..[2] = serial
         ..[3] = method
         ..[4] = keys
         ..[5] = values;
@@ -129,7 +129,7 @@ class Message {
     var request = new List(6)
         ..[0] = 0  // Make room for OOB message type.
         ..[1] = receivePort.sendPort
-        ..[2] = serial.toString()
+        ..[2] = serial
         ..[3] = method
         ..[4] = keys
         ..[5] = values;

runtime/vm/service/service.md

@@ -97,7 +97,7 @@ example [getVersion](#getversion) request:
 }
 ```
 
-Currently the _id_ property must be a string. The Service Protocol
+The _id_ property must be a string, number, or `null`. The Service Protocol
 optionally accepts requests without the _jsonprc_ property.
 
 An RPC response is a JSON object (http://json.org/). The response always specifies an

runtime/vm/service_test.cc

@@ -530,15 +530,15 @@ TEST_CASE(Service_EmbedderRootHandler) {
 
 
   Array& service_msg = Array::Handle();
-  service_msg = Eval(lib, "[0, port, '0', 'alpha', [], []]");
+  service_msg = Eval(lib, "[0, port, '\"', 'alpha', [], []]");
   Service::HandleRootMessage(service_msg);
   handler.HandleNextMessage();
-  EXPECT_STREQ("{\"jsonrpc\":\"2.0\", \"result\":alpha, \"id\":\"0\"}",
+  EXPECT_STREQ("{\"jsonrpc\":\"2.0\", \"result\":alpha, \"id\":\"\\\"\"}",
                handler.msg());
-  service_msg = Eval(lib, "[0, port, '0', 'beta', [], []]");
+  service_msg = Eval(lib, "[0, port, 1, 'beta', [], []]");
   Service::HandleRootMessage(service_msg);
   handler.HandleNextMessage();
-  EXPECT_STREQ("{\"jsonrpc\":\"2.0\", \"result\":beta, \"id\":\"0\"}",
+  EXPECT_STREQ("{\"jsonrpc\":\"2.0\", \"result\":beta, \"id\":1}",
                handler.msg());
 }