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

Contextify should use ES6 Proxy to wrap the global object. #7820

Closed
hashseed opened this issue Jul 21, 2016 · 6 comments
Closed

Contextify should use ES6 Proxy to wrap the global object. #7820

hashseed opened this issue Jul 21, 2016 · 6 comments
Labels
feature request Issues that request new features to be added to Node.js. vm Issues and PRs related to the vm subsystem.

Comments

@hashseed
Copy link
Member

hashseed commented Jul 21, 2016

@ofrobots @fhinkel @verwaest @isaacs

I'm recently browsing through node_contextify.cc and found this comment describing ContextifyContext::CopyProperties:

  // XXX(isaacs): This function only exists because of a shortcoming of
  // the V8 SetNamedPropertyHandler function.
  //
  // It does not provide a way to intercept Object.defineProperty(..)
  // calls.  As a result, these properties are not copied onto the
  // contextified sandbox when a new global property is added via either
  // a function declaration or a Object.defineProperty(global, ...) call.
  //
  // Note that any function declarations or Object.defineProperty()
  // globals that are created asynchronously (in a setTimeout, callback,
  // etc.) will happen AFTER the call to copy properties, and thus not be
  // caught.
  //
  // The way to properly fix this is to add some sort of a
  // Object::SetNamedDefinePropertyHandler() function that takes a callback,
  // which receives the property name and property descriptor as arguments.
  //
  // Luckily, such situations are rare, and asynchronously-added globals
  // weren't supported by Node's VM module until 0.12 anyway.  But, this
  // should be fixed properly in V8, and this copy function should be
  // removed once there is a better way.

So let me see if I understood this correctly:

  • In ContextifyContext::CreateV8Context we set up a hidden prototype with named property interceptors to intercept accesses to the global object (to sync with the contextified object).
  • Named property interceptors do not trigger for Object.defineProperty.

Seems like wrapping the global object in an ES6 Proxy would solve the issue. I wonder whether doing that is straight-forward. We could then also get rid of the named property interceptors and the hidden prototype.

@hashseed hashseed changed the title Contextify Contextify should use ES6 Proxy to wrap the global object. Jul 21, 2016
@thefourtheye
Copy link
Contributor

cc @bnoordhuis

@mscdex mscdex added vm Issues and PRs related to the vm subsystem. feature request Issues that request new features to be added to Node.js. labels Jul 21, 2016
@targos
Copy link
Member

targos commented Jul 21, 2016

I can have a look at this

@targos targos self-assigned this Jul 21, 2016
@ofrobots
Copy link
Contributor

ofrobots commented Jul 21, 2016

Please note the tracking issue listing all the known bugs w/ contextify here: #6283.

@verwaest, @fhinkel, @domenic, @jeisinger and I had a discussion on this a while back and came to the conclusion that the only real solution to the problem is going to be if the global object in the new context is physically the same object in the outer context for things like === to work (#855) and for the communication to be possible in both directions without conceptual leaks. I am a bit hazy on all the details now – but it will come back to me.

IMO, the real problem stems from the fact that interceptors don't trigger for all events. A new API in V8 that would allow us to intercept these would be the way to fix this problem.

@domenic
Copy link
Contributor

domenic commented Jul 21, 2016

A Proxy would definitely help with some of these issues at the cost of making every operation run inside the vm slower. Using dedicated V8 APIs as discussed previously seems like a better path.

@hashseed
Copy link
Member Author

So performance is a real concern here? Do you have any benchmarks?

I do agree that at this point, Proxies are slow. But I personally would prefer making Proxies faster, for everyone, instead of adding a special hook for this particular use case. One that can be solved by using a Proxy for its designed purpose.

Like Ali said, interceptors don't trigger for all events. Is there any other event other than Object.defineProperty that does not trigger?

@hashseed
Copy link
Member Author

Some offline discussions and knowing more about the requirements, I now see that Proxies are indeed not a valid solution. We do want to keep the sandbox object and the context's global object separate.

@targos targos removed their assignment Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

No branches or pull requests

6 participants