fix: exceptions in nested conversions live in the target world (#37898)

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,10 @@ 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);
+          auto val = PassValueToOtherContext(
+              global_source_context.Get(isolate),
+              global_destination_context.Get(isolate), result, &object_cache,
+              false, 0, BridgeErrorTarget::kSource);
           if (!val.IsEmpty())
             proxied_promise->Resolve(val.ToLocalChecked());
         },
@@ -268,10 +268,10 @@ 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);
+          auto val = PassValueToOtherContext(
+              global_source_context.Get(isolate),
+              global_destination_context.Get(isolate), result, &object_cache,
+              false, 0, BridgeErrorTarget::kSource);
           if (!val.IsEmpty())
             proxied_promise->Reject(val.ToLocalChecked());
         },
@@ -324,7 +324,7 @@ v8::MaybeLocal<v8::Value> PassValueToOtherContext(
       auto value_for_array = PassValueToOtherContext(
           source_context, destination_context,
           arr->Get(source_context, i).ToLocalChecked(), object_cache,
-          support_dynamic_properties, recursion_depth + 1);
+          support_dynamic_properties, recursion_depth + 1, error_target);
       if (value_for_array.IsEmpty())
         return v8::MaybeLocal<v8::Value>();
 
@@ -358,7 +358,7 @@ v8::MaybeLocal<v8::Value> PassValueToOtherContext(
     auto object_value = value.As<v8::Object>();
     auto passed_value = CreateProxyForAPI(
         object_value, source_context, destination_context, object_cache,
-        support_dynamic_properties, recursion_depth + 1);
+        support_dynamic_properties, recursion_depth + 1, error_target);
     if (passed_value.IsEmpty())
       return v8::MaybeLocal<v8::Value>();
     return v8::MaybeLocal<v8::Value>(passed_value.ToLocalChecked());
