Issue of Script.runInNewContext and vm.runInNewContext

[fbzhong]

Hi all,

I may found a issue of vm in Node.JS 0.4.8. Actually, the issue are founded from executing external javascript for url http://www.google.com in jsdom. When executing javascripts in jsdom, it always throw "google is not defined" error.

After debug, I know how to reproduce and write down the following test code:

var util = require('util'),
    Context = process.binding('evals').Context,
    Script = process.binding('evals').NodeScript || process.binding('evals').Script;


// test 1.
(function(){
    console.log('execute test1');

    var window = {};
    window.window = window;

    var sandbox = new Context();
    sandbox.__proto__ = window;

    Script.runInNewContext('window.google = {};', sandbox);
    Script.runInNewContext('google.test = "test";', sandbox);
    console.log(util.inspect(window));
})();

// test 2.
(function(){
    console.log('execute test2');

    var window = {};
    window.window = window;

    var sandbox = new Context();
    sandbox.__proto__ = window;

    Script.runInNewContext('window.google = {};google.test = "test";', sandbox);
    console.log(util.inspect(window));

})();

The result is the following:

tigger-pro:robin ~$ node test_script.js 
execute test1
{ window: [Circular], google: { test: 'test' } }
execute test2

node.js:134
        throw e; // process.nextTick error, or 'error' event on first tick
        ^
ReferenceError: google is not defined
    at evalmachine.<anonymous>:1:20
    at /Users/robin/test_script.js:31:12
    at Object.<anonymous> (/Users/robin/test_script.js:34:2)
    at Module._compile (module.js:407:26)
    at Object..js (module.js:413:10)
    at Module.load (module.js:339:31)
    at Function._load (module.js:298:12)
    at Array.<anonymous> (module.js:426:10)
    at EventEmitter._tickCallback (node.js:126:26)

Those two tests do the same thing. The only difference is in test 2 the codes are merge to one line to execute.

I have no idea on this, could anyone help me to solve this issue?

[fbzhong]

If I use the vm, the error is the same:

tigger-pro:robin ~$ cat test_vm.js 
var util = require('util'),
    vm = require('vm');


// test 1.
(function(){
    console.log('execute test1');

    var window = {};
    window.window = window;

    var sandbox = {};
    sandbox.__proto__ = window;

    vm.runInNewContext('window.google = {};', sandbox);
    vm.runInNewContext('google.test = "test";', sandbox);
    console.log(util.inspect(window));
})();

// test 2.
(function(){
    console.log('execute test2');

    var window = {};
    window.window = window;

    var sandbox = {};
    sandbox.__proto__ = window;

    vm.runInNewContext('window.google = {};google.test = "test";', sandbox);
    console.log(util.inspect(window));

})();tigger-pro:robin ~$ node test_vm.js 
execute test1
{ window: [Circular], google: { test: 'test' } }
execute test2

node.js:134
        throw e; // process.nextTick error, or 'error' event on first tick
        ^
ReferenceError: google is not defined
    at evalmachine.<anonymous>:1:20
    at /Users/robin/test_vm.js:30:8
    at Object.<anonymous> (/Users/robin/test_vm.js:33:2)
    at Module._compile (module.js:407:26)
    at Object..js (module.js:413:10)
    at Module.load (module.js:339:31)
    at Function._load (module.js:298:12)
    at Array.<anonymous> (module.js:426:10)
    at EventEmitter._tickCallback (node.js:126:26)
tigger-pro:robin ~$ 

[fbzhong]

After discussion with indutny on irc channel of Node.JS, he gives the following code to make it works as expected:

tigger-pro:robin ~$ cat test_vm.js 
var util = require('util'),
    vm = require('vm');


// test 1.
(function(){
    console.log('execute test1');

    var window = {};
    window.window = window;

    var sandbox = {};
    sandbox.__proto__ = window;

    vm.runInNewContext('window.google = {};', sandbox);
    vm.runInNewContext('google.test = "test";', sandbox);
    console.log(util.inspect(window));
})();

