fix: exceptions during function/promise result conversions live in calling world (#37924)

Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com>
Co-authored-by: Samuel Attard <[email protected]>

shell/renderer/api/electron_api_context_bridge.cc

@@ -241,10 +241,29 @@ v8::MaybeLocal<v8::Value> PassValueToOtherContext(
               global_destination_context.IsEmpty())
             return;
           context_bridge::ObjectCache object_cache;
-          auto val = PassValueToOtherContext(
-              global_source_context.Get(isolate),
-              global_destination_context.Get(isolate), result, &object_cache,
-              false, 0, BridgeErrorTarget::kSource);
+          v8::MaybeLocal<v8::Value> val;
+          {
+            v8::TryCatch try_catch(isolate);
+            val = PassValueToOtherContext(
+                global_source_context.Get(isolate),
+                global_destination_context.Get(isolate), result, &object_cache,
+                false, 0, BridgeErrorTarget::kDestination);
+            if (try_catch.HasCaught()) {
+              if (try_catch.Message().IsEmpty()) {
+                proxied_promise->RejectWithErrorMessage(
+                    "An error was thrown while sending a promise result over "
+                    "the context bridge but it was not actually an Error "
+                    "object. This normally means that a promise was resolved "
+                    "with a value that is not supported by the Context "
+                    "Bridge.");
+              } else {
+                proxied_promise->Reject(
+                    v8::Exception::Error(try_catch.Message()->Get()));
+              }
+              return;
+            }
+          }
+          DCHECK(!val.IsEmpty());
           if (!val.IsEmpty())
             proxied_promise->Resolve(val.ToLocalChecked());
         },
@@ -268,10 +287,28 @@ v8::MaybeLocal<v8::Value> PassValueToOtherContext(
               global_destination_context.IsEmpty())
             return;
           context_bridge::ObjectCache object_cache;
-          auto val = PassValueToOtherContext(
-              global_source_context.Get(isolate),
-              global_destination_context.Get(isolate), result, &object_cache,
-              false, 0, BridgeErrorTarget::kSource);
+          v8::MaybeLocal<v8::Value> val;
+          {
+            v8::TryCatch try_catch(isolate);
+            val = PassValueToOtherContext(
+                global_source_context.Get(isolate),
+                global_destination_context.Get(isolate), result, &object_cache,
+                false, 0, BridgeErrorTarget::kDestination);
+            if (try_catch.HasCaught()) {
+              if (try_catch.Message().IsEmpty()) {
+                proxied_promise->RejectWithErrorMessage(
+                    "An error was thrown while sending a promise rejection "
+                    "over the context bridge but it was not actually an Error "
+                    "object. This normally means that a promise was rejected "
+                    "with a value that is not supported by the Context "
+                    "Bridge.");
+              } else {
+                proxied_promise->Reject(
+                    v8::Exception::Error(try_catch.Message()->Get()));
+              }
+              return;
+            }
+          }
           if (!val.IsEmpty())
             proxied_promise->Reject(val.ToLocalChecked());
         },
