node_contextify tracking issue #6283

Open
ofrobots opened this Issue Apr 19, 2016 · 14 comments

Projects

None yet

8 participants

@ofrobots
Contributor
ofrobots commented Apr 19, 2016 edited

The implementation of the vm module has some limitation that results in non-intuitive behaviour. There are already a few bugs open for this (see list below). At this point I do not think there are incremental fixes that can solve the issues with the vm module. I suspect that a revamp of the vm module might be needed, with some API help from the V8 team, to fix these issues properly.

Existing open issues:

  • #855: references to context inside objects are not === the original context
  • #2734: accessor properties get converted to data properties
  • #5350: inherited properties get flattened
  • #6158: Proxy passed as vm context hides built-in properties
  • #5344: Strict mode global property update should not succeed
  • #5679: Object.defineProperty
  • [code comment] Async updates in context aren't reflected in the sandbox object.
  • #548, fixed upstream in V8 (September 2016)
  • nodejs/node-v0.x-archive#9084 (but it may be a dupe of #769 which was fixed in recent V8)
  • #6287 deleting a property in a context has no effect
  • #7458 Built-in properties cannot be accessed using Reflect, when Proxy is passed as vm context
  • probably more that I am not aware of... (please add in comments below and I will update the issue)

I am creating this issue to make it easier to keep track of these issues, and possible solutions.

/cc @nodejs/v8 @domenic.

@ofrobots ofrobots added the vm label Apr 19, 2016
@domenic
Member
domenic commented Apr 19, 2016
@fhinkel
Member
fhinkel commented Jul 2, 2016
@fhinkel fhinkel added a commit to fhinkel/node that referenced this issue Jul 22, 2016
@fhinkel fhinkel Proof of Concept: intercept defineProperty()
A lot of contextify issues seem to be related to the fact that we
cannot intercept defineProperty(), see
nodejs#6283.

Here is a proof of concept implementation that gets rid of the
CopyProperties() hack in contextify. For simplicty, only getters from the
descriptors are copied. Also, function declarations are not intercepted,
but that should be easy to do.

It'll be a while until I get this cleanly into V8, but I think once the
V8 API allows for intercepting defineProperty() and function declarations,
a lot of contextify issues can be solved.
4025fbd
@fhinkel fhinkel self-assigned this Sep 7, 2016
@bmeck
Contributor
bmeck commented Sep 29, 2016

The problem is API design that I can tell. We "enhance" and return the sandbox object passed in to vm.createContext() but the behavior of the v8::Context::Global() in v8 cannot delegate fully with behaviors back to the sandbox (like Object.defineProperty). We should really return a new Proxy from vm.createContext that can propagate into the VM Global(). This is a breaking change but this relates to most of this list.

@fhinkel
Member
fhinkel commented Sep 29, 2016

We discussed the proxy idea in #7820.

We made API changes in V8. Fixing the issues here should be possible as soon as we pull in a newer V8 version.

@bmeck
Contributor
bmeck commented Sep 29, 2016

@fhinkel that was describing a Proxy inside of the Context as the Global proxy, I was thinking of a Proxy outside of the v8::Context and using something like newRemoteContext. The use case described by @domenic to my knowledge was around needing to generate the Global() reference (that is ===) prior to creating a full context. Looking at the code as well, it seems that Node should be using FromSnapshot which it currently does not. If possible I would like to move away from interceptors if possible.

@isiahmeadows
isiahmeadows commented Oct 3, 2016 edited

BTW the box for #5344 should probably now be checked, since that issue is now closed.

@fhinkel
Member
fhinkel commented Dec 11, 2016
@cpojer
cpojer commented Dec 15, 2016

I'm really excited about improvements to vm context. Jest, the JavaScript testing framework, heavily uses the vm module to isolate tests. Is there any way we can make sure that Jest keeps working or that we are aware of potential breaking changes? Is it at all possible for you to include yarn test (or yarn test -i to run everything within a single process) inside of the Jest repo to the test plans for changes here? Jest is used for testing by a ton of companies, so it may make sense to ensure we don't break anything here and that we are aware of changes that are coming.

It may actually be a good real-world test for you, too. I'm happy to talk more about the architecture but in essence Jest parallelizes across worker processes and has a custom node-like (or jsdom) env in a vm context that also comes with a powerful custom require implementation that can be used for module-boundary mocking.

I also had thoughts around vm.Script. It has the cached data feature via the produceCachedData and cachedData fields. I had hopes that this would speed up repeated script execution which could be pretty significant for Jest but was surprised there was no performance benefit of using this at all across thousands of test files and tens of thousands of modules at FB. Besides the API of this feature, which is really odd, is there any way this feature can be changed to be more efficient? I kind of don't really get why v8 has to validate the cached data as that also requires to store both the original source and the cached data. This may be topic for a separate issue but I just wanted to bring it up.

Context:

@fhinkel
Member
fhinkel commented Dec 15, 2016

Good idea to use Jest tests as test cases for vm changes. Anything we break, we can add as regression tests (and fix it of course). The vm module could certainly have a better test coverage.

@bnoordhuis recently ran some benchmarks with and without vm. Slowdown is pretty dramatic. Definitely worth looking into how to speed it up.

@bmeck
Contributor
bmeck commented Dec 15, 2016

@cpojer in order to see speed gains vs the IO you need to implement a large cache store, doing it per file might actually slow things down. I can walk you through it sometime if you want.

@cpojer
cpojer commented Dec 15, 2016

A large cache store of what? In the third link I shared above you can see how we share vm.Script instances during a single run; as long as the file doesn't change, so there isn't any IO involved.

@bmeck
Contributor
bmeck commented Dec 15, 2016

@cpojer i see no call w/ produceCachedData and no file in which those results are stored in the source https://github.com/facebook/jest/search?utf8=%E2%9C%93&q=produceCachedData&type=Issues . Even if this is implemented the wrapper you show has a per file cache of the source transform, and it can be expensive to load in cache files off disk ad-hoc vs as a large bundle of cachedData. This is somewhat off topic and might be better done offline.

@cjihrig
Member
cjihrig commented Dec 16, 2016

@TheAlphaNerd perhaps we should add jest to CITGM. Looks like it was mentioned in nodejs/citgm#127.

@MylesBorins
Member

I've pinged someone to put together a PR of the various testing frameworks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment