Bug 1312485: Properly sparsify frozen objects. r?jandem draft
authorEmilio Cobos Álvarez <ecoal95@gmail.com>
Tue, 25 Oct 2016 11:38:05 +0200
changeset 430230 e18cd69cd6961c3eeb6c2df2344c20cb40646f37
parent 430201 4deda99ef02ea6732dade8126321b6fda7475408
child 535161 81b4192b3a86dd8435f35ae9119c95baf6fc703e
push id33777
push userbmo:ecoal95@gmail.com
push dateThu, 27 Oct 2016 10:23:20 +0000
reviewersjandem
bugs1312485
milestone52.0a1
Bug 1312485: Properly sparsify frozen objects. r?jandem MozReview-Commit-ID: CqGZuZqTBfa
js/src/tests/ecma_5/Array/frozen-dense-array.js
js/src/vm/NativeObject-inl.h
js/src/vm/NativeObject.cpp
js/src/vm/NativeObject.h
js/src/vm/Shape-inl.h
--- a/js/src/tests/ecma_5/Array/frozen-dense-array.js
+++ b/js/src/tests/ecma_5/Array/frozen-dense-array.js
@@ -4,39 +4,58 @@
  * Author: Emilio Cobos Álvarez <ecoal95@gmail.com>
  */
 var BUGNUMBER = 1310744;
 var summary = "Dense array properties shouldn't be modified when they're frozen";
 
 print(BUGNUMBER + ": " + summary);
 
 var a = Object.freeze([4, 5, 1]);
-var methods = [
-  'reverse',
-  'shift',
-  'pop',
-];
+
+function assertArrayIsExpected() {
+  assertEq(a.length, 3);
+  assertEq(a[0], 4);
+  assertEq(a[1], 5);
+  assertEq(a[2], 1);
+}
 
 assertThrowsInstanceOf(() => a.reverse(), TypeError);
 assertThrowsInstanceOf(() => a.shift(), TypeError);
 assertThrowsInstanceOf(() => a.unshift(0), TypeError);
 assertThrowsInstanceOf(() => a.sort(function() {}), TypeError);
 assertThrowsInstanceOf(() => a.pop(), TypeError);
 assertThrowsInstanceOf(() => a.fill(0), TypeError);
 assertThrowsInstanceOf(() => a.splice(0, 1, 1), TypeError);
 assertThrowsInstanceOf(() => a.push("foo"), TypeError);
 assertThrowsInstanceOf(() => { "use strict"; a.length = 5; }, TypeError);
+assertThrowsInstanceOf(() => { "use strict"; a[2] = "foo"; }, TypeError);
 assertThrowsInstanceOf(() => { "use strict"; delete a[0]; }, TypeError);
 assertThrowsInstanceOf(() => a.splice(Math.a), TypeError);
 
 // Shouldn't throw, since this is not strict mode, but shouldn't change the
 // value of the property.
 a.length = 5;
+a[2] = "foo";
 assertEq(delete a[0], false);
 
+assertArrayIsExpected();
 
-assertEq(a.length, 3);
-assertEq(a[0], 4);
-assertEq(a[1], 5);
-assertEq(a[2], 1);
+var watchpointCalled = false;
+// NOTE: Be careful with the position of this test, since this sparsifies the
+// elements and you might not test what you think you're testing otherwise.
+a.watch(2, function(prop, oldValue, newValue) {
+  watchpointCalled = true;
+  assertEq(prop, 2);
+  assertEq(oldValue, 1);
+  assertEq(newValue, "foo");
+});
+
+assertArrayIsExpected();
+
+a.length = 5;
+a[2] = "foo";
+assertEq(watchpointCalled, true);
+assertEq(delete a[0], false);
+
+assertArrayIsExpected();
 
 if (typeof reportCompare === "function")
   reportCompare(true, true);
--- a/js/src/vm/NativeObject-inl.h
+++ b/js/src/vm/NativeObject-inl.h
@@ -94,17 +94,17 @@ NativeObject::setDenseElementHole(Exclus
 }
 
 /* static */ inline void
 NativeObject::removeDenseElementForSparseIndex(ExclusiveContext* cx,
                                                HandleNativeObject obj, uint32_t index)
 {
     MarkObjectGroupFlags(cx, obj, OBJECT_FLAG_NON_PACKED | OBJECT_FLAG_SPARSE_INDEXES);
     if (obj->containsDenseElement(index))
-        obj->setDenseElement(index, MagicValue(JS_ELEMENTS_HOLE));
+        obj->setDenseElementUnchecked(index, MagicValue(JS_ELEMENTS_HOLE));
 }
 
 inline bool
 NativeObject::writeToIndexWouldMarkNotPacked(uint32_t index)
 {
     return getElementsHeader()->initializedLength < index;
 }
 
