From 943b2c61a80d24d67e353c90abd7ec6f1549f6d4 Mon Sep 17 00:00:00 2001
From: isaacs <i@izs.me>
Date: Mon, 15 Mar 2010 23:27:21 -0700
Subject: [PATCH] Make evalcx work like it's supposed to.

1. Move the context->Enter() call so that the global obj is available for writing.
2. On success, copy the modified global out to the sandbox object.
3. Don't copy functions in either direction.  They have scope and closures, and make for craziness when trying to keep contexts separate.
4. Only do the ->ToObject->Clone() on objects, so that simple values stay simple.
5. Update the test so that it tests all this stuff.
---
 src/node.cc                 | 24 ++++++++++++++++++++----
 test/simple/test-eval-cx.js | 17 +++++++++++------
 2 files changed, 31 insertions(+), 10 deletions(-)

diff --git a/src/node.cc b/src/node.cc
index cedcc64d74..78b0def92e 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -862,6 +862,9 @@ Handle<Value> EvalCX(const Arguments& args) {
   // Create the new context
   Persistent<Context> context = Context::New();
 
+  // Enter and compile script
+  context->Enter();
+
   // Copy objects from global context, to our brand new context
   Handle<Array> keys = sandbox->GetPropertyNames();
 
@@ -869,12 +872,13 @@ Handle<Value> EvalCX(const Arguments& args) {
   for (i = 0; i < keys->Length(); i++) {
     Handle<String> key = keys->Get(Integer::New(i))->ToString();
     Handle<Value> value = sandbox->Get(key);
-    context->Global()->Set(key, value->ToObject()->Clone());
+    if (value->IsFunction()) continue;
+    if (value->IsObject()) {
+      value = value->ToObject()->Clone();
+    }
+    context->Global()->Set(key, value);
   }
 
-  // Enter and compile script
-  context->Enter();
-
   // Catch errors
   TryCatch try_catch;
 
@@ -887,6 +891,18 @@ Handle<Value> EvalCX(const Arguments& args) {
     result = script->Run();
     if (result.IsEmpty()) {
       result = ThrowException(try_catch.Exception());
+    } else {
+      // success! copy changes back onto the sandbox object.
+      keys = context->Global()->GetPropertyNames();
+      for (i = 0; i < keys->Length(); i++) {
+        Handle<String> key = keys->Get(Integer::New(i))->ToString();
+        Handle<Value> value = context->Global()->Get(key);
+        if (value->IsFunction()) continue;
+        if (value->IsObject()) {
+          value = value->ToObject()->Clone();
+        }
+        sandbox->Set(key, value);
+      }
     }
   }
 
diff --git a/test/simple/test-eval-cx.js b/test/simple/test-eval-cx.js
index 32d5c94436..49b8ca10a0 100644
--- a/test/simple/test-eval-cx.js
+++ b/test/simple/test-eval-cx.js
@@ -14,14 +14,19 @@ process.evalcx('hello = 2');
 assert.equal(5, hello);
 
 
-code = "foo = 1; bar = 2;";
-foo = 2;
-obj = { foo : 0 };
-process.evalcx(code, obj);
+code = "foo = 1;"
+     + "bar = 2;"
+     + "if (baz !== 3) throw new Error('test fail');"
+     + "quux.pwned = true;";
 
-/* TODO?
+foo = 2;
+var quux = { pwned : false };
+obj = { foo : 0, baz : 3, quux : quux };
+var baz = process.evalcx(code, obj);
 assert.equal(1, obj.foo);
 assert.equal(2, obj.bar);
-*/
+assert.equal(obj.quux.pwned, true);
+assert.equal(quux.pwned, false);
+assert.notEqual(quux, obj.quux);
 
 assert.equal(2, foo);