Bug 1138499, part 2 - Strengthen assertComplete() to require that both [[Get]] and [[Set]] be present on accessor properties. r=Waldo.
authorJason Orendorff <jorendorff@mozilla.com>
Mon, 23 Mar 2015 14:32:27 -0500
changeset 237282 034027f41aaf1c3a522e47dcdbafaf3525f898c7
parent 237281 f9c99e8ce20747e7b233f475d2be424bcbad8399
child 237283 b3ef9fce0df57fbda404722ae24b63658bd41d28
push id57907
push userjorendorff@mozilla.com
push dateThu, 02 Apr 2015 15:02:59 +0000
treeherdermozilla-inbound@b3ef9fce0df5 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersWaldo
bugs1138499
milestone40.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 1138499, part 2 - Strengthen assertComplete() to require that both [[Get]] and [[Set]] be present on accessor properties. r=Waldo.
js/src/jsapi.h
js/src/jsarray.cpp
js/src/vm/NativeObject-inl.h
js/src/vm/NativeObject.cpp
js/src/vm/NativeObject.h
js/src/vm/Shape.cpp
--- a/js/src/jsapi.h
+++ b/js/src/jsapi.h
@@ -2613,16 +2613,17 @@ class PropertyDescriptorOperations
         MOZ_ASSERT((attributes() & ~(JSPROP_ENUMERATE |
                                      JSPROP_PERMANENT |
                                      JSPROP_READONLY |
                                      JSPROP_GETTER |
                                      JSPROP_SETTER |
                                      JSPROP_SHARED |
                                      JSPROP_REDEFINE_NONCONFIGURABLE |
                                      SHADOWABLE)) == 0);