// test 2.
(function(){
    console.log('execute test2');

    var window = {};
    window.window = window;

    var sandbox = {};
    sandbox.__proto__ = window;
    sandbox.window = sandbox;

    vm.runInNewContext('window.google = {};google.test = "test";', sandbox);
    console.log(util.inspect(sandbox));
    console.log(util.inspect(window));

})();tigger-pro:robin ~$ node test_vm.js 
execute test1
{ window: [Circular], google: { test: 'test' } }
execute test2
{ window: [Circular], google: { test: 'test' } }
{ window: [Circular] }

[fbzhong]

Sorry for mis-operation.

Anyone could help me to explain the internal differences of those test codes?

Thanks.

[ghost]

runInNewContext does not use sandbox object itself as the global for the new context, the sandbox is copied from before running and copied to after running. proto is not copied, of course (nor are getters and setters), so forget proto. Second thing is, first level self-references (sandbox.window = sandbox) are translated to self-references in the actual global object and vice-versa, that's why window.google = ... assigns to (new context's) global google and the next assignment works.

[d-ash]

@Herby is it correct what nodejs does with self-references?

Node translates first level refs, but other refs on sublevels are not translated.
And that causes many issues and I suppose even memory leaks: #637 (comment)

[ghost]

This can't be the cause of memory leaks at all - just imagine it would not translate anything at all. It's just the object reference like any other. If a memory leak occurs, then something else did not release the pointer.
As for "correctness" from more philosophical point of view - shallow copying is done, so shallow translation occurs. You definitely do not want to deep copy sandbox into context's global. Other possibility is not to translate at all - but then you simply cannot pass self-references in sandbox at all (like, 'global' global variable pointing to context's global object - which is actually used in module code for NODE_MODULE_CONTEXT=1 case).

[d-ash]

@Herby I'm not sure, but I think that the LOOP references between javascript objects in different contexts are not garbage collected correctly. Due to V8::Context is a special type of handle and should be freed manualy.

[ghost]

Maybe. I coded node_script.cc by borrowing from process.evalcx and process.compile and adding a possibility of standalone Script object, which is functionality in runInThisContext and runInNewContext methods. They work fine (well, runInThisContext does not when called from other than main context, which I filed as a bug, and which is not a matter of the code itself but it's proper wiring into v8, but no one got interested); but as for runInContext, it's someone else's addon, so either he or someone else should devise some solution if it is at all possible (maybe MakeWeak and gc callback if it is possible for a Context?).

[ELLIOTTCABLE]

As documented in #1801, I believe I’ve closed this with elliottcable@cf21650. I’d appreciate if you’d clone and compile against that, and see if you can still reproduce this issue. (Many of these issues seem related.)

[isaacs]

@ELLIOTTCABLE This doesn't appear to be affected by the referenced pull. Inherited properties of the sandbox object don't make it into the context.

> vm.runInContext('foo', vm.createContext({foo: "bar"}))
'bar'
> vm.runInContext('foo', vm.createContext(Object.create({foo: "bar"})))
ReferenceError: foo is not defined

[ELLIOTTCABLE]

@isaacs I … must be mis-reading something. Wasn’t that the entire point of what we just changed? Now, the context should receive the entire prototype chain of the sandbox. I can’t build and test from here, on my iPad; but that massively surprises me. :x

[trevnorris]

Not sure what is supposed to be the output, but running @fbzhong latest test case against v0.8.14 I get the following for test 1:

/var/projects/tmp/node-issues/1105.js:15
    vm.runInNewContext('window.google = {};', sandbox);
       ^
ReferenceError: window is not defined
    at evalmachine.<anonymous>:1:1
    at /var/projects/tmp/node-issues/1105.js:15:8
    at Object.<anonymous> (/var/projects/tmp/node-issues/1105.js:18:3)
    at Module._compile (module.js:449:26)
    at Object.Module._extensions..js (module.js:467:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Module.runMain (module.js:492:10)
    at process.startup.processNextTick.process._tickCallback (node.js:244:9)

and the following for test 2:

{ window: [Circular], google: { test: 'test' } }
{ window: [Circular] }

So... any feedback on what has happened to test 1?

[bnoordhuis]

You can't use __proto__. Besides, the test uses process.binding('evals') - an internal API and not in any way stable. Closing.