nodejs / node-chakracore

Node.js on ChakraCore :sparkles::turtle::rocket::sparkles:
Other
1.92k stars 342 forks source link

Inconsistent behaviour with vm context of `Object.create(null)` #508

Open alexlamsl opened 6 years ago

alexlamsl commented 6 years ago

Node.js

$ node -v
v8.11.1

$ node
> vm.runInNewContext('0 & this', Object.create(null))
0

node-chakracore

$ node -v
v8.10.0

$ node
> vm.runInNewContext('0 & this', Object.create(null))
evalmachine.<anonymous>:1
0 & this
^

TypeError: Number expected
   at Global code (evalmachine.<anonymous>:1:1)
   at Script.prototype.runInContext (vm.js:59:5)
   at Script.prototype.runInNewContext (vm.js:65:3)
   at runInNewContext (vm.js:135:3)
   at Global code (repl:1:1)
   at Script.prototype.runInThisContext (vm.js:50:5)
   at defaultEval (repl.js:240:13)
   at bound (domain.js:301:5)
   at runBound (domain.js:314:5)
   at onLine (repl.js:468:5)
MSLaguana commented 6 years ago

I don't believe that this is to do with the vm module actually; just running 0 & Object.create(null) in ch (bare chakracore) gives the same error.

MSLaguana commented 6 years ago

Unless you expect that, but you expect that using that as the global should have different behavior?

alexlamsl commented 6 years ago

0 & Object.create(null) behaves consistently between node and node-chakracore (both at 8.11.1)

$ nvs use node/8
PATH = node\8.11.1\x64

$ node
> 0 & Object.create(null)
TypeError: Cannot convert object to primitive value
$ nvs use chakracore
PATH = chakracore\8.11.1\x64

$ node
> 0 & Object.create(null)
TypeError: Number expected
   at Global code (repl:1:1)

whereas the top example works for node but cause exception to be throw in node-chakracore

MSLaguana commented 6 years ago

Ok, just had a look and a bare 0 & Object.create(null) in node-v8 gives an equivalent error, but in the vm context it does not. I think that this is because in the v8 case the global object keeps its prototype and so there is a valueOf, while in the node-chakracore case we replace the prototype with the sandbox and so it no longer has a valueOf.

MSLaguana commented 6 years ago

I somewhat feel like this might be one of those edge-cases that may not be fully intended in node upstream? I know I asked about what happens in some strange cases when properties conflict between the sandbox and the global, and there wasn't a good answer to what was expected. I would somewhat expect that if you say "Use this sandbox object, and I've removed everything from it" that you wouldn't want it to have things like the prototype properties; just a bare minimal global object.

alexlamsl commented 6 years ago

I want to be absolutely sure to avoid any Object.prototype properties leaking into the sandbox context, which is why it is coded this way.

Having said that, I do understand your reason for the current behaviour in node-chakracore - but then as a user I just want to be able to switch between the platforms with as few quirks as possible.

MSLaguana commented 6 years ago

We definitely want to have minimal platform differences. The problem here is trying to understand what the underlying expected behavior is, given that this vm context stuff isn't strongly specced out, and "what node-v8 does" isn't necessarily right in all cases either since there are some questions about whether they do the right thing in some circumstances.

alexlamsl commented 6 years ago

Thanks for the dicussion and clarifications - meanwhile I'll see if I can come up with an alternative test case in uglify-js which avoids this issue.

alexlamsl commented 6 years ago

Reverted the workaround in uglify-js to confirm the difference in behaviour between node and node-chakracore still persists as of 11.0.0-nightly20180522db93156017

alexlamsl commented 6 years ago

Still an issue as of v10.1.0

alexlamsl commented 6 years ago

Reproducible in v10.6.0