--- a/js/src/vm/NativeObject.cpp
+++ b/js/src/vm/NativeObject.cpp
@@ -514,18 +514,29 @@ NativeObject::sparsifyDenseElement(Exclu
         return false;
 
     RootedValue value(cx, obj->getDenseElement(index));
     MOZ_ASSERT(!value.isMagic(JS_ELEMENTS_HOLE));
 
     removeDenseElementForSparseIndex(cx, obj, index);
 
     uint32_t slot = obj->slotSpan();
-    if (!obj->addDataProperty(cx, INT_TO_JSID(index), slot, JSPROP_ENUMERATE)) {
-        obj->setDenseElement(index, value);
+
+    RootedId id(cx, INT_TO_JSID(index));
+
+    ShapeTable::Entry* entry = nullptr;
+    if (obj->inDictionaryMode())
+        entry = &obj->lastProperty()->table().search<MaybeAdding::Adding>(id);
+
+    // NOTE: We don't use addDataProperty because we don't want the
+    // extensibility check if we're, for example, sparsifying frozen objects..
+    if (!addPropertyInternal(cx, obj, id, nullptr, nullptr, slot,
+                             obj->getElementsHeader()->elementAttributes(),
+                             0, entry, true)) {
+        obj->setDenseElementUnchecked(index, value);
         return false;
     }
 
     MOZ_ASSERT(slot == obj->slotSpan() - 1);
     obj->initSlot(slot, value);
 
     return true;
 }
@@ -543,17 +554,17 @@ NativeObject::sparsifyDenseElements(js::
         if (obj->getDenseElement(i).isMagic(JS_ELEMENTS_HOLE))
             continue;
 
         if (!sparsifyDenseElement(cx, obj, i))
             return false;
     }
 
     if (initialized)
-        obj->setDenseInitializedLength(0);
+        obj->setDenseInitializedLengthUnchecked(0);
 
     /*
      * Reduce storage for dense elements which are now holes. Explicitly mark
      * the elements capacity as zero, so that any attempts to add dense
      * elements will be caught in ensureDenseElements.
      */
     if (obj->getDenseCapacity()) {
         obj->shrinkElements(cx, 0);
@@ -1003,17 +1014,17 @@ NativeObject::addDataProperty(ExclusiveC
     MOZ_ASSERT(!(attrs & (JSPROP_GETTER | JSPROP_SETTER)));
     RootedNativeObject self(cx, this);
     RootedId id(cx, idArg);
     return addProperty(cx, self, id, nullptr, nullptr, slot, attrs, 0);
 }
 
 Shape*
 NativeObject::addDataProperty(ExclusiveContext* cx, HandlePropertyName name,
-                          uint32_t slot, unsigned attrs)
+                              uint32_t slot, unsigned attrs)
 {
     MOZ_ASSERT(!(attrs & (JSPROP_GETTER | JSPROP_SETTER)));
     RootedNativeObject self(cx, this);
     RootedId id(cx, NameToId(name));
     return addProperty(cx, self, id, nullptr, nullptr, slot, attrs, 0);
 }
 
 template <AllowGC allowGC>
--- a/js/src/vm/NativeObject.h
+++ b/js/src/vm/NativeObject.h
@@ -300,16 +300,22 @@ class ObjectElements
         flags |= FROZEN;
     }
     void markNotFrozen() {
         MOZ_ASSERT(isFrozen());
         MOZ_ASSERT(!isCopyOnWrite());
         flags &= ~FROZEN;
     }
 
+    uint8_t elementAttributes() const {
+        if (isFrozen())
+            return JSPROP_ENUMERATE | JSPROP_PERMANENT | JSPROP_READONLY;
+        return JSPROP_ENUMERATE;
+    }
+
     // This is enough slots to store an object of this class. See the static
     // assertion below.
     static const size_t VALUES_PER_HEADER = 2;
 };
 
 static_assert(ObjectElements::VALUES_PER_HEADER * sizeof(HeapSlot) == sizeof(ObjectElements),
               "ObjectElements doesn't fit in the given number of slots");
 
@@ -859,34 +865,34 @@ class NativeObject : public ShapedObject
 
     // MAX_FIXED_SLOTS is the biggest number of fixed slots our GC
     // size classes will give an object.
     static const uint32_t MAX_FIXED_SLOTS = 16;
 
   protected:
     inline bool updateSlotsForSpan(ExclusiveContext* cx, size_t oldSpan, size_t newSpan);
 
