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.

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

fix: ensure that errors thrown in the context bridge are created in the correct context #24534

Merged
merged 1 commit into from
Jul 23, 2020

Conversation

MarshallOfSound
Copy link
Member

@MarshallOfSound MarshallOfSound commented Jul 13, 2020

This PR fixes two cases where errors were being created in context A but thrown in context B.

  • When the V8 serializer threw an error, it always threw it in the source context, now it throws in the appropriate error context
  • If we throw an error as a result of a return value failing to be converted, it created the error in the source context when it should create it in the destination context as that is where the error will be thrown / caught.

Notes: no-notes

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jul 13, 2020
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jul 14, 2020
shell/renderer/api/electron_api_context_bridge.cc Outdated Show resolved Hide resolved
shell/renderer/api/electron_api_context_bridge.cc Outdated Show resolved Hide resolved
shell/renderer/api/electron_api_context_bridge.cc Outdated Show resolved Hide resolved
Comment on lines +415 to +419
auto ret = PassValueToOtherContext(
func_owning_context, calling_context,
maybe_return_value.ToLocalChecked(), &object_cache,
support_dynamic_properties, 0, BridgeErrorTarget::kDestination);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it also be correct to use TryCatch here to capture the error and throw a new error in the correct context? e.g.

Suggested change
auto ret = PassValueToOtherContext(
func_owning_context, calling_context,
maybe_return_value.ToLocalChecked(), &object_cache,
support_dynamic_properties, 0, BridgeErrorTarget::kDestination);
v8::TryCatch try_catch(isolate);
auto ret = PassValueToOtherContext(...);
if (try_catch.HasCaught()) {
ThrowException("Conversion of return value failed");
return;
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is handled inside PassValueToOtherContext, the way that function is written and intended to be used is that it always does everything in the right context. Any errors thrown are thrown in the correct context too so this try catch would be unnecessary.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was just thinking about this as an alternative implementation that would be less intrusive (i.e. there would no need for this extra enum).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It means we'd have to trycatch every call to PassValueToOtherContext which would be considerably more intrusive?

spec-main/api-context-bridge-spec.ts Outdated Show resolved Hide resolved
@MarshallOfSound MarshallOfSound merged commit 5cfe956 into master Jul 23, 2020
@MarshallOfSound MarshallOfSound deleted the ctx-bridge-errors branch July 23, 2020 21:32
@release-clerk
Copy link

release-clerk bot commented Jul 23, 2020

No Release Notes

@trop
Copy link
Contributor

trop bot commented Jul 23, 2020

I was unable to backport this PR to "7-3-x" cleanly;
you will need to perform this backport manually.

@trop
Copy link
Contributor

trop bot commented Jul 23, 2020

I was unable to backport this PR to "8-x-y" cleanly;
you will need to perform this backport manually.

@trop
Copy link
Contributor

trop bot commented Jul 23, 2020

I have automatically backported this PR to "10-x-y", please check out #24713

@trop
Copy link
Contributor

trop bot commented Jul 23, 2020

I was unable to backport this PR to "9-x-y" cleanly;
you will need to perform this backport manually.

@trop
Copy link
Contributor

trop bot commented Jul 27, 2020

@MarshallOfSound has manually backported this PR to "9-x-y", please check out #24747

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants