From 85b55ed04d789c67dfdf5ee5ea29f3466d68c822 Mon Sep 17 00:00:00 2001 From: Tom Van Cutsem Date: Mon, 24 Sep 2012 14:21:51 +0200 Subject: [PATCH] Fixed issue with returning non-configurable descriptors from getOwnPropertyDescriptor trap. Also patched Object.defineProperty to explicitly throw if the trap returns false (spidermonkey silently ignored a 'false' return value instead of throwing) --- README.md | 1 + reflect.js | 62 +++++++++++++++++++++++++++++++++++++-------- test/testProxies.js | 40 ++++++++++++++--------------- 3 files changed, 73 insertions(+), 30 deletions(-) diff --git a/README.md b/README.md index b4b02fd..adfac25 100644 --- a/README.md +++ b/README.md @@ -54,6 +54,7 @@ After loading `reflect.js` into your page or other JS environment, be aware that Object.getPrototypeOf Object.prototype.valueOf Object.getOwnPropertyDescriptor + Object.defineProperty Function.prototype.toString Date.prototype.toString Proxy diff --git a/reflect.js b/reflect.js index 22a5c12..a6125ca 100644 --- a/reflect.js +++ b/reflect.js @@ -341,7 +341,8 @@ var prim_preventExtensions = Object.preventExtensions, prim_isSealed = Object.isSealed, prim_isFrozen = Object.isFrozen, prim_getPrototypeOf = Object.getPrototypeOf, - prim_getOwnPropertyDescriptor = Object.getOwnPropertyDescriptor; + prim_getOwnPropertyDescriptor = Object.getOwnPropertyDescriptor, + prim_defineProperty = Object.defineProperty; // these will point to the patched versions of the respective methods on // Object. They are used within this module as the "intrinsic" bindings @@ -530,13 +531,14 @@ Validator.prototype = { } } - if (!desc.configurable && !isFixed(name, this.target)) { - // if the property is non-existent on the target, but is reported - // as a non-configurable property, it may later be reported as - // non-existent, which violates the invariant that if the property - // might disappear, the configurable attribute must be true. + if (!desc.configurable && !isSealed(name, this.target)) { + // if the property is configurable or non-existent on the target, + // but is reported as a non-configurable property, it may later be + // reported as configurable or non-existent, which violates the + // invariant that if the property might change or disappear, the + // configurable attribute must be true. throw new TypeError("cannot report a non-configurable descriptor "+ - "for non-existent property '"+name+"'"); + "for configurable or non-existent property '"+name+"'"); } return desc; @@ -606,7 +608,6 @@ Validator.prototype = { var success = trap(this.target, name, desc); success = !!success; // coerce to Boolean - if (success === true) { // Note: we could collapse the following two if-tests into a single @@ -633,7 +634,7 @@ Validator.prototype = { } } - + return success; }, @@ -1343,7 +1344,8 @@ Object.getPrototypeOf = Object_getPrototypeOf = function(subject) { // the Validator.prototype.getOwnPropertyDescriptor trap // This is to circumvent an assertion in the built-in Proxy // trapping mechanism of v8, which disallows that trap to -// return non-configurable property descriptors. +// return non-configurable property descriptors (as per the +// old Proxy design) Object.getOwnPropertyDescriptor = function(subject, name) { var vhandler = directProxies.get(subject); if (vhandler !== undefined) { @@ -1353,6 +1355,39 @@ Object.getOwnPropertyDescriptor = function(subject, name) { } }; +// patch Object.defineProperty to directly call +// the Validator.prototype.defineProperty trap +// This is to circumvent two issues with the built-in +// trap mechanism: +// 1) the current tracemonkey implementation of proxies +// auto-completes 'desc', which is not correct. 'desc' should be +// normalized, but not completed. Consider: +// Object.defineProperty(proxy, 'foo', {enumerable:false}) +// This trap will receive desc = +// {value:undefined,writable:false,enumerable:false,configurable:false} +// This will also set all other attributes to their default value, +// which is unexpected and different from [[DefineOwnProperty]]. +// Bug filed: https://bugzilla.mozilla.org/show_bug.cgi?id=601329 +// 2) the current spidermonkey implementation does not +// throw an exception when this trap returns 'false', but instead silently +// ignores the operation (this is regardless of strict-mode) +// 2a) v8 does throw an exception for this case, but includes the rather +// unhelpful error message: +// 'Proxy handler # returned false from 'defineProperty' trap' +Object.defineProperty = function(subject, name, desc) { + var vhandler = directProxies.get(subject); + if (vhandler !== undefined) { + var normalizedDesc = normalizePropertyDescriptor(desc); + var success = vhandler.defineProperty(name, normalizedDesc); + if (success === false) { + throw new TypeError("can't redefine property '"+name+"'"); + } + return success; + } else { + return prim_defineProperty(subject, name, desc); + } +}; + // returns a new function of zero arguments that recursively // unwraps any proxies specified as the |this|-value. // The primitive is assumed to be a zero-argument method @@ -1403,6 +1438,13 @@ var Reflect = global.Reflect = { return Object.getOwnPropertyNames(target); }, defineProperty: function(target, name, desc) { + + // if target is a proxy, invoke its "defineProperty" trap + var handler = directProxies.get(target); + if (handler !== undefined) { + return handler.defineProperty(target, name, desc); + } + // Implementation transliterated from [[DefineOwnProperty]] // see ES5.1 section 8.12.9 // this is the _exact same algorithm_ as the validateProperty diff --git a/test/testProxies.js b/test/testProxies.js index 15ecc5d..1e1d9a7 100644 --- a/test/testProxies.js +++ b/test/testProxies.js @@ -73,7 +73,9 @@ load('../reflect.js'); fn(); assert(false, 'expected exception, but succeeded. Message was: '+message); } catch(e) { - assert(re.test(e.message) && re.test(message), "assertThrows: "+e.message); + assert(e.message === message, "assertThrows: "+message+" !== " +e.message); + // FIXME: relax test suite message detection + // re.test(e.message) && re.test(message), "assertThrows: "+e.message); } } @@ -205,15 +207,28 @@ load('../reflect.js'); }; TESTS.testCantEmulateNonExistentNonConfigurableProps = - function(brokenProxy, emulatedProps, emulatedProto, success) { + function(brokenProxy, emulatedProps, emulatedProto, success, target) { emulatedProps.x = {value:1,configurable:false}; assertThrows("cannot report a non-configurable descriptor for "+ - "non-existent property 'x'", + "configurable or non-existent property 'x'", + function() { Object.getOwnPropertyDescriptor(brokenProxy, 'x'); }); + }; + + TESTS.testCantEmulateConfigurableAsNonConfigurableProps = + function(brokenProxy, emulatedProps, emulatedProto, success, target) { + Object.defineProperty(target, 'x', { + value:1, + configurable:true, + writable:true, + enumerable:true}); + emulatedProps.x = {value:1,configurable:false,writable:true,enumerable:true}; + assertThrows("cannot report a non-configurable descriptor for "+ + "configurable or non-existent property 'x'", function() { Object.getOwnPropertyDescriptor(brokenProxy, 'x'); }); }; TESTS.testCantDefineNonExistentNonConfigurableProp = - function(brokenProxy, emulatedProps, emulatedProto, success) { + function(brokenProxy, emulatedProps, emulatedProto, success, target) { success.x = true; assertThrows("cannot successfully define a non-configurable "+ "descriptor for non-existent property 'x'", @@ -271,21 +286,6 @@ load('../reflect.js'); }); }; - TESTS.testNonConfigurableMergeOnProtect = - function(brokenProxy, emulatedProps, emulatedProto, success, target) { - emulatedProps.x = {value:1,configurable:false}; - // to emulate non-configurable props, must make sure they exist on target - Object.defineProperty(target, 'x', emulatedProps.x); - var result = Object.getOwnPropertyDescriptor(brokenProxy, 'x'); - assert(result.value === 1 && result.configurable === false, - 'x was observed as non-configurable'); - emulatedProps.x = {value:1,configurable:true}; - // fixing the proxy will merge all reported properties returned - // by the fix() trap with existing properties - assertThrows("can't redefine non-configurable property 'x'", - function() { Object.preventExtensions(brokenProxy); }); - }; - TESTS.testNonConfigurableNoDelete = function(brokenProxy, emulatedProps, emulatedProto, success, target) { emulatedProps.x = {value:1, configurable:false}; @@ -479,7 +479,7 @@ load('../reflect.js'); assert(target.x === 1, 'target.x === 1'); assert(proxy.x === 1, 'proxy.x === 1'); - assertThrows("can't redefine non-configurable property 'x'", + assertThrows("can't redefine property 'x'", function() { Object.defineProperty(proxy,'x',{configurable:false,value:2}); });