Fix v8 test suite failure

[polotek]

I'm not sure about this "fix". But this is what got the proxy tests to run on v8. Before this it would throw hard errors before finishing the suite. Spidermonkey (via Firefox) had no such problems. I'm still not sure I fully understand this part of the spec. But it seems that Object.getOwnPropertyDescriptor has some special behavior when used on a proxy. I believe this is your comment here http://wiki.ecmascript.org/doku.php?id=harmony:proxies#non-configurable_properties

What's not clear to me is which js engine is working correctly, SM or v8. This patch allows v8 to complete the tests (with failures). But this patch also makes SM start failing tests. I hope you can shed some light here. I'll also post an issue for the v8 failure with a simple test that demonstrates it.

[polotek]

This is a simple test that demonstrates the failure in v8.

// for node.js
if(typeof require === 'function') {
  require('./reflect.js');
}

// start with configurable property
var target = { test: true }
var proxy = Proxy(target, {});
var desc;

console.log('Configurable property');

desc = Object.getOwnPropertyDescriptor(target, 'test');
console.log('target: ' + desc.configurable);

desc = Object.getOwnPropertyDescriptor(proxy, 'test');
console.log('proxy: ' + desc.configurable);

// change to non-configurable property
Object.defineProperty(target, 'test', { configurable: false });

console.log('Non-configurable property');

desc = Object.getOwnPropertyDescriptor(target, 'test');
console.log('target: ' + desc.configurable);

desc = Object.getOwnPropertyDescriptor(proxy, 'test');
console.log('proxy: ' + desc.configurable);

The last getOwnPropertyDescriptor call on the proxy throws.

[tvcutsem]

Thanks for taking the time to test reflect.js on node.

What's going on here is the following:

v8 currently still implements the "old" version of the Proxy API. As you found out, the "old" spec prevented the getOwnPropertyDescriptor trap from ever returning a non-configurable descriptor on a proxy. Now, SM never implemented this "check", while v8 did. As noted in the preamble, reflect.js currently depends on this check being absent, that's what's meant by the dependency: "the ""old" (i.e. non-direct) Harmony Proxies with non-standard support for passing through non-configurable properties".

In the new Proxy API, which reflect.js is supposed to emulate, this check no longer exists. No problem on SM since the check was never there, but on v8 we would need to explicitly work around it. Your current fix doesn't really solve the issue, since it changes non-configurable into configurable descriptors (I guess that's what causes SM tests to fail).

I think we can fix the issue by modifying the Validator.prototype.getOwnPropertyDescriptor trap on v8 to do what you did (set configurable to true), but additionally set some flag or store some entry into a WeakMap, which the monkey-patched Object.getOwnPropertyDescriptor then checks. If it sees a raised flag, it sets the configurable property to false again, so that clients see the "right" descriptor.

I'll see if I can add such a check soon.

[tvcutsem]

Actually, since reflect.js is in a position to simply monkey-patch Object.getOwnPropertyDescriptor, an even easier fix would be to monkey-patch it similarly to e.g. Object.preventExtensions:

var prim_getOwnPropertyDescriptor = Object.getOwnPropertyDescriptor;

Object.getOwnPropertyDescriptor = function(subject, name) {
  var vhandler = directProxies.get(subject);
  if (vhandler !== undefined) {
    return vhandler.getOwnPropertyDescriptor(name);
  } else {
    return prim_getOwnPropertyDescriptor(subject);
  }
};

By directly calling the getOwnPropertyDescriptor trap ourselves, we circumvent the primitive proxy-trapping mechanism entirely and subvert the check on the return value.

[tvcutsem]

I added the monkey-patched getOwnPropertyDescriptor and the tests under test/testProxies.js now runs fine under v8 --harmony_proxies --harmony_weakmaps (3.11.0).

See https://github.com/tvcutsem/harmony-reflect/commit/9bcdec278a8375fa36360cfd7245895e6fd9dca3

[polotek]

This is awesome thanks. It's difficult parsing the various changes in spec. If I'm understanding you correctly, the old proxies proposal is defunct. But because you're using it to emulate the new proposal spec, we have to work around some of the incompatible ideas in the old spec? That's reasonable.

What I'm doing is using the tests to get familiar with the details of the direct proxies spec, and then using it for some usage experiments. Hopefully I'll be able to share some stuff soon.

[tvcutsem]

You got it: this fix was indeed needed to work around incompatible ideas in the old spec.

I'm well aware that for people not closely following the TC39 standards process, the Proxy API is something of a moving target. This library is an attempt at an accurate reference implementation until the actual built-in Proxy APIs catch up.

Don't hesitate to post further questions when things aren't clear.