Follow-up after https://crrev.com/c/3472077

1. Use `StringBuffer` instead of `StringView` in `WebDriverValue`.
2. Add some `DCHECK`s.
3. Reserve vector size.
4. Respect properties with `undefined` values.
5. Minor clean-ups.

Change-Id: Ic109acb1e3adf2d950767173c17a9203e3c816dc
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3596173Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Auto-Submit: Maksim Sadym <sadym@chromium.org>
Cr-Commit-Position: refs/heads/main@{#80296}

include/js_protocol.pdl

@@ -1347,7 +1347,9 @@ domain Runtime
       optional string objectGroup
       # Whether to throw an exception if side effect cannot be ruled out during evaluation.
       experimental optional boolean throwOnSideEffect
-      # Whether the result should be serialized according to https://w3c.github.io/webdriver-bidi.
+      # Whether the result should contain `webDriverValue`, serialized according to
+      # https://w3c.github.io/webdriver-bidi. This is mutually exclusive with `returnByValue`, but
+      # resulting `objectId` is still provided.
       experimental optional boolean generateWebDriverValue
     returns
       # Call result.

include/v8-inspector.h

@@ -207,10 +207,10 @@ class V8_EXPORT V8InspectorSession {
 
 class V8_EXPORT WebDriverValue {
  public:
-  explicit WebDriverValue(StringView type, v8::MaybeLocal<v8::Value> value = {})
-      : type(type), value(value) {}
-
-  StringView type;
+  explicit WebDriverValue(std::unique_ptr<StringBuffer> type,
+                          v8::MaybeLocal<v8::Value> value = {})
+      : type(std::move(type)), value(value) {}
+  std::unique_ptr<StringBuffer> type;
   v8::MaybeLocal<v8::Value> value;
 };
 

src/inspector/injected-script.cc

@@ -576,8 +576,8 @@ Response InjectedScript::wrapObjectMirror(
   }
   if (wrapMode == WrapMode::kGenerateWebDriverValue) {
     int maxDepth = 1;
-    std::unique_ptr<protocol::Runtime::WebDriverValue> webDriverValue;
-    response = mirror.buildWebDriverValue(context, maxDepth, &webDriverValue);
+    std::unique_ptr<protocol::Runtime::WebDriverValue> webDriverValue =
+        mirror.buildWebDriverValue(context, maxDepth);
     if (!response.IsSuccess()) return response;
     (*result)->setWebDriverValue(std::move(webDriverValue));
   }

src/inspector/v8-webdriver-serializer.h

@@ -15,10 +15,9 @@
 namespace v8_inspector {
 class V8WebDriverSerializer {
  public:
-  static protocol::Response serializeV8Value(
+  static std::unique_ptr<protocol::Runtime::WebDriverValue> serializeV8Value(
       v8::Local<v8::Object> value, v8::Local<v8::Context> context,
-      int max_depth,
-      std::unique_ptr<protocol::Runtime::WebDriverValue>* result);
+      int max_depth);
 };
 }  // namespace v8_inspector
 

src/inspector/value-mirror.cc

@@ -439,44 +439,40 @@ class PrimitiveValueMirror final : public ValueMirror {
       (*preview)->setSubtype(RemoteObject::SubtypeEnum::Null);
   }
 
-  protocol::Response buildWebDriverValue(
-      v8::Local<v8::Context> context, int max_depth,
-      std::unique_ptr<protocol::Runtime::WebDriverValue>* result)
-      const override {
+  std::unique_ptr<protocol::Runtime::WebDriverValue> buildWebDriverValue(
+      v8::Local<v8::Context> context, int max_depth) const override {
     // https://w3c.github.io/webdriver-bidi/#data-types-protocolValue-primitiveProtocolValue-serialization
 
     if (m_value->IsUndefined()) {
-      *result =
-          protocol::Runtime::WebDriverValue::create()
+      return protocol::Runtime::WebDriverValue::create()
           .setType(protocol::Runtime::WebDriverValue::TypeEnum::Undefined)
           .build();
-      return Response::Success();
     }
     if (m_value->IsNull()) {
-      *result = protocol::Runtime::WebDriverValue::create()
+      return protocol::Runtime::WebDriverValue::create()
           .setType(protocol::Runtime::WebDriverValue::TypeEnum::Null)
           .build();
-      return Response::Success();
     }
     if (m_value->IsString()) {
-      *result =
-          protocol::Runtime::WebDriverValue::create()
+      return protocol::Runtime::WebDriverValue::create()
           .setType(protocol::Runtime::WebDriverValue::TypeEnum::String)
           .setValue(protocol::StringValue::create(toProtocolString(
               context->GetIsolate(), m_value.As<v8::String>())))
           .build();
-      return Response::Success();
     }
     if (m_value->IsBoolean()) {
-      *result =
-          protocol::Runtime::WebDriverValue::create()
+      return protocol::Runtime::WebDriverValue::create()
           .setType(protocol::Runtime::WebDriverValue::TypeEnum::Boolean)
           .setValue(protocol::FundamentalValue::create(
               m_value.As<v8::Boolean>()->Value()))
           .build();
-      return Response::Success();
     }
-    return Response::ServerError("unexpected primitive type");
+
+    DCHECK(false);
+    // Fallback.
+    return protocol::Runtime::WebDriverValue::create()
+        .setType(protocol::Runtime::WebDriverValue::TypeEnum::Object)
+        .build();
   }
 
  private:
@@ -529,23 +525,21 @@ class NumberMirror final : public ValueMirror {
             .build();
   }
 
-  protocol::Response buildWebDriverValue(
-      v8::Local<v8::Context> context, int max_depth,
-      std::unique_ptr<protocol::Runtime::WebDriverValue>* result)
-      const override {
+  std::unique_ptr<protocol::Runtime::WebDriverValue> buildWebDriverValue(
+      v8::Local<v8::Context> context, int max_depth) const override {
     // https://w3c.github.io/webdriver-bidi/#data-types-protocolValue-primitiveProtocolValue-serialization
-    *result = protocol::Runtime::WebDriverValue::create()
+    std::unique_ptr<protocol::Runtime::WebDriverValue> result =
+        protocol::Runtime::WebDriverValue::create()
             .setType(protocol::Runtime::WebDriverValue::TypeEnum::Number)
             .build();
-
     bool unserializable = false;
     String16 descriptionValue = description(&unserializable);
     if (unserializable) {
-      (*result)->setValue(protocol::StringValue::create(descriptionValue));
+      result->setValue(protocol::StringValue::create(descriptionValue));
     } else {
-      (*result)->setValue(toProtocolValue(m_value.As<v8::Number>()->Value()));
+      result->setValue(toProtocolValue(m_value.As<v8::Number>()->Value()));
     }
-    return Response::Success();
+    return result;
   }
 
  private:
@@ -608,18 +602,15 @@ class BigIntMirror final : public ValueMirror {
 
   v8::Local<v8::Value> v8Value() const override { return m_value; }
 
-  protocol::Response buildWebDriverValue(
-      v8::Local<v8::Context> context, int max_depth,
-      std::unique_ptr<protocol::Runtime::WebDriverValue>* result)
-      const override {
+  std::unique_ptr<protocol::Runtime::WebDriverValue> buildWebDriverValue(
+      v8::Local<v8::Context> context, int max_depth) const override {
     // https://w3c.github.io/webdriver-bidi/#data-types-protocolValue-primitiveProtocolValue-serialization
 
-    *result = protocol::Runtime::WebDriverValue::create()
+    return protocol::Runtime::WebDriverValue::create()
         .setType(protocol::Runtime::WebDriverValue::TypeEnum::Bigint)
         .setValue(protocol::StringValue::create(
             descriptionForBigInt(context, m_value)))
         .build();
-    return Response::Success();
   }
 
  private:
@@ -658,15 +649,12 @@ class SymbolMirror final : public ValueMirror {
 
   v8::Local<v8::Value> v8Value() const override { return m_symbol; }
 
-  protocol::Response buildWebDriverValue(
-      v8::Local<v8::Context> context, int max_depth,
-      std::unique_ptr<protocol::Runtime::WebDriverValue>* result)
-      const override {
+  std::unique_ptr<protocol::Runtime::WebDriverValue> buildWebDriverValue(
+      v8::Local<v8::Context> context, int max_depth) const override {
     // https://w3c.github.io/webdriver-bidi/#data-types-protocolValue-RemoteValue-serialization
-    *result = protocol::Runtime::WebDriverValue::create()
+    return protocol::Runtime::WebDriverValue::create()
         .setType(protocol::Runtime::WebDriverValue::TypeEnum::Symbol)
         .build();
-    return Response::Success();
   }
 
  private:
@@ -713,14 +701,11 @@ class LocationMirror final : public ValueMirror {
   }
   v8::Local<v8::Value> v8Value() const override { return m_value; }
 
-  protocol::Response buildWebDriverValue(
-      v8::Local<v8::Context> context, int max_depth,
-      std::unique_ptr<protocol::Runtime::WebDriverValue>* result)
-      const override {
-    *result = protocol::Runtime::WebDriverValue::create()
+  std::unique_ptr<protocol::Runtime::WebDriverValue> buildWebDriverValue(
+      v8::Local<v8::Context> context, int max_depth) const override {
+    return protocol::Runtime::WebDriverValue::create()
         .setType(protocol::Runtime::WebDriverValue::TypeEnum::Object)
         .build();
-    return Response::Success();
   }
 
  private:
@@ -800,16 +785,12 @@ class FunctionMirror final : public ValueMirror {
             .build();
   }
 
-  protocol::Response buildWebDriverValue(
-      v8::Local<v8::Context> context, int max_depth,
-      std::unique_ptr<protocol::Runtime::WebDriverValue>* result)
-      const override {
+  std::unique_ptr<protocol::Runtime::WebDriverValue> buildWebDriverValue(
+      v8::Local<v8::Context> context, int max_depth) const override {
     // https://w3c.github.io/webdriver-bidi/#data-types-protocolValue-RemoteValue-serialization
-    *result =
-        protocol::Runtime::WebDriverValue::create()
+    return protocol::Runtime::WebDriverValue::create()
         .setType(protocol::Runtime::WebDriverValue::TypeEnum::Function)
         .build();
-    return Response::Success();
   }
 
  private:
@@ -1097,10 +1078,8 @@ class ObjectMirror final : public ValueMirror {
     if (m_hasSubtype) (*result)->setSubtype(m_subtype);
   }
 
-  protocol::Response buildWebDriverValue(
-      v8::Local<v8::Context> context, int max_depth,
-      std::unique_ptr<protocol::Runtime::WebDriverValue>* result)
-      const override {
+  std::unique_ptr<protocol::Runtime::WebDriverValue> buildWebDriverValue(
+      v8::Local<v8::Context> context, int max_depth) const override {
     // https://w3c.github.io/webdriver-bidi/#data-types-protocolValue-RemoteValue-serialization
 
     // Check if embedder implemented custom serialization.
@@ -1109,27 +1088,24 @@ class ObjectMirror final : public ValueMirror {
 
     if (embedder_serialized_result) {
       // Embedder-implemented serialization.
-      *result = protocol::Runtime::WebDriverValue::create()
-                    .setType(toString16(embedder_serialized_result->type))
+      std::unique_ptr<protocol::Runtime::WebDriverValue> result =
+          protocol::Runtime::WebDriverValue::create()
+              .setType(toString16(embedder_serialized_result->type->string()))
               .build();
 
-      if (!embedder_serialized_result->value.IsEmpty()) {
+      v8::Local<v8::Value> v8_value;
+      if (embedder_serialized_result->value.ToLocal(&v8_value)) {
         // Embedder-implemented serialization has value.
         std::unique_ptr<protocol::Value> protocol_value;
-        Response response = toProtocolValue(
-            context, embedder_serialized_result->value.ToLocalChecked(),
-            &protocol_value);
-        if (!response.IsSuccess()) return response;
-
-        (*result)->setValue(std::move(protocol_value));
+        Response response = toProtocolValue(context, v8_value, &protocol_value);
+        DCHECK(response.IsSuccess());
+        result->setValue(std::move(protocol_value));
       }
-      return Response::Success();
+      return result;
     }
 
     // No embedder-implemented serialization. Serialize as V8 Object.
-    Response response = V8WebDriverSerializer::serializeV8Value(
-        m_value, context, max_depth, result);
-    return response;
+    return V8WebDriverSerializer::serializeV8Value(m_value, context, max_depth);
   }
 
  private:

src/inspector/value-mirror.h

@@ -66,9 +66,8 @@ class ValueMirror {
       v8::Local<v8::Context> context, int* nameLimit, int* indexLimit,
       std::unique_ptr<protocol::Runtime::ObjectPreview>*) const {}
   virtual v8::Local<v8::Value> v8Value() const = 0;
-  virtual protocol::Response buildWebDriverValue(
-      v8::Local<v8::Context> context, int max_depth,
-      std::unique_ptr<protocol::Runtime::WebDriverValue>* result) const = 0;
+  virtual std::unique_ptr<protocol::Runtime::WebDriverValue>
+  buildWebDriverValue(v8::Local<v8::Context> context, int max_depth) const = 0;
 
   class PropertyAccumulator {
    public:

test/inspector/runtime/add-web-driver-value-expected.txt → test/inspector/runtime/generate-web-driver-value-expected.txt

@@ -193,7 +193,7 @@ Runtime.callFunctionOn
 }
 
 Running test: Array
-testing expression: [1,2]
+testing expression: [1,2,undefined]
 Runtime.evaluate
 {
     type : array
@@ -206,6 +206,9 @@ Runtime.evaluate
             type : number
             value : 2
         }
+        [2] : {
+            type : undefined
+        }
     ]
 }
 Runtime.callFunctionOn
@@ -220,9 +223,12 @@ Runtime.callFunctionOn
             type : number
             value : 2
         }
+        [2] : {
+            type : undefined
+        }
     ]
 }
-testing expression: new Array(1,2)
+testing expression: new Array(1,2,undefined)
 Runtime.evaluate
 {
     type : array
@@ -235,6 +241,9 @@ Runtime.evaluate
             type : number
             value : 2
         }
+        [2] : {
+            type : undefined
+        }
     ]
 }
 Runtime.callFunctionOn
@@ -249,6 +258,9 @@ Runtime.callFunctionOn
             type : number
             value : 2
         }
+        [2] : {
+            type : undefined
+        }
     ]
 }
 
@@ -399,7 +411,7 @@ Runtime.callFunctionOn
 }
 
 Running test: Set
-testing expression: new Set([{valueObject1: 1}, 'valueString2', new Array()])
+testing expression: new Set([{valueObject1: 1}, 'valueString2', new Array(), undefined])
 Runtime.evaluate
 {
     type : set
@@ -414,6 +426,9 @@ Runtime.evaluate
         [2] : {
             type : array
         }
+        [3] : {
+            type : undefined
+        }
     ]
 }
 Runtime.callFunctionOn
@@ -430,6 +445,9 @@ Runtime.callFunctionOn
         [2] : {
             type : array
         }
+        [3] : {
+            type : undefined
+        }
     ]
 }
 
