Fixed issue with returning non-configurable descriptors from getOwnPr…

…opertyDescriptor 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

@@ -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

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 #<Object> 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

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});
       });