[inspector] Prepend isolateId to remoteObjectId

This changes remoteObjectId format from
"{injectedScriptId:123,id:456}" to "<isolateId>.<contextId>.<id>".

Prepending isolateId fixes the problem that
remote object ids clash between processes. This is especially
troubling during cross-process navigation in Chromium, see bug.

We also stop producing and parsing unnecessary json for object ids.

Drive-by: fixed some tests dumping object ids. Most tests avoid
dumping unstable values like ids, but there were few that still did.

BUG=chromium:1137143

Change-Id: Ia019757fb95704ccb718d3ea6cc54bde1a133382
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2461731
Commit-Queue: Dmitry Gozman <dgozman@chromium.org>
Reviewed-by: Yang Guo <yangguo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#70592}

src/inspector/injected-script.cc

@@ -518,19 +518,9 @@ Response InjectedScript::getInternalAndPrivateProperties(
 }
 
 void InjectedScript::releaseObject(const String16& objectId) {
-  std::vector<uint8_t> cbor;
-  v8_crdtp::json::ConvertJSONToCBOR(
-      v8_crdtp::span<uint16_t>(objectId.characters16(), objectId.length()),
-      &cbor);
-  std::unique_ptr<protocol::Value> parsedObjectId =
-      protocol::Value::parseBinary(cbor.data(), cbor.size());
-  if (!parsedObjectId) return;
-  protocol::DictionaryValue* object =
-      protocol::DictionaryValue::cast(parsedObjectId.get());
-  if (!object) return;
-  int boundId = 0;
-  if (!object->getInteger("id", &boundId)) return;
-  unbindObject(boundId);
+  std::unique_ptr<RemoteObjectId> remoteId;
+  Response response = RemoteObjectId::parse(objectId, &remoteId);
+  if (response.IsSuccess()) unbindObject(remoteId->id());
 }
 
 Response InjectedScript::wrapObject(
@@ -722,10 +712,12 @@ Response InjectedScript::resolveCallArgument(
     Response response =
         RemoteObjectId::parse(callArgument->getObjectId(""), &remoteObjectId);
     if (!response.IsSuccess()) return response;
-    if (remoteObjectId->contextId() != m_context->contextId())
+    if (remoteObjectId->contextId() != m_context->contextId() ||
+        remoteObjectId->isolateId() != m_context->inspector()->isolateId()) {
       return Response::ServerError(
           "Argument should belong to the same JavaScript world as target "
           "object");
+    }
     return findObject(*remoteObjectId, result);
   }
   if (callArgument->hasValue() || callArgument->hasUnserializableValue()) {
@@ -1012,10 +1004,8 @@ String16 InjectedScript::bindObject(v8::Local<v8::Value> value,
     m_idToObjectGroupName[id] = groupName;
     m_nameToObjectGroup[groupName].push_back(id);
   }
-  // TODO(dgozman): get rid of "injectedScript" notion.
-  return String16::concat(
-      "{\"injectedScriptId\":", String16::fromInteger(m_context->contextId()),
-      ",\"id\":", String16::fromInteger(id), "}");
+  return RemoteObjectId::serialize(m_context->inspector()->isolateId(),
+                                   m_context->contextId(), id);
 }
 
 // static

src/inspector/remote-object-id.cc

@@ -10,63 +10,68 @@
 
 namespace v8_inspector {
 
-RemoteObjectIdBase::RemoteObjectIdBase() : m_injectedScriptId(0) {}
+namespace {
 
-std::unique_ptr<protocol::DictionaryValue>
-RemoteObjectIdBase::parseInjectedScriptId(const String16& objectId) {
-  std::vector<uint8_t> cbor;
-  v8_crdtp::json::ConvertJSONToCBOR(
-      v8_crdtp::span<uint16_t>(objectId.characters16(), objectId.length()),
-      &cbor);
-  std::unique_ptr<protocol::Value> parsedValue =
-      protocol::Value::parseBinary(cbor.data(), cbor.size());
-  if (!parsedValue || parsedValue->type() != protocol::Value::TypeObject)
-    return nullptr;
-
-  std::unique_ptr<protocol::DictionaryValue> parsedObjectId(
-      protocol::DictionaryValue::cast(parsedValue.release()));
-  bool success =
-      parsedObjectId->getInteger("injectedScriptId", &m_injectedScriptId);
-  if (success) return parsedObjectId;
-  return nullptr;
+String16 serializeId(uint64_t isolateId, int injectedScriptId, int id) {
+  return String16::concat(
+      String16::fromInteger64(static_cast<int64_t>(isolateId)), ".",
+      String16::fromInteger(injectedScriptId), ".", String16::fromInteger(id));
 }
 
-RemoteObjectId::RemoteObjectId() : RemoteObjectIdBase(), m_id(0) {}
+}  // namespace
+
+RemoteObjectIdBase::RemoteObjectIdBase()
+    : m_isolateId(0), m_injectedScriptId(0), m_id(0) {}
+
+bool RemoteObjectIdBase::parseId(const String16& objectId) {
+  const UChar dot = '.';
+  size_t firstDotPos = objectId.find(dot);
+  if (firstDotPos == String16::kNotFound) return false;
+  bool ok = false;
+  int64_t isolateId = objectId.substring(0, firstDotPos).toInteger64(&ok);
+  if (!ok) return false;
+  firstDotPos++;
+  size_t secondDotPos = objectId.find(dot, firstDotPos);
+  if (secondDotPos == String16::kNotFound) return false;
+  int injectedScriptId =
+      objectId.substring(firstDotPos, secondDotPos - firstDotPos)
+          .toInteger(&ok);
+  if (!ok) return false;
+  secondDotPos++;
+  int id = objectId.substring(secondDotPos).toInteger(&ok);
+  if (!ok) return false;
+  m_isolateId = static_cast<uint64_t>(isolateId);
+  m_injectedScriptId = injectedScriptId;
+  m_id = id;
+  return true;
+}
 
 Response RemoteObjectId::parse(const String16& objectId,
                                std::unique_ptr<RemoteObjectId>* result) {
   std::unique_ptr<RemoteObjectId> remoteObjectId(new RemoteObjectId());
-  std::unique_ptr<protocol::DictionaryValue> parsedObjectId =
-      remoteObjectId->parseInjectedScriptId(objectId);
-  if (!parsedObjectId) return Response::ServerError("Invalid remote object id");
-
-  bool success = parsedObjectId->getInteger("id", &remoteObjectId->m_id);
-  if (!success) return Response::ServerError("Invalid remote object id");
+  if (!remoteObjectId->parseId(objectId))
+    return Response::ServerError("Invalid remote object id");
   *result = std::move(remoteObjectId);
   return Response::Success();
 }
 
-RemoteCallFrameId::RemoteCallFrameId()
-    : RemoteObjectIdBase(), m_frameOrdinal(0) {}
+String16 RemoteObjectId::serialize(uint64_t isolateId, int injectedScriptId,
+                                   int id) {
+  return serializeId(isolateId, injectedScriptId, id);
+}
 
 Response RemoteCallFrameId::parse(const String16& objectId,
                                   std::unique_ptr<RemoteCallFrameId>* result) {
   std::unique_ptr<RemoteCallFrameId> remoteCallFrameId(new RemoteCallFrameId());
-  std::unique_ptr<protocol::DictionaryValue> parsedObjectId =
-      remoteCallFrameId->parseInjectedScriptId(objectId);
-  if (!parsedObjectId) return Response::ServerError("Invalid call frame id");
-
-  bool success =
-      parsedObjectId->getInteger("ordinal", &remoteCallFrameId->m_frameOrdinal);
-  if (!success) return Response::ServerError("Invalid call frame id");
+  if (!remoteCallFrameId->parseId(objectId))
+    return Response::ServerError("Invalid call frame id");
   *result = std::move(remoteCallFrameId);
   return Response::Success();
 }
 
-String16 RemoteCallFrameId::serialize(int injectedScriptId, int frameOrdinal) {
-  return "{\"ordinal\":" + String16::fromInteger(frameOrdinal) +
-         ",\"injectedScriptId\":" + String16::fromInteger(injectedScriptId) +
-         "}";
+String16 RemoteCallFrameId::serialize(uint64_t isolateId, int injectedScriptId,
+                                      int frameOrdinal) {
+  return serializeId(isolateId, injectedScriptId, frameOrdinal);
 }
 
 }  // namespace v8_inspector

src/inspector/remote-object-id.h

@@ -15,16 +15,18 @@ using protocol::Response;
 
 class RemoteObjectIdBase {
  public:
+  uint64_t isolateId() const { return m_isolateId; }
   int contextId() const { return m_injectedScriptId; }
 
  protected:
   RemoteObjectIdBase();
   ~RemoteObjectIdBase() = default;
 
-  std::unique_ptr<protocol::DictionaryValue> parseInjectedScriptId(
-      const String16&);
+  bool parseId(const String16&);
 
+  uint64_t m_isolateId;
   int m_injectedScriptId;
+  int m_id;
 };
 
 class RemoteObjectId final : public RemoteObjectIdBase {
@@ -33,10 +35,7 @@ class RemoteObjectId final : public RemoteObjectIdBase {
   ~RemoteObjectId() = default;
   int id() const { return m_id; }
 
- private:
-  RemoteObjectId();
-
-  int m_id;
+  static String16 serialize(uint64_t isolateId, int injectedScriptId, int id);
 };
 
 class RemoteCallFrameId final : public RemoteObjectIdBase {
@@ -44,14 +43,10 @@ class RemoteCallFrameId final : public RemoteObjectIdBase {
   static Response parse(const String16&, std::unique_ptr<RemoteCallFrameId>*);
   ~RemoteCallFrameId() = default;
 
-  int frameOrdinal() const { return m_frameOrdinal; }
-
-  static String16 serialize(int injectedScriptId, int frameOrdinal);
-
- private:
-  RemoteCallFrameId();
+  int frameOrdinal() const { return m_id; }
 
-  int m_frameOrdinal;
+  static String16 serialize(uint64_t isolateId, int injectedScriptId,
+                            int frameOrdinal);
 };
 
 }  // namespace v8_inspector

src/inspector/string-16.h

@@ -39,10 +39,12 @@ class String16 {
   static String16 fromInteger(int);
   static String16 fromInteger(size_t);
   static String16 fromInteger64(int64_t);
+  static String16 fromUInt64(uint64_t);
   static String16 fromDouble(double);
   static String16 fromDouble(double, int precision);
 
   int64_t toInteger64(bool* ok = nullptr) const;
+  uint64_t toUInt64(bool* ok = nullptr) const;
   int toInteger(bool* ok = nullptr) const;
   String16 stripWhiteSpace() const;
   const UChar* characters16() const { return m_impl.c_str(); }

src/inspector/v8-debugger-agent-impl.cc

@@ -1446,8 +1446,8 @@ Response V8DebuggerAgentImpl::currentCallFrames(
     int contextId = iterator->GetContextId();
     InjectedScript* injectedScript = nullptr;
     if (contextId) m_session->findInjectedScript(contextId, injectedScript);
-    String16 callFrameId =
-        RemoteCallFrameId::serialize(contextId, frameOrdinal);
+    String16 callFrameId = RemoteCallFrameId::serialize(
+        m_inspector->isolateId(), contextId, frameOrdinal);
 
     v8::debug::Location loc = iterator->GetSourceLocation();
 

src/inspector/v8-inspector-session-impl.cc

@@ -239,6 +239,8 @@ Response V8InspectorSessionImpl::findInjectedScript(
 
 Response V8InspectorSessionImpl::findInjectedScript(
     RemoteObjectIdBase* objectId, InjectedScript*& injectedScript) {
+  if (objectId->isolateId() != m_inspector->isolateId())
+    return Response::ServerError("Cannot find context with specified id");
   return findInjectedScript(objectId->contextId(), injectedScript);
 }
 

test/debugger/test-api.js

@@ -593,7 +593,7 @@ class DebugWrapper {
     const column = frame.location.columnNumber;
     const loc = %ScriptLocationFromLine2(scriptid, line, column, 0);
     const func = { name : () => frame.functionName };
-    const index = JSON.parse(frame.callFrameId).ordinal;
+    const index = +frame.callFrameId.split(".")[2];
 
     function allScopes() {
       const scopes = [];

test/inspector/debugger/class-private-methods-static-nested-expected.txt

@@ -8,7 +8,7 @@ privateProperties on class A
         value : {
             className : Function
             description : #method() {       debugger;     }
-            objectId : {"injectedScriptId":1,"id":39}
+            objectId : <objectId>
             type : function
         }
     }

test/inspector/debugger/class-private-methods-static-nested.js

@@ -43,7 +43,7 @@ InspectorTest.runAsyncTestSuite([
     let { result } = await Protocol.Runtime.getProperties({
       objectId: frame.this.objectId
     });
-    InspectorTest.logObject(result.privateProperties);
+    InspectorTest.logMessage(result.privateProperties);
     Protocol.Debugger.resume();
 
     ({ params: { callFrames } } = await Protocol.Debugger.oncePaused());  // B.test();

test/inspector/debugger/class-private-methods-unused-expected.txt

@@ -8,7 +8,7 @@ Get privateProperties of A in testStatic()
         value : {
             className : Function
             description : #staticMethod() { return 1; }
-            objectId : {"injectedScriptId":1,"id":34}
+            objectId : <objectId>
             type : function
         }
     }
@@ -20,19 +20,19 @@ Access A.#staticMethod() in testStatic()
         exception : {
             className : ReferenceError
             description : ReferenceError: A is not defined     at eval (eval at testStatic (:1:1), <anonymous>:1:1)     at Function.testStatic (<anonymous>:6:29)     at run (<anonymous>:9:7)     at <anonymous>:1:1
-            objectId : {"injectedScriptId":1,"id":36}
+            objectId : <objectId>
             subtype : error
             type : object
         }
-        exceptionId : 1
+        exceptionId : <exceptionId>
         lineNumber : 0
-        scriptId : 5
+        scriptId : <scriptId>
         text : Uncaught
     }
     result : {
         className : ReferenceError
         description : ReferenceError: A is not defined     at eval (eval at testStatic (:1:1), <anonymous>:1:1)     at Function.testStatic (<anonymous>:6:29)     at run (<anonymous>:9:7)     at <anonymous>:1:1
-        objectId : {"injectedScriptId":1,"id":35}
+        objectId : <objectId>
         subtype : error
         type : object
     }
@@ -44,19 +44,19 @@ Access this.#staticMethod() in testStatic()
         exception : {
             className : Error
             description : Error: Unused static private method '#staticMethod' cannot be accessed at debug time     at eval (eval at testStatic (:1:1), <anonymous>:1:1)     at Function.testStatic (<anonymous>:6:29)     at run (<anonymous>:9:7)     at <anonymous>:1:1
-            objectId : {"injectedScriptId":1,"id":38}
+            objectId : <objectId>
             subtype : error
             type : object
         }
-        exceptionId : 2
+        exceptionId : <exceptionId>
         lineNumber : 0
-        scriptId : 6
+        scriptId : <scriptId>
         text : Uncaught
     }
     result : {
         className : Error
         description : Error: Unused static private method '#staticMethod' cannot be accessed at debug time     at eval (eval at testStatic (:1:1), <anonymous>:1:1)     at Function.testStatic (<anonymous>:6:29)     at run (<anonymous>:9:7)     at <anonymous>:1:1
-        objectId : {"injectedScriptId":1,"id":37}
+        objectId : <objectId>
         subtype : error
         type : object
     }
@@ -68,7 +68,7 @@ get privateProperties of a in testInstance()
         value : {
             className : Function
             description : #instanceMethod() { return 2; }
-            objectId : {"injectedScriptId":1,"id":61}
+            objectId : <objectId>
             type : function
         }
     }

test/inspector/debugger/class-private-methods-unused.js

@@ -38,7 +38,7 @@ InspectorTest.runAsyncTestSuite([
     let { result } = await Protocol.Runtime.getProperties({
       objectId: frame.this.objectId
     });
-    InspectorTest.logObject(result.privateProperties);
+    InspectorTest.logMessage(result.privateProperties);
 
     // Variables not referenced in the source code are currently
     // considered "optimized away".
@@ -47,14 +47,14 @@ InspectorTest.runAsyncTestSuite([
       expression: 'A.#staticMethod();',
       callFrameId: callFrames[0].callFrameId
     }));
-    InspectorTest.logObject(result);
+    InspectorTest.logMessage(result);
 
     InspectorTest.log('Access this.#staticMethod() in testStatic()');
     ({ result } = await Protocol.Debugger.evaluateOnCallFrame({
       expression: 'this.#staticMethod();',
       callFrameId: callFrames[0].callFrameId
     }));
-    InspectorTest.logObject(result);
+    InspectorTest.logMessage(result);
 
     Protocol.Debugger.resume();
     ({ params: { callFrames } } = await Protocol.Debugger.oncePaused());  // a.testInstatnce();
@@ -64,14 +64,14 @@ InspectorTest.runAsyncTestSuite([
     ({ result } = await Protocol.Runtime.getProperties({
       objectId: frame.this.objectId
     }));
-    InspectorTest.logObject(result.privateProperties);
+    InspectorTest.logMessage(result.privateProperties);
 
     InspectorTest.log('Evaluating this.#instanceMethod() in testInstance()');
     ({ result } = await Protocol.Debugger.evaluateOnCallFrame({
       expression: 'this.#instanceMethod();',
       callFrameId: callFrames[0].callFrameId
     }));
-    InspectorTest.logObject(result);
+    InspectorTest.logMessage(result);
 
     Protocol.Debugger.resume();
     Protocol.Debugger.disable();

test/inspector/runtime/custom-preview-expected.txt

@@ -2,7 +2,13 @@ RemoteObject.CustomPreview
 Dump custom previews..
 {
     bodyGetterId : <bodyGetterId>
-    header : ["span",{},"Header formatted by 1 ","a"]
+    header : [
+        [0] : span
+        [1] : {
+        }
+        [2] : Header formatted by 1 
+        [3] : a
+    ]
 }
 {
     id : <messageId>
@@ -30,7 +36,13 @@ Dump custom previews..
 }
 {
     bodyGetterId : <bodyGetterId>
-    header : ["span",{},"Header formatted by 2 ","b"]
+    header : [
+        [0] : span
+        [1] : {
+        }
+        [2] : Header formatted by 2 
+        [3] : b
+    ]
 }
 {
     id : <messageId>
@@ -49,7 +61,13 @@ Dump custom previews..
 }
 {
     bodyGetterId : <bodyGetterId>
-    header : ["span",{},"Header formatted by 1 ","c"]
+    header : [
+        [0] : span
+        [1] : {
+        }
+        [2] : Header formatted by 1 
+        [3] : c
+    ]
 }
 {
     id : <messageId>
@@ -76,12 +94,36 @@ Dump custom previews..
     }
 }
 {
-    header : ["span",{},"Formatter with config ",["object",{"type":"object","className":"Object","description":"Object","objectId":"{\"injectedScriptId\":1,\"id\":10}","customPreview":{"header":"[\"span\",{},\"Header \",\"info: \",\"additional info\"]","bodyGetterId":"{\"injectedScriptId\":1,\"id\":11}"}}]]
+    header : [
+        [0] : span
+        [1] : {
+        }
+        [2] : Formatter with config 
+        [3] : [
+            [0] : object
+            [1] : {
+                className : Object
+                customPreview : {
+                    bodyGetterId : <bodyGetterId>
+                    header : ["span",{},"Header ","info: ","additional info"]
+                }
+                description : Object
+                objectId : <objectId>
+                type : object
+            }
+        ]
+    ]
 }
 Change formatters order and dump again..
 {
     bodyGetterId : <bodyGetterId>
-    header : ["span",{},"Header formatted by 1 ","a"]
+    header : [
+        [0] : span
+        [1] : {
+        }
+        [2] : Header formatted by 1 
+        [3] : a
+    ]
 }
 {
     id : <messageId>
@@ -109,7 +151,13 @@ Change formatters order and dump again..
 }
 {
     bodyGetterId : <bodyGetterId>
-    header : ["span",{},"Header formatted by 2 ","b"]
+    header : [
+        [0] : span
+        [1] : {
+        }
+        [2] : Header formatted by 2 
+        [3] : b
+    ]
 }
 {
     id : <messageId>
@@ -128,7 +176,13 @@ Change formatters order and dump again..
 }
 {
     bodyGetterId : <bodyGetterId>
-    header : ["span",{},"Header formatted by 2 ","c"]
+    header : [
+        [0] : span
+        [1] : {
+        }
+        [2] : Header formatted by 2 
+        [3] : c
+    ]
 }
 {
     id : <messageId>
@@ -146,12 +200,36 @@ Change formatters order and dump again..
     }
 }
 {
-    header : ["span",{},"Formatter with config ",["object",{"type":"object","className":"Object","description":"Object","objectId":"{\"injectedScriptId\":1,\"id\":21}","customPreview":{"header":"[\"span\",{},\"Header \",\"info: \",\"additional info\"]","bodyGetterId":"{\"injectedScriptId\":1,\"id\":22}"}}]]
+    header : [
+        [0] : span
+        [1] : {
+        }
+        [2] : Formatter with config 
+        [3] : [
+            [0] : object
+            [1] : {
+                className : Object
+                customPreview : {
+                    bodyGetterId : <bodyGetterId>
+                    header : ["span",{},"Header ","info: ","additional info"]
+                }
+                description : Object
+                objectId : <objectId>
+                type : object
+            }
+        ]
+    ]
 }
 Test Runtime.getProperties
 {
     bodyGetterId : <bodyGetterId>
-    header : ["span",{},"Header formatted by 1 ","a"]
+    header : [
+        [0] : span
+        [1] : {
+        }
+        [2] : Header formatted by 1 
+        [3] : a
+    ]
 }
 {
     id : <messageId>
@@ -162,7 +240,7 @@ Test Runtime.getProperties
                 [0] : span
                 [1] : {
                 }
-                [2] : Body formatted by 1
+                [2] : Body formatted by 1 
                 [3] : a
                 [4] : [
                     [0] : object

test/inspector/runtime/custom-preview.js

@@ -120,6 +120,8 @@ function dumpCustomPreviewForEvaluate(result) {
 
 async function dumpCustomPreview(result) {
   const { objectId, customPreview } = result;
+  if (customPreview.header)
+    customPreview.header = JSON.parse(customPreview.header);
   InspectorTest.logMessage(customPreview);
   if (customPreview.bodyGetterId) {
     const body = await Protocol.Runtime.callFunctionOn({