Bug 601046: fix freelist maintenance and modify shapes in place in changeProperty, r=brendan
authorDavid Mandelin <dmandelin@mozilla.com>
Mon, 06 Dec 2010 16:27:39 -0800
changeset 58749 8cd47ad6b71d259c78747c96df4538f44bcebc72
parent 58748 cc202b48606f1cbbbfe8d110579935ac41fa4cc5
child 58750 4e41063ec08f7a5b4a303422d0ab338b7de62144
push id17414
push userrsayre@mozilla.com
push dateTue, 07 Dec 2010 03:47:09 +0000
treeherdermozilla-central@37b29506a7d4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbrendan
bugs601046
milestone2.0b8pre
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 601046: fix freelist maintenance and modify shapes in place in changeProperty, r=brendan
js/src/jit-test/tests/basic/bug601046.js
js/src/jsobj.cpp
js/src/jsobj.h
js/src/jsscope.cpp
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/basic/bug601046.js
@@ -0,0 +1,8 @@
+// don't assert
+var f = function(){};
+for (var p in f);
+Object.defineProperty(f, "j", ({configurable: true, value: "a"}));
+Object.defineProperty(f, "k", ({configurable: true, value: "b"}));
+Object.defineProperty(f, "j", ({configurable: true, get: function() {}}));
+delete f.k;
+Object.defineProperty(f, "j", ({configurable: false}));
--- a/js/src/jsobj.cpp
+++ b/js/src/jsobj.cpp
@@ -4279,17 +4279,17 @@ JSObject::allocSlot(JSContext *cx, uint3
         return false;
 
     /* JSObject::growSlots or JSObject::freeSlot should set the free slots to void. */
     JS_ASSERT(getSlot(slot).isUndefined());
     *slotp = slot;
     return true;
 }
 
