vm.runInContext is not fully supported after upstream change

[MSLaguana]

With the inclusion of nodejs/node#16293 we need to make some additional changes to properly support vm.runInContext.

When running vm.runInContext(code, context) the context object is intended to act like the global object for the duration of the code. Currently I believe we do this by creating a proxy around the context object, creating a new global object, and then setting the prototype of the new global object to the the proxy of the context object.

Prior to the upstream changes, there was a step that copied properties from the global inside the context back to the context object. In fact, we already mishandled this somewhat: In node-chakracore, you could do this:

var vm = require("vm");
var sb = {}
vm.createContext(sb);
vm.runInContext("", sb);
sb.Object // sb now has the Object property from the context when it shouldn't

With the upstream changes, that step has been removed in favor of new v8 support for additional interceptors. I've tried adding implementations for those interceptors, but our current approach to configuring the context doesn't end up with them being called: modifications are made to the global context object, and it does not defer to the proxy that is its prototype.

It seems like we should probably be reversing the relationship here, with the sandbox object (or its proxy) deferring to the global object as its prototype instead, so modifications can be intercepted by the native code and persisted as appropriate.

[MSLaguana]

See eaf6763 for some of the failing tests showing missing functionality.

[fhinkel]

Let me know if you want me to look at any changes. The copyProperties() function was a hack because our old interceptors didn't intercept all calls, namely defineProperty() was skipped. I've added a bit of documentation to give a broad overview how the current documentation works, nodejs/node@f0c674d.

[avaer]

I found another case that doesn't work: Promise microtasks.

const c = {
  console,
};
vm.createContext(c);
vm.runInContext(`
  console.log('script ok 0');
  new Promise(accept => accept())
    .then(() => {
      console.log('script ok 1'); // never runs, but should. does on regular node
    });
`, c);

[avaer]

Re: the above, let me know if it should be filed separately. It's not strictly anything to do with sandbox properties, but tick management.

[MSLaguana]

It seems that the package uglify-js2 is broken in node-chakracore at the moment because of vm.runInContext issues.

[jdalton]

When using vm.runInContext in a non-trivial example I'm getting the notorious 3221225477 exit code.

[alexlamsl]

As @MSLaguana pointed out, #478 may be related. There is also #508.

[jdalton]

In node-chakra v10 the repl module is now broken when passing a context object to eval:

var repl = require("repl")
var vm = require("vm")

var c = vm.createContext({})
var r = repl.start({})

r.eval("var a=1", c, "test", () => {
    console.log("a", c.a) // logs "a", undefined instead of 1
})

r.close()

[MSLaguana]

Thanks for the report, this looks like the same root issue impacting uglify (var declarations not being forwarded to the sandbox object).

[jdalton]

Any plans or leads on how to tackle this issue?
It's now moved from WIP bits to bugs in shipped releases (like v10).

[MSLaguana]

I have a monkeypatching workaround for some limited scenarios (would fix most of the var stuff not applying back to the context object) but I'm still not sure what is causing issues with promises, and there are many edge cases that wouldn't be addressed. We're looking at whether there are some changes necessary in chakracore to support these scenarios.

[MSLaguana]

I've got a PR out with a partial workaround for this issue now, although there are known deficiencies in it. It should fix the case of uglify.js and the repl example above.

[alexlamsl]

@MSLaguana thanks a lot for your fixes on issues related to uglify-js

I shall keep a close eye on new releases of node-chakracore and report back.

[MSLaguana]

This fix is in the most recent node-chakracore nightly if you were interested in trying it out soon; otherwise we expect to put out a new release of 10.x in the near future.

[alexlamsl]

@MSLaguana using 11.0.0-nightly20180522db93156017, #478 still fails.

OT: so do #462 and #509

[MSLaguana]

Sorry; I meant that the fix for vm.runInContext should be in. #509 will be fixed once we bump the version of chakracore (alternately, should be fixed in chakracore-master branch).

I had forgotten about #478; that looks to be a little harder to resolve. The root of the issue is still that we are limited in where we can put the interceptors for the context, and in this case we are attempting to check whether the sandbox has a property (in this case A) and getting back undefined rather than "no".

[alexlamsl]

@MSLaguana thanks for the clarifications 👍