From c01dff8d8989c6015e4ab78f629451b6a372f666 Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Tue, 11 Apr 2023 02:57:48 -0700 Subject: [PATCH] fix: exceptions during function/promise result conversions live in calling world (#37904) --- .../api/electron_api_context_bridge.cc | 101 +++++++++++++++--- spec/api-context-bridge-spec.ts | 30 +++++- 2 files changed, 115 insertions(+), 16 deletions(-) diff --git a/shell/renderer/api/electron_api_context_bridge.cc b/shell/renderer/api/electron_api_context_bridge.cc index 36fe0346511..e84b9987ee4 100644 --- a/shell/renderer/api/electron_api_context_bridge.cc +++ b/shell/renderer/api/electron_api_context_bridge.cc @@ -241,10 +241,29 @@ v8::MaybeLocal 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 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 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 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& 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 ret; + v8::Local 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())); + } else { + args.isolate()->ThrowException(v8::Exception::Error(exception)); + } + return; + } + DCHECK(!ret.IsEmpty()); if (ret.IsEmpty()) return; info.GetReturnValue().Set(ret.ToLocalChecked()); diff --git a/spec/api-context-bridge-spec.ts b/spec/api-context-bridge-spec.ts index 606394b5647..b54fba85550 100644 --- a/spec/api-context-bridge-spec.ts +++ b/spec/api-context-bridge-spec.ts @@ -814,10 +814,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(); @@ -826,15 +837,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 () => {