Bug 739380 - Convert the defineElement hooks to use an ECMAScript-style [[DefineOwnProperty]] interface, not the old-school SpiderMonkey defineProperty interface. r=bhackett

--HG--
extra : rebase_source : e43fcbfdb76c61b4ecd7a5f4b1d17aefa0d19f83

js/src/jsobj.h

@@ -75,18 +75,6 @@ struct NativeIterator;
 
 namespace mjit { class Compiler; }
 
-static inline PropertyOp
-CastAsPropertyOp(JSObject *object)
-{
-    return JS_DATA_TO_FUNC_PTR(PropertyOp, object);
-}
-
-static inline StrictPropertyOp
-CastAsStrictPropertyOp(JSObject *object)
-{
-    return JS_DATA_TO_FUNC_PTR(StrictPropertyOp, object);
-}
-
 inline JSObject *
 CastAsObject(PropertyOp op)
 {
@@ -127,115 +115,6 @@ CastAsObjectJsval(StrictPropertyOp op)
 
 /******************************************************************************/
 
-/*
- * A representation of ECMA-262 ed. 5's internal Property Descriptor data
- * structure.
- */
-struct PropDesc {
-    /*
-     * Original object from which this descriptor derives, passed through for
-     * the benefit of proxies.
-     */
-    js::Value pd;
-
-    js::Value value, get, set;
-
-    /* Property descriptor boolean fields. */
-    uint8_t attrs;
-
-    /* Bits indicating which values are set. */
-    bool hasGet : 1;
-    bool hasSet : 1;
-    bool hasValue : 1;
-    bool hasWritable : 1;
-    bool hasEnumerable : 1;
-    bool hasConfigurable : 1;
-
-    friend class js::AutoPropDescArrayRooter;
-
-    PropDesc();
-
-    /*
-     * 8.10.5 ToPropertyDescriptor(Obj)
-     *
-     * If checkAccessors is false, skip steps 7.b and 8.b, which throw a
-     * TypeError if .get or .set is neither a callable object nor undefined.
-     *
-     * (DebuggerObject_defineProperty uses this: the .get and .set properties
-     * are expected to be Debugger.Object wrappers of functions, which are not
-     * themselves callable.)
-     */
-    bool initialize(JSContext* cx, const js::Value &v, bool checkAccessors=true);
-
-    /*
-     * 8.10.4 FromPropertyDescriptor(Desc)
-     *
-     * initFromPropertyDescriptor sets pd to undefined and populates all the
-     * other fields of this PropDesc from desc.
-     *
-     * makeObject populates pd based on the other fields of *this, creating a
-     * new property descriptor JSObject and defining properties on it.
-     */
-    void initFromPropertyDescriptor(const PropertyDescriptor &desc);
-    bool makeObject(JSContext *cx);
-
-    /* 8.10.1 IsAccessorDescriptor(desc) */
-    bool isAccessorDescriptor() const {
-        return hasGet || hasSet;
-    }
-
-    /* 8.10.2 IsDataDescriptor(desc) */
-    bool isDataDescriptor() const {
-        return hasValue || hasWritable;
-    }
-
-    /* 8.10.3 IsGenericDescriptor(desc) */
-    bool isGenericDescriptor() const {
-        return !isAccessorDescriptor() && !isDataDescriptor();
-    }
-
-    bool configurable() const {
-        return (attrs & JSPROP_PERMANENT) == 0;
-    }
-
-    bool enumerable() const {
-        return (attrs & JSPROP_ENUMERATE) != 0;
-    }
-
-    bool writable() const {
-        return (attrs & JSPROP_READONLY) == 0;
-    }
-
-    JSObject* getterObject() const {
-        return get.isUndefined() ? NULL : &get.toObject();
-    }
-    JSObject* setterObject() const {
-        return set.isUndefined() ? NULL : &set.toObject();
-    }
-
-    const js::Value &getterValue() const {
-        return get;
-    }
-    const js::Value &setterValue() const {
-        return set;
-    }
-
-    PropertyOp getter() const {
-        return js::CastAsPropertyOp(getterObject());
-    }
-    StrictPropertyOp setter() const {
-        return js::CastAsStrictPropertyOp(setterObject());
-    }
-
-    /*
-     * Throw a TypeError if a getter/setter is present and is neither callable
-     * nor undefined. These methods do exactly the type checks that are skipped
-     * by passing false as the checkAccessors parameter of initialize.
-     */
-    inline bool checkGetter(JSContext *cx);
-    inline bool checkSetter(JSContext *cx);
-};
-
 typedef Vector<PropDesc, 1> PropDescArray;
 
 } /* namespace js */