@@ -372,8 +372,9 @@ v8::MaybeLocal<v8::Value> PassValueToOtherContext(
                                                    : destination_context;
     v8::Context::Scope error_scope(error_context);
     // V8 serializer will throw an error if required
-    if (!gin::ConvertFromV8(error_context->GetIsolate(), value, &ret))
+    if (!gin::ConvertFromV8(error_context->GetIsolate(), value, &ret)) {
       return v8::MaybeLocal<v8::Value>();
+    }
   }
 
   {
@@ -420,9 +421,9 @@ void ProxyFunctionWrapper(const v8::FunctionCallbackInfo<v8::Value>& info) {
     args.GetRemaining(&original_args);
 
     for (auto value : original_args) {
-      auto arg =
-          PassValueToOtherContext(calling_context, func_owning_context, value,
-                                  &object_cache, support_dynamic_properties, 0);
+      auto arg = PassValueToOtherContext(
+          calling_context, func_owning_context, value, &object_cache,
+          support_dynamic_properties, 0, BridgeErrorTarget::kSource);
       if (arg.IsEmpty())
         return;
       proxied_args.push_back(arg.ToLocalChecked());
@@ -485,7 +486,8 @@ v8::MaybeLocal<v8::Object> CreateProxyForAPI(
     const v8::Local<v8::Context>& destination_context,
     context_bridge::ObjectCache* object_cache,
     bool support_dynamic_properties,
-    int recursion_depth) {
+    int recursion_depth,
+    BridgeErrorTarget error_target) {
   gin_helper::Dictionary api(source_context->GetIsolate(), api_object);
 
   {
@@ -526,14 +528,16 @@ v8::MaybeLocal<v8::Object> CreateProxyForAPI(
             if (!getter.IsEmpty()) {
               if (!PassValueToOtherContext(source_context, destination_context,
                                            getter, object_cache,
-                                           support_dynamic_properties, 1)
+                                           support_dynamic_properties, 1,
+                                           error_target)
                        .ToLocal(&getter_proxy))
                 continue;
             }
             if (!setter.IsEmpty()) {
               if (!PassValueToOtherContext(source_context, destination_context,
                                            setter, object_cache,
-                                           support_dynamic_properties, 1)
+                                           support_dynamic_properties, 1,
+                                           error_target)
                        .ToLocal(&setter_proxy))
                 continue;
             }
@@ -551,7 +555,7 @@ v8::MaybeLocal<v8::Object> CreateProxyForAPI(
 
       auto passed_value = PassValueToOtherContext(
           source_context, destination_context, value, object_cache,
-          support_dynamic_properties, recursion_depth + 1);
+          support_dynamic_properties, recursion_depth + 1, error_target);
       if (passed_value.IsEmpty())
         return v8::MaybeLocal<v8::Object>();
       proxy.Set(key, passed_value.ToLocalChecked());
@@ -597,9 +601,9 @@ void ExposeAPIInWorld(v8::Isolate* isolate,
     context_bridge::ObjectCache object_cache;
     v8::Context::Scope target_context_scope(target_context);
 
-    v8::MaybeLocal<v8::Value> maybe_proxy =
-        PassValueToOtherContext(electron_isolated_context, target_context, api,
-                                &object_cache, false, 0);
+    v8::MaybeLocal<v8::Value> maybe_proxy = PassValueToOtherContext(
+        electron_isolated_context, target_context, api, &object_cache, false, 0,
+        BridgeErrorTarget::kSource);
     if (maybe_proxy.IsEmpty())
       return;
     auto proxy = maybe_proxy.ToLocalChecked();
@@ -649,7 +653,7 @@ void OverrideGlobalValueFromIsolatedWorld(
     context_bridge::ObjectCache object_cache;
     v8::MaybeLocal<v8::Value> maybe_proxy = PassValueToOtherContext(
         value->GetCreationContextChecked(), main_context, value, &object_cache,
-        support_dynamic_properties, 1);
+        support_dynamic_properties, 1, BridgeErrorTarget::kSource);
     DCHECK(!maybe_proxy.IsEmpty());
     auto proxy = maybe_proxy.ToLocalChecked();
 
@@ -685,14 +689,14 @@ bool OverrideGlobalPropertyFromIsolatedWorld(
     if (!getter->IsNullOrUndefined()) {
       v8::MaybeLocal<v8::Value> maybe_getter_proxy = PassValueToOtherContext(
           getter->GetCreationContextChecked(), main_context, getter,
-          &object_cache, false, 1);
+          &object_cache, false, 1, BridgeErrorTarget::kSource);
       DCHECK(!maybe_getter_proxy.IsEmpty());
       getter_proxy = maybe_getter_proxy.ToLocalChecked();
     }
     if (!setter->IsNullOrUndefined() && setter->IsObject()) {
       v8::MaybeLocal<v8::Value> maybe_setter_proxy = PassValueToOtherContext(
           getter->GetCreationContextChecked(), main_context, setter,
-          &object_cache, false, 1);
+          &object_cache, false, 1, BridgeErrorTarget::kSource);
       DCHECK(!maybe_setter_proxy.IsEmpty());
       setter_proxy = maybe_setter_proxy.ToLocalChecked();
     }

shell/renderer/api/electron_api_context_bridge.h

@@ -39,7 +39,7 @@ v8::MaybeLocal<v8::Value> PassValueToOtherContext(
     context_bridge::ObjectCache* object_cache,
     bool support_dynamic_properties,
     int recursion_depth,
-    BridgeErrorTarget error_target = BridgeErrorTarget::kSource);
+    BridgeErrorTarget error_target);
 
 v8::MaybeLocal<v8::Object> CreateProxyForAPI(
     const v8::Local<v8::Object>& api_object,
@@ -47,7 +47,8 @@ v8::MaybeLocal<v8::Object> CreateProxyForAPI(
     const v8::Local<v8::Context>& destination_context,
     context_bridge::ObjectCache* object_cache,
     bool support_dynamic_properties,
-    int recursion_depth);
+    int recursion_depth,
+    BridgeErrorTarget error_target);
 
 }  // namespace electron::api
 

shell/renderer/api/electron_api_web_frame.cc

@@ -142,7 +142,7 @@ class ScriptExecutionCallback {
       context_bridge::ObjectCache object_cache;
       maybe_result = PassValueToOtherContext(
           result->GetCreationContextChecked(), promise_.GetContext(), result,
-          &object_cache, false, 0);
+          &object_cache, false, 0, BridgeErrorTarget::kSource);
       if (maybe_result.IsEmpty() || try_catch.HasCaught()) {
         success = false;
       }

shell/renderer/renderer_client_base.cc

@@ -608,7 +608,8 @@ void RendererClientBase::SetupMainWorldOverrides(
   if (global.GetHidden("guestViewInternal", &guest_view_internal)) {
     api::context_bridge::ObjectCache object_cache;
     auto result = api::PassValueToOtherContext(
-        source_context, context, guest_view_internal, &object_cache, false, 0);
+        source_context, context, guest_view_internal, &object_cache, false, 0,
+        api::BridgeErrorTarget::kSource);
     if (!result.IsEmpty()) {
       isolated_api.Set("guestViewInternal", result.ToLocalChecked());
     }

spec/api-context-bridge-spec.ts

@@ -802,6 +802,14 @@ describe('contextBridge', () => {
             throwNotClonable: () => {
               return Object(Symbol('foo'));
             },
+            throwNotClonableNestedArray: () => {
+              return [Object(Symbol('foo'))];
+            },
+            throwNotClonableNestedObject: () => {
+              return {
+                bad: Object(Symbol('foo'))
+              };
+            },
             argumentConvert: () => {}
           });
         });
@@ -817,10 +825,12 @@ describe('contextBridge', () => {
           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 argumentConvertIsError = Object.getPrototypeOf(getError(() => root.example.argumentConvert(Object(Symbol('test'))))) === Error.prototype;
-          return [normalIsError, weirdIsError, notClonableIsError, argumentConvertIsError];
+          return [normalIsError, weirdIsError, notClonableIsError, notClonableNestedArrayIsError, notClonableNestedObjectIsError, argumentConvertIsError];
         });
-        expect(result).to.deep.equal([true, true, true, true], 'should all be errors in the current context');
+        expect(result).to.deep.equal([true, true, true, true, true, true], 'should all be errors in the current context');
       });
 
       it('should not leak prototypes', async () => {