Bug 978236 - Implement Proxy.[[DefineProperty]] to ES6 standard. (r=jorendorff)
authorEric Faust <efaustbmo@gmail.com>
Tue, 03 Jun 2014 13:00:59 -0700
changeset 207309 6934964f2c99157fcb9435dccfbf82d50c313633
parent 207308 be43c638938711dc5f36a341db2d7e2e507e4c0c
child 207310 257a99604e3009420bb4f7811586a145b0531d1f
push id494
push userraliiev@mozilla.com
push dateMon, 25 Aug 2014 18:42:16 +0000
treeherdermozilla-release@a3cc3e46b571 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjorendorff
bugs978236
milestone32.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 978236 - Implement Proxy.[[DefineProperty]] to ES6 standard. (r=jorendorff)
js/src/jit-test/tests/proxy/bug897403.js
js/src/jsobj.cpp
js/src/jsproxy.cpp
js/src/tests/ecma_5/extensions/proxy-array-target-length-definition.js
--- a/js/src/jit-test/tests/proxy/bug897403.js
+++ b/js/src/jit-test/tests/proxy/bug897403.js
@@ -1,3 +1,5 @@
+// |jit-test| error: TypeError
+
 var f = (function () {}).bind({});
 var p = new Proxy(f, {});
 Object.defineProperty(p, "caller", {get: function(){}});
