Bug 1478503: Shrink capacity when modifying length on a non-extensible array. r=jandem

js/src/builtin/Array.cpp

@@ -526,7 +526,7 @@ DeleteArrayElement(JSContext* cx, HandleObject obj, uint64_t index, ObjectOpResu
                 if (!aobj->maybeCopyElementsForWrite(cx))
                     return false;
                 if (idx+1 == aobj->getDenseInitializedLength()) {
-                    aobj->setDenseInitializedLength(idx);
+                    aobj->setDenseInitializedLengthMaybeNonExtensible(cx, idx);
                 } else {
                     aobj->markDenseElementsNotPacked(cx);
                     aobj->setDenseElement(idx, MagicValue(JS_ELEMENTS_HOLE));
@@ -790,7 +790,7 @@ js::ArraySetLength(JSContext* cx, Handle<ArrayObject*> arr, HandleId id,
             uint32_t oldInitializedLength = arr->getDenseInitializedLength();
             MOZ_ASSERT(oldCapacity >= oldInitializedLength);
             if (oldInitializedLength > newLen)
-                arr->setDenseInitializedLength(newLen);
+                arr->setDenseInitializedLengthMaybeNonExtensible(cx, newLen);
             if (oldCapacity > newLen)
                 arr->shrinkElements(cx, newLen);
 
@@ -930,6 +930,9 @@ js::ArraySetLength(JSContext* cx, Handle<ArrayObject*> arr, HandleId id,
     ObjectElements* header = arr->getElementsHeader();
     header->initializedLength = Min(header->initializedLength, newLen);
 
+    if (!arr->isExtensible())
+        arr->shrinkCapacityToInitializedLength(cx);
+
     if (attrs & JSPROP_READONLY)
         arr->setNonWritableLength(cx);
 
@@ -2395,6 +2398,8 @@ js::ArrayShiftMoveElements(NativeObject* obj)
 static inline void
 SetInitializedLength(JSContext* cx, NativeObject* obj, size_t initlen)
 {
+    MOZ_ASSERT(obj->isExtensible());
+
     size_t oldInitlen = obj->getDenseInitializedLength();
     obj->setDenseInitializedLength(initlen);
     if (initlen < oldInitlen)
@@ -2405,8 +2410,7 @@ static DenseElementResult
 MoveDenseElements(JSContext* cx, NativeObject* obj, uint32_t dstStart, uint32_t srcStart,
                   uint32_t length)
 {
-    if (!obj->isExtensible())
-        return DenseElementResult::Incomplete;
+    MOZ_ASSERT(obj->isExtensible());
 
     if (!obj->maybeCopyElementsForWrite(cx))
         return DenseElementResult::Failure;
@@ -2424,6 +2428,9 @@ ArrayShiftDenseKernel(JSContext* cx, HandleObject obj, MutableHandleValue rval)
     if (MaybeInIteration(obj, cx))
         return DenseElementResult::Incomplete;
 
+    if (!obj->as<NativeObject>().isExtensible())
+        return DenseElementResult::Incomplete;
+
     size_t initlen = obj->as<NativeObject>().getDenseInitializedLength();
     if (initlen == 0)
         return DenseElementResult::Incomplete;

js/src/jit-test/tests/basic/non-extensible-elements9.js

@@ -0,0 +1,187 @@
+function testNonExtensibleStoreFallibleT() {
+    // Create an array with initialized-length = capacity = 2.
+    var x = [8, 0];
+
+    // Make it non-extensible.
+    Object.preventExtensions(x);
+
+    // Now reduce initialized-length by one, so that initialized-length < capacity is true.
+    x.length = 1;
+
+    // There's enough capacity in the elements storage to save the new element,
+    // but we still need to reject the store since the object is non-extensible.
+    x[1] = 4;
+
+    assertEq(x.length, 1);
+    assertEq(x[0], 8);
+}
+
+for (var i = 0; i < 15; ++i)
+    testNonExtensibleStoreFallibleT();
+
+// Repeat testNonExtensibleStoreFallibleT for the MIRType::Value specialization.
+function testNonExtensibleStoreFallibleV(i) {
+    // Create an array with initialized-length = capacity = 2.
+    var x = [8, ""];
+
+    // Make it non-extensible.
+    Object.preventExtensions(x);
+
+    // Now reduce initialized-length by one, so that initialized-length < capacity is true.
+    x.length = 1;
+
+    // There's enough capacity in the elements storage to save the new element,
+    // but we still need to reject the store since the object is non-extensible.
+    x[1] = [true, 1][i & 1];
+
+    assertEq(x.length, 1);
+    assertEq(x[0], 8);
+}
+
+for (var i = 0; i < 15; ++i)
+    testNonExtensibleStoreFallibleV(i);
+
+function testForInIterationNonExtensibleStoreFallibleT() {
+    // Create an array with initialized-length = capacity = 2.
+    var x = [8, 0];
+
+    // Make it non-extensible.
+    Object.preventExtensions(x);
+
+    // Modifying an array's length takes a different path during for-in
+    // iteration of the array.
+    for (var k in x) {
+        // Now reduce initialized-length by one, so that initialized-length < capacity is true.
+        x.length = 1;
+    }
+
+    // There's enough capacity in the elements storage to save the new element,
+    // but we still need to reject the store since the object is non-extensible.
+    x[1] = 4;
+
+    assertEq(x.length, 1);
+    assertEq(x[0], 8);
+}
+
+for (var i = 0; i < 15; ++i)
+    testForInIterationNonExtensibleStoreFallibleT();
+
+// Repeat testForInIterationNonExtensibleStoreFallibleT for the MIRType::Value specialization.
+function testForInIterationNonExtensibleStoreFallibleV(i) {
+    // Create an array with initialized-length = capacity = 2.
+    var x = [8, ""];
+
+    // Make it non-extensible.
+    Object.preventExtensions(x);
+
+    // Modifying an array's length takes a different path during for-in
+    // iteration of the array.
+    for (var k in x) {
+        // Now reduce initialized-length by one, so that initialized-length < capacity is true.
+        x.length = 1;
+        break;
+    }
+
+    // There's enough capacity in the elements storage to save the new element,
+    // but we still need to reject the store since the object is non-extensible.
+    x[1] = [true, 1][i & 1];
+
+    assertEq(x.length, 1);
+    assertEq(x[0], 8);
+}
+
+for (var i = 0; i < 15; ++i)
+    testForInIterationNonExtensibleStoreFallibleV(i);
+
+function testNonExtensibleArrayPop() {
+    // Create an array with initialized-length = capacity = 2.
+    var x = [8, 0];
+
+    // Make it non-extensible.
+    Object.preventExtensions(x);
+
+    // Now reduce initialized-length by one, so that initialized-length < capacity is true.
+    x.pop();
+
+    // There's enough capacity in the elements storage to save the new element,
+    // but we still need to reject the store since the object is non-extensible.
+    x[1] = 4;
+
+    assertEq(x.length, 1);
+    assertEq(x[0], 8);
+}
+
+for (var i = 0; i < 15; ++i)
+    testNonExtensibleArrayPop();
+
+function testNonExtensibleArrayPopNonWritable() {
+    // Create an array with initialized-length = capacity = 2.
+    var x = [8, 0];
+
+    // Make it non-extensible.
+    Object.preventExtensions(x);
+
+    // And make the "length" property non-writable.
+    Object.defineProperty(x, "length", {writable: false});
+
+    // Now reduce initialized-length by one, so that initialized-length < capacity is true.
+    // This doesn't update |x.length|, because the "length" property is non-writable.
+    try {
+        x.pop();
+    } catch {}
+
+    // There's enough capacity in the elements storage to save the new element,
+    // but we still need to reject the store since the object is non-extensible.
+    for (var i = 0; i < 100; ++i)
+        x[1] = 4;
+
+    assertEq(x.length, 2);
+    assertEq(x[0], 8);
+    assertEq(x[1], undefined);
+    assertEq(1 in x, false);
+}
+
+for (var i = 0; i < 15; ++i)
+    testNonExtensibleArrayPopNonWritable();
+
+function testNonExtensibleArrayShift() {
+    // Create an array with initialized-length = capacity = 2.
+    var x = [8, 0];
+
+    // Make it non-extensible.
+    Object.preventExtensions(x);
+
+    // Now reduce initialized-length by one, so that initialized-length < capacity is true.
+    x.shift();
+
+    // There's enough capacity in the elements storage to save the new element,
+    // but we still need to reject the store since the object is non-extensible.
+    x[1] = 4;
+
+    assertEq(x.length, 1);
+    assertEq(x[0], 0);
+}
+
+for (var i = 0; i < 15; ++i)
+    testNonExtensibleArrayShift();
+
+function testNonExtensibleArraySplice() {
+    // Create an array with initialized-length = capacity = 2.
+    var x = [8, 0];
+
+    // Make it non-extensible.
+    Object.preventExtensions(x);
+
+    // Now reduce initialized-length by one, so that initialized-length < capacity is true.
+    x.splice(1, 1);
+
+    // There's enough capacity in the elements storage to save the new element,
+    // but we still need to reject the store since the object is non-extensible.
+    x[1] = 4;
+
+    assertEq(x.length, 1);
+    assertEq(x[0], 8);
+}
+
+for (var i = 0; i < 15; ++i)
+    testNonExtensibleArraySplice();

js/src/jit/MCallOptimize.cpp

@@ -674,18 +674,18 @@ IonBuilder::inlineArrayPopShift(CallInfo& callInfo, MArrayPopShift::Mode mode)
     // Pop and shift are only handled for dense arrays that have never been
     // used in an iterator: popping elements does not account for suppressing
     // deleted properties in active iterators.
+    // Don't optimize shift if the array may be non-extensible (this matters
+    // when there are holes). Don't optimize pop if the array may be
+    // non-extensible, so we don't need to adjust the capacity for
+    // non-extensible arrays (non-extensible objects always have a capacity
+    // equal to their initialized length). We check this here because there's
+    // no non-extensible ObjectElements flag so we would need an extra guard
+    // on the BaseShape flags.
     ObjectGroupFlags unhandledFlags =
         OBJECT_FLAG_SPARSE_INDEXES |
         OBJECT_FLAG_LENGTH_OVERFLOW |
-        OBJECT_FLAG_ITERATED;
-
-    // Don't optimize shift if the array may be non-extensible (this matters
-    // when there are holes). We check this here because there's no
-    // non-extensible ObjectElements flag so we would need an extra guard on the
-    // BaseShape flags. For pop this doesn't matter, guarding on the SEALED
-    // ObjectElements flag in JIT code is sufficient.
-    if (mode == MArrayPopShift::Shift)
-        unhandledFlags |= OBJECT_FLAG_NON_EXTENSIBLE_ELEMENTS;
+        OBJECT_FLAG_ITERATED |
+        OBJECT_FLAG_NON_EXTENSIBLE_ELEMENTS;
 
     MDefinition* obj = convertUnboxedObjects(callInfo.thisArg());
     TemporaryTypeSet* thisTypes = obj->resultTypeSet();

js/src/vm/NativeObject-inl.h

@@ -202,11 +202,12 @@ NativeObject::initDenseElements(const Value* src, uint32_t count)
 inline bool
 NativeObject::tryShiftDenseElements(uint32_t count)
 {
+    MOZ_ASSERT(isExtensible());
+
     ObjectElements* header = getElementsHeader();
     if (header->initializedLength == count ||
         count > ObjectElements::MaxShiftedElements ||
         header->isCopyOnWrite() ||
-        !isExtensible() ||
         header->hasNonwritableArrayLength())
     {
         return false;
@@ -219,6 +220,8 @@ NativeObject::tryShiftDenseElements(uint32_t count)
 inline void
 NativeObject::shiftDenseElementsUnchecked(uint32_t count)
 {
+    MOZ_ASSERT(isExtensible());
+
     ObjectElements* header = getElementsHeader();
     MOZ_ASSERT(count > 0);
     MOZ_ASSERT(count < header->initializedLength);
@@ -320,6 +323,7 @@ NativeObject::ensureDenseInitializedLengthNoPackedCheck(uint32_t index, uint32_t
 {
     MOZ_ASSERT(!denseElementsAreCopyOnWrite());
     MOZ_ASSERT(!denseElementsAreFrozen());
+    MOZ_ASSERT(isExtensible() || (containsDenseElement(index) && extra == 1));
 
     /*
      * Ensure that the array's contents have been initialized up to index, and
@@ -346,6 +350,8 @@ NativeObject::ensureDenseInitializedLengthNoPackedCheck(uint32_t index, uint32_t
 inline void
 NativeObject::ensureDenseInitializedLength(JSContext* cx, uint32_t index, uint32_t extra)
 {
+    MOZ_ASSERT(isExtensible());
+
     if (writeToIndexWouldMarkNotPacked(index))
         markDenseElementsNotPacked(cx);
     ensureDenseInitializedLengthNoPackedCheck(index, extra);
@@ -385,6 +391,7 @@ inline DenseElementResult
 NativeObject::ensureDenseElements(JSContext* cx, uint32_t index, uint32_t extra)
 {
     MOZ_ASSERT(isNative());
+    MOZ_ASSERT(isExtensible() || (containsDenseElement(index) && extra == 1));
 
     if (writeToIndexWouldMarkNotPacked(index))
         markDenseElementsNotPacked(cx);

js/src/vm/NativeObject.cpp

@@ -659,6 +659,8 @@ NativeObject::maybeDensifySparseElements(JSContext* cx, HandleNativeObject obj)
 void
 NativeObject::moveShiftedElements()
 {
+    MOZ_ASSERT(isExtensible());
+
     ObjectElements* header = getElementsHeader();
     uint32_t numShifted = header->numShiftedElements();
     MOZ_ASSERT(numShifted > 0);
@@ -691,6 +693,8 @@ NativeObject::moveShiftedElements()
 void
 NativeObject::maybeMoveShiftedElements()
 {
+    MOZ_ASSERT(isExtensible());
+
     ObjectElements* header = getElementsHeader();
     MOZ_ASSERT(header->numShiftedElements() > 0);
 
@@ -702,6 +706,7 @@ NativeObject::maybeMoveShiftedElements()
 bool
 NativeObject::tryUnshiftDenseElements(uint32_t count)
 {
+    MOZ_ASSERT(isExtensible());
     MOZ_ASSERT(count > 0);
 
     ObjectElements* header = getElementsHeader();
@@ -717,7 +722,6 @@ NativeObject::tryUnshiftDenseElements(uint32_t count)
         // limit.
         if (header->initializedLength <= 10 ||
             header->isCopyOnWrite() ||
-            !isExtensible() ||
             header->hasNonwritableArrayLength() ||
             MOZ_UNLIKELY(count > ObjectElements::MaxShiftedElements))
         {
@@ -965,7 +969,7 @@ void
 NativeObject::shrinkElements(JSContext* cx, uint32_t reqCapacity)
 {
     MOZ_ASSERT(canHaveNonEmptyElements());
-    MOZ_ASSERT(reqCapacity >= getDenseInitializedLength());
+    MOZ_ASSERT(reqCapacity >= getDenseInitializedLengthUnchecked());
 
     if (denseElementsAreCopyOnWrite())
         MOZ_CRASH();

js/src/vm/NativeObject.h

@@ -530,7 +530,20 @@ class NativeObject : public ShapedObject
     bool containsDenseElement(uint32_t idx) {
         return idx < getDenseInitializedLength() && !elements_[idx].isMagic(JS_ELEMENTS_HOLE);
     }
+
+  private:
+    uint32_t getDenseInitializedLengthUnchecked() const {
+        return getElementsHeader()->initializedLength;
+    }
+
+  public:
     uint32_t getDenseInitializedLength() const {
+        // If the following assertion fails, there's somewhere else a missing
+        // call to shrinkCapacityToInitializedLength(). Good luck for the hunt
+        // and finding the offender!
+        MOZ_ASSERT_IF(!isExtensible(),
+                      getElementsHeader()->initializedLength == getElementsHeader()->capacity);
+
         return getElementsHeader()->initializedLength;
     }
     uint32_t getDenseCapacity() const {
@@ -1251,7 +1264,8 @@ class NativeObject : public ShapedObject
         }
     }
 
-    void setDenseInitializedLength(uint32_t length) {
+  private:
+    void setDenseInitializedLengthInternal(uint32_t length) {
         MOZ_ASSERT(length <= getDenseCapacity());
         MOZ_ASSERT(!denseElementsAreCopyOnWrite());
         MOZ_ASSERT(!denseElementsAreFrozen());
@@ -1259,6 +1273,18 @@ class NativeObject : public ShapedObject
         getElementsHeader()->initializedLength = length;
     }
 
+  public:
+    void setDenseInitializedLength(uint32_t length) {
+        MOZ_ASSERT(isExtensible());
+        setDenseInitializedLengthInternal(length);
+    }
+
+    void setDenseInitializedLengthMaybeNonExtensible(JSContext* cx, uint32_t length) {
+        setDenseInitializedLengthInternal(length);
+        if (!isExtensible())
+            shrinkCapacityToInitializedLength(cx);
+    }
+
     inline void ensureDenseInitializedLength(JSContext* cx,
                                              uint32_t index, uint32_t extra);