vm: access to Symbols on global context does not work across sandbox boundary

[Sebmaster]

This is more a guess than anything, but apparently accessing Symbols in a vm context is not forwarded to the original object handle, resulting in different values depending on which side of the sandbox boundary the access happens.

Reduced test case:

"use strict";

var vm = require("vm");

var symbol = Symbol();

function Document() {
  this[symbol] = "foo";
  this.prop = "bar";
}

Document.prototype.symbol = function () {
  return this[symbol];
};

Document.prototype.property = function () {
  return this.prop;
};

var context = new Document();
vm.createContext(context);

console.log(context.symbol());
console.log(vm.runInContext("this.symbol()", context)); // should be foo, returns undefined

// compare:

console.log(context.property());
console.log(vm.runInContext("this.property()", context)); // should be bar, correct

[domenic]

Might be more global vs. global proxy stuff (#855), or might be because @isaacs's good ol' hack uses the V8 GetOwnPropertyNames API which probably doesn't give back symbols.

[bnoordhuis]

I think it's a bit of both. v8::Object::GetOwnPropertyNames() indeed doesn't return symbols. That could be scripted around if it weren't for #864 because:

1. v8::Object::GetOwnPropertyNames is subtly incompatible with Object.getOwnPropertyNames(), see https://code.google.com/p/v8/issues/detail?id=3861
2. There is no v8::Object::GetOwnPropertySymbols() counterpart to Object.getOwnPropertySymbols(), see https://code.google.com/p/v8/issues/detail?id=3901
3. Thanks to Change from 0.10 => 1.2; enumerable of this.escape in vm.runInNewContext was false, now true #864, it's not possible to tell built-in properties apart from user-defined ones because everything is enumerable.

Combined, it makes it pretty much impossible to make it work in either C++ or JS. Fixing #864 isn't easy either because you can't look up a property's attributes without going through interceptors (creating infinite recursion) like you can for a property's value.

EDIT: s/#855/#864/ - linked to the wrong issue.

[bnoordhuis]

On the upside, adding a v8::Object::GetRealNamedPropertyAttributes() method turned out pretty straightforward. When I have some time, I'll try to get it landed upstream.

diff --git a/deps/v8/src/api.cc b/deps/v8/src/api.cc
index 88d3c88..e16a594 100644
--- a/deps/v8/src/api.cc
+++ b/deps/v8/src/api.cc
@@ -3774,6 +3774,23 @@ Local<Value> v8::Object::GetRealNamedProperty(Handle<String> key) {
 }


+PropertyAttribute v8::Object::GetRealNamedPropertyAttributes(
+    Handle<String> key) {
+  i::Isolate* isolate = Utils::OpenHandle(this)->GetIsolate();
+  ON_BAILOUT(isolate, "v8::Object::GetRealNamedPropertyAttributes()",
+             return static_cast<PropertyAttribute>(NONE));
+  ENTER_V8(isolate);
+  i::Handle<i::JSObject> self_obj = Utils::OpenHandle(this);
+  i::Handle<i::String> key_obj = Utils::OpenHandle(*key);
+  i::LookupIterator it(self_obj, key_obj,
+                       i::LookupIterator::PROTOTYPE_CHAIN_SKIP_INTERCEPTOR);
+  Maybe<PropertyAttributes> result = self_obj->GetPropertyAttributes(&it);
+  DCHECK(result.has_value);
+  if (result.value == ABSENT) return static_cast<PropertyAttribute>(NONE);
+  return static_cast<PropertyAttribute>(result.value);
+}
+
+
 // Turns on access checks by copying the map and setting the check flag.
 // Because the object gets a new map, existing inline cache caching
 // the old map of this object will fail.

[domenic]

One thing that might be helpful in fixing this is using ObjectTemplate::SetHandler instead of SetNamedPropertyHandler. The latter calls the former with PropertyHandlerFlags::kOnlyInterceptStrings which sounds like exactly the opposite of what we want. Going to try it soon...

[Sebmaster]

Seems like this is a change from how v8-master to how it is handled in the version currently in node. Seems like SetHandler takes a bool in the currently pulled version, indicating whether it takes into account symbols.

[domenic]

Yeah I am working in the next branch on this.

[domenic]

This is harder than it seems because to use v8::Name you need to buy in to the MaybeLocal revolution.

[bnoordhuis]

Fixed by 9002cc2.