Bug 1133081, part 2 - Switch from js::PropDesc to JSPropertyDescriptor for js::StandardDefineProperty implementation. r=efaust.
authorJason Orendorff <jorendorff@mozilla.com>
Fri, 13 Feb 2015 18:52:45 -0600
changeset 233465 00c7748956ad6ad3b4d12f2c7d629a4711995021
parent 233464 2a96f2eed5c9a450fac568ebd93d5f83ff05ad0b
child 233466 fd31041fe5d6fe0dca57841c0f055be2f8570a19
push id56858
push userjorendorff@mozilla.com
push dateFri, 13 Mar 2015 09:53:30 +0000
treeherdermozilla-inbound@c78a9d1273c5 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersefaust
bugs1133081
milestone39.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 1133081, part 2 - Switch from js::PropDesc to JSPropertyDescriptor for js::StandardDefineProperty implementation. r=efaust.
js/src/jsapi.h
js/src/jsobj.cpp
--- a/js/src/jsapi.h
+++ b/js/src/jsapi.h
@@ -2533,19 +2533,30 @@ template <typename Outer>
 class PropertyDescriptorOperations
 {
     const JSPropertyDescriptor * desc() const { return static_cast<const Outer*>(this)->extract(); }
 
   public:
     // Descriptors with JSGetterOp/JSSetterOp are considered data
     // descriptors. It's complicated.
     bool isAccessorDescriptor() const { return hasGetterOrSetterObject(); }
-    bool isDataDescriptor() const { return !isAccessorDescriptor(); }
-
-    bool hasValue() const { return isDataDescriptor() && !hasAttributes(JSPROP_IGNORE_VALUE); }
+    bool isGenericDescriptor() const {
+        return (desc()->attrs &
+                (JSPROP_GETTER | JSPROP_SETTER | JSPROP_IGNORE_READONLY | JSPROP_IGNORE_VALUE)) ==
+               (JSPROP_IGNORE_READONLY | JSPROP_IGNORE_VALUE);
+    }
+    bool isDataDescriptor() const { return !isAccessorDescriptor() && !isGenericDescriptor(); }
+
+    bool hasConfigurable() const { return !hasAttributes(JSPROP_IGNORE_PERMANENT); }
+    bool configurable() const { MOZ_ASSERT(hasConfigurable()); return !isPermanent(); }
+    bool hasEnumerable() const { return !hasAttributes(JSPROP_IGNORE_ENUMERATE); }
+    bool enumerable() const { MOZ_ASSERT(hasEnumerable()); return isEnumerable(); }
+    bool hasValue() const { return !isAccessorDescriptor() && !hasAttributes(JSPROP_IGNORE_VALUE); }
+    bool hasWritable() const { return !isAccessorDescriptor() && !hasAttributes(JSPROP_IGNORE_READONLY); }
+    bool writable() const { MOZ_ASSERT(hasWritable()); return !isReadonly(); }
 
     bool isEnumerable() const { return desc()->attrs & JSPROP_ENUMERATE; }
     bool isReadonly() const { return desc()->attrs & JSPROP_READONLY; }
     bool isPermanent() const { return desc()->attrs & JSPROP_PERMANENT; }
     bool hasGetterObject() const { return desc()->attrs & JSPROP_GETTER; }
     bool hasSetterObject() const { return desc()->attrs & JSPROP_SETTER; }
     bool hasGetterOrSetterObject() const { return desc()->attrs & (JSPROP_GETTER | JSPROP_SETTER); }
     bool hasGetterOrSetter() const { return desc()->getter || desc()->setter; }
--- a/js/src/jsobj.cpp
+++ b/js/src/jsobj.cpp
@@ -454,18 +454,18 @@ js::Throw(JSContext *cx, JSObject *obj, 
     }
     return false;
 }
 
 
 /*** Standard-compliant property definition (used by Object.defineProperty) **********************/
 
 static bool
