Fix #1801 vm: Use 'sandbox' as global_prototype

Squashed commit:

(- re tests) Cleaning up the `Script` test suite.

For whatever reason, there were several duplicate test files related to `Script`
and the `'vm'` module. I removed these, and fixed a few other small issues.
(More fixes coming in subsequent commits.)

Squashes: 19e86045a0..1e3dcff4eb

(api fix:1801 new:1801) `'vm'` module uses sandbox as prototype

As described in GH-1801, the `'vm'` module was handling the `sandbox` object
provided by the API consumer in a particularly terrible and fragile fashion: it
was simply shallow-copying any enumerable properties from the sandbox onto the
global context before executing the code, and then eventually copying any values
on the global context back into the sandbox object *afterwards*.

This commit removes all of that implementation, and utilizes the passed sandbox
object as the *prototype of the context* instead. A bit of a hack, but a very
effective one.

This no longer allows for new variables created in the global context to be
placed into your sandbox after execution has completed, but that’s for the best
anyway, as it’s not very in line with the concept of a “box of passed-in
context.” I’m planning to further implement an interface for API consumers to
acquire the *actual global* from within the VM soon, thus allowing for
separation-of-concerns: providing data *to* the VM via the sandbox-prototype,
and exploring the internal environment of the VM itself.

// GitHub cruft: closes #1801

Squashes: 43b8e3c..209ed86

src/node_script.cc