-  public:
+  private:
+    void prepareElementRangeForOverwrite(size_t start, size_t end) {
+        MOZ_ASSERT(end <= getDenseInitializedLength());
+        MOZ_ASSERT(!denseElementsAreCopyOnWrite());
+        for (size_t i = start; i < end; i++)
+            elements_[i].HeapSlot::~HeapSlot();
+    }
+
     /*
      * Trigger the write barrier on a range of slots that will no longer be
      * reachable.
      */
     void prepareSlotRangeForOverwrite(size_t start, size_t end) {
         for (size_t i = start; i < end; i++)
             getSlotAddressUnchecked(i)->HeapSlot::~HeapSlot();
     }
 
-    void prepareElementRangeForOverwrite(size_t start, size_t end) {
-        MOZ_ASSERT(end <= getDenseInitializedLength());
-        MOZ_ASSERT(!denseElementsAreCopyOnWrite());
-        MOZ_ASSERT(!denseElementsAreFrozen());
-        for (size_t i = start; i < end; i++)
-            elements_[i].HeapSlot::~HeapSlot();
-    }
-
+  public:
     static bool rollbackProperties(ExclusiveContext* cx, HandleNativeObject obj,
                                    uint32_t slotSpan);
 
     inline void setSlotWithType(ExclusiveContext* cx, Shape* shape,
                                 const Value& value, bool overwriting = true);
 
     inline const Value& getReservedSlot(uint32_t index) const {
         MOZ_ASSERT(index < JSSLOT_FREE(getClass()));
@@ -1010,32 +1016,45 @@ class NativeObject : public ShapedObject
                 JS::shadow::Runtime* shadowRuntime = shadowRuntimeFromMainThread();
                 shadowRuntime->gcStoreBufferPtr()->putSlot(this, HeapSlot::Element,
                                                            start + i, count - i);
                 return;
             }
         }
     }
 
-  public:
-    void setDenseInitializedLength(uint32_t length) {
+    // See the comment over setDenseElementUnchecked, this applies in the same way.
+    void setDenseInitializedLengthUnchecked(uint32_t length) {
         MOZ_ASSERT(length <= getDenseCapacity());
         MOZ_ASSERT(!denseElementsAreCopyOnWrite());
-        MOZ_ASSERT(!denseElementsAreFrozen());
         prepareElementRangeForOverwrite(length, getElementsHeader()->initializedLength);
         getElementsHeader()->initializedLength = length;
     }
 
+    // Use this function with care.  This is done to allow sparsifying frozen
+    // objects, but should only be called in a few places, and should be
+    // audited carefully!
+    void setDenseElementUnchecked(uint32_t index, const Value& val) {
+        MOZ_ASSERT(index < getDenseInitializedLength());
+        MOZ_ASSERT(!denseElementsAreCopyOnWrite());
+        elements_[index].set(this, HeapSlot::Element, index, val);
+    }
+
+  public:
+    void setDenseInitializedLength(uint32_t length) {
+        MOZ_ASSERT(!denseElementsAreFrozen());
+        setDenseInitializedLengthUnchecked(length);
+    }
+
     inline void ensureDenseInitializedLength(ExclusiveContext* cx,
                                              uint32_t index, uint32_t extra);
+
     void setDenseElement(uint32_t index, const Value& val) {
-        MOZ_ASSERT(index < getDenseInitializedLength());
-        MOZ_ASSERT(!denseElementsAreCopyOnWrite());
         MOZ_ASSERT(!denseElementsAreFrozen());
-        elements_[index].set(this, HeapSlot::Element, index, val);
+        setDenseElementUnchecked(index, val);
     }
 
     void initDenseElement(uint32_t index, const Value& val) {
         MOZ_ASSERT(index < getDenseInitializedLength());
         MOZ_ASSERT(!denseElementsAreCopyOnWrite());
         MOZ_ASSERT(!denseElementsAreFrozen());
         elements_[index].init(this, HeapSlot::Element, index, val);
     }
--- a/js/src/vm/Shape-inl.h
+++ b/js/src/vm/Shape-inl.h
@@ -170,19 +170,17 @@ AutoRooterGetterSetter::AutoRooterGetter
 static inline uint8_t
 GetShapeAttributes(JSObject* obj, Shape* shape)
 {
     MOZ_ASSERT(obj->isNative());
 
     if (IsImplicitDenseOrTypedArrayElement(shape)) {
         if (obj->is<TypedArrayObject>())
             return JSPROP_ENUMERATE | JSPROP_PERMANENT;
-        if (obj->as<NativeObject>().getElementsHeader()->isFrozen())
-            return JSPROP_ENUMERATE | JSPROP_PERMANENT | JSPROP_READONLY;
-        return JSPROP_ENUMERATE;
+        return obj->as<NativeObject>().getElementsHeader()->elementAttributes();
     }
 
     return shape->attributes();
 }
 
 } /* namespace js */
 
 #endif /* vm_Shape_inl_h */