Skip to content

Commit

Permalink
fix: correctly track receiver for methods called via ctx bridge (elec…
Browse files Browse the repository at this point in the history 
…tron#39978)

* fix: correctly track receiver for methods called via ctx bridge

* spec: test for correct contextBridge passage

---------

Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
  • Loading branch information
@MarshallOfSound @codebytere @felixrieseberg
2 people authored and felixrieseberg committed Oct 25, 2023
1 parent 1790166 commit d45f829
Show file tree
Hide file tree
Showing 5 changed files with 91 additions and 48 deletions.
77 changes: 50 additions & 27 deletions shell/renderer/api/electron_api_context_bridge.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ namespace api {
namespace context_bridge {

const char kProxyFunctionPrivateKey[] = "electron_contextBridge_proxy_fn";
const char kProxyFunctionReceiverPrivateKey[] =
"electron_contextBridge_proxy_fn_receiver";
const char kSupportsDynamicPropertiesPrivateKey[] =
"electron_contextBridge_supportsDynamicProperties";
const char kOriginalFunctionPrivateKey[] = "electron_contextBridge_original_fn";
Expand Down Expand Up @@ -138,6 +140,7 @@ v8::MaybeLocal<v8::Value> PassValueToOtherContext(
v8::Local<v8::Context> source_context,
v8::Local<v8::Context> destination_context,
v8::Local<v8::Value> value,
v8::Local<v8::Value> parent_value,
context_bridge::ObjectCache* object_cache,
bool support_dynamic_properties,
int recursion_depth,
Expand Down Expand Up @@ -199,6 +202,9 @@ v8::MaybeLocal<v8::Value> PassValueToOtherContext(
v8::Object::New(destination_context->GetIsolate());
SetPrivate(destination_context, state,
context_bridge::kProxyFunctionPrivateKey, func);
SetPrivate(destination_context, state,
context_bridge::kProxyFunctionReceiverPrivateKey,
parent_value);
SetPrivate(destination_context, state,
context_bridge::kSupportsDynamicPropertiesPrivateKey,
gin::ConvertToV8(destination_context->GetIsolate(),
Expand Down Expand Up @@ -246,10 +252,12 @@ v8::MaybeLocal<v8::Value> PassValueToOtherContext(
v8::MaybeLocal<v8::Value> val;
{
v8::TryCatch try_catch(isolate);
v8::Local<v8::Context> source_context =
global_source_context.Get(isolate);
val = PassValueToOtherContext(
global_source_context.Get(isolate),
global_destination_context.Get(isolate), result, &object_cache,
false, 0, BridgeErrorTarget::kDestination);
source_context, global_destination_context.Get(isolate), result,
source_context->Global(), &object_cache, false, 0,
BridgeErrorTarget::kDestination);
if (try_catch.HasCaught()) {
if (try_catch.Message().IsEmpty()) {
proxied_promise->RejectWithErrorMessage(
Expand Down Expand Up @@ -292,10 +300,12 @@ v8::MaybeLocal<v8::Value> PassValueToOtherContext(
v8::MaybeLocal<v8::Value> val;
{
v8::TryCatch try_catch(isolate);
v8::Local<v8::Context> source_context =
global_source_context.Get(isolate);
val = PassValueToOtherContext(
global_source_context.Get(isolate),
global_destination_context.Get(isolate), result, &object_cache,
false, 0, BridgeErrorTarget::kDestination);
source_context, global_destination_context.Get(isolate), result,
source_context->Global(), &object_cache, false, 0,
BridgeErrorTarget::kDestination);
if (try_catch.HasCaught()) {
if (try_catch.Message().IsEmpty()) {
proxied_promise->RejectWithErrorMessage(
Expand Down Expand Up @@ -362,7 +372,7 @@ v8::MaybeLocal<v8::Value> PassValueToOtherContext(
for (size_t i = 0; i < length; i++) {
auto value_for_array = PassValueToOtherContext(
source_context, destination_context,
arr->Get(source_context, i).ToLocalChecked(), object_cache,
arr->Get(source_context, i).ToLocalChecked(), value, object_cache,
support_dynamic_properties, recursion_depth + 1, error_target);
if (value_for_array.IsEmpty())
return v8::MaybeLocal<v8::Value>();
Expand Down Expand Up @@ -442,8 +452,10 @@ void ProxyFunctionWrapper(const v8::FunctionCallbackInfo<v8::Value>& info) {
context_bridge::kSupportsDynamicPropertiesPrivateKey);
v8::MaybeLocal<v8::Value> maybe_func = GetPrivate(
calling_context, data, context_bridge::kProxyFunctionPrivateKey);
v8::MaybeLocal<v8::Value> maybe_recv = GetPrivate(
calling_context, data, context_bridge::kProxyFunctionReceiverPrivateKey);
v8::Local<v8::Value> func_value;
if (sdp_value.IsEmpty() || maybe_func.IsEmpty() ||
if (sdp_value.IsEmpty() || maybe_func.IsEmpty() || maybe_recv.IsEmpty() ||
!gin::ConvertFromV8(args.isolate(), sdp_value.ToLocalChecked(),
&support_dynamic_properties) ||
!maybe_func.ToLocal(&func_value))
Expand All @@ -463,8 +475,9 @@ void ProxyFunctionWrapper(const v8::FunctionCallbackInfo<v8::Value>& info) {

for (auto value : original_args) {
auto arg = PassValueToOtherContext(
calling_context, func_owning_context, value, &object_cache,
support_dynamic_properties, 0, BridgeErrorTarget::kSource);
calling_context, func_owning_context, value,
calling_context->Global(), &object_cache, support_dynamic_properties,
0, BridgeErrorTarget::kSource);
if (arg.IsEmpty())
return;
proxied_args.push_back(arg.ToLocalChecked());
Expand All @@ -475,8 +488,9 @@ void ProxyFunctionWrapper(const v8::FunctionCallbackInfo<v8::Value>& info) {
v8::Local<v8::Value> error_message;
{
v8::TryCatch try_catch(args.isolate());
maybe_return_value = func->Call(func_owning_context, func,
proxied_args.size(), proxied_args.data());
maybe_return_value =
func->Call(func_owning_context, maybe_recv.ToLocalChecked(),
proxied_args.size(), proxied_args.data());
if (try_catch.HasCaught()) {
did_error = true;
v8::Local<v8::Value> exception = try_catch.Exception();
Expand Down Expand Up @@ -531,6 +545,7 @@ void ProxyFunctionWrapper(const v8::FunctionCallbackInfo<v8::Value>& info) {
v8::TryCatch try_catch(args.isolate());
ret = PassValueToOtherContext(func_owning_context, calling_context,
maybe_return_value.ToLocalChecked(),
func_owning_context->Global(),
&object_cache, support_dynamic_properties,
0, BridgeErrorTarget::kDestination);
if (try_catch.HasCaught()) {
Expand Down Expand Up @@ -607,18 +622,18 @@ v8::MaybeLocal<v8::Object> CreateProxyForAPI(
v8::Local<v8::Value> getter_proxy;
v8::Local<v8::Value> setter_proxy;
if (!getter.IsEmpty()) {
if (!PassValueToOtherContext(source_context, destination_context,
getter, object_cache,
support_dynamic_properties, 1,
error_target)
if (!PassValueToOtherContext(
source_context, destination_context, getter,
api.GetHandle(), object_cache,
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,
error_target)
if (!PassValueToOtherContext(
source_context, destination_context, setter,
api.GetHandle(), object_cache,
support_dynamic_properties, 1, error_target)
.ToLocal(&setter_proxy))
continue;
}
Expand All @@ -635,8 +650,9 @@ v8::MaybeLocal<v8::Object> CreateProxyForAPI(
continue;

auto passed_value = PassValueToOtherContext(
source_context, destination_context, value, object_cache,
support_dynamic_properties, recursion_depth + 1, error_target);
source_context, destination_context, value, api.GetHandle(),
object_cache, support_dynamic_properties, recursion_depth + 1,
error_target);
if (passed_value.IsEmpty())
return v8::MaybeLocal<v8::Object>();
proxy.Set(key, passed_value.ToLocalChecked());
Expand Down Expand Up @@ -683,7 +699,8 @@ void ExposeAPIInWorld(v8::Isolate* isolate,
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,
electron_isolated_context, target_context, api,
electron_isolated_context->Global(), &object_cache, false, 0,
BridgeErrorTarget::kSource);
if (maybe_proxy.IsEmpty())
return;
Expand Down Expand Up @@ -732,9 +749,11 @@ void OverrideGlobalValueFromIsolatedWorld(
{
v8::Context::Scope main_context_scope(main_context);
context_bridge::ObjectCache object_cache;
v8::Local<v8::Context> source_context = value->GetCreationContextChecked();
v8::MaybeLocal<v8::Value> maybe_proxy = PassValueToOtherContext(
value->GetCreationContextChecked(), main_context, value, &object_cache,
support_dynamic_properties, 1, BridgeErrorTarget::kSource);
source_context, main_context, value, source_context->Global(),
&object_cache, support_dynamic_properties, 1,
BridgeErrorTarget::kSource);
DCHECK(!maybe_proxy.IsEmpty());
auto proxy = maybe_proxy.ToLocalChecked();

Expand Down Expand Up @@ -768,15 +787,19 @@ bool OverrideGlobalPropertyFromIsolatedWorld(
v8::Local<v8::Value> getter_proxy;
v8::Local<v8::Value> setter_proxy;
if (!getter->IsNullOrUndefined()) {
v8::Local<v8::Context> source_context =
getter->GetCreationContextChecked();
v8::MaybeLocal<v8::Value> maybe_getter_proxy = PassValueToOtherContext(
getter->GetCreationContextChecked(), main_context, getter,
source_context, main_context, getter, source_context->Global(),
&object_cache, false, 1, BridgeErrorTarget::kSource);
DCHECK(!maybe_getter_proxy.IsEmpty());
getter_proxy = maybe_getter_proxy.ToLocalChecked();
}
if (!setter->IsNullOrUndefined() && setter->IsObject()) {
v8::Local<v8::Context> source_context =
getter->GetCreationContextChecked();
v8::MaybeLocal<v8::Value> maybe_setter_proxy = PassValueToOtherContext(
getter->GetCreationContextChecked(), main_context, setter,
source_context, main_context, setter, source_context->Global(),
&object_cache, false, 1, BridgeErrorTarget::kSource);
DCHECK(!maybe_setter_proxy.IsEmpty());
setter_proxy = maybe_setter_proxy.ToLocalChecked();
Expand Down
8 changes: 8 additions & 0 deletions shell/renderer/api/electron_api_context_bridge.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,14 @@ v8::MaybeLocal<v8::Value> PassValueToOtherContext(
v8::Local<v8::Context> source_context,
v8::Local<v8::Context> destination_context,
v8::Local<v8::Value> value,
/**
* Used to automatically bind a function across
* worlds to its appropriate default "this" value.
*
* If this value is the root of a tree going over
* the bridge set this to the "context" of the value.
*/
v8::Local<v8::Value> parent_value,
context_bridge::ObjectCache* object_cache,
bool support_dynamic_properties,
int recursion_depth,
Expand Down
9 changes: 6 additions & 3 deletions shell/renderer/api/electron_api_web_frame.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -140,9 +140,12 @@ class ScriptExecutionCallback {
{
v8::TryCatch try_catch(isolate);
context_bridge::ObjectCache object_cache;
maybe_result = PassValueToOtherContext(
result->GetCreationContextChecked(), promise_.GetContext(), result,
&object_cache, false, 0, BridgeErrorTarget::kSource);
v8::Local<v8::Context> source_context =
result->GetCreationContextChecked();
maybe_result =
PassValueToOtherContext(source_context, promise_.GetContext(), result,
source_context->Global(), &object_cache,
false, 0, BridgeErrorTarget::kSource);
if (maybe_result.IsEmpty() || try_catch.HasCaught()) {
success = false;
}
Expand Down
4 changes: 2 additions & 2 deletions shell/renderer/renderer_client_base.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -608,8 +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,
api::BridgeErrorTarget::kSource);
source_context, context, guest_view_internal, source_context->Global(),
&object_cache, false, 0, api::BridgeErrorTarget::kSource);
if (!result.IsEmpty()) {
isolated_api.Set("guestViewInternal", result.ToLocalChecked());
}
Expand Down
41 changes: 25 additions & 16 deletions spec/webview-spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2020,25 +2020,34 @@ describe('<webview> tag', function () {

// TODO(miniak): figure out why this is failing on windows
ifdescribe(process.platform !== 'win32')('<webview>.capturePage()', () => {
it('returns a Promise with a NativeImage', async () => {
it('returns a Promise with a NativeImage', async function () {
this.retries(5);

const src = 'data:text/html,%3Ch1%3EHello%2C%20World!%3C%2Fh1%3E';
await loadWebViewAndWaitForEvent(w, { src }, 'did-stop-loading');

// Retry a few times due to flake.
for (let i = 0; i < 5; i++) {
try {
const image = await w.executeJavaScript('webview.capturePage()');
const imgBuffer = image.toPNG();

// Check the 25th byte in the PNG.
// Values can be 0,2,3,4, or 6. We want 6, which is RGB + Alpha
expect(imgBuffer[25]).to.equal(6);
return;
} catch {
/* drop the error */
}
}
expect(false).to.be.true('could not successfully capture the page');
const image = await w.executeJavaScript('webview.capturePage()');
expect(image.isEmpty()).to.be.false();

// Check the 25th byte in the PNG.
// Values can be 0,2,3,4, or 6. We want 6, which is RGB + Alpha
const imgBuffer = image.toPNG();
expect(imgBuffer[25]).to.equal(6);
});

it('returns a Promise with a NativeImage in the renderer', async function () {
this.retries(5);

const src = 'data:text/html,%3Ch1%3EHello%2C%20World!%3C%2Fh1%3E';
await loadWebViewAndWaitForEvent(w, { src }, 'did-stop-loading');

const byte = await w.executeJavaScript(`new Promise(resolve => {
webview.capturePage().then(image => {
resolve(image.toPNG()[25])
});
})`);

expect(byte).to.equal(6);
});
});

Expand Down

0 comments on commit d45f829

Please sign in to comment.