Skip to content

Commit

Permalink
fix: exceptions during function/promise result conversions live in ca…
Browse files Browse the repository at this point in the history 
…lling world (#37925)

Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com>
Co-authored-by: Samuel Attard <marshallofsound@electronjs.org>
  • Loading branch information
@trop @MarshallOfSound
trop[bot] and MarshallOfSound committed Apr 11, 2023
1 parent 6f9cc3c commit 95729e6
Show file tree
Hide file tree
Showing 2 changed files with 115 additions and 16 deletions.
101 changes: 89 additions & 12 deletions shell/renderer/api/electron_api_context_bridge.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
},
Expand All @@ -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());
},
Expand Down Expand Up @@ -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());
Expand Down
30 changes: 26 additions & 4 deletions spec/api-context-bridge-spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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 () => {
Expand Down

0 comments on commit 95729e6

Please sign in to comment.