Don't modify dictionary shapes in place, bug 703157. r=luke
authorBrian Hackett <bhackett1024@gmail.com>
Thu, 05 Jan 2012 06:38:46 -0800
changeset 85108 faf5f8842fecfdb4fbee3fb112e90baf56bfb27c
parent 85107 c0b62edd29173fff8fa7f23cf45e0dbe78321614
child 85109 26d7324c8d37eac60d1abea7e4f645a55084ca68
push id805
push userakeybl@mozilla.com
push dateWed, 01 Feb 2012 18:17:35 +0000
treeherdermozilla-aurora@6fb3bf232436 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersluke
bugs703157
milestone12.0a1
Don't modify dictionary shapes in place, bug 703157. r=luke
js/src/jit-test/tests/basic/bug703157.js
js/src/jsobj.h
js/src/jsscope.cpp
js/src/jsscope.h
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/basic/bug703157.js
@@ -0,0 +1,36 @@
+// Define a test object
+var test = {x:1,y:2};
+
+// Put the object into dictionary mode by deleting 
+// a property that was not the last one added.
+delete test.x;
+
+// Define a an accessor property with a setter that 
+// itself calls Object.defineProperty
+Object.defineProperty(test, "foo", {
+    get: function() { return 1; },
+    set: function(v) { 
+        Object.defineProperty(this, "foo", { value: v });
+        // Prints the correct object descriptor
+        assertEq(this.foo, 33);
+    },
+    configurable: true
+});
+
+// Add another property, so generateOwnShape does not replace the foo property.
+test.other = 0;
+
+// This line prints 1, as expected
+assertEq(test.foo, 1);
+
+// Now set the property.  This calls the setter method above.
+// And the setter method prints the expected value and property descriptor.
+test.foo = 33;
+
+// Finally read the newly set value.
+assertEq(test.foo, 33);
+
+// Check that enumeration order is correct.
+var arr = [];
+for (var x in test) arr.push(x);
+assertEq("" + arr, "y,other");
--- a/js/src/jsobj.h
+++ b/js/src/jsobj.h
@@ -612,19 +612,24 @@ struct JSObject : js::gc::Cell
      * Objects with an uncacheable proto can have their prototype mutated
      * without inducing a shape change on the object. Property cache entries
      * and JIT inline caches should not be filled for lookups across prototype
      * lookups on the object.
      */
     inline bool hasUncacheableProto() const;
     inline bool setUncacheableProto(JSContext *cx);
 
-    bool generateOwnShape(JSContext *cx, js::Shape *newShape = NULL);
+    bool generateOwnShape(JSContext *cx, js::Shape *newShape = NULL) {
+        return replaceWithNewEquivalentShape(cx, lastProperty(), newShape);
+    }
 
   private:
+    js::Shape *replaceWithNewEquivalentShape(JSContext *cx, js::Shape *existingShape,
+                                             js::Shape *newShape = NULL);
+
     enum GenerateShape {
         GENERATE_NONE,
         GENERATE_SHAPE
     };
 
     bool setFlag(JSContext *cx, /*BaseShape::Flag*/ uint32_t flag,
                  GenerateShape generateShape = GENERATE_NONE);
 
--- a/js/src/jsscope.cpp
+++ b/js/src/jsscope.cpp
@@ -786,53 +786,49 @@ JSObject::putProperty(JSContext *cx, jsi
         if (!self->toDictionaryMode(cx))
             return NULL;
         spp = self->lastProperty()->table().search(shape->propid(), false);
         shape = SHAPE_FETCH(spp);
     }
 
     JS_ASSERT_IF(shape->hasSlot() && !(attrs & JSPROP_SHARED), shape->slot() == slot);
 
