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
This commit is contained in:
Jason Orendorff 2014-10-29 01:22:28 -05:00
parent 69bbdf683d
commit 7ae9cc2401
3 changed files with 39 additions and 15 deletions

View File

@ -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);
}

View File

@ -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);

View File

@ -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);
}