+        MOZ_ASSERT_IF(isAccessorDescriptor(), has(JSPROP_GETTER) && has(JSPROP_SETTER));
 #endif
     }
 
     void assertCompleteIfFound() const {
 #ifdef DEBUG
         if (object())
             assertComplete();
 #endif
--- a/js/src/jsarray.cpp
+++ b/js/src/jsarray.cpp
@@ -694,18 +694,19 @@ js::ArraySetLength(JSContext* cx, Handle
 
     /* Steps 12, 16. */
 
     // Yes, we totally drop a non-stub getter/setter from a defineProperty
     // API call on the floor here.  Given that getter/setter will go away in
     // the long run, with accessors replacing them both internally and at the
     // API level, just run with this.
     RootedShape lengthShape(cx, arr->lookup(cx, id));
-    if (!NativeObject::changeProperty(cx, arr, lengthShape, attrs,
-                                      JSPROP_PERMANENT | JSPROP_READONLY | JSPROP_SHARED,
+    if (!NativeObject::changeProperty(cx, arr, lengthShape,
+                                      attrs | JSPROP_PERMANENT | JSPROP_SHARED |
+                                      (lengthShape->attributes() & JSPROP_READONLY),
                                       array_length_getter, array_length_setter))
     {
         return false;
     }
 
     arr->setLength(cx, newLen);
 
     // All operations past here until the |!succeeded| code must be infallible,
--- a/js/src/vm/NativeObject-inl.h
+++ b/js/src/vm/NativeObject-inl.h
@@ -23,23 +23,16 @@ namespace js {
 inline uint8_t*
 NativeObject::fixedData(size_t nslots) const
 {
     MOZ_ASSERT(ClassCanHaveFixedData(getClass()));
     MOZ_ASSERT(nslots == numFixedSlots() + (hasPrivate() ? 1 : 0));
     return reinterpret_cast<uint8_t*>(&fixedSlots()[nslots]);
 }
 
-/* static */ inline bool
-NativeObject::changePropertyAttributes(JSContext* cx, HandleNativeObject obj,
-                                       HandleShape shape, unsigned attrs)
-{
-    return !!changeProperty(cx, obj, shape, attrs, 0, shape->getter(), shape->setter());
-}
-
 inline void
 NativeObject::removeLastProperty(ExclusiveContext* cx)
 {
     MOZ_ASSERT(canRemoveLastProperty());
     JS_ALWAYS_TRUE(setLastProperty(cx, lastProperty()->previous()));
 }
 
 inline bool
--- a/js/src/vm/NativeObject.cpp
+++ b/js/src/vm/NativeObject.cpp
@@ -1362,29 +1362,35 @@ js::NativeDefineProperty(ExclusiveContex
                 if (!NativeObject::sparsifyDenseElement(cx, obj, JSID_TO_INT(id)))
                     return false;
                 shape = obj->lookup(cx, id);
             }
             if (shape->isAccessorDescriptor()) {
                 if (!CheckAccessorRedefinition(cx, obj, shape, getter, setter, id, attrs))
                     return false;
                 attrs = ApplyOrDefaultAttributes(attrs, shape);
-                shape = NativeObject::changeProperty(cx, obj, shape, attrs,
-                                                     JSPROP_GETTER | JSPROP_SETTER,
+                shape = NativeObject::changeProperty(cx, obj, shape,
+                                                     attrs | JSPROP_GETTER | JSPROP_SETTER,
                                                      (attrs & JSPROP_GETTER)
                                                      ? getter
                                                      : shape->getter(),
                                                      (attrs & JSPROP_SETTER)
                                                      ? setter
                                                      : shape->setter());
                 if (!shape)
                     return false;
                 shouldDefine = false;
             }
         }
+
+        // Either we are converting a data property to an accessor property, or
+        // creating a new accessor property; either way [[Get]] and [[Set]]
+        // must both be filled in.
+        if (shouldDefine)
+            attrs |= JSPROP_GETTER | JSPROP_SETTER;
     } else if (desc.hasValue()) {
         // If we did a normal lookup here, it would cause resolve hook recursion in
         // the following case. Suppose the first script we run in a lazy global is
         // |parseInt()|.
         //   - js::InitNumberClass is called to resolve parseInt.
         //   - js::InitNumberClass tries to define the Number constructor on the
         //     global.
         //   - We end up here.
@@ -1406,19 +1412,17 @@ js::NativeDefineProperty(ExclusiveContex
                 // Do not redefine a nonconfigurable accessor property.
                 if (shape->isAccessorDescriptor()) {
                     if (!CheckAccessorRedefinition(cx, obj, shape, getter, setter, id, attrs))
                         return false;
                 }
             }
         }
     } else {
-        // We have been asked merely to update some attributes. If the
-        // property already exists and it's a data property, we can just
-        // call JSObject::changeProperty.
+        // We have been asked merely to update JSPROP_PERMANENT and/or JSPROP_ENUMERATE.
         if (!NativeLookupOwnProperty<CanGC>(cx, obj, id, &shape))
             return false;
 
         if (shape) {
             // Don't forget about arrays.
             if (IsImplicitDenseOrTypedArrayElement(shape)) {
                 if (IsAnyTypedArray(obj)) {
                     /*
--- a/js/src/vm/NativeObject.h
+++ b/js/src/vm/NativeObject.h
@@ -689,22 +689,18 @@ class NativeObject : public JSObject
     static inline Shape*
     putProperty(ExclusiveContext* cx, HandleObject obj, PropertyName* name,
                 JSGetterOp getter, JSSetterOp setter,
                 uint32_t slot, unsigned attrs,
                 unsigned flags);
 
     /* Change the given property into a sibling with the same id in this scope. */
     static Shape*
-    changeProperty(ExclusiveContext* cx, HandleNativeObject obj,
-                   HandleShape shape, unsigned attrs, unsigned mask,
-                   JSGetterOp getter, JSSetterOp setter);
-
-    static inline bool changePropertyAttributes(JSContext* cx, HandleNativeObject obj,
-                                                HandleShape shape, unsigned attrs);
+    changeProperty(ExclusiveContext* cx, HandleNativeObject obj, HandleShape shape,
+                   unsigned attrs, JSGetterOp getter, JSSetterOp setter);
 
     /* Remove the property named by id from this object. */
     bool removeProperty(ExclusiveContext* cx, jsid id);
 
     /* Clear the scope, making it empty. */
     static void clear(ExclusiveContext* cx, HandleNativeObject obj);
 
   protected:
--- a/js/src/vm/Shape.cpp
+++ b/js/src/vm/Shape.cpp
@@ -864,23 +864,21 @@ NativeObject::putProperty(ExclusiveConte
 
     obj->checkShapeConsistency();
 
     return shape;
 }
 
 /* static */ Shape*
 NativeObject::changeProperty(ExclusiveContext* cx, HandleNativeObject obj, HandleShape shape,
-                             unsigned attrs, unsigned mask, GetterOp getter, SetterOp setter)
+                             unsigned attrs, GetterOp getter, SetterOp setter)
 {
     MOZ_ASSERT(obj->containsPure(shape));
     MOZ_ASSERT(getter != JS_PropertyStub);
     MOZ_ASSERT(setter != JS_StrictPropertyStub);
-
-    attrs |= shape->attrs & mask;
     MOZ_ASSERT_IF(attrs & (JSPROP_GETTER | JSPROP_SETTER), attrs & JSPROP_SHARED);
 
     /* Allow only shared (slotless) => unshared (slotful) transition. */
     MOZ_ASSERT(!((attrs ^ shape->attrs) & JSPROP_SHARED) ||
                !(attrs & JSPROP_SHARED));
 
     MarkTypePropertyNonData(cx, obj, shape->propid());