-    /*
-     * Optimize the case of a dictionary-mode object based on the property that
-     * dictionaries exclusively own their mutable shape structs, each of which
-     * has a unique shape (not shared via a shape tree). We can update the
-     * shape in place, though after each modification we need to generate a new
-     * last property to invalidate shape guards.
-     *
-     * This is more than an optimization: it is required to preserve for-in
-     * enumeration order (see bug 601399).
-     */
     if (self->inDictionaryMode()) {
+        /*
+         * Updating some property in a dictionary-mode object. Create a new
+         * shape for the existing property, and also generate a new shape for
+         * the last property of the dictionary (unless the modified property
+         * is also the last property).
+         */
         bool updateLast = (shape == self->lastProperty());
-        if (!self->generateOwnShape(cx))
+        shape = self->replaceWithNewEquivalentShape(cx, shape);
+        if (!shape)
             return NULL;
-        if (updateLast)
-            shape = self->lastProperty();
+        if (!updateLast && !self->generateOwnShape(cx))
+            return NULL;
 
         /* FIXME bug 593129 -- slot allocation and JSObject *this must move out of here! */
         if (slot == SHAPE_INVALID_SLOT && !(attrs & JSPROP_SHARED)) {
             if (!self->allocSlot(cx, &slot))
                 return NULL;
         }
 
-        if (shape == self->lastProperty())
+        if (updateLast)
             shape->base()->adoptUnowned(nbase);
         else
             shape->base_ = nbase;
 
         shape->setSlot(slot);
         shape->attrs = uint8_t(attrs);
         shape->flags = flags | Shape::IN_DICTIONARY;
         shape->shortid_ = int16_t(shortid);
     } else {
         /*
-         * Updating the last property in a non-dictionary-mode object.  If any
-         * shape in the property tree has a property hashtable, it is shared
-         * and immutable too, therefore we must not update *spp.
+         * Updating the last property in a non-dictionary-mode object. Find an
+         * alternate shared child of the last property's previous shape.
          */
         StackBaseShape base(self->lastProperty()->base());
         base.updateGetterSetter(attrs, getter, setter);
         UnownedBaseShape *nbase = BaseShape::getUnowned(cx, base);
         if (!nbase)
             return NULL;
 
         JS_ASSERT(shape == self->lastProperty());
@@ -1057,49 +1053,63 @@ JSObject::rollbackProperties(JSContext *
      */
     JS_ASSERT(!inDictionaryMode() && slotSpan <= this->slotSpan());
     while (this->slotSpan() != slotSpan) {
         JS_ASSERT(lastProperty()->hasSlot() && getSlot(lastProperty()->slot()).isUndefined());
         removeLastProperty(cx);
     }
 }
 
-bool
-JSObject::generateOwnShape(JSContext *cx, Shape *newShape)
+Shape *
+JSObject::replaceWithNewEquivalentShape(JSContext *cx, Shape *oldShape, Shape *newShape)
 {
-    RootedVarObject self(cx, this);
+    JS_ASSERT_IF(oldShape != lastProperty(),
+                 inDictionaryMode() &&
+                 nativeLookup(cx, oldShape->maybePropid()) == oldShape);
+
+    JSObject *self = this;
 
-    if (!inDictionaryMode() && !toDictionaryMode(cx))
-        return false;
+    if (!inDictionaryMode()) {
+        RootObject selfRoot(cx, &self);
+        RootShape newRoot(cx, &newShape);
+        if (!toDictionaryMode(cx))
+            return false;
+        oldShape = lastProperty();
+    }
 
     if (!newShape) {
+        RootObject selfRoot(cx, &self);
+        RootShape oldRoot(cx, &oldShape);
         newShape = js_NewGCShape(cx);
         if (!newShape)
             return false;
-        new (newShape) Shape(self->lastProperty()->base()->unowned(), 0);
+        new (newShape) Shape(oldShape->base()->unowned(), 0);
     }
 
     PropertyTable &table = self->lastProperty()->table();
-    Shape **spp = self->lastProperty()->isEmptyShape()
+    Shape **spp = oldShape->isEmptyShape()
                   ? NULL
-                  : table.search(self->lastProperty()->maybePropid(), false);
+                  : table.search(oldShape->maybePropid(), false);
 
-    Shape *oldShape = self->lastProperty();
-
-    StackShape nshape(self->lastProperty());
-    newShape->initDictionaryShape(nshape, self->numFixedSlots(), &self->shape_);
+    /*
+     * Splice the new shape into the same position as the old shape, preserving
+     * enumeration order (see bug 601399).
+     */
+    StackShape nshape(oldShape);
+    newShape->initDictionaryShape(nshape, self->numFixedSlots(), oldShape->listp);
 
     JS_ASSERT(newShape->parent == oldShape);
     oldShape->removeFromDictionary(self);
 
-    oldShape->handoffTableTo(newShape);
+    if (newShape == lastProperty())
+        oldShape->handoffTableTo(newShape);
 
     if (spp)
         SHAPE_STORE_PRESERVING_COLLISION(spp, newShape);
-    return true;
+    return newShape;
 }
 
 Shape *
 JSObject::methodShapeChange(JSContext *cx, const Shape &shape)
 {
     JS_ASSERT(shape.isMethod());
 
     if (!inDictionaryMode() && !toDictionaryMode(cx))
--- a/js/src/jsscope.h
+++ b/js/src/jsscope.h
@@ -86,17 +86,18 @@
  *    a property tree is a shape lineage. Property trees permit full (or
  *    partial) sharing of Shapes between objects that have fully (or partly)
  *    identical layouts. The root is an EmptyShape whose identity is determined
  *    by the object's class, compartment and prototype. These Shapes are shared
  *    and immutable.
  * 
  * 2. Dictionary mode lists. Shapes in such lists are said to be "in
  *    dictionary mode", as are objects that point to such Shapes. These Shapes
- *    are unshared, private to a single object, and mutable.
+ *    are unshared, private to a single object, and immutable except for their
+ *    links in the dictionary list.
  * 
  * All shape lineages are bi-directionally linked, via the |parent| and
  * |kids|/|listp| members.
  * 
  * Shape lineages start out life in the property tree. They can be converted
  * (by copying) to dictionary mode lists in the following circumstances.
  * 
  * 1. The shape lineage's size reaches MAX_HEIGHT. This reasonable limit avoids