@@ -364,12 +364,25 @@ Handle<Value> WrappedScript::EvalMachine(const Arguments& args) {
 
   // New and user context share code. DRY it up.
   if (context_flag == userContext || context_flag == newContext) {
-    // Enter the context
-    context->Enter();
+    // First, we grab the "global proxy" (see v8's documentation for an
+    // explanation of this) by calling `Context::Global()` *before* we call
+    // `Context::DetachGlobal()`, which will then give us access to the *actual*
+    // global object.
+
+    // We have to set the prototype of the *actual* global object, because the
+    // prototype of v8's 'global proxy' is the global object itself, and v8
+    // blows up in approximately twenty different ways if you mess with that
+    // relationship.
+
+    // Once we have the `global_proxy` reference, we utilize that to
+    // `ReattachGlobal()` before entering the context.
+    Handle<Object> global_proxy = context->Global();
+
+    context->DetachGlobal();
+    context->Global()->SetPrototype(sandbox);
+    context->ReattachGlobal(global_proxy);
 
-    // Copy everything from the passed in sandbox (either the persistent
-    // context for runInContext(), or the sandbox arg to runInNewContext()).
-    CloneObject(args.This(), sandbox, context->Global()->GetPrototype());
+    context->Enter();
   }
 
   // Catch errors
@@ -409,7 +422,6 @@ Handle<Value> WrappedScript::EvalMachine(const Arguments& args) {
     result = script->Run();
     if (result.IsEmpty()) {
       if (context_flag == newContext) {
-        context->DetachGlobal();
         context->Exit();
         context.Dispose();
       }
@@ -432,7 +444,6 @@ Handle<Value> WrappedScript::EvalMachine(const Arguments& args) {
 
   if (context_flag == newContext) {
     // Clean up, clean up, everybody everywhere!
-    context->DetachGlobal();
     context->Exit();
     context.Dispose();
   } else if (context_flag == userContext) {

test/simple/test-script-context.js

@@ -32,7 +32,8 @@ var result = script.runInContext(context);
 assert.equal('passed', result);
 
 common.debug('create a new pre-populated context');
-context = script.createContext({'foo': 'bar', 'thing': 'lala'});
+var sandbox = {'foo': 'bar', 'thing': 'lala'};
+context = script.createContext(sandbox);
 assert.equal('bar', context.foo);
 assert.equal('lala', context.thing);
 
@@ -42,6 +43,12 @@ result = script.runInContext(context);
 assert.equal(3, context.foo);
 assert.equal('lala', context.thing);
 
+// Issue GH-1801
+common.debug('test dynamic modification');
+context.fun = function(){ context.widget = 42 };
+script = new Script('fun(); widget');
+result = script.runInContext(context);
+assert.equal(42, result);
 
 // Issue GH-227:
 Script.runInNewContext('', null, 'some.js');

test/simple/test-script-new.js

@@ -66,11 +66,11 @@ code = 'foo = 1;' +
        'bar = 2;' +
        'if (baz !== 3) throw new Error(\'test fail\');';
 foo = 2;
-obj = { foo: 0, baz: 3 };
+sandbox = { foo: 0, baz: 3 };
 script = new Script(code);
-var baz = script.runInNewContext(obj);
-assert.equal(1, obj.foo);
-assert.equal(2, obj.bar);
+var baz = script.runInNewContext(sandbox);
+assert.equal(1, sandbox.foo);
+assert.equal(2, sandbox.bar);
 assert.equal(2, foo);
 
 common.debug('call a function by reference');
@@ -85,6 +85,29 @@ var f = { a: 1 };
 script.runInNewContext({ f: f });
 assert.equal(f.a, 2);
 
+// Issue GH-1801
+common.debug('test dynamic modification');
+sandbox.fun = function(){ sandbox.widget = 42 };
+script = new Script('fun(); widget');
+result = script.runInNewContext(sandbox);
+assert.equal(42, result);
+
+// Issue GH-1801
+common.debug('indirectly modify an object');
+var sandbox = { proxy: {}, common: common, process: process };
+sandbox.finish = function(){
+   process.nextTick(function(){
+      common.debug('(FINISH)');
+      assert.equal(42, sandbox.proxy.widget)
+   })
+};
+script = new Script("  process.nextTick(function(){ " +
+                    "    common.debug('(TICK)');    " +
+                    "    proxy.widget = 42;         " +
+                    "  });                          " +
+                    "  finish()                     ");
+script.runInNewContext(sandbox);
+
 common.debug('invalid this');
 assert.throws(function() {
   script.runInNewContext.call('\'hello\';');

test/simple/test-script-static-context.js

@@ -1,40 +0,0 @@
-// Copyright Joyent, Inc. and other Node contributors.
-//
-// Permission is hereby granted, free of charge, to any person obtaining a
-// copy of this software and associated documentation files (the
-// "Software"), to deal in the Software without restriction, including
-// without limitation the rights to use, copy, modify, merge, publish,
-// distribute, sublicense, and/or sell copies of the Software, and to permit
-// persons to whom the Software is furnished to do so, subject to the
-// following conditions:
-//
-// The above copyright notice and this permission notice shall be included
-// in all copies or substantial portions of the Software.
-//
-// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
-// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
-// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
-// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
-// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
-// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
-// USE OR OTHER DEALINGS IN THE SOFTWARE.
-
-var common = require('../common');
-var assert = require('assert');
-
-var Script = require('vm').Script;
-
-common.debug('run in a new empty context');
-var context = Script.createContext();
-var result = Script.runInContext('"passed";', context);
-assert.equal('passed', result);
-
-common.debug('create a new pre-populated context');
-context = Script.createContext({'foo': 'bar', 'thing': 'lala'});
-assert.equal('bar', context.foo);
-assert.equal('lala', context.thing);
-
-common.debug('test updating context');
-result = Script.runInContext('var foo = 3;', context);
-assert.equal(3, context.foo);
-assert.equal('lala', context.thing);

test/simple/test-script-static-new.js

@@ -1,62 +0,0 @@
-// Copyright Joyent, Inc. and other Node contributors.
-//
-// Permission is hereby granted, free of charge, to any person obtaining a
-// copy of this software and associated documentation files (the
-// "Software"), to deal in the Software without restriction, including
-// without limitation the rights to use, copy, modify, merge, publish,
-// distribute, sublicense, and/or sell copies of the Software, and to permit
-// persons to whom the Software is furnished to do so, subject to the
-// following conditions:
-//
-// The above copyright notice and this permission notice shall be included
-// in all copies or substantial portions of the Software.
-//
-// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
-// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
-// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
-// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
-// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
-// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
-// USE OR OTHER DEALINGS IN THE SOFTWARE.
-
-var common = require('../common');
-var assert = require('assert');
-var Script = require('vm').Script;
-
-common.globalCheck = false;
-
-common.debug('run a string');
-var result = Script.runInNewContext('\'passed\';');
-assert.equal('passed', result);
-
-common.debug('thrown error');
-assert.throws(function() {
-  Script.runInNewContext('throw new Error(\'test\');');
-});
-
-hello = 5;
-Script.runInNewContext('hello = 2');
-assert.equal(5, hello);
-
-
-common.debug('pass values in and out');
-code = 'foo = 1;' +
-       'bar = 2;' +
-       'if (baz !== 3) throw new Error(\'test fail\');';
-foo = 2;
-obj = { foo: 0, baz: 3 };
-var baz = Script.runInNewContext(code, obj);
-assert.equal(1, obj.foo);
-assert.equal(2, obj.bar);
-assert.equal(2, foo);
-
-common.debug('call a function by reference');
-function changeFoo() { foo = 100 }
-Script.runInNewContext('f()', { f: changeFoo });
-assert.equal(foo, 100);
-
-common.debug('modify an object by reference');
-var f = { a: 1 };
-Script.runInNewContext('f.a = 2', { f: f });
-assert.equal(f.a, 2);
-

test/simple/test-script-static-this.js

@@ -1,56 +0,0 @@
-// Copyright Joyent, Inc. and other Node contributors.
-//
-// Permission is hereby granted, free of charge, to any person obtaining a
-// copy of this software and associated documentation files (the
-// "Software"), to deal in the Software without restriction, including
-// without limitation the rights to use, copy, modify, merge, publish,
-// distribute, sublicense, and/or sell copies of the Software, and to permit
-// persons to whom the Software is furnished to do so, subject to the
-// following conditions:
-//
-// The above copyright notice and this permission notice shall be included
-// in all copies or substantial portions of the Software.
-//
-// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
-// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
-// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
-// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
-// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
-// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
-// USE OR OTHER DEALINGS IN THE SOFTWARE.
-
-var common = require('../common');
-var assert = require('assert');
-var Script = require('vm').Script;
-
-common.globalCheck = false;
-
-common.debug('run a string');
-var result = Script.runInThisContext('\'passed\';');
-assert.equal('passed', result);
-
-common.debug('thrown error');
-assert.throws(function() {
-  Script.runInThisContext('throw new Error(\'test\');');
-});
-
-hello = 5;
-Script.runInThisContext('hello = 2');
-assert.equal(2, hello);
-
-
-common.debug('pass values');
-code = 'foo = 1;' +
-       'bar = 2;' +
-       'if (typeof baz !== \'undefined\') throw new Error(\'test fail\');';
-foo = 2;
-obj = { foo: 0, baz: 3 };
-var baz = Script.runInThisContext(code);
-assert.equal(0, obj.foo);
-assert.equal(2, bar);
-assert.equal(1, foo);
-
-common.debug('call a function');
-f = function() { foo = 100 };
-Script.runInThisContext('f()');
-assert.equal(100, foo);

test/simple/test-script-this.js

@@ -49,7 +49,7 @@ code = 'foo = 1;' +
 foo = 2;
 obj = { foo: 0, baz: 3 };
 script = new Script(code);
-script.runInThisContext(script);
+script.runInThisContext();
 assert.equal(0, obj.foo);
 assert.equal(2, bar);
 assert.equal(1, foo);