vm: fix symbol access

By using the new SetHandler API instead of SetNamedPropertyHandler, we can
intercept symbols now.

Fixes #884.

Author: domenic

src/node_contextify.cc

@@ -26,6 +26,9 @@ using v8::HandleScope;
 using v8::Integer;
 using v8::Isolate;
 using v8::Local;
+using v8::MaybeLocal;
+using v8::Name;
+using v8::NamedPropertyHandlerConfiguration;
 using v8::None;
 using v8::Object;
 using v8::ObjectTemplate;
@@ -202,12 +205,14 @@ class ContextifyContext {
 
     Local<ObjectTemplate> object_template =
         function_template->InstanceTemplate();
-    object_template->SetNamedPropertyHandler(GlobalPropertyGetterCallback,
+
+    NamedPropertyHandlerConfiguration config(GlobalPropertyGetterCallback,
                                              GlobalPropertySetterCallback,
                                              GlobalPropertyQueryCallback,
                                              GlobalPropertyDeleterCallback,
                                              GlobalPropertyEnumeratorCallback,
                                              CreateDataWrapper(env));
+    object_template->SetHandler(config);
 
     Local<Context> ctx = Context::New(env->isolate(), nullptr, object_template);
     if (!ctx.IsEmpty())
@@ -342,7 +347,7 @@ class ContextifyContext {
 
 
   static void GlobalPropertyGetterCallback(
-      Local<String> property,
+      Local<Name> property,
       const PropertyCallbackInfo<Value>& args) {
     Isolate* isolate = args.GetIsolate();
     HandleScope scope(isolate);
@@ -351,22 +356,27 @@ class ContextifyContext {
         Unwrap<ContextifyContext>(args.Data().As<Object>());
 
     Local<Object> sandbox = PersistentToLocal(isolate, ctx->sandbox_);
-    Local<Value> rv = sandbox->GetRealNamedProperty(property);
+    MaybeLocal<Value> rv =
+      sandbox->GetRealNamedProperty(ctx->context(), property);
     if (rv.IsEmpty()) {
       Local<Object> proxy_global = PersistentToLocal(isolate,
                                                      ctx->proxy_global_);
-      rv = proxy_global->GetRealNamedProperty(property);
-    }
-    if (!rv.IsEmpty() && rv == ctx->sandbox_) {
-      rv = PersistentToLocal(isolate, ctx->proxy_global_);
+      rv = proxy_global->GetRealNamedProperty(ctx->context(), property);
     }
 
-    args.GetReturnValue().Set(rv);
+    Local<Value> localRV;
+    if (rv.ToLocal(&localRV)) {
+      if (localRV == ctx->sandbox_) {
+        rv = PersistentToLocal(isolate, ctx->proxy_global_);
+      }
+
+      args.GetReturnValue().Set(localRV);
+    }
   }
 
 
   static void GlobalPropertySetterCallback(
-      Local<String> property,
+      Local<Name> property,
       Local<Value> value,
       const PropertyCallbackInfo<Value>& args) {
     Isolate* isolate = args.GetIsolate();
@@ -380,7 +390,7 @@ class ContextifyContext {
 
 
   static void GlobalPropertyQueryCallback(
-      Local<String> property,
+      Local<Name> property,
       const PropertyCallbackInfo<Integer>& args) {
     Isolate* isolate = args.GetIsolate();
     HandleScope scope(isolate);
@@ -392,31 +402,43 @@ class ContextifyContext {
     Local<Object> proxy_global = PersistentToLocal(isolate,
                                                    ctx->proxy_global_);
 
-    if (sandbox->HasRealNamedProperty(property)) {
-      PropertyAttribute propAttr =
-        sandbox->GetRealNamedPropertyAttributes(property).FromJust();
-      args.GetReturnValue().Set(propAttr);
-    } else if (proxy_global->HasRealNamedProperty(property)) {
+    bool in_sandbox =
+      sandbox->HasRealNamedProperty(ctx->context(), property).FromMaybe(false);
+
+    if (in_sandbox) {
       PropertyAttribute propAttr =
-        proxy_global->GetRealNamedPropertyAttributes(property).FromJust();
+        sandbox->
+          GetRealNamedPropertyAttributes(ctx->context(), property).FromJust();
       args.GetReturnValue().Set(propAttr);
     } else {
-      args.GetReturnValue().Set(None);
+      bool in_proxy_global =
+        proxy_global->HasRealNamedProperty(ctx->context(), property)
+        .FromMaybe(false);
+      if (in_proxy_global) {
+        PropertyAttribute propAttr =
+          proxy_global->
+            GetRealNamedPropertyAttributes(ctx->context(), property)
+              .FromJust();
+        args.GetReturnValue().Set(propAttr);
+      }
+      else {
+        args.GetReturnValue().Set(None);
+      }
     }
   }
 
 
   static void GlobalPropertyDeleterCallback(
-      Local<String> property,
+      Local<Name> property,
       const PropertyCallbackInfo<Boolean>& args) {
     Isolate* isolate = args.GetIsolate();
     HandleScope scope(isolate);
 
     ContextifyContext* ctx =
         Unwrap<ContextifyContext>(args.Data().As<Object>());
+    Local<Object> sandbox = PersistentToLocal(isolate, ctx->sandbox_);
 
-    bool success = PersistentToLocal(isolate,
-                                     ctx->sandbox_)->Delete(property);
+    bool success = sandbox->Delete(ctx->context(), property).FromMaybe(false);
     args.GetReturnValue().Set(success);
   }
 

test/parallel/test-vm-symbols.js

@@ -0,0 +1,23 @@
+var common = require('../common');
+var assert = require('assert');
+
+var vm = require('vm');
+
+var symbol = Symbol();
+
+function Document() {
+  this[symbol] = 'foo';
+}
+
+Document.prototype.getSymbolValue = function () {
+  return this[symbol];
+};
+
+var context = new Document();
+vm.createContext(context);
+
+assert.equal(context.getSymbolValue(), 'foo',
+    'should return symbol-keyed value from the outside');
+
+assert.equal(vm.runInContext('this.getSymbolValue()', context), 'foo',
+    'should return symbol-keyed value from the inside');