Bug 1148750, part 9 - Implement ValidateAndApplyPropertyDescriptor step 7. r=efaust.
authorJason Orendorff <jorendorff@mozilla.com>
Thu, 09 Apr 2015 15:19:02 -0500
changeset 240428 f6edead66897a40dbdc63d72cc968a938a31cc92
parent 240427 00959f6f41b74ed9f03001ea4cfc9eb893bac30a
child 240429 6d51d528b8c8e9d60530b4cf0bdb103cd89f7ac6
push id28636
push userkwierso@gmail.com
push dateThu, 23 Apr 2015 00:16:12 +0000
treeherdermozilla-central@a5af73b32ac8 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersefaust
bugs1148750
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 1148750, part 9 - Implement ValidateAndApplyPropertyDescriptor step 7. r=efaust. The new code takes over some cases that used to be handled by each of the three cases that follow it. Therefore there are changes in all three cases, particularly the desc.isAccessorDescriptor() case, which no longer needs the sparsify code.
js/src/vm/NativeObject.cpp
js/xpconnect/tests/chrome/test_xrayToJS.xul
--- a/js/src/vm/NativeObject.cpp
+++ b/js/src/vm/NativeObject.cpp
@@ -1422,117 +1422,100 @@ js::NativeDefineProperty(ExclusiveContex
             if (!GetExistingPropertyValue(cx, obj, id, shape, &currentValue))
                 return false;
             desc.setValue(currentValue);
             desc.setWritable(IsWritable(shapeAttrs));
         } else {
             desc.setGetterObject(shape->getterObject());
             desc.setSetterObject(shape->setterObject());
         }
-    } else if (desc.isAccessorDescriptor()) {
-        // If defining a getter or setter, we must check for its counterpart
-        // and update the attributes and property ops.  A getter or setter is
-        // really only half of a property.
+    } else if (desc.isDataDescriptor() != IsDataDescriptor(shapeAttrs)) {
+        // Step 7.
+        if (!IsConfigurable(shapeAttrs) && !skipRedefineChecks)
+            return result.fail(JSMSG_CANT_REDEFINE_PROP);
 
-        // If we are defining a getter whose setter was already defined, or
-        // vice versa, finish the job via obj->changeProperty.
         if (IsImplicitDenseOrTypedArrayElement(shape)) {
             if (IsAnyTypedArray(obj)) {
                 // Ignore getter/setter properties added to typed arrays.
                 return result.succeed();
             }
             if (!NativeObject::sparsifyDenseElement(cx, obj, JSID_TO_INT(id)))
                 return false;
             shape = obj->lookup(cx, id);
         }
-        if (shape->isAccessorDescriptor()) {
-            if (!CheckAccessorRedefinition(cx, obj, shape, desc))
-                return false;
 
-            desc.setAttributes(ApplyOrDefaultAttributes(desc.attributes(), shape));
-            if (!desc.hasGetterObject())
-                desc.setGetter(shape->getter());
-            if (!desc.hasSetterObject())
-                desc.setSetter(shape->setter());
-            desc.attributesRef() |= JSPROP_GETTER | JSPROP_SETTER;
-            desc.assertComplete();
+        // Fill in desc fields with default values (steps 7.b.i and 7.c.i).
+        CompletePropertyDescriptor(&desc);
+    } else if (desc.isAccessorDescriptor()) {
+        // If defining a getter or setter, we must check for its counterpart
+        // and update the attributes and property ops.  A getter or setter is
+        // really only half of a property.
+
+        // If we are defining a getter whose setter was already defined, or
+        // vice versa, finish the job via obj->changeProperty.
+        if (!CheckAccessorRedefinition(cx, obj, shape, desc))
+            return false;
 
-            shape = NativeObject::changeProperty(cx, obj, shape, desc.attributes(),
-                                                 desc.getter(), desc.setter());
-            if (!shape)
-                return false;
-            if (!PurgeScopeChain(cx, obj, id))
-                return false;
+        desc.setAttributes(ApplyOrDefaultAttributes(desc.attributes(), shape));
+        if (!desc.hasGetterObject())
+            desc.setGetter(shape->getter());
+        if (!desc.hasSetterObject())
+            desc.setSetter(shape->setter());
+        desc.attributesRef() |= JSPROP_GETTER | JSPROP_SETTER;
+        desc.assertComplete();
 
-            JS_ALWAYS_TRUE(UpdateShapeTypeAndValue(cx, obj, shape, desc.value()));
-            if (!CallAddPropertyHook(cx, obj, shape, desc.value()))
-                return false;
-            return result.succeed();
-        }
+        shape = NativeObject::changeProperty(cx, obj, shape, desc.attributes(),
+                                             desc.getter(), desc.setter());
+        if (!shape)
+            return false;
+        if (!PurgeScopeChain(cx, obj, id))
+            return 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.
-        desc.attributesRef() |= JSPROP_GETTER | JSPROP_SETTER;
+        JS_ALWAYS_TRUE(UpdateShapeTypeAndValue(cx, obj, shape, desc.value()));
+        if (!CallAddPropertyHook(cx, obj, shape, desc.value()))
+            return false;
+        return result.succeed();
     } else if (desc.hasValue()) {
         // If any other JSPROP_IGNORE_* attributes are present, copy the
         // corresponding JSPROP_* attributes from the existing property.
         if (IsImplicitDenseOrTypedArrayElement(shape)) {
             desc.setAttributes(ApplyAttributes(desc.attributes(), true, true,
                                                !IsAnyTypedArray(obj)));
         } else {
             desc.setAttributes(ApplyOrDefaultAttributes(desc.attributes(), shape));
-
-            // Do not redefine a nonconfigurable accessor property.
-            if (shape->isAccessorDescriptor()) {
-                if (!CheckAccessorRedefinition(cx, obj, shape, desc))
-                    return false;
-            }
         }
     } else {
-        // We have been asked merely to update JSPROP_PERMANENT and/or JSPROP_ENUMERATE.
+        // We have been asked merely to update JSPROP_READONLY (and possibly
+        // JSPROP_CONFIGURABLE and/or JSPROP_ENUMERABLE, handled above).
 
         // Don't forget about arrays.
         if (IsImplicitDenseOrTypedArrayElement(shape)) {
             if (IsAnyTypedArray(obj)) {
                 // Silently ignore attempts to change individual index attributes.
                 // FIXME: Uses the same broken behavior as for accessors. This should
                 //        fail.
                 return result.succeed();
             }
             if (!NativeObject::sparsifyDenseElement(cx, obj, JSID_TO_INT(id)))
                 return false;
             shape = obj->lookup(cx, id);
         }
 
-        if (shape->isAccessorDescriptor() &&
-            !CheckAccessorRedefinition(cx, obj, shape, desc))
-        {
-            return false;
-        }
-
         desc.setAttributes(ApplyOrDefaultAttributes(desc.attributes(), shape));
 
-        if (shape->isAccessorDescriptor() && desc.hasWritable()) {
-            // ES6 draft 2014-10-14 9.1.6.3 step 7.c: Since [[Writable]]
-            // is present, change the existing accessor property to a data
-            // property.
-            desc.value().setUndefined();
-        } else {
-            // We are at most changing some attributes, and cannot convert
-            // from data descriptor to accessor, or vice versa. Take
-            // everything from the shape that we aren't changing.
-            uint32_t propMask = JSPROP_ENUMERATE | JSPROP_READONLY | JSPROP_PERMANENT;
-            desc.setAttributes((shape->attributes() & ~propMask) |
-                               (desc.attributes() & propMask));
-            desc.setGetter(shape->getter());
-            desc.setSetter(shape->setter());
-            if (shape->hasSlot())
-                desc.value().set(obj->getSlot(shape->slot()));
-        }
+        // We are at most changing some attributes, and cannot convert
+        // from data descriptor to accessor, or vice versa. Take
+        // everything from the shape that we aren't changing.
+        uint32_t propMask = JSPROP_ENUMERATE | JSPROP_READONLY | JSPROP_PERMANENT;
+        desc.setAttributes((shape->attributes() & ~propMask) |
+                           (desc.attributes() & propMask));
+        desc.setGetter(shape->getter());
+        desc.setSetter(shape->setter());
+        if (shape->hasSlot())
+            desc.value().set(obj->getSlot(shape->slot()));
     }
 
     // Dispense with any remaining JSPROP_IGNORE_* attributes. Any bits that
     // needed to be copied from an existing property have been copied by
     // now. Since we can never get here with JSPROP_IGNORE_VALUE relevant, just
     // clear it.
     desc.setAttributes(ApplyOrDefaultAttributes(desc.attributes()) & ~JSPROP_IGNORE_VALUE);
 