--- a/js/src/jsobj.cpp
+++ b/js/src/jsobj.cpp
@@ -1044,20 +1044,16 @@ js::DefineProperty(JSContext *cx, Handle
                    bool throwError, bool *rval)
 {
     if (obj->is<ArrayObject>()) {
         Rooted<ArrayObject*> arr(cx, &obj->as<ArrayObject>());
         return DefinePropertyOnArray(cx, arr, id, desc, throwError, rval);
     }
 
     if (obj->getOps()->lookupGeneric) {
-        /*
-         * FIXME: Once ScriptedIndirectProxies are removed, this code should call
-         * TrapDefineOwnProperty directly
-         */
         if (obj->is<ProxyObject>()) {
             RootedValue pd(cx, desc.descriptorValue());
             return Proxy::defineProperty(cx, obj, id, pd);
         }
         return Reject(cx, obj, JSMSG_OBJECT_NOT_EXTENSIBLE, throwError, rval);
     }
 
     return DefinePropertyOnObject(cx, obj, id, desc, throwError, rval);
@@ -1130,20 +1126,16 @@ js::DefineProperties(JSContext *cx, Hand
         for (size_t i = 0, len = ids.length(); i < len; i++) {
             if (!DefinePropertyOnArray(cx, arr, ids[i], descs[i], true, &dummy))
                 return false;
         }
         return true;
     }
 
     if (obj->getOps()->lookupGeneric) {
-        /*
-         * FIXME: Once ScriptedIndirectProxies are removed, this code should call
-         * TrapDefineOwnProperty directly
-         */
         if (obj->is<ProxyObject>()) {
             for (size_t i = 0, len = ids.length(); i < len; i++) {
                 RootedValue pd(cx, descs[i].descriptorValue());
                 if (!Proxy::defineProperty(cx, obj, ids[i], pd))
                     return false;
             }
             return true;
         }
--- a/js/src/jsproxy.cpp
+++ b/js/src/jsproxy.cpp
@@ -1194,40 +1194,38 @@ IsDataDescriptor(const PropertyDescripto
 }
 
 static inline bool
 IsAccessorDescriptor(const PropertyDescriptor &desc)
 {
     return desc.obj && desc.attrs & (JSPROP_GETTER | JSPROP_SETTER);
 }
 
-// Aux.5 ValidateProperty(O, P, Desc)
+// ES6 (5 April 2014) ValidateAndApplyPropertyDescriptor(O, P, Extensible, Desc, Current)
+// Since we are actually performing 9.1.6.2 IsCompatiblePropertyDescriptor(Extensible, Desc,
+// Current), some parameters are omitted.
 static bool
-ValidateProperty(JSContext *cx, HandleObject obj, HandleId id, Handle<PropDesc> desc, bool *bp)
+ValidatePropertyDescriptor(JSContext *cx, Handle<PropDesc> desc, Handle<PropertyDescriptor> current,
+                           bool *bp)
 {
-    // step 1
-    Rooted<PropertyDescriptor> current(cx);
-    if (!GetOwnPropertyDescriptor(cx, obj, id, &current))
-        return false;
-
     /*
-     * steps 2-4 are redundant since ValidateProperty is never called unless
+     * step 2 is redundant since ValidatePropertyDescriptor is never called unless
      * target.[[HasOwn]](P) is true
      */
     JS_ASSERT(current.object());
 
-    // step 5
+    // step 3
     if (!desc.hasValue() && !desc.hasWritable() && !desc.hasGet() && !desc.hasSet() &&
         !desc.hasEnumerable() && !desc.hasConfigurable())
     {
         *bp = true;
         return true;
     }
 
-    // step 6
+    // step 4
     if ((!desc.hasWritable() || desc.writable() == !current.isReadonly()) &&
         (!desc.hasGet() || desc.getter() == current.getter()) &&
         (!desc.hasSet() || desc.setter() == current.setter()) &&
         (!desc.hasEnumerable() || desc.enumerable() == current.isEnumerable()) &&
         (!desc.hasConfigurable() || desc.configurable() == !current.isPermanent()))
     {
         if (!desc.hasValue()) {
             *bp = true;
@@ -1237,46 +1235,46 @@ ValidateProperty(JSContext *cx, HandleOb
         if (!SameValue(cx, desc.value(), current.value(), &same))
             return false;
         if (same) {
             *bp = true;
             return true;
         }
     }
 
-    // step 7
+    // step 5
     if (current.isPermanent()) {
         if (desc.hasConfigurable() && desc.configurable()) {
             *bp = false;
             return true;
         }
 
         if (desc.hasEnumerable() &&
             desc.enumerable() != current.isEnumerable())
         {
             *bp = false;
             return true;
         }
     }
 
-    // step 8
+    // step 6
     if (desc.isGenericDescriptor()) {
         *bp = true;
         return true;
     }
 
-    // step 9
+    // step 7a
     if (IsDataDescriptor(current) != desc.isDataDescriptor()) {
         *bp = !current.isPermanent();
         return true;
     }
 
-    // step 10
+    // step 8
     if (IsDataDescriptor(current)) {
-        JS_ASSERT(desc.isDataDescriptor()); // by step 9
+        JS_ASSERT(desc.isDataDescriptor()); // by step 7a
         if (current.isPermanent() && current.isReadonly()) {
             if (desc.hasWritable() && desc.writable()) {
                 *bp = false;
                 return true;
             }
 
             if (desc.hasValue()) {
                 bool same;
@@ -1288,25 +1286,37 @@ ValidateProperty(JSContext *cx, HandleOb
                 }
             }
         }
 
         *bp = true;
         return true;
     }
 
-    // steps 11-12
-    JS_ASSERT(IsAccessorDescriptor(current)); // by step 10
-    JS_ASSERT(desc.isAccessorDescriptor()); // by step 9
+    // step 9
+    JS_ASSERT(IsAccessorDescriptor(current)); // by step 8
+    JS_ASSERT(desc.isAccessorDescriptor()); // by step 7a
     *bp = (!current.isPermanent() ||
            ((!desc.hasSet() || desc.setter() == current.setter()) &&
             (!desc.hasGet() || desc.getter() == current.getter())));
     return true;
 }
 
+static bool
+ValidatePropertyDescriptor(JSContext *cx, HandleObject obj, HandleId id, Handle<PropDesc> desc, bool *bp)
+{
+    // step 1
+    Rooted<PropertyDescriptor> current(cx);
+    if (!GetOwnPropertyDescriptor(cx, obj, id, &current))
+        return false;
+
+    return ValidatePropertyDescriptor(cx, desc, current, bp);
+}
+
+
 // Aux.6 IsSealed(O, P)
 static bool
 IsSealed(JSContext* cx, HandleObject obj, HandleId id, bool *bp)
 {
     // step 1
     Rooted<PropertyDescriptor> desc(cx);
     if (!GetOwnPropertyDescriptor(cx, obj, id, &desc))
         return false;
@@ -1429,17 +1439,17 @@ TrapGetOwnProperty(JSContext *cx, Handle
 
     Rooted<PropDesc> desc(cx);
     if (!desc.initialize(cx, trapResult))
         return false;
 
     /* step 10 */
     if (isFixed) {
         bool valid;
-        if (!ValidateProperty(cx, target, id, desc, &valid))
+        if (!ValidatePropertyDescriptor(cx, target, id, desc, &valid))
             return false;
 
         if (!valid) {
             JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_CANT_REPORT_INVALID);
             return false;
         }
     }
 
@@ -1449,101 +1459,16 @@ TrapGetOwnProperty(JSContext *cx, Handle
         return false;
     }
 
     // step 12
     rval.set(trapResult);
     return true;
 }
 
-// TrapDefineOwnProperty(O, P, DescObj, Throw)
-static bool
-TrapDefineOwnProperty(JSContext *cx, HandleObject proxy, HandleId id, MutableHandleValue vp)
-{
-    // step 1
-    RootedObject handler(cx, GetDirectProxyHandlerObject(proxy));
-
-    // step 2
-    RootedObject target(cx, proxy->as<ProxyObject>().target());
-
-    // step 3
-    RootedValue trap(cx);
-    if (!JSObject::getProperty(cx, handler, handler, cx->names().defineProperty, &trap))
-        return false;
-
-    // step 4
-    if (trap.isUndefined()) {
-        Rooted<PropertyDescriptor> desc(cx);
-        if (!ParsePropertyDescriptorObject(cx, proxy, vp, &desc))
-            return false;
-        return JS_DefinePropertyById(cx, target, id, desc.value(), desc.attributes(),
-                                     desc.getter(), desc.setter());
-    }
-
-    // step 5
-    RootedValue normalizedDesc(cx, vp);
-    if (!NormalizePropertyDescriptor(cx, &normalizedDesc))
-        return false;
-
-    // step 6
-    RootedValue value(cx);
-    if (!IdToExposableValue(cx, id, &value))
-        return false;
-    Value argv[] = {
-        ObjectValue(*target),
-        value,
-        normalizedDesc
-    };
-    RootedValue trapResult(cx);
-    if (!Invoke(cx, ObjectValue(*handler), trap, ArrayLength(argv), argv, &trapResult))
-        return false;
-
-    // steps 7-8
-    if (ToBoolean(trapResult)) {
-        bool isFixed;
-        if (!HasOwn(cx, target, id, &isFixed))
-            return false;
-
-        bool extensible;
-        if (!JSObject::isExtensible(cx, target, &extensible))
-            return false;
-        if (!extensible && !isFixed) {
-            JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_CANT_DEFINE_NEW);
-            return false;
-        }
-
-        Rooted<PropDesc> desc(cx);
-        if (!desc.initialize(cx, normalizedDesc))
-            return false;
-
-        if (isFixed) {
-            bool valid;
-            if (!ValidateProperty(cx, target, id, desc, &valid))
-                return false;
-            if (!valid) {
-                JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_CANT_DEFINE_INVALID);
-                return false;
-            }
-        }
-
-        if (!desc.configurable() && !isFixed) {
-            JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_CANT_DEFINE_NE_AS_NC);
-            return false;
-        }
-
-        vp.set(BooleanValue(true));
-        return true;
-    }
-
-    // step 9
-    // FIXME: API does not include a Throw parameter
-    vp.set(BooleanValue(false));
-    return true;
-}
-
 static inline void
 ReportInvalidTrapResult(JSContext *cx, JSObject *proxy, JSAtom *atom)
 {
     RootedValue v(cx, ObjectOrNullValue(proxy));
     JSAutoByteString bytes;
     if (!AtomToPrintableString(cx, atom, &bytes))
         return;
     js_ReportValueError2(cx, JSMSG_INVALID_TRAP_RESULT, JSDVG_IGNORE_STACK, v,
@@ -1739,29 +1664,100 @@ ScriptedDirectProxyHandler::getOwnProper
         desc.object().set(nullptr);
         return true;
     }
 
     // steps 3-4
     return ParsePropertyDescriptorObject(cx, proxy, v, desc, true);
 }
 
+// ES6 (5 April 2014) Proxy.[[DefineOwnProperty]](O,P)
 bool
 ScriptedDirectProxyHandler::defineProperty(JSContext *cx, HandleObject proxy, HandleId id,
                                            MutableHandle<PropertyDescriptor> desc)
 {
-    // step 1
-    Rooted<PropDesc> d(cx);
-    d.initFromPropertyDescriptor(desc);
-    RootedValue v(cx);
-    if (!FromGenericPropertyDescriptor(cx, &d, &v))
+    // step 2
+    RootedObject handler(cx, GetDirectProxyHandlerObject(proxy));
+
+    // TODO: step 3: Implement revocation semantics. See bug 978279.
+
+    // step 4
+    RootedObject target(cx, proxy->as<ProxyObject>().target());
+
+    // step 5-6
+    RootedValue trap(cx);
+    if (!JSObject::getProperty(cx, handler, handler, cx->names().defineProperty, &trap))
+         return false;
+
+    // step 7
+    if (trap.isUndefined())
+        return DirectProxyHandler::defineProperty(cx, proxy, id, desc);
+
+    // step 8-9, with 9 blatantly defied.
+    // FIXME: This is incorrect with respect to [[Origin]]. See bug 601379.
+    RootedValue descObj(cx);
+    if (!NewPropertyDescriptorObject(cx, desc, &descObj))
+        return false;
+
+    // step 10, 12
+    RootedValue propKey(cx);
+    if (!IdToExposableValue(cx, id, &propKey))
+        return false;
+
+    Value argv[] = {
+        ObjectValue(*target),
+        propKey,
+        descObj
+    };
+    RootedValue trapResult(cx);
+    if (!Invoke(cx, ObjectValue(*handler), trap, ArrayLength(argv), argv, &trapResult))
         return false;
 
-    // step 2
-    return TrapDefineOwnProperty(cx, proxy, id, &v);
+    // step 11, 13
+    if (ToBoolean(trapResult)) {
+        // step 14-15
+        Rooted<PropertyDescriptor> targetDesc(cx);
+        if (!GetOwnPropertyDescriptor(cx, target, id, &targetDesc))
+            return false;
+
+        // step 16-17
+        bool extensibleTarget;
+        if (!JSObject::isExtensible(cx, target, &extensibleTarget))
+            return false;
+
+        // step 18-19
+        bool settingConfigFalse = desc.isPermanent();
+        if (!targetDesc.object()) {
+            // step 20a
+            if (!extensibleTarget) {
+                JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_CANT_DEFINE_NEW);
+                return false;
+            }
+            // step 20b
+            if (settingConfigFalse) {
+                JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_CANT_DEFINE_NE_AS_NC);
+                return false;
+            }
+        } else {
+            // step 21
+            bool valid;
+            Rooted<PropDesc> pd(cx);
+            pd.initFromPropertyDescriptor(desc);
+            if (!ValidatePropertyDescriptor(cx, pd, targetDesc, &valid))
+                return false;
+            if (!valid || (settingConfigFalse && !targetDesc.isPermanent())) {
+                JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_CANT_DEFINE_INVALID);
+                return false;
+            }
+        }
+    }
+
+    // [[DefineProperty]] should return a boolean value, which is used to do things like
+    // strict-mode throwing. At present, the engine is not prepared to do that. See bug 826587.
+    return true;
 }
 
 bool
 ScriptedDirectProxyHandler::getOwnPropertyNames(JSContext *cx, HandleObject proxy,
                                                 AutoIdVector &props)
 {
     // step a
     RootedObject handler(cx, GetDirectProxyHandlerObject(proxy));
--- a/js/src/tests/ecma_5/extensions/proxy-array-target-length-definition.js
+++ b/js/src/tests/ecma_5/extensions/proxy-array-target-length-definition.js
@@ -13,24 +13,33 @@ print(BUGNUMBER + ": " + summary);
 
 /**************
  * BEGIN TEST *
  **************/
 
 var arr = [];
 var p = new Proxy(arr, {});
 
-// This really should throw a TypeError, but we're buggy just yet, and this
-// silently does nothing.
-Object.defineProperty(p, "length", { value: 17, configurable: true });
+function assertThrowsTypeError(f)
+{
+    try {
+        f();
+        assertEq(false, true, "Must have thrown");
+    } catch (e) {
+        assertEq(e instanceof TypeError, true, "Must have thrown TypeError");
+    }
+}
+
+// Redefining non-configurable length should throw a TypeError
+assertThrowsTypeError(function () { Object.defineProperty(p, "length", { value: 17, configurable: true }); });
 
 // Same here.
-Object.defineProperty(p, "length", { value: 42, enumerable: true });
+assertThrowsTypeError(function () { Object.defineProperty(p, "length", { value: 42, enumerable: true }); });
 
-// But at least we can check the property went unchanged.
+// Check the property went unchanged.
 var pd = Object.getOwnPropertyDescriptor(p, "length");
 assertEq(pd.value, 0);
 assertEq(pd.writable, true);
 assertEq(pd.enumerable, false);
 assertEq(pd.configurable, false);
 
 var ad = Object.getOwnPropertyDescriptor(arr, "length");
 assertEq(ad.value, 0);