Bug 1357330. r=shu
authorAndré Bargull <andre.bargull@gmail.com>
Tue, 18 Apr 2017 07:07:13 -0700
changeset 410387 76bff2cde94c44b7bd1c3d2ce847db095175577f
parent 410386 4aa7086a8141caa44bcbc6f34ef0f068ab7f8fe8
child 410388 da5a14ae4ce3084aa63a502e96af1a4c6ef76853
push id7391
push usermtabara@mozilla.com
push dateMon, 12 Jun 2017 13:08:53 +0000
treeherdermozilla-beta@2191d7f87e2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersshu
bugs1357330
milestone55.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 1357330. r=shu
js/src/vm/NativeObject.cpp
--- a/js/src/vm/NativeObject.cpp
+++ b/js/src/vm/NativeObject.cpp
@@ -1403,35 +1403,34 @@ GetExistingPropertyValue(JSContext* cx, 
     MOZ_ASSERT(obj->contains(cx, prop.shape()));
 
     RootedValue receiver(cx, ObjectValue(*obj));
     RootedShape shape(cx, prop.shape());
     return GetExistingProperty<CanGC>(cx, receiver, obj, shape, vp);
 }
 
 /*
- * If ES6 draft rev 37 9.1.6.3 ValidateAndApplyPropertyDescriptor step 4 would
- * return early, because desc is redundant with an existing own property obj[id],
- * then set *redundant = true and return true.
+ * If desc is redundant with an existing own property obj[id], then set
+ * |*redundant = true| and return true.
  */
 static bool
 DefinePropertyIsRedundant(JSContext* cx, HandleNativeObject obj, HandleId id,
                           Handle<PropertyResult> prop, unsigned shapeAttrs,
                           Handle<PropertyDescriptor> desc, bool *redundant)
 {
     *redundant = false;
 
-    if (desc.hasConfigurable() && desc.configurable() != ((shapeAttrs & JSPROP_PERMANENT) == 0))
+    if (desc.hasConfigurable() && desc.configurable() != IsConfigurable(shapeAttrs))
         return true;
-    if (desc.hasEnumerable() && desc.enumerable() != ((shapeAttrs & JSPROP_ENUMERATE) != 0))
+    if (desc.hasEnumerable() && desc.enumerable() != IsEnumerable(shapeAttrs))
         return true;
     if (desc.isDataDescriptor()) {
-        if ((shapeAttrs & (JSPROP_GETTER | JSPROP_SETTER)) != 0)
+        if (IsAccessorDescriptor(shapeAttrs))
             return true;
-        if (desc.hasWritable() && desc.writable() != ((shapeAttrs & JSPROP_READONLY) == 0))
+        if (desc.hasWritable() && desc.writable() != IsWritable(shapeAttrs))
             return true;
         if (desc.hasValue()) {
             // Get the current value of the existing property.
             RootedValue currentValue(cx);
             if (!prop.isDenseOrTypedArrayElement() &&
                 prop.shape()->hasSlot() &&
                 prop.shape()->hasDefaultGetter())
             {
@@ -1439,18 +1438,18 @@ DefinePropertyIsRedundant(JSContext* cx,
                 // correctness assertion that's too strict for this particular
                 // call site. For details, see bug 1125624 comments 13-16.
                 currentValue.set(obj->getSlot(prop.shape()->slot()));
             } else {
                 if (!GetExistingPropertyValue(cx, obj, id, prop, &currentValue))
                     return false;
             }
 
-            // The specification calls for SameValue here, but it seems to be a
-            // bug. See <https://bugs.ecmascript.org/show_bug.cgi?id=3508>.
+            // Don't call SameValue here to ensure we properly update distinct
+            // NaN values.
             if (desc.value() != currentValue)
                 return true;
         }
 
         GetterOp existingGetterOp =
             prop.isDenseOrTypedArrayElement() ? nullptr : prop.shape()->getter();
         if (desc.getter() != existingGetterOp)
             return true;
@@ -1478,17 +1477,18 @@ DefinePropertyIsRedundant(JSContext* cx,
 
 bool
 js::NativeDefineProperty(JSContext* cx, HandleNativeObject obj, HandleId id,
                          Handle<PropertyDescriptor> desc_,
                          ObjectOpResult& result)
 {
     desc_.assertValid();
 
-    // Section numbers and step numbers below refer to ES2016.
+    // Section numbers and step numbers below refer to ES2018, draft rev
+    // 540b827fccf6122a984be99ab9af7be20e3b5562.
     //
     // This function aims to implement 9.1.6 [[DefineOwnProperty]] as well as
     // the [[DefineOwnProperty]] methods described in 9.4.2.1 (arrays), 9.4.4.2
     // (arguments), and 9.4.5.3 (typed array views).
 
     // Dispense with custom behavior of exotic native objects first.
     if (obj->is<ArrayObject>()) {
         // 9.4.2.1 step 2. Redefining an array's length is very special.
@@ -1515,18 +1515,18 @@ js::NativeDefineProperty(JSContext* cx, 
             MOZ_ASSERT(!cx->helperThread());
             return DefineTypedArrayElement(cx, obj, index, desc_, result);
         }
     } else if (obj->is<ArgumentsObject>()) {
         if (id == NameToId(cx->names().length)) {
             // Either we are resolving the .length property on this object, or
             // redefining it. In the latter case only, we must set a bit. To
             // distinguish the two cases, we note that when resolving, the
-            // property won't already exist; whereas the first time it is
-            // redefined, it will.
+            // JSPROP_RESOLVING mask is set; whereas the first time it is
+            // redefined, it isn't set.
             if ((desc_.attributes() & JSPROP_RESOLVING) == 0)
                 obj->as<ArgumentsObject>().markLengthOverridden();
         } else if (JSID_IS_SYMBOL(id) && JSID_TO_SYMBOL(id) == cx->wellKnownSymbols().iterator) {
             // Do same thing as .length for [@@iterator].
             if ((desc_.attributes() & JSPROP_RESOLVING) == 0)
                 obj->as<ArgumentsObject>().markIteratorOverridden();
         } else if (JSID_IS_INT(id)) {
             if ((desc_.attributes() & JSPROP_RESOLVING) == 0)
@@ -1564,17 +1564,17 @@ js::NativeDefineProperty(JSContext* cx, 
         // Fill in missing desc fields with defaults.
         CompletePropertyDescriptor(&desc);
 
         if (!AddOrChangeProperty<IsAddOrChange::Add>(cx, obj, id, desc))
             return false;
         return result.succeed();
     }
 
-    // Steps 3-4. (Step 3 is a special case of step 4.) We use shapeAttrs as a
+    // Step 3 and 7.a.i.3, 8.a.iii, 10 (partially). We use shapeAttrs as a
     // stand-in for shape in many places below, since shape might not be a
     // pointer to a real Shape (see IsImplicitDenseOrTypedArrayElement).
     unsigned shapeAttrs = GetPropertyAttributes(obj, prop);
     bool redundant;
     if (!DefinePropertyIsRedundant(cx, obj, id, prop, shapeAttrs, desc, &redundant))
         return false;
     if (redundant) {
         // In cases involving JSOP_NEWOBJECT and JSOP_INITPROP, obj can have a
@@ -1591,33 +1591,33 @@ js::NativeDefineProperty(JSContext* cx, 
     // global. The idea is that a DOM object can never have such a thing on
     // its proto chain directly on the web, so we should be OK optimizing
     // access to accessors found on such an object. Bug 1105518 contemplates
     // removing this hack.
     bool skipRedefineChecks = (desc.attributes() & JSPROP_REDEFINE_NONCONFIGURABLE) &&
                               obj->is<GlobalObject>() &&
                               !obj->getClass()->isDOMClass();
 
-    // Step 5.
+    // Step 4.
     if (!IsConfigurable(shapeAttrs) && !skipRedefineChecks) {
         if (desc.hasConfigurable() && desc.configurable())
             return result.fail(JSMSG_CANT_REDEFINE_PROP);
         if (desc.hasEnumerable() && desc.enumerable() != IsEnumerable(shapeAttrs))
             return result.fail(JSMSG_CANT_REDEFINE_PROP);
     }
 
     // Fill in desc.[[Configurable]] and desc.[[Enumerable]] if missing.
     if (!desc.hasConfigurable())
         desc.setConfigurable(IsConfigurable(shapeAttrs));
     if (!desc.hasEnumerable())
         desc.setEnumerable(IsEnumerable(shapeAttrs));
 
-    // Steps 6-9.
+    // Steps 5-8.
     if (desc.isGenericDescriptor()) {
-        // Step 6. No further validation is required.
+        // Step 5. No further validation is required.
 
         // Fill in desc. A generic descriptor has none of these fields, so copy
         // everything from shape.
         MOZ_ASSERT(!desc.hasValue());
         MOZ_ASSERT(!desc.hasWritable());
         MOZ_ASSERT(!desc.hasGetterObject());
         MOZ_ASSERT(!desc.hasSetterObject());
         if (IsDataDescriptor(shapeAttrs)) {
@@ -1626,32 +1626,34 @@ js::NativeDefineProperty(JSContext* cx, 
                 return false;
             desc.setValue(currentValue);
             desc.setWritable(IsWritable(shapeAttrs));
         } else {
             desc.setGetterObject(prop.shape()->getterObject());
             desc.setSetterObject(prop.shape()->setterObject());
         }
     } else if (desc.isDataDescriptor() != IsDataDescriptor(shapeAttrs)) {
-        // Step 7.
+        // Step 6.
         if (!IsConfigurable(shapeAttrs) && !skipRedefineChecks)
             return result.fail(JSMSG_CANT_REDEFINE_PROP);
 
         if (prop.isDenseOrTypedArrayElement()) {
             MOZ_ASSERT(!obj->is<TypedArrayObject>());
             if (!NativeObject::sparsifyDenseElement(cx, obj, JSID_TO_INT(id)))
                 return false;
             prop.setNativeProperty(obj->lookup(cx, id));
         }
 
-        // Fill in desc fields with default values (steps 7.b.i and 7.c.i).
+        // Fill in desc fields with default values (steps 6.b.i and 6.c.i).
         CompletePropertyDescriptor(&desc);
     } else if (desc.isDataDescriptor()) {
-        // Step 8.
+        // Step 7.
         bool frozen = !IsConfigurable(shapeAttrs) && !IsWritable(shapeAttrs);
+
+        // Step 7.a.i.1.
         if (frozen && desc.hasWritable() && desc.writable() && !skipRedefineChecks)
             return result.fail(JSMSG_CANT_REDEFINE_PROP);
 
         if (frozen || !desc.hasValue()) {
             if (prop.isDenseOrTypedArrayElement()) {
                 MOZ_ASSERT(!obj->is<TypedArrayObject>());
                 if (!NativeObject::sparsifyDenseElement(cx, obj, JSID_TO_INT(id)))
                     return false;
@@ -1661,62 +1663,72 @@ js::NativeDefineProperty(JSContext* cx, 
             RootedValue currentValue(cx);
             if (!GetExistingPropertyValue(cx, obj, id, prop, &currentValue))
                 return false;
 
             if (!desc.hasValue()) {
                 // Fill in desc.[[Value]].
                 desc.setValue(currentValue);
             } else {
-                // Step 8.a.ii.1.
+                // Step 7.a.i.2.
                 bool same;
                 MOZ_ASSERT(!cx->helperThread());
                 if (!SameValue(cx, desc.value(), currentValue, &same))
                     return false;
                 if (!same && !skipRedefineChecks)
                     return result.fail(JSMSG_CANT_REDEFINE_PROP);
             }
         }
 
+        // Step 7.a.i.3.
+        if (frozen && !skipRedefineChecks)
+            return result.succeed();
+
         if (!desc.hasWritable())
             desc.setWritable(IsWritable(shapeAttrs));
     } else {
-        // Step 9.
+        // Step 8.
         MOZ_ASSERT(prop.shape()->isAccessorDescriptor());
         MOZ_ASSERT(desc.isAccessorDescriptor());
 
         // The spec says to use SameValue, but since the values in
         // question are objects, we can just compare pointers.
         if (desc.hasSetterObject()) {
+            // Step 8.a.i.
             if (!IsConfigurable(shapeAttrs) &&
                 desc.setterObject() != prop.shape()->setterObject() &&
                 !skipRedefineChecks)
             {
                 return result.fail(JSMSG_CANT_REDEFINE_PROP);
             }
         } else {
             // Fill in desc.[[Set]] from shape.
             desc.setSetterObject(prop.shape()->setterObject());
         }
         if (desc.hasGetterObject()) {
+            // Step 8.a.ii.
             if (!IsConfigurable(shapeAttrs) &&
                 desc.getterObject() != prop.shape()->getterObject() &&
                 !skipRedefineChecks)
             {
                 return result.fail(JSMSG_CANT_REDEFINE_PROP);
             }
         } else {
             // Fill in desc.[[Get]] from shape.
             desc.setGetterObject(prop.shape()->getterObject());
         }
+
+        // Step 8.a.iii (Omitted).
     }
 
+    // Step 9.
+    if (!AddOrChangeProperty<IsAddOrChange::AddOrChange>(cx, obj, id, desc))
+        return false;
+
     // Step 10.
-    if (!AddOrChangeProperty<IsAddOrChange::AddOrChange>(cx, obj, id, desc))
-        return false;
     return result.succeed();
 }
 
 bool
 js::NativeDefineProperty(JSContext* cx, HandleNativeObject obj, HandleId id,
                          HandleValue value, GetterOp getter, SetterOp setter, unsigned attrs,
                          ObjectOpResult& result)
 {