Bug 1090636, part 5 - In SetExistingProperty, correctly handle assigning to array.length. r=efaust.

This one is a little complicated. There is some code in SetExistingProperty that is supposed to determine whether or not we should shadow a property that wasn't found on the receiver. This code does not work correctly for array.length properties. (array.length is a data property, so of course it should be shadowed -- but it is non-shadowable() because hasSlot() is false.)

Having fixed that bug, the call to SetArrayLength is only necessary in the non-shadowing case. (In fact, it would be unnecessary altogether but for Parallel JS.) This in turn makes 'attrs' unnecessary, so delete that.

This is a behavior change (a bug fix) in the case of proxies, as the test illustrates.

--HG--
extra : rebase_source : c968a95baf41944ca82a16d40af51a1512ad0938
extra : source : f322eac33cfa1b54486d1d73f62658019774e373

js/src/jit-test/tests/proxy/testDirectProxySet9.js

@@ -5,6 +5,10 @@ var t = {x: 1};
 var p = new Proxy(t, {
     defineProperty(t, id, desc) {
         hits++;
+
+        // ES6 draft rev 28 (2014 Oct 14) 9.1.9 step 5.e.i.
+        // Since the property already exists, the system only changes
+        // the value. desc is otherwise empty.
         assertEq(Object.getOwnPropertyNames(desc).join(","), "value");
         assertEq(desc.value, 42);
     }

js/src/jit-test/tests/proxy/testDirectProxySetArray3.js

@@ -0,0 +1,20 @@
+// Assigning to the length property of a proxy to an array
+// calls the proxy's defineProperty handler.
+
+var a = [0, 1, 2, 3];
+var p = new Proxy(a, {
+    defineProperty(t, id, desc) {
+        hits++;
+
+        // ES6 draft rev 28 (2014 Oct 14) 9.1.9 step 5.e.i.
+        // Since the property already exists, the system only changes
+        // the value. desc is otherwise empty.
+        assertEq(Object.getOwnPropertyNames(desc).join(","), "value");
+        assertEq(desc.value, 2);
+    }
+});
+var hits = 0;
+p.length = 2;
+assertEq(hits, 1);
+assertEq(a.length, 4);
+assertEq(a[2], 2);

js/src/vm/NativeObject.cpp

@@ -2110,7 +2110,6 @@ SetExistingProperty(typename ExecutionModeTraits<mode>::ContextType cxArg,
                     HandleObject pobj, HandleShape foundShape, MutableHandleValue vp, bool strict)
 {
     RootedShape shape(cxArg, foundShape);
-    unsigned attrs = JSPROP_ENUMERATE;
     if (IsImplicitDenseOrTypedArrayElement(shape)) {
         /* ES5 8.12.4 [[Put]] step 2, for a dense data property on pobj. */
         if (pobj != obj)
@@ -2147,22 +2146,24 @@ SetExistingProperty(typename ExecutionModeTraits<mode>::ContextType cxArg,
             }
         }
 
-        if (pobj == receiver) {
-            attrs = shape->attributes();
-        } else {
-            // We found id in a prototype object: prepare to share or shadow.
+        if (pobj != receiver) {
+            // pobj[id] is not an own property of receiver.
 
-            if (!shape->shadowable()) {
+            if (!shape->shadowable() &&
+                !(pobj->is<ArrayObject>() && id == NameToId(cxArg->names().length)))
+            {
+                // Weird special case: slotless property with default setter.
                 if (shape->hasDefaultSetter() && !shape->hasGetterValue())
                     return true;
 
+                // We're setting an accessor property.
                 if (mode == ParallelExecution)
                     return false;
-
                 return shape->set(cxArg->asJSContext(), obj, receiver, strict, vp);
             }
 
-            // Forget we found the proto-property since we're shadowing it.
+            // Forget about pobj[id]: we're going to shadow it by defining a new
+            // data property receiver[id].
             shape = nullptr;
         }
     }
@@ -2217,15 +2218,14 @@ SetExistingProperty(typename ExecutionModeTraits<mode>::ContextType cxArg,
         return true;
     }
 
-    if (obj->is<ArrayObject>() && id == NameToId(cxArg->names().length)) {
-        Rooted<ArrayObject*> arr(cxArg, &obj->as<ArrayObject>());
-        return ArraySetLength<mode>(cxArg, arr, id, attrs, vp, strict);
-    }
+    if (shape) {
+        if (obj->is<ArrayObject>() && id == NameToId(cxArg->names().length)) {
+            Rooted<ArrayObject*> arr(cxArg, &obj->as<ArrayObject>());
+            return ArraySetLength<mode>(cxArg, arr, id, shape->attributes(), vp, strict);
+        }
 
-    if (shape)
         return NativeSet<mode>(cxArg, obj, receiver, shape, strict, vp);
-
-    MOZ_ASSERT(attrs == JSPROP_ENUMERATE);
+    }
     return SetPropertyByDefining<mode>(cxArg, receiver, id, vp, strict);
 }