Repository navigation
VM: weird behaviour in runInContext/runInThisContext #5344
Description
While working on my electron based REPL application, I noticed a weird behaviour in vm.runInContext.
Program
Desktop 🙈 ₹ cat repl-vm-strict.js
'use strict';
const vm = require('vm');
const ctx = vm.createContext();
try {
const result = vm.runInContext('"use strict"; x = 1', ctx);
} catch(e) {
console.log(e.stack);
console.log('x is', ctx.x)
}Output
Desktop 🙈 ₹ node repl-vm-strict.js
ReferenceError: x is not defined
at evalmachine.<anonymous>:1:17
at Object.exports.runInContext (vm.js:44:17)
at Object.<anonymous> (/Users/princejohnwesley/Desktop/repl-vm-strict.js:7:21)
at Module._compile (module.js:413:34)
at Object.Module._extensions..js (module.js:422:10)
at Module.load (module.js:357:32)
at Function.Module._load (module.js:314:12)
at Function.Module.runMain (module.js:447:10)
at startup (node.js:140:18)
at node.js:1001:3
x is 1Behaviour
vm.runInThisContext/vm.runInContextthrows error. ✅- context is also updated as if variable is created with
varorletbinding ❌
Versions
Tested versions: v5.6.0 & v5.1.1
Platform
Desktop 🙈 ₹ uname -a
Darwin Princes-MacBook-Pro.local 15.3.0 Darwin Kernel Version 15.3.0: Thu Dec 10 18:40:58 PST 2015; root:xnu-3248.30.4~1/RELEASE_X86_64 x86_64
Subsystem
vm
repl with strict is affected by this issue.
Activity
bnoordhuis commentedon Feb 21, 2016
I can confirm the bug but I suspect it's a V8 issue - it calls the global object's named property handlers (specifically the
GlobalPropertySetterCallback) when it shouldn't. I'll have to investigate if it still happens with V8 HEAD.bnoordhuis commentedon Feb 21, 2016
Still happens with V8 4.10.253 and 5.0.48. I'll see if I can put a standalone test case together for upstream to take a look at.
hashseed commentedon Mar 1, 2016
I talked to my colleagues. V8 seems to be working as intended in this case. I assume that there is an interceptor on the global object installed by node. The interceptor probably stores the value onto the global object, but returns false, signalling that it did not intercept. V8 then attempts to store and throws due to strict mode.
So the interceptor should either not store and return false, or throw if v8::PropertyCallbackInfo::ShouldThrowOnError() returns true.
bnoordhuis commentedon Mar 1, 2016
@hashseed Yang, thanks for chiming in. Is there anything we can do with 4.6 and 4.8? They don't have the
ShouldThrowOnError()method.The best I've been able to come up with is to sniff the script for a
'use strict'string and omit the call tov8::PropertyCallback<T>::Set()in the setter for strict mode scripts if there is no corresponding property on the global object. Basically this:hashseed commentedon Mar 1, 2016
Would this work for you? Whenever the interceptor is going to do a normal store, it does not store and returns false. V8 will then do the store, unless strict mode causes an exception before that.
thefourtheye commentedon Mar 13, 2016
@bnoordhuis If the property is newly created, neither the sandbox object nor the global object will have the property defined on them when
GlobalPropertySetterCallbackis called. So,sandbox->Has(property)will fail and the new property will not be added to sandbox, right?src: do not copy on failing setProperty()
mmc41 commentedon Dec 13, 2016
The possible bug behind this "#5344" seems to be fixed now.
mbroadst commentedon Feb 21, 2017
I'm still experiencing weirdness with a slightly modified version of this test case:
results in:
ReferenceError: x is not definedmscdex commentedon Apr 9, 2017
This does seem to be happening still, even on current master.
Thoughts @nodejs/v8?
bnoordhuis commentedon Apr 10, 2017
Moved to #12300 so it has its own bug number. It's related but separate.
src: do not copy on failing setProperty()