vm: next branch (V8 4.3) has test failures

[domenic]

See #1632 for details. Creating this to track.

What I've tried so far (no success):

• Converting GlobalPropertyGetterCallback to use MaybeLocals. I might have been doing this wrong though?
• vm: fix a variety of bugs, using V8 4.3 APIs #1773, which was a nice fix but turns out to be unrelated.

[domenic]

@jeisinger can you think of anything that might have changed between 4.2 and 4.3 regarding ObjectTemplate::SetNamedPropertyHandler and friends that would change a

foo.bar = 5;
^
ReferenceError: foo is not defined

to a

foo.bar = 5;
        ^
TypeError: Cannot set property 'bar' of undefined

?

[domenic]

Barest-bones repro on top of contextify:

const binding = process.binding('contextify');

const context = {};
binding.makeContext(context);

const script = new binding.ContextifyScript('foo;');
var foo = script.runInContext(context); // should throw an exception

console.log(foo); // logs undefined

My guess is that something in the Local -> MaybeLocal transition made it so that errors get swallowed unless you're more careful about things?

[domenic]

Another repro:

const binding = process.binding('contextify');

const context = {};
binding.makeContext(context);

const script = new binding.ContextifyScript('"asdfasdfdf" in this;');
console.log(script.runInContext(context)); // logs true, not false

This implies it's less about exceptions and more about screwing up the callbacks to somehow always signal something is present/undefined.

[domenic]

OK, I can fix the second test case, by ensuring GlobalPropertyQueryCallback returns an empty handle if there is no property, instead of returning None. (This appears to be a V8 change.)

The first test case (and the one in the OP) is less tractable. I am debugging through GlobalPropertyGetterCallback and it seems like while the sandbox gives back an empty handle, the proxy global does not. I wonder if there was some change in V8 semantics such that global objects for contexts now return undefined instead of empty handles when calling GetRealNamedProperty ??

[domenic]

At this point I am 90% sure this is a V8 bug caused by this commit v8/v8@6130b02

The new code for GetRealNamedProperty is as follows:

MaybeLocal<Value> v8::Object::GetRealNamedProperty(Local<Context> context,
                                                   Local<Name> key) {
  PREPARE_FOR_EXECUTION(
      context, "v8::Object::GetRealNamedPropertyInPrototypeChain()", Value);
  auto self = Utils::OpenHandle(this);
  auto key_obj = Utils::OpenHandle(*key);
  i::LookupIterator it(self, key_obj,
                       i::LookupIterator::PROTOTYPE_CHAIN_SKIP_INTERCEPTOR);
  if (!it.IsFound()) return MaybeLocal<Value>();
  Local<Value> result;
  has_pending_exception = !ToLocal<Value>(i::Object::GetProperty(&it), &result);
  RETURN_ON_FAILED_EXECUTION(Value);
  RETURN_ESCAPED(result);
}

The line if (!it.IsFound()) return MaybeLocal<Value>(); is incorrect, because it is still in its initial state. It should be after the call to i::Object::GetProperty(&it).

Reported at https://code.google.com/p/v8/issues/detail?id=4143. If someone else wants to give a patch a try, that'd be great, as I already took much of today to work on io.js and I should go back to Chrome soon.

[targos]

It has just been fixed in master v8/v8@7b24219

[jeisinger]

I'll merge that soon to 4.4 and 4.3

[rvagg]

#1805 floated that patch, looking good sans a couple of odd failures

[domenic]

Closing since this is fixed in next via the floating patch (70716fd). We should also update V8 in next to the latest branch head though so we can stop floating the patch; I will open a separate issue.