vm: fix symbol access

By using the new SetHandler API instead of SetNamedPropertyHandler, we can intercept symbols now. This forces us to use Maybes and MaybeLocals more, since this new API does not have a non-maybe variant. Fixes nodejs#884.
domenic · Jun 1, 2015 · 5f69cda · 5f69cda
1 parent 6443054
commit 5f69cda
Show file tree

Hide file tree

Showing 2 changed files with 67 additions and 28 deletions.
diff --git a/src/node_contextify.cc b/src/node_contextify.cc
@@ -26,6 +26,10 @@ using v8::HandleScope;
 using v8::Integer;
 using v8::Isolate;
 using v8::Local;
+using v8::Maybe;
+using v8::MaybeLocal;
+using v8::Name;
+using v8::NamedPropertyHandlerConfiguration;
 using v8::None;
 using v8::Object;
 using v8::ObjectTemplate;
@@ -202,12 +206,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 +348,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 +357,26 @@ class ContextifyContext {
         Unwrap<ContextifyContext>(args.Data().As<Object>());
 
     Local<Object> sandbox = PersistentToLocal(isolate, ctx->sandbox_);
-    Local<Value> rv = sandbox->GetRealNamedProperty(property);
-    if (rv.IsEmpty()) {
+    MaybeLocal<Value> maybe_rv =
+        sandbox->GetRealNamedProperty(ctx->context(), property);
+    if (maybe_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_);
+      maybe_rv = proxy_global->GetRealNamedProperty(ctx->context(), property);
     }
 
-    args.GetReturnValue().Set(rv);
+    Local<Value> rv;
+    if (maybe_rv.ToLocal(&rv)) {
+      if (rv == ctx->sandbox_)
+        rv = PersistentToLocal(isolate, ctx->proxy_global_);
+
+      args.GetReturnValue().Set(rv);
+    }
   }
 
 
   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);
@@ -389,35 +399,39 @@ class ContextifyContext {
         Unwrap<ContextifyContext>(args.Data().As<Object>());
 
     Local<Object> sandbox = PersistentToLocal(isolate, ctx->sandbox_);
-    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)) {
-      PropertyAttribute propAttr =
-          proxy_global->GetRealNamedPropertyAttributes(property).FromJust();
-      args.GetReturnValue().Set(propAttr);
-    } else {
-      args.GetReturnValue().Set(None);
+    Maybe<PropertyAttribute> maybe_prop_attr =
+        sandbox->GetRealNamedPropertyAttributes(ctx->context(), property);
+
+    if (maybe_prop_attr.IsNothing()) {
+      Local<Object> proxy_global = PersistentToLocal(isolate,
+          ctx->proxy_global_);
+
+      maybe_prop_attr =
+          proxy_global->GetRealNamedPropertyAttributes(ctx->context(),
+              property);
+    }
+
+    if (maybe_prop_attr.IsJust()) {
+      PropertyAttribute prop_attr = maybe_prop_attr.FromJust();
+      args.GetReturnValue().Set(prop_attr);
     }
   }
 
 
   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_);
+
+    Maybe<bool> success = sandbox->Delete(ctx->context(), property);
 
-    bool success = PersistentToLocal(isolate,
-                                     ctx->sandbox_)->Delete(property);
-    args.GetReturnValue().Set(success);
+    if (success.IsJust())
+      args.GetReturnValue().Set(success.FromJust());
   }
 
 

diff --git a/test/parallel/test-vm-symbols.js b/test/parallel/test-vm-symbols.js
@@ -0,0 +1,25 @@
+'use strict';
+
+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');