-DefinePropertyOnObject(JSContext *cx, HandleNativeObject obj, HandleId id, const PropDesc &desc,
-                       ObjectOpResult &result)
+DefinePropertyOnObject(JSContext *cx, HandleNativeObject obj, HandleId id,
+                       Handle<PropertyDescriptor> desc, ObjectOpResult &result)
 {
     /* 8.12.9 step 1. */
     RootedShape shape(cx);
     MOZ_ASSERT(!obj->getOps()->lookupProperty);
     if (!NativeLookupOwnProperty<CanGC>(cx, obj, id, &shape))
         return false;
 
     MOZ_ASSERT(!obj->getOps()->defineProperty);
@@ -476,27 +476,29 @@ DefinePropertyOnObject(JSContext *cx, Ha
         if (!IsExtensible(cx, obj, &extensible))
             return false;
         if (!extensible)
             return result.fail(JSMSG_OBJECT_NOT_EXTENSIBLE);
 
         if (desc.isGenericDescriptor() || desc.isDataDescriptor()) {
             MOZ_ASSERT(!obj->getOps()->defineProperty);
             RootedValue v(cx, desc.hasValue() ? desc.value() : UndefinedValue());
-            unsigned attrs = desc.attributes();
+            unsigned attrs = desc.attributes() & (JSPROP_PERMANENT | JSPROP_ENUMERATE | JSPROP_READONLY);
+
             if (!desc.hasConfigurable())
                 attrs |= JSPROP_PERMANENT;
             if (!desc.hasWritable())
                 attrs |= JSPROP_READONLY;
             return NativeDefineProperty(cx, obj, id, v, nullptr, nullptr, attrs, result);
         }
 
         MOZ_ASSERT(desc.isAccessorDescriptor());
 
-        unsigned attrs = desc.attributes();
+        unsigned attrs = desc.attributes() & (JSPROP_PERMANENT | JSPROP_ENUMERATE |
+                                              JSPROP_SHARED | JSPROP_GETTER | JSPROP_SETTER);
         if (!desc.hasConfigurable())
             attrs |= JSPROP_PERMANENT;
         return NativeDefineProperty(cx, obj, id, UndefinedHandleValue,
                                     desc.getter(), desc.setter(), attrs, result);
     }
 
     /* 8.12.9 steps 5-6 (note 5 is merely a special case of 6). */
     RootedValue v(cx);
@@ -524,31 +526,23 @@ DefinePropertyOnObject(JSContext *cx, Ha
         shapeAttributes = shape->attributes();
     }
 
     do {
         if (desc.isAccessorDescriptor()) {
             if (!shapeAccessorDescriptor)
                 break;
 
-            if (desc.hasGet()) {
-                RootedValue getter(cx, shape->getterOrUndefined());
-                bool same;
-                if (!SameValue(cx, desc.getterValue(), getter, &same))
-                    return false;
-                if (!same)
+            if (desc.hasGetterObject()) {
+                if (!shape->hasGetterValue() || desc.getterObject() != shape->getterObject())
                     break;
             }
 
-            if (desc.hasSet()) {
-                RootedValue setter(cx, shape->setterOrUndefined());
-                bool same;
-                if (!SameValue(cx, desc.setterValue(), setter, &same))
-                    return false;
-                if (!same)
+            if (desc.hasSetterObject()) {
+                if (!shape->hasSetterValue() || desc.setterObject() != shape->setterObject())
                     break;
             }
         } else {
             /*
              * Determine the current value of the property once, if the current
              * value might actually need to be used or preserved later.  NB: we
              * guard on whether the current property is a data descriptor to
              * avoid calling a getter; we won't need the value if it's not a
@@ -660,32 +654,31 @@ DefinePropertyOnObject(JSContext *cx, Ha
             }
         }
 
         callDelProperty = !shapeHasDefaultGetter || !shapeHasDefaultSetter;
     } else {
         /* 8.12.9 step 11. */
         MOZ_ASSERT(desc.isAccessorDescriptor() && shape->isAccessorDescriptor());
         if (!shape->configurable()) {
-            if (desc.hasSet()) {
-                RootedValue setter(cx, shape->setterOrUndefined());
-                bool same;
-                if (!SameValue(cx, desc.setterValue(), setter, &same))
-                    return false;
-                if (!same)
-                    return result.fail(JSMSG_CANT_REDEFINE_PROP);
+            // The hasSetterValue() and hasGetterValue() calls below ought to
+            // be redundant here, because accessor shapes should always have
+            // both JSPROP_GETTER and JSPROP_SETTER. But this is not the case
+            // currently; in particular Object.defineProperty(obj, key, {get: fn})
+            // creates a property without JSPROP_SETTER (bug 1133315).
+            if (desc.hasSetterObject() &&
+                desc.setterObject() != (shape->hasSetterValue() ? shape->setterObject() : nullptr))
+            {
+                return result.fail(JSMSG_CANT_REDEFINE_PROP);
             }
 
-            if (desc.hasGet()) {
-                RootedValue getter(cx, shape->getterOrUndefined());
-                bool same;
-                if (!SameValue(cx, desc.getterValue(), getter, &same))
-                    return false;
-                if (!same)
-                    return result.fail(JSMSG_CANT_REDEFINE_PROP);
+            if (desc.hasGetterObject() &&
+                desc.getterObject() != (shape->hasGetterValue() ? shape->getterObject() : nullptr))
+            {
+                return result.fail(JSMSG_CANT_REDEFINE_PROP);
             }
         }
     }
 
     /* 8.12.9 step 12. */
     unsigned attrs;
     GetterOp getter;
     SetterOp setter;
@@ -695,57 +688,59 @@ DefinePropertyOnObject(JSContext *cx, Ha
             changed |= JSPROP_PERMANENT;
         if (desc.hasEnumerable())
             changed |= JSPROP_ENUMERATE;
 
         attrs = (shapeAttributes & ~changed) | (desc.attributes() & changed);
         getter = IsImplicitDenseOrTypedArrayElement(shape) ? nullptr : shape->getter();
         setter = IsImplicitDenseOrTypedArrayElement(shape) ? nullptr : shape->setter();
     } else if (desc.isDataDescriptor()) {
-        unsigned unchanged = 0, descAttrs = desc.attributes();
-        if (!desc.hasConfigurable())
-            unchanged |= JSPROP_PERMANENT;
-        if (!desc.hasEnumerable())
-            unchanged |= JSPROP_ENUMERATE;
         /* Watch out for accessor -> data transformations here. */
-        if (!desc.hasWritable()) {
-            if (shapeDataDescriptor)
-                unchanged |= JSPROP_READONLY;
-            else
-                descAttrs |= JSPROP_READONLY;
+        unsigned changed = JSPROP_GETTER | JSPROP_SETTER | JSPROP_SHARED;
+        unsigned descAttrs = desc.attributes();
+        if (desc.hasConfigurable())
+            changed |= JSPROP_PERMANENT;
+        if (desc.hasEnumerable())
+            changed |= JSPROP_ENUMERATE;
+
+        if (desc.hasWritable()) {
+            changed |= JSPROP_READONLY;
+        } else if (!shapeDataDescriptor) {
+            changed |= JSPROP_READONLY;
+            descAttrs |= JSPROP_READONLY;
         }
 
         if (desc.hasValue())
             v = desc.value();
-        attrs = (descAttrs & ~unchanged) | (shapeAttributes & unchanged);
+        attrs = (descAttrs & changed) | (shapeAttributes & ~changed);
         getter = nullptr;
         setter = nullptr;
     } else {
         MOZ_ASSERT(desc.isAccessorDescriptor());
 
         /* 8.12.9 step 12. */
         unsigned changed = 0;
         if (desc.hasConfigurable())
             changed |= JSPROP_PERMANENT;
         if (desc.hasEnumerable())
             changed |= JSPROP_ENUMERATE;
-        if (desc.hasGet())
+        if (desc.hasGetterObject())
             changed |= JSPROP_GETTER | JSPROP_SHARED | JSPROP_READONLY;
-        if (desc.hasSet())
+        if (desc.hasSetterObject())
             changed |= JSPROP_SETTER | JSPROP_SHARED | JSPROP_READONLY;
 
         attrs = (desc.attributes() & changed) | (shapeAttributes & ~changed);
-        if (desc.hasGet()) {
+        if (desc.hasGetterObject()) {
             getter = desc.getter();
         } else {
             getter = (shapeHasDefaultGetter && !shapeHasGetterValue)
                      ? nullptr
                      : shape->getter();
         }
-        if (desc.hasSet()) {
+        if (desc.hasSetterObject()) {
             setter = desc.setter();
         } else {
             setter = (shapeHasDefaultSetter && !shapeHasSetterValue)
                      ? nullptr
                      : shape->setter();
         }
     }
 
@@ -764,18 +759,18 @@ DefinePropertyOnObject(JSContext *cx, Ha
             return false;
     }
 
     return NativeDefineProperty(cx, obj, id, v, getter, setter, attrs, result);
 }
 
 /* ES6 20130308 draft 8.4.2.1 [[DefineOwnProperty]] */
 static bool
-DefinePropertyOnArray(JSContext *cx, Handle<ArrayObject*> arr, HandleId id, const PropDesc &desc,
-                      ObjectOpResult &result)
+DefinePropertyOnArray(JSContext *cx, Handle<ArrayObject*> arr, HandleId id,
+                      Handle<PropertyDescriptor> desc, ObjectOpResult &result)
 {
     /* Step 2. */
     if (id == NameToId(cx->names().length)) {
         // Canonicalize value, if necessary, before proceeding any further.  It
         // would be better if this were always/only done by ArraySetLength.
         // But canonicalization may throw a RangeError (or other exception, if
         // the value is an object with user-defined conversion semantics)
         // before other attributes are checked.  So as long as our internal
@@ -826,18 +821,18 @@ DefinePropertyOnArray(JSContext *cx, Han
     }
 
     /* Step 4. */
     return DefinePropertyOnObject(cx, arr, id, desc, result);
 }
 
 // ES6 draft rev31 9.4.5.3 [[DefineOwnProperty]]
 static bool
-DefinePropertyOnTypedArray(JSContext *cx, HandleObject obj, HandleId id, const PropDesc &desc,
-                           ObjectOpResult &result)
+DefinePropertyOnTypedArray(JSContext *cx, HandleObject obj, HandleId id,
+                           Handle<PropertyDescriptor> desc, ObjectOpResult &result)
 {
     MOZ_ASSERT(IsAnyTypedArray(obj));
     // Steps 3.a-c.
     uint64_t index;
     if (IsTypedArrayIndex(id, &index)) {
         // These are all substeps of 3.c.
         // Steps i-vi.
         // We (wrongly) ignore out of range defines with a value.
@@ -877,36 +872,32 @@ DefinePropertyOnTypedArray(JSContext *cx
     }
 
     // Step 4.
     return DefinePropertyOnObject(cx, obj.as<NativeObject>(), id, desc, result);
 }
 
 bool
 js::StandardDefineProperty(JSContext *cx, HandleObject obj, HandleId id,
-                           Handle<PropertyDescriptor> descriptor, ObjectOpResult &result)
+                           Handle<PropertyDescriptor> desc, ObjectOpResult &result)
 {
-    Rooted<PropDesc> desc(cx);
-    desc.initFromPropertyDescriptor(descriptor);
-
     if (obj->is<ArrayObject>()) {
         Rooted<ArrayObject*> arr(cx, &obj->as<ArrayObject>());
         return DefinePropertyOnArray(cx, arr, id, desc, result);
     }
 
     if (IsAnyTypedArray(obj))
         return DefinePropertyOnTypedArray(cx, obj, id, desc, result);
 
     if (obj->is<UnboxedPlainObject>() && !UnboxedPlainObject::convertToNative(cx, obj))
         return false;
 
     if (obj->getOps()->lookupProperty) {
         if (obj->is<ProxyObject>()) {
-            Rooted<PropertyDescriptor> pd(cx);
-            desc.populatePropertyDescriptor(obj, &pd);
+            Rooted<PropertyDescriptor> pd(cx, desc);
             pd.object().set(obj);
             return Proxy::defineProperty(cx, obj, id, &pd, result);
         }
         return result.fail(JSMSG_OBJECT_NOT_EXTENSIBLE);
     }
 
     return DefinePropertyOnObject(cx, obj.as<NativeObject>(), id, desc, result);
 }