@@ -489,7 +507,7 @@ Runtime.callFunctionOn
 }
 
 Running test: Object
-testing expression: {nullKey: null, stringKey: 'foo',boolKey: true,numberKey: 123,bigintKey: 123n,symbolKey: Symbol('foo'),functionKey: () => {},arrayKey:[1]}
+testing expression: {nullKey: null, stringKey: 'foo',boolKey: true,numberKey: 123,bigintKey: 123n,symbolKey: Symbol('foo'),functionKey: () => {},arrayKey:[1],undefinedKey:undefined}
 Runtime.evaluate
 {
     type : object
@@ -546,6 +564,12 @@ Runtime.evaluate
                 type : array
             }
         ]
+        [8] : [
+            [0] : undefinedKey
+            [1] : {
+                type : undefined
+            }
+        ]
     ]
 }
 Runtime.callFunctionOn
@@ -604,6 +628,12 @@ Runtime.callFunctionOn
                 type : array
             }
         ]
+        [8] : [
+            [0] : undefinedKey
+            [1] : {
+                type : undefined
+            }
+        ]
     ]
 }
 testing expression: {key_level_1: {key_level_2: {key_level_3: 'value_level_3'}}}

test/inspector/runtime/add-web-driver-value.js → test/inspector/runtime/generate-web-driver-value.js