--- a/js/xpconnect/tests/chrome/test_xrayToJS.xul
+++ b/js/xpconnect/tests/chrome/test_xrayToJS.xul
@@ -493,23 +493,26 @@ https://bugzilla.mozilla.org/show_bug.cg
     is(trickyObject.wrappedJSObject.getterSetterProp, 'redefined', "Redefinition forwards");
 
     // We should NOT be able to overwrite an existing non-configurable accessor
     // prop, though.
     is(trickyObject.wrappedJSObject.nonConfigurableGetterSetterProp, 5,
        "Underlying object has getter");
     is(trickyObject.nonConfigurableGetterSetterProp, undefined,
        "Filtering properly over Xray here too");
+    is((trickyObject.nonConfigurableGetterSetterProp = 'redefined'), 'redefined',
+       "Assigning to non-configurable prop should fail silently in non-strict mode");
     checkThrows(function() {
-	trickyObject.nonConfigurableGetterSetterProp = 'redefined';
-    }, /config/, "Should throw when redefining non-configurable prop");
+      "use strict";
+      trickyObject.nonConfigurableGetterSetterProp = 'redefined';
+    }, /config/, "Should throw when redefining non-configurable prop in strict mode");
     is(trickyObject.nonConfigurableGetterSetterProp, undefined,
-       "Redefinition should have thrown");
+       "Redefinition should have failed");
     is(trickyObject.wrappedJSObject.nonConfigurableGetterSetterProp, 5,
-       "Redefinition really should have thrown");
+       "Redefinition really should have failed");
 
     checkThrows(function() { trickyObject.hasOwnProperty = 33; }, /shadow/,
                 "Should reject shadowing of pre-existing inherited properties over Xrays");
 
     checkThrows(function() { Object.defineProperty(trickyObject, 'rejectedProp', { get: function() {}}); },
                 /accessor property/, "Should reject accessor property definition");
   }