js/src/vm/ObjectImpl.cpp

@@ -168,9 +168,8 @@ js::ObjectImpl::markChildren(JSTracer *trc)
 }
 
 bool
-js::SparseElementsHeader::defineElement(JSContext *cx, ObjectImpl *obj,
-                                        uint32_t index, const Value &value,
-                                        PropertyOp getter, StrictPropertyOp setter, unsigned attrs)
+SparseElementsHeader::defineElement(JSContext *cx, ObjectImpl *obj, uint32_t index,
+                                    const PropDesc &desc, bool shouldThrow, bool *succeeded)
 {
     MOZ_ASSERT(this == &obj->elementsHeader());
 
@@ -179,30 +178,55 @@ js::SparseElementsHeader::defineElement(JSContext *cx, ObjectImpl *obj,
 }
 
 bool
-js::DenseElementsHeader::defineElement(JSContext *cx, ObjectImpl *obj,
-                                       uint32_t index, const Value &value,
-                                       PropertyOp getter, StrictPropertyOp setter, unsigned attrs)
+DenseElementsHeader::defineElement(JSContext *cx, ObjectImpl *obj, uint32_t index,
+                                   const PropDesc &desc, bool shouldThrow, bool *succeeded)
 {
     MOZ_ASSERT(this == &obj->elementsHeader());
 
+    MOZ_ASSERT_IF(desc.hasGet || desc.hasSet, !desc.hasValue && !desc.hasWritable);
+    MOZ_ASSERT_IF(desc.hasValue || desc.hasWritable, !desc.hasGet && !desc.hasSet);
+
     /*
-     * If the new property doesn't have the right attributes, or an atypical
-     * getter or setter is being used, go sparse.
+     * If desc is an accessor descriptor or a data descriptor with atypical
+     * attributes, convert to sparse and retry.
      */
-    if (attrs != JSPROP_ENUMERATE ||
-        (attrs & (JSPROP_GETTER | JSPROP_SETTER)) || getter || setter)
+    if (desc.hasGet || desc.hasSet ||
+        (desc.hasEnumerable && !desc.enumerable()) ||
+        (desc.hasConfigurable && !desc.configurable()) ||
+        (desc.hasWritable && !desc.writable()))
     {
         if (!obj->makeElementsSparse(cx))
             return false;
         SparseElementsHeader &elts = obj->elementsHeader().asSparseElements();
-        return elts.defineElement(cx, obj, index, value, getter, setter, attrs);
+        return elts.defineElement(cx, obj, index, desc, shouldThrow, succeeded);
     }
 
-    /* If space for the dense element already exists, we only need set it. */
+    /* Does the element exist?  All behavior depends upon this. */
     uint32_t initLen = initializedLength();
     if (index < initLen) {
-        obj->elements[index].set(obj->asObjectPtr(), index, value);
-        return true;
+        HeapSlot &slot = obj->elements[index];
+        if (!slot.isMagic(JS_ARRAY_HOLE)) {
+            /*
+             * The element exists with attributes { [[Enumerable]]: true,
+             * [[Configurable]]: true, [[Writable]]: true, [[Value]]: slot }.
+             */
+            // XXX jwalden fill this in!
+        }
+    }
+
+    /*
+     * If the element doesn't exist, we can only add it if the object is
+     * extensible.
+     */
+    if (!obj->isExtensible()) {
+        *succeeded = false;
+        if (!shouldThrow)
+            return true;
+        MOZ_ALWAYS_FALSE(js_ReportValueErrorFlags(cx, JSREPORT_ERROR, JSMSG_OBJECT_NOT_EXTENSIBLE,
+                                                  JSDVG_IGNORE_STACK,
+                                                  ObjectValue(*obj->asObjectPtr()),
+                                                  NULL, NULL, NULL));
+        return false;
     }
 
     /* Otherwise we ensure space for it exists and that it's initialized. */
@@ -217,12 +241,13 @@ js::DenseElementsHeader::defineElement(JSContext *cx, ObjectImpl *obj,
         if (!obj->makeElementsSparse(cx))
             return false;
         SparseElementsHeader &elts = obj->elementsHeader().asSparseElements();
-        return elts.defineElement(cx, obj, index, value, getter, setter, attrs);
+        return elts.defineElement(cx, obj, index, desc, shouldThrow, succeeded);
     }
 
     /* But if we were able to ensure the element's existence, we're good. */
     MOZ_ASSERT(res == ObjectImpl::Succeeded);
-    obj->elements[index].set(obj->asObjectPtr(), index, value);
+    obj->elements[index].set(obj->asObjectPtr(), index, desc.value);
+    *succeeded = true;
     return true;
 }
 
@@ -239,37 +264,35 @@ ArrayBufferDelegate(JSContext *cx, ObjectImpl *obj)
 
 template <typename T>
 bool
-js::TypedElementsHeader<T>::defineElement(JSContext *cx, ObjectImpl *obj,
-                                       uint32_t index, const Value &value,
-                                       PropertyOp getter, StrictPropertyOp setter, unsigned attrs)
+TypedElementsHeader<T>::defineElement(JSContext *cx, ObjectImpl *obj,
+                                      uint32_t index, const PropDesc &desc, bool shouldThrow,
+                                      bool *succeeded)
 {
-    /*
-     * XXX This isn't really a good error message, if this is even how typed
-     *     arrays should behave...
-     */
+    /* XXX jwalden This probably isn't how typed arrays should behave... */
+    *succeeded = false;
     js_ReportValueErrorFlags(cx, JSREPORT_ERROR, JSMSG_OBJECT_NOT_EXTENSIBLE,
-                             JSDVG_IGNORE_STACK, ObjectValue(*(JSObject*)obj), // XXX
+                             JSDVG_IGNORE_STACK,
+                             ObjectValue(*(JSObject*)obj), // XXX jwalden dodgy cast
                              NULL, NULL, NULL);
     return false;
 }
 
 bool
-js::ArrayBufferElementsHeader::defineElement(JSContext *cx, ObjectImpl *obj,
-                                             uint32_t index, const Value &value,
-                                             PropertyOp getter, StrictPropertyOp setter,
-                                             unsigned attrs)
+ArrayBufferElementsHeader::defineElement(JSContext *cx, ObjectImpl *obj,
+                                         uint32_t index, const PropDesc &desc, bool shouldThrow,
+                                         bool *succeeded)
 {
     MOZ_ASSERT(this == &obj->elementsHeader());
 
     JSObject *delegate = ArrayBufferDelegate(cx, obj);
     if (!delegate)
         return false;
-    return DefineElement(cx, delegate, index, value, getter, setter, attrs);
+    return DefineElement(cx, delegate, index, desc, shouldThrow, succeeded);
 }
 
 bool
-js::DefineElement(JSContext *cx, ObjectImpl *obj, uint32_t index, const Value &value,
-                  PropertyOp getter, StrictPropertyOp setter, unsigned attrs)
+js::DefineElement(JSContext *cx, ObjectImpl *obj, uint32_t index, const PropDesc &desc,
+                  bool shouldThrow, bool *succeeded)
 {
     NEW_OBJECT_REPRESENTATION_ONLY();
 
@@ -277,41 +300,41 @@ js::DefineElement(JSContext *cx, ObjectImpl *obj, uint32_t index, const Value &v
 
     switch (header.kind()) {
       case DenseElements:
-        return header.asDenseElements().defineElement(cx, obj, index, value, getter, setter,
-                                                      attrs);
+        return header.asDenseElements().defineElement(cx, obj, index, desc, shouldThrow,
+                                                      succeeded);
       case SparseElements:
-        return header.asSparseElements().defineElement(cx, obj, index, value, getter, setter,
-                                                       attrs);
+        return header.asSparseElements().defineElement(cx, obj, index, desc, shouldThrow,
+                                                       succeeded);
       case Uint8Elements:
-        return header.asUint8Elements().defineElement(cx, obj, index, value, getter, setter,
-                                                      attrs);
+        return header.asUint8Elements().defineElement(cx, obj, index, desc, shouldThrow,
+                                                      succeeded);
       case Int8Elements:
-        return header.asInt8Elements().defineElement(cx, obj, index, value, getter, setter,
-                                                     attrs);
+        return header.asInt8Elements().defineElement(cx, obj, index, desc, shouldThrow,
+                                                     succeeded);
       case Uint16Elements:
-        return header.asUint16Elements().defineElement(cx, obj, index, value, getter, setter,
-                                                       attrs);
+        return header.asUint16Elements().defineElement(cx, obj, index, desc, shouldThrow,
+                                                       succeeded);
       case Int16Elements:
-        return header.asInt16Elements().defineElement(cx, obj, index, value, getter, setter,
-                                                      attrs);
+        return header.asInt16Elements().defineElement(cx, obj, index, desc, shouldThrow,
+                                                      succeeded);
       case Uint32Elements:
-        return header.asUint32Elements().defineElement(cx, obj, index, value, getter, setter,
-                                                       attrs);
+        return header.asUint32Elements().defineElement(cx, obj, index, desc, shouldThrow,
+                                                       succeeded);
       case Int32Elements:
-        return header.asInt32Elements().defineElement(cx, obj, index, value, getter, setter,
-                                                      attrs);
+        return header.asInt32Elements().defineElement(cx, obj, index, desc, shouldThrow,
+                                                      succeeded);
       case Uint8ClampedElements:
-        return header.asUint8ClampedElements().defineElement(cx, obj, index, value,
-                                                             getter, setter, attrs);
+        return header.asUint8ClampedElements().defineElement(cx, obj, index, desc, shouldThrow,
+                                                             succeeded);
       case Float32Elements:
-        return header.asFloat32Elements().defineElement(cx, obj, index, value, getter, setter,
-                                                        attrs);
+        return header.asFloat32Elements().defineElement(cx, obj, index, desc, shouldThrow,
+                                                        succeeded);
       case Float64Elements:
-        return header.asFloat64Elements().defineElement(cx, obj, index, value, getter, setter,
-                                                        attrs);
+        return header.asFloat64Elements().defineElement(cx, obj, index, desc, shouldThrow,
+                                                        succeeded);
       case ArrayBufferElements:
-        return header.asArrayBufferElements().defineElement(cx, obj, index, value, getter, setter,
-                                                            attrs);
+        return header.asArrayBufferElements().defineElement(cx, obj, index, desc, shouldThrow,
+                                                            succeeded);
     }
 
     MOZ_NOT_REACHED("bad elements kind!");

js/src/vm/ObjectImpl.h

@@ -21,6 +21,129 @@ namespace js {
 
 class ObjectImpl;
 
+class AutoPropDescArrayRooter;
+
+static inline PropertyOp
+CastAsPropertyOp(JSObject *object)
+{
+    return JS_DATA_TO_FUNC_PTR(PropertyOp, object);
+}
+
+static inline StrictPropertyOp
+CastAsStrictPropertyOp(JSObject *object)
+{
+    return JS_DATA_TO_FUNC_PTR(StrictPropertyOp, object);
+}
+
+/*
+ * A representation of ECMA-262 ed. 5's internal Property Descriptor data
+ * structure.
+ */
+struct PropDesc {
+    /*
+     * Original object from which this descriptor derives, passed through for
+     * the benefit of proxies.
+     */
+    Value pd;
+
+    Value value, get, set;
+
+    /* Property descriptor boolean fields. */
+    uint8_t attrs;
+
+    /* Bits indicating which values are set. */
+    bool hasGet : 1;
+    bool hasSet : 1;
+    bool hasValue : 1;
+    bool hasWritable : 1;
+    bool hasEnumerable : 1;
+    bool hasConfigurable : 1;
+
+    friend class AutoPropDescArrayRooter;
+
+    PropDesc();
+
+    /*
+     * 8.10.5 ToPropertyDescriptor(Obj)
+     *
+     * If checkAccessors is false, skip steps 7.b and 8.b, which throw a
+     * TypeError if .get or .set is neither a callable object nor undefined.
+     *
+     * (DebuggerObject_defineProperty uses this: the .get and .set properties
+     * are expected to be Debugger.Object wrappers of functions, which are not
+     * themselves callable.)
+     */
+    bool initialize(JSContext* cx, const Value &v, bool checkAccessors = true);
+
+    /*
+     * 8.10.4 FromPropertyDescriptor(Desc)
+     *
+     * initFromPropertyDescriptor sets pd to undefined and populates all the
+     * other fields of this PropDesc from desc.
+     *
+     * makeObject populates pd based on the other fields of *this, creating a
+     * new property descriptor JSObject and defining properties on it.
+     */
+    void initFromPropertyDescriptor(const PropertyDescriptor &desc);
+    bool makeObject(JSContext *cx);
+
+    /* 8.10.1 IsAccessorDescriptor(desc) */
+    bool isAccessorDescriptor() const {
+        return hasGet || hasSet;
+    }
+
+    /* 8.10.2 IsDataDescriptor(desc) */
+    bool isDataDescriptor() const {
+        return hasValue || hasWritable;
+    }
+
+    /* 8.10.3 IsGenericDescriptor(desc) */
+    bool isGenericDescriptor() const {
+        return !isAccessorDescriptor() && !isDataDescriptor();
+    }
+
+    bool configurable() const {
+        return (attrs & JSPROP_PERMANENT) == 0;
+    }
+
+    bool enumerable() const {
+        return (attrs & JSPROP_ENUMERATE) != 0;
+    }
+
+    bool writable() const {
+        return (attrs & JSPROP_READONLY) == 0;
+    }
+
+    JSObject* getterObject() const {
+        return get.isUndefined() ? NULL : &get.toObject();
+    }
+    JSObject* setterObject() const {
+        return set.isUndefined() ? NULL : &set.toObject();
+    }
+
+    const Value &getterValue() const {
+        return get;
+    }
+    const Value &setterValue() const {
+        return set;
+    }
+
+    PropertyOp getter() const {
+        return CastAsPropertyOp(getterObject());
+    }
+    StrictPropertyOp setter() const {
+        return CastAsStrictPropertyOp(setterObject());
+    }
+
+    /*
+     * Throw a TypeError if a getter/setter is present and is neither callable
+     * nor undefined. These methods do exactly the type checks that are skipped
+     * by passing false as the checkAccessors parameter of initialize.
+     */
+    inline bool checkGetter(JSContext *cx);
+    inline bool checkSetter(JSContext *cx);
+};
+
 class DenseElementsHeader;
 class SparseElementsHeader;
 class Uint8ElementsHeader;
@@ -129,9 +252,8 @@ class DenseElementsHeader : public ElementsHeader
         return ElementsHeader::length;
     }
 
-    bool defineElement(JSContext *cx, ObjectImpl *obj,
-                       uint32_t index, const Value &value,
-                       PropertyOp getter, StrictPropertyOp setter, unsigned attrs);
+    bool defineElement(JSContext *cx, ObjectImpl *obj, uint32_t index, const PropDesc &desc,
+                       bool shouldThrow, bool *succeeded);
 
   private:
     inline bool isDenseElements() const MOZ_DELETE;
@@ -154,9 +276,8 @@ class SparseElementsHeader : public ElementsHeader
         return ElementsHeader::length;
     }
 
-    bool defineElement(JSContext *cx, ObjectImpl *obj,
-                       uint32_t index, const Value &value,
-                       PropertyOp getter, StrictPropertyOp setter, unsigned attrs);
+    bool defineElement(JSContext *cx, ObjectImpl *obj, uint32_t index, const PropDesc &desc,
+                       bool shouldThrow, bool *succeeded);
 
   private:
     inline bool isSparseElements() const MOZ_DELETE;
@@ -263,9 +384,8 @@ class TypedElementsHeader : public ElementsHeader
         return ElementsHeader::length;
     }
 
-    bool defineElement(JSContext *cx, ObjectImpl *obj,
-                       uint32_t index, const Value &value,
-                       PropertyOp getter, StrictPropertyOp setter, unsigned attrs);
+    bool defineElement(JSContext *cx, ObjectImpl *obj, uint32_t index, const PropDesc &desc,
+                       bool shouldThrow, bool *succeeded);
 
   private:
     TypedElementsHeader(const TypedElementsHeader &other) MOZ_DELETE;
@@ -349,9 +469,8 @@ class Uint8ClampedElementsHeader : public TypedElementsHeader<uint8_clamped>
 class ArrayBufferElementsHeader : public ElementsHeader
 {
   public:
-    bool defineElement(JSContext *cx, ObjectImpl *obj,
-                       uint32_t index, const Value &value,
-                       PropertyOp getter, StrictPropertyOp setter, unsigned attrs);
+    bool defineElement(JSContext *cx, ObjectImpl *obj, uint32_t index, const PropDesc &desc,
+                       bool shouldThrow, bool *succeeded);
 
   private:
     inline bool isArrayBufferElements() const MOZ_DELETE;
@@ -909,8 +1028,8 @@ class ObjectImpl : public gc::Cell
 };
 
 extern bool
-DefineElement(JSContext *cx, ObjectImpl *obj, uint32_t index, const Value &value,
-              PropertyOp getter, StrictPropertyOp setter, unsigned attrs);
+DefineElement(JSContext *cx, ObjectImpl *obj, uint32_t index, const PropDesc &desc,
+              bool shouldThrow, bool *succeeded);
 
 } /* namespace js */