-// Copyright 2018 the V8 project authors. All rights reserved.
+// Copyright 2022 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -28,8 +28,8 @@ InspectorTest.runAsyncTestSuite([
     await testExpression("[function qwe(){}, ()=>{}]");
   },
   async function Array() {
-    await testExpression("[1,2]");
-    await testExpression("new Array(1,2)");
+    await testExpression("[1,2,undefined]");
+    await testExpression("new Array(1,2,undefined)");
   },
   async function RegExp() {
     await testExpression("[new RegExp('ab+c'), new RegExp('ab+c', 'ig')]");
@@ -49,7 +49,7 @@ InspectorTest.runAsyncTestSuite([
     await testExpression("new WeakMap([[{valueObject1: 1}, 'keyString1'],[{valueObject2: 2}, 'keyString2']])");
   },
   async function Set() {
-    await testExpression("new Set([{valueObject1: 1}, 'valueString2', new Array()])");
+    await testExpression("new Set([{valueObject1: 1}, 'valueString2', new Array(), undefined])");
   },
   async function Weakset() {
     await testExpression("new WeakSet([{valueObject1: 1}, {valueObject2: 2}])");
@@ -68,7 +68,7 @@ InspectorTest.runAsyncTestSuite([
   },
   async function Object() {
     // Object.
-    await testExpression("{nullKey: null, stringKey: 'foo',boolKey: true,numberKey: 123,bigintKey: 123n,symbolKey: Symbol('foo'),functionKey: () => {},arrayKey:[1]}");
+    await testExpression("{nullKey: null, stringKey: 'foo',boolKey: true,numberKey: 123,bigintKey: 123n,symbolKey: Symbol('foo'),functionKey: () => {},arrayKey:[1],undefinedKey:undefined}");
     // Object in-depth serialization.
     await testExpression("{key_level_1: {key_level_2: {key_level_3: 'value_level_3'}}}");
  }]);