Bindings: Early exit in value->IDBKey conversion after exception

In cases where an array is being processed to generate a key (either a
JS array, or an array key path), if an exception occurs the conversion
needs to terminate immediately so that the exception can be delivered
to script.

Add early exits in a couple missing places, and add tests. Note that
these cases are not reachable from script, because the conversion code
is defined to run on "structured clones" of input data, so any getters
that throw (for example) will have been filtered out by the previous
serialize/deserialize process producing the clone.

Change-Id: I53576eb1d47d23b0be4b712bc12e9de47078a419
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1777060
Commit-Queue: Joshua Bell <jsbell@chromium.org>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#692282}

third_party/blink/renderer/bindings/modules/v8/v8_binding_for_modules.cc

@@ -182,6 +182,8 @@ static const size_t kMaximumDepth = 1000;
 // returned but with an 'Invalid' entry; this is used for "multi-entry"
 // indexes where an array with invalid members is permitted will later be
 // sanitized.
+// A V8 exception may be thrown on bad data or by script's getters; if so,
+// callers should not make further V8 calls.
 static std::unique_ptr<IDBKey> CreateIDBKeyFromValue(
     v8::Isolate* isolate,
     v8::Local<v8::Value> value,
@@ -245,6 +247,10 @@ static std::unique_ptr<IDBKey> CreateIDBKeyFromValue(
       }
       std::unique_ptr<IDBKey> subkey =
           CreateIDBKeyFromValue(isolate, item, stack, exception_state);
+      if (exception_state.HadException()) {
+        exception_state.RethrowV8Exception(block.Exception());
+        return nullptr;
+      }
       if (!subkey)
         subkeys.emplace_back(IDBKey::CreateInvalid());
       else
@@ -265,6 +271,8 @@ static std::unique_ptr<IDBKey> CreateIDBKeyFromValue(
 // sanitized. This is used to implement both of the following spec algorithms:
 // https://w3c.github.io/IndexedDB/#convert-value-to-key
 // https://w3c.github.io/IndexedDB/#convert-a-value-to-a-multientry-key
+// A V8 exception may be thrown on bad data or by script's getters; if so,
+// callers should not make further V8 calls.
 static std::unique_ptr<IDBKey> CreateIDBKeyFromValue(
     v8::Isolate* isolate,
     v8::Local<v8::Value> value,
@@ -310,6 +318,8 @@ static Vector<String> ParseKeyPath(const String& key_path) {
 // to a key is representing by returning an 'Invalid' key. (An array with
 // invalid members will be returned if needed.)
 // https://w3c.github.io/IndexedDB/#evaluate-a-key-path-on-a-value
+// A V8 exception may be thrown on bad data or by script's getters; if so,
+// callers should not make further V8 calls.
 static std::unique_ptr<IDBKey> CreateIDBKeyFromValueAndKeyPath(
     v8::Isolate* isolate,
     v8::Local<v8::Value> v8_value,
@@ -397,6 +407,8 @@ static std::unique_ptr<IDBKey> CreateIDBKeyFromValueAndKeyPath(
 // paths, nullptr is returned if evaluation of any sub-path fails, otherwise
 // an array key is returned (with potentially 'Invalid' members).
 // https://w3c.github.io/IndexedDB/#evaluate-a-key-path-on-a-value
+// A V8 exception may be thrown on bad data or by script's getters; if so,
+// callers should not make further V8 calls.
 static std::unique_ptr<IDBKey> CreateIDBKeyFromValueAndKeyPath(
     v8::Isolate* isolate,
     v8::Local<v8::Value> value,
@@ -411,6 +423,8 @@ static std::unique_ptr<IDBKey> CreateIDBKeyFromValueAndKeyPath(
     for (const String& path : array) {
       auto key = CreateIDBKeyFromValueAndKeyPath(isolate, value, path,
                                                  exception_state);
+      if (exception_state.HadException())
+        return nullptr;
       // Evaluation of path failed - overall failure.
       if (!key)
         return nullptr;

third_party/blink/renderer/bindings/modules/v8/v8_binding_for_modules.h

@@ -60,9 +60,8 @@ struct NativeValueTraits<std::unique_ptr<IDBKey>> {
   // Note that an Array key may contain Invalid members, as the "multi-entry"
   // index case allows these, and will filter them out later. Use IsValid() to
   // recursively check.
-  static std::unique_ptr<IDBKey> NativeValue(v8::Isolate*,
-                                             v8::Local<v8::Value>,
-                                             ExceptionState&);
+  MODULES_EXPORT static std::unique_ptr<IDBKey>
+  NativeValue(v8::Isolate*, v8::Local<v8::Value>, ExceptionState&);
 
   // Implementation for ScriptValue::To<std::unique_ptr<IDBKey>>().
   //

third_party/blink/renderer/bindings/modules/v8/v8_binding_for_modules_test.cc

@@ -48,6 +48,15 @@ namespace blink {
 
 namespace {
 
+v8::Local<v8::Object> EvaluateScriptAsObject(V8TestingScope& scope,
+                                             const char* source) {
+  v8::Local<v8::Script> script =
+      v8::Script::Compile(scope.GetContext(),
+                          V8String(scope.GetIsolate(), source))
+          .ToLocalChecked();
+  return script->Run(scope.GetContext()).ToLocalChecked().As<v8::Object>();
+}
+
 std::unique_ptr<IDBKey> CheckKeyFromValueAndKeyPathInternal(
     v8::Isolate* isolate,
     const ScriptValue& value,
@@ -227,6 +236,71 @@ TEST(IDBKeyFromValueAndKeyPathTest, SubProperty) {
   CheckKeyPathNullValue(isolate, script_value, "bar");
 }
 
+TEST(IDBKeyFromValue, Exceptions) {
+  V8TestingScope scope;
+  {
+    // Value is an array with a getter that throws.
+    ScriptValue script_value(
+        scope.GetScriptState(),
+        EvaluateScriptAsObject(
+            scope,
+            "(()=>{"
+            "  const a = [0, 1, 2];"
+            "  Object.defineProperty(a, 1, {get: () => { throw Error(); }});"
+            "  return a;"
+            "})()"));
+
+    DummyExceptionStateForTesting exception_state;
+    auto key = ScriptValue::To<std::unique_ptr<IDBKey>>(
+        scope.GetIsolate(), script_value, exception_state);
+    EXPECT_FALSE(key->IsValid());
+    EXPECT_TRUE(exception_state.HadException());
+  }
+  {
+    // Value is an array containing an array with a getter that throws.
+    ScriptValue script_value(
+        scope.GetScriptState(),
+        EvaluateScriptAsObject(
+            scope,
+            "(()=>{"
+            "  const a = [0, 1, 2];"
+            "  Object.defineProperty(a, 1, {get: () => { throw Error(); }});"
+            "  return ['x', a, 'z'];"
+            "})()"));
+
+    DummyExceptionStateForTesting exception_state;
+    auto key = ScriptValue::To<std::unique_ptr<IDBKey>>(
+        scope.GetIsolate(), script_value, exception_state);
+    EXPECT_FALSE(key->IsValid());
+    EXPECT_TRUE(exception_state.HadException());
+  }
+}
+
+TEST(IDBKeyFromValueAndKeyPathTest, Exceptions) {
+  V8TestingScope scope;
+  ScriptValue script_value(
+      scope.GetScriptState(),
+      EvaluateScriptAsObject(scope,
+                             "({id:1, get throws() { throw Error(); }})"));
+  {
+    // Key path references a property that throws.
+    DummyExceptionStateForTesting exception_state;
+    EXPECT_FALSE(ScriptValue::To<std::unique_ptr<IDBKey>>(
+        scope.GetIsolate(), script_value, exception_state,
+        IDBKeyPath("throws")));
+    EXPECT_TRUE(exception_state.HadException());
+  }
+
+  {
+    // Compound key path references a property that throws.
+    DummyExceptionStateForTesting exception_state;
+    EXPECT_FALSE(ScriptValue::To<std::unique_ptr<IDBKey>>(
+        scope.GetIsolate(), script_value, exception_state,
+        IDBKeyPath(Vector<String>{"id", "throws"})));
+    EXPECT_TRUE(exception_state.HadException());
+  }
+}
+
 TEST(InjectIDBKeyTest, ImplicitValues) {
   V8TestingScope scope;
   v8::Isolate* isolate = scope.GetIsolate();