Repository navigation
Contextify should use ES6 Proxy to wrap the global object. #7820
Description
@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.
Activity
[-]Contextify[/-][+]Contextify should use ES6 Proxy to wrap the global object.[/+]thefourtheye commentedon Jul 21, 2016
cc @bnoordhuis
targos commentedon Jul 21, 2016
I can have a look at this
ofrobots commentedon 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 commentedon 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 commentedon Jul 22, 2016
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 commentedon Jul 22, 2016
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.