Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: correctly track receiver for methods called via ctx bridge #40263

Merged
merged 2 commits into from
Oct 18, 2023
Merged

fix: correctly track receiver for methods called via ctx bridge #40263

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8cc87e1
fix: correctly track receiver for methods called via ctx bridge
trop[bot] Oct 18, 2023
9311f59
spec: test for correct contextBridge passage
trop[bot] Oct 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
77 changes: 50 additions & 27 deletions shell/renderer/api/electron_api_context_bridge.cc
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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