-void
+bool
 JSObject::freeSlot(JSContext *cx, uint32 slot)
 {
     uint32 limit = slotSpan();
     JS_ASSERT(slot < limit);
 
     Value &vref = getSlotRef(slot);
     if (inDictionaryMode() && lastProp->table) {
         uint32 &last = lastProp->table->freelist;
@@ -4302,20 +4302,21 @@ JSObject::freeSlot(JSContext *cx, uint32
          * shape (and not a reserved slot; see bug 595230): push the slot onto
          * the dictionary property table's freelist. We want to let the last
          * slot be freed by shrinking the dslots vector; see js_TraceObject.
          */
         if (JSSLOT_FREE(clasp) <= slot && slot + 1 < limit) {
             JS_ASSERT_IF(last != SHAPE_INVALID_SLOT, last < slotSpan());
             vref.setPrivateUint32(last);
             last = slot;
-            return;
+            return true;
         }
     }
     vref.setUndefined();
+    return false;
 }
 
 /* JSBOXEDWORD_INT_MAX as a string */
 #define JSBOXEDWORD_INT_MAX_STRING "1073741823"
 
 /*
  * Convert string indexes that convert to int jsvals as ints to save memory.
  * Care must be taken to use this macro every time a property name is used, or
--- a/js/src/jsobj.h
+++ b/js/src/jsobj.h
@@ -1026,17 +1026,20 @@ struct JSObject : js::gc::Cell {
     inline void freeSlotsArray(JSContext *cx);
 
     /* Free the slots array and copy slots that fit into the fixed array. */
     inline void revertToFixedSlots(JSContext *cx);
 
     inline bool hasProperty(JSContext *cx, jsid id, bool *foundp, uintN flags = 0);
 
     bool allocSlot(JSContext *cx, uint32 *slotp);
-    void freeSlot(JSContext *cx, uint32 slot);
+    /*
+     * Return true iff this is a dictionary-mode object and the freed slot was
+     * added to the freelist. */
+    bool freeSlot(JSContext *cx, uint32 slot);
 
     bool reportReadOnly(JSContext* cx, jsid id, uintN report = JSREPORT_ERROR);
     bool reportNotConfigurable(JSContext* cx, jsid id, uintN report = JSREPORT_ERROR);
     bool reportNotExtensible(JSContext *cx, uintN report = JSREPORT_ERROR);
 
   private:
     js::Shape *getChildProperty(JSContext *cx, js::Shape *parent, js::Shape &child);
 
--- a/js/src/jsscope.cpp
+++ b/js/src/jsscope.cpp
@@ -1072,51 +1072,60 @@ JSObject::changeProperty(JSContext *cx, 
 
     if (getter == PropertyStub)
         getter = NULL;
     if (setter == PropertyStub)
         setter = NULL;
     if (shape->attrs == attrs && shape->getter() == getter && shape->setter() == setter)
         return shape;
 
-    Shape child(shape->id, getter, setter, shape->slot, attrs, shape->flags, shape->shortid);
-
     const Shape *newShape;
 
+    /*
+     * Dictionary-mode objects exclusively own their mutable shape structs, so
+     * we simply modify in place.
+     */
     if (inDictionaryMode()) {
-        shape->removeFromDictionary(this);
-        newShape = Shape::newDictionaryShape(cx, child, &lastProp);
-        if (newShape) {
-            JS_ASSERT(newShape == lastProp);
-
-            /*
-             * Let tableShape be the shape with non-null table, either the one
-             * we removed or the parent of lastProp.
-             */
-            const Shape *tableShape = shape->table ? shape : lastProp->parent;
+        /* FIXME bug 593129 -- slot allocation and JSObject *this must move out of here! */
+        uint32 slot = shape->slot;
+        if (slot == SHAPE_INVALID_SLOT && !(attrs & JSPROP_SHARED) && !(flags & Shape::ALIAS)) {
+            if (!allocSlot(cx, &slot))
+                return NULL;
+        }
 
-            if (PropertyTable *table = tableShape->table) {
-                /* Overwrite shape with newShape in the property table. */
-                Shape **spp = table->search(shape->id, true);
-                SHAPE_STORE_PRESERVING_COLLISION(spp, newShape);
+        Shape *mutableShape = const_cast<Shape *>(shape);
+        mutableShape->slot = slot;
+        if (slot != SHAPE_INVALID_SLOT && slot >= shape->slotSpan) {
+            mutableShape->slotSpan = slot + 1;
 
-                /* Hand the table off from tableShape to newShape. */
-                tableShape->setTable(NULL);
-                newShape->setTable(table);
-            }
-
-            updateFlags(newShape);
-            updateShape(cx);
-
-            if (!js_UpdateWatchpointsForShape(cx, this, newShape)) {
-                METER(wrapWatchFails);
-                return NULL;
+            for (Shape *temp = lastProp; temp != shape; temp = temp->parent) {
+                if (temp->slotSpan <= slot)
+                    temp->slotSpan = slot + 1;
             }
         }
+
+        mutableShape->rawGetter = getter;
+        mutableShape->rawSetter = setter;
+        mutableShape->attrs = uint8(attrs);
+
+        updateFlags(shape);
+
+        /* See the corresponding code in putProperty. */
+        lastProp->shape = js_GenerateShape(cx, false);
+        clearOwnShape();
+
+        if (!js_UpdateWatchpointsForShape(cx, this, shape)) {
+            METER(wrapWatchFails);
+            return NULL;
+        }
+
+        newShape = mutableShape;
     } else if (shape == lastProp) {
+        Shape child(shape->id, getter, setter, shape->slot, attrs, shape->flags, shape->shortid);
+
         newShape = getChildProperty(cx, shape->parent, child);
 #ifdef DEBUG
         if (newShape) {
             JS_ASSERT(newShape == lastProp);
             if (newShape->table) {
                 Shape **spp = nativeSearch(shape->id);
                 JS_ASSERT(SHAPE_FETCH(spp) == newShape);
             }
@@ -1124,16 +1133,17 @@ JSObject::changeProperty(JSContext *cx, 
 #endif
     } else {
         /*
          * Let JSObject::putProperty handle this |overwriting| case, including
          * the conservation of shape->slot (if it's valid). We must not call
          * removeProperty because it will free an allocated shape->slot, and
          * putProperty won't re-allocate it.
          */
+        Shape child(shape->id, getter, setter, shape->slot, attrs, shape->flags, shape->shortid);
         newShape = putProperty(cx, child.id, child.rawGetter, child.rawSetter, child.slot,
                                child.attrs, child.flags, child.shortid);
 #ifdef DEBUG
         if (newShape)
             METER(changePuts);
 #endif
     }
 
@@ -1153,19 +1163,20 @@ JSObject::removeProperty(JSContext *cx, 
     Shape **spp = nativeSearch(id);
     Shape *shape = SHAPE_FETCH(spp);
     if (!shape) {
         METER(uselessRemoves);
         return true;
     }
 
     /* First, if shape is unshared and not has a slot, free its slot number. */
+    bool addedToFreelist = false;
     bool hadSlot = !shape->isAlias() && shape->hasSlot();
     if (hadSlot) {
-        freeSlot(cx, shape->slot);
+        addedToFreelist = freeSlot(cx, shape->slot);
         JS_ATOMIC_INCREMENT(&cx->runtime->propertyRemovals);
     }
 
 
     /* If shape is not the last property added, switch to dictionary mode. */
     if (shape != lastProp && !inDictionaryMode()) {
         if (!toDictionaryMode(cx))
             return false;
@@ -1220,22 +1231,30 @@ JSObject::removeProperty(JSContext *cx, 
         if (table) {
             if (shape == oldLastProp) {
                 JS_ASSERT(shape->table == table);
                 JS_ASSERT(shape->parent == lastProp);
                 JS_ASSERT(shape->slotSpan >= lastProp->slotSpan);
                 JS_ASSERT_IF(hadSlot, shape->slot + 1 <= shape->slotSpan);
 
                 /*
-                 * If the dictionary table's freelist is non-empty, we must
-                 * preserve lastProp->slotSpan. We can't reduce slotSpan even
-                 * by one or we might lose non-decreasing slotSpan order.
-                 */
-                if (table->freelist != SHAPE_INVALID_SLOT)
+                 * Maintain slot freelist consistency. The only constraint we
+                 * have is that slot numbers on the freelist are less than 
+                 * lastProp->slotSpan. Thus, if the freelist is non-empty,
+                 * then lastProp->slotSpan may not decrease.
+                 */ 
+                if (table->freelist != SHAPE_INVALID_SLOT) {
                     lastProp->slotSpan = shape->slotSpan;
+                    
+                    /* Add the slot to the freelist if it wasn't added in freeSlot. */
+                    if (hadSlot && !addedToFreelist) {
+                        getSlotRef(shape->slot).setPrivateUint32(table->freelist);
+                        table->freelist = shape->slot;
+                    }
+                }
             }
 
             /* Hand off table from old to new lastProp. */
             oldLastProp->setTable(NULL);
             lastProp->setTable(table);
         }
     } else {
         /*