@@ -470,10 +507,50 @@ void ProxyFunctionWrapper(const v8::FunctionCallbackInfo<v8::Value>& info) {
     if (maybe_return_value.IsEmpty())
       return;
 
-    auto ret = PassValueToOtherContext(
-        func_owning_context, calling_context,
-        maybe_return_value.ToLocalChecked(), &object_cache,
-        support_dynamic_properties, 0, BridgeErrorTarget::kDestination);
+    // In the case where we encounted an exception converting the return value
+    // of the function we need to ensure that the exception / thrown value is
+    // safely transferred from the function_owning_context (where it was thrown)
+    // into the calling_context (where it needs to be thrown) To do this we pull
+    // the message off the exception and later re-throw it in the right context.
+    // In some cases the caught thing is not an exception i.e. it's technically
+    // valid to `throw 123`.  In these cases to avoid infinite
+    // PassValueToOtherContext recursion we bail early as being unable to send
+    // the value from one context to the other.
+    // TODO(MarshallOfSound): In this case and other cases where the error can't
+    // be sent _across_ worlds we should probably log it globally in some way to
+    // allow easier debugging.  This is not trivial though so is left to a
+    // future change.
+    bool did_error_converting_result = false;
+    v8::MaybeLocal<v8::Value> ret;
+    v8::Local<v8::String> exception;
+    {
+      v8::TryCatch try_catch(args.isolate());
+      ret = PassValueToOtherContext(func_owning_context, calling_context,
+                                    maybe_return_value.ToLocalChecked(),
+                                    &object_cache, support_dynamic_properties,
+                                    0, BridgeErrorTarget::kDestination);
+      if (try_catch.HasCaught()) {
+        did_error_converting_result = true;
+        if (!try_catch.Message().IsEmpty()) {
+          exception = try_catch.Message()->Get();
+        }
+      }
+    }
+    if (did_error_converting_result) {
+      v8::Context::Scope calling_context_scope(calling_context);
+      if (exception.IsEmpty()) {
+        const char err_msg[] =
+            "An unknown exception occurred while sending a function return "
+            "value over the context bridge, an error "
+            "occurred but a valid exception was not thrown.";
+        args.isolate()->ThrowException(v8::Exception::Error(
+            gin::StringToV8(args.isolate(), err_msg).As<v8::String>()));
+      } else {
+        args.isolate()->ThrowException(v8::Exception::Error(exception));
+      }
+      return;
+    }
+    DCHECK(!ret.IsEmpty());
     if (ret.IsEmpty())
       return;
     info.GetReturnValue().Set(ret.ToLocalChecked());

spec/api-context-bridge-spec.ts

@@ -810,10 +810,21 @@ describe('contextBridge', () => {
                 bad: Object(Symbol('foo'))
               };
             },
-            argumentConvert: () => {}
+            throwDynamic: () => {
+              return {
+                get bad () {
+                  throw new Error('damm');
+                }
+              };
+            },
+            argumentConvert: () => {},
+            rejectNotClonable: async () => {
+              throw Object(Symbol('foo'));
+            },
+            resolveNotClonable: async () => Object(Symbol('foo'))
           });
         });
-        const result = await callWithBindings((root: any) => {
+        const result = await callWithBindings(async (root: any) => {
           const getError = (fn: Function) => {
             try {
               fn();
@@ -822,15 +833,26 @@ describe('contextBridge', () => {
             }
             return null;
           };
+          const getAsyncError = async (fn: Function) => {
+            try {
+              await fn();
+            } catch (e) {
+              return e;
+            }
+            return null;
+          };
           const normalIsError = Object.getPrototypeOf(getError(root.example.throwNormal)) === Error.prototype;
           const weirdIsError = Object.getPrototypeOf(getError(root.example.throwWeird)) === Error.prototype;
           const notClonableIsError = Object.getPrototypeOf(getError(root.example.throwNotClonable)) === Error.prototype;
           const notClonableNestedArrayIsError = Object.getPrototypeOf(getError(root.example.throwNotClonableNestedArray)) === Error.prototype;
           const notClonableNestedObjectIsError = Object.getPrototypeOf(getError(root.example.throwNotClonableNestedObject)) === Error.prototype;
+          const dynamicIsError = Object.getPrototypeOf(getError(root.example.throwDynamic)) === Error.prototype;
           const argumentConvertIsError = Object.getPrototypeOf(getError(() => root.example.argumentConvert(Object(Symbol('test'))))) === Error.prototype;
-          return [normalIsError, weirdIsError, notClonableIsError, notClonableNestedArrayIsError, notClonableNestedObjectIsError, argumentConvertIsError];
+          const rejectNotClonableIsError = Object.getPrototypeOf(await getAsyncError(root.example.rejectNotClonable)) === Error.prototype;
+          const resolveNotClonableIsError = Object.getPrototypeOf(await getAsyncError(root.example.resolveNotClonable)) === Error.prototype;
+          return [normalIsError, weirdIsError, notClonableIsError, notClonableNestedArrayIsError, notClonableNestedObjectIsError, dynamicIsError, argumentConvertIsError, rejectNotClonableIsError, resolveNotClonableIsError];
         });
-        expect(result).to.deep.equal([true, true, true, true, true, true], 'should all be errors in the current context');
+        expect(result).to.deep.equal([true, true, true, true, true, true, true, true, true], 'should all be errors in the current context');
       });
 
       it('should not leak prototypes', async () => {