Ensure consistency between an owned base shape and its unowned version, bug 712428. r=luke
authorBrian Hackett <bhackett1024@gmail.com>
Sat, 24 Dec 2011 06:32:27 -0800
changeset 84569 c93d8c55f67d07b17be78bfefaf77641d0b95d6b
parent 84568 eaac85c4c05f32affdc377a0ebe373379541325f
child 84570 45206fca898d121626f890cb2bdde1ac37a1535f
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
bugs712428
milestone12.0a1
Ensure consistency between an owned base shape and its unowned version, bug 712428. r=luke
js/src/jsfun.cpp
js/src/jsgcmark.cpp
js/src/jsobj.h
js/src/jsobjinlines.h
js/src/jsscope.cpp
js/src/jsscope.h
js/src/jsscopeinlines.h
--- a/js/src/jsfun.cpp
+++ b/js/src/jsfun.cpp
@@ -1721,17 +1721,18 @@ JSFunction::initBoundFunction(JSContext 
     /*
      * Convert to a dictionary to set the BOUND_FUNCTION flag and increase
      * the slot span to cover the arguments and additional slots for the 'this'
      * value and arguments count.
      */
     if (!toDictionaryMode(cx))
         return false;
 
-    lastProperty()->base()->setObjectFlag(BaseShape::BOUND_FUNCTION);
+    if (!setFlag(cx, BaseShape::BOUND_FUNCTION))
+        return false;
 
     if (!setSlotSpan(cx, BOUND_FUNCTION_RESERVED_SLOTS + argslen))
         return false;
 
     setSlot(JSSLOT_BOUND_FUNCTION_THIS, thisArg);
     setSlot(JSSLOT_BOUND_FUNCTION_ARGS_COUNT, PrivateUint32Value(argslen));
 
     copySlotRange(BOUND_FUNCTION_RESERVED_SLOTS, args, argslen, false);
--- a/js/src/jsgcmark.cpp
+++ b/js/src/jsgcmark.cpp
@@ -683,39 +683,36 @@ ScanShape(GCMarker *gcmarker, const Shap
     shape = shape->previous();
     if (shape && shape->markIfUnmarked(gcmarker->getMarkColor()))
         goto restart;
 }
 
 static inline void
 ScanBaseShape(GCMarker *gcmarker, BaseShape *base)
 {
-    for (;;) {
-        if (base->hasGetterObject())
-            PushMarkStack(gcmarker, base->getterObject());
+    base->assertConsistency();
 
-        if (base->hasSetterObject())
-            PushMarkStack(gcmarker, base->setterObject());
+    if (base->hasGetterObject())
+        PushMarkStack(gcmarker, base->getterObject());
 
-        if (JSObject *parent = base->getObjectParent())
-            PushMarkStack(gcmarker, parent);
+    if (base->hasSetterObject())
+        PushMarkStack(gcmarker, base->setterObject());
 
-        if (base->isOwned()) {
-            /*
-             * Make sure that ScanBaseShape is not recursive so its inlining
-             * is possible.
-             */
-            UnownedBaseShape *unowned = base->baseUnowned();
-            JS_SAME_COMPARTMENT_ASSERT(base, unowned);
-            if (unowned->markIfUnmarked(gcmarker->getMarkColor())) {
-                base = unowned;
-                continue;
-            }
-        }
-        break;
+    if (JSObject *parent = base->getObjectParent())
+        PushMarkStack(gcmarker, parent);
+
+    /*
+     * All children of the owned base shape are consistent with its
+     * unowned one, thus we do not need to trace through children of the
+     * unowned base shape.
+     */
+    if (base->isOwned()) {
+        UnownedBaseShape *unowned = base->baseUnowned();
+        JS_SAME_COMPARTMENT_ASSERT(base, unowned);
+        unowned->markIfUnmarked(gcmarker->getMarkColor());
     }
 }
 
 static inline void
 ScanLinearString(GCMarker *gcmarker, JSLinearString *str)
 {
     JS_COMPARTMENT_ASSERT_STR(gcmarker->runtime, str);
     JS_ASSERT(str->isMarked());
@@ -947,34 +944,30 @@ MarkChildren(JSTracer *trc, BaseShape *b
  * marked only if it isn't the same as prevParent, which will be
  * updated to the current shape's parent.
  */
 inline void
 MarkCycleCollectorChildren(JSTracer *trc, BaseShape *base, JSObject **prevParent)
 {
     JS_ASSERT(base);
 
+    /*
+     * The cycle collector does not need to trace unowned base shapes,
+     * as they have the same getter, setter and parent as the original
+     * base shape.
+     */
+    base->assertConsistency();
+
     MarkBaseShapeGetterSetter(trc, base);
 
     JSObject *parent = base->getObjectParent();
     if (parent && parent != *prevParent) {
         MarkObjectUnbarriered(trc, parent, "parent");
         *prevParent = parent;
     }
-
-    // An owned base shape has the same parent, getter and setter as
-    // its baseUnowned().
-#ifdef DEBUG
-    if (base->isOwned()) {
-        UnownedBaseShape *unowned = base->baseUnowned();
-        JS_ASSERT_IF(base->hasGetterObject(), base->getterObject() == unowned->getterObject());
-        JS_ASSERT_IF(base->hasSetterObject(), base->setterObject() == unowned->setterObject());
-        JS_ASSERT(base->getObjectParent() == unowned->getObjectParent());
-    }
-#endif
 }
 
 /*
  * This function is used by the cycle collector to trace through a
  * shape. The cycle collector does not care about shapes or base
  * shapes, so those are not marked. Instead, any shapes or base shapes
  * that are encountered have their children marked. Stack space is
  * bounded. If two shapes in a row have the same parent pointer, the
--- a/js/src/jsobj.h
+++ b/js/src/jsobj.h
@@ -610,21 +610,18 @@ struct JSObject : js::gc::Cell
      * Defined in jsobjinlines.h, but not declared inline per standard style in
      * order to avoid gcc warnings.
      */
     js::Shape *methodReadBarrier(JSContext *cx, const js::Shape &shape, js::Value *vp);
 
     /* Whether method shapes can be added to this object. */
     inline bool canHaveMethodBarrier() const;
 
+    /* Whether there may be indexed properties on this object. */
     inline bool isIndexed() const;
-    inline bool setIndexed(JSContext *cx);
-
-    /* Set the indexed flag on this object if id is an indexed property. */
-    inline bool maybeSetIndexed(JSContext *cx, jsid id);
 
     /*
      * Return true if this object is a native one that has been converted from
      * shared-immutable prototype-rooted shape storage to dictionary-shapes in
      * a doubly-linked list.
      */
     inline bool inDictionaryMode() const;
 
@@ -784,19 +781,16 @@ struct JSObject : js::gc::Cell
     const js::Value &getFixedSlot(uintN slot) const {
         JS_ASSERT(slot < numFixedSlots());
         return fixedSlots()[slot];
     }
 
     inline void setFixedSlot(uintN slot, const js::Value &value);
     inline void initFixedSlot(uintN slot, const js::Value &value);
 
-    /* Extend this object to have shape as its last-added property. */
-    inline bool extend(JSContext *cx, const js::Shape *shape, bool isDefinitelyAtom = false);
-
     /*
      * Whether this is the only object which has its specified type. This
      * object will have its type constructed lazily as needed by analysis.
      */
     bool hasSingletonType() const { return !!type_->singleton; }
 
     /*
      * Whether the object's type has not been constructed yet. If an object
--- a/js/src/jsobjinlines.h
+++ b/js/src/jsobjinlines.h
@@ -888,21 +888,16 @@ inline bool JSObject::isDelegate() const
     return lastProperty()->hasObjectFlag(js::BaseShape::DELEGATE);
 }
 
 inline bool JSObject::setDelegate(JSContext *cx)
 {
     return setFlag(cx, js::BaseShape::DELEGATE, GENERATE_SHAPE);
 }
 
-inline bool JSObject::setIndexed(JSContext *cx)
-{
-    return setFlag(cx, js::BaseShape::INDEXED);
-}
-
 inline bool JSObject::isVarObj() const
 {
     return lastProperty()->hasObjectFlag(js::BaseShape::VAROBJ);
 }
 
 inline bool JSObject::setVarObj(JSContext *cx)
 {
     return setFlag(cx, js::BaseShape::VAROBJ);
--- a/js/src/jsscope.cpp
+++ b/js/src/jsscope.cpp
@@ -649,28 +649,28 @@ JSObject::addPropertyInternal(JSContext 
             if (!table->grow(cx))
                 return NULL;
 
             spp = table->search(id, true);
             JS_ASSERT(!SHAPE_FETCH(spp));
         }
     }
 
-    if (!maybeSetIndexed(cx, id))
-        return NULL;
-
     /* Find or create a property tree node labeled by our arguments. */
     Shape *shape;
     {
+        jsuint index;
+        bool indexed = js_IdIsIndex(id, &index);
         UnownedBaseShape *nbase;
-        if (lastProperty()->base()->matchesGetterSetter(getter, setter)) {
+        if (lastProperty()->base()->matchesGetterSetter(getter, setter) && !indexed) {
             nbase = lastProperty()->base()->unowned();
         } else {
-            BaseShape base(getClass(), getParent(), lastProperty()->getObjectFlags(),
-                           attrs, getter, setter);
+            uint32_t flags = lastProperty()->getObjectFlags()
+                             | (indexed ? BaseShape::INDEXED : 0);
+            BaseShape base(getClass(), getParent(), flags, attrs, getter, setter);
             nbase = BaseShape::getUnowned(cx, base);
             if (!nbase)
                 return NULL;
         }
 
         Shape child(nbase, id, slot, numFixedSlots(), attrs, flags, shortid);
         shape = getChildProperty(cx, lastProperty(), child);
     }
@@ -759,18 +759,21 @@ JSObject::putProperty(JSContext *cx, jsi
      */
     bool hadSlot = shape->hasSlot();
     uint32_t oldSlot = shape->maybeSlot();
     if (!(attrs & JSPROP_SHARED) && slot == SHAPE_INVALID_SLOT && hadSlot)
         slot = oldSlot;
 
     UnownedBaseShape *nbase;
     {
-        BaseShape base(getClass(), getParent(), lastProperty()->getObjectFlags(),
-                       attrs, getter, setter);
+        jsuint index;
+        bool indexed = js_IdIsIndex(id, &index);
+        uint32_t flags = lastProperty()->getObjectFlags()
+                         | (indexed ? BaseShape::INDEXED : 0);
+        BaseShape base(getClass(), getParent(), flags, attrs, getter, setter);
         nbase = BaseShape::getUnowned(cx, base);
         if (!nbase)
             return NULL;
     }
 
     /*
      * Now that we've possibly preserved slot, check whether all members match.
      * If so, this is a redundant "put" and we can return without more work.
@@ -819,26 +822,16 @@ JSObject::putProperty(JSContext *cx, jsi
             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);
-
-        /*
-         * We are done updating shape and the last property. Now we may need to
-         * update flags. In the last non-dictionary property case in the else
-         * clause just below, getChildProperty handles this for us. First update
-         * flags.
-         */
-        jsuint index;
-        if (js_IdIsIndex(shape->propid(), &index))
-            shape->base()->setObjectFlag(BaseShape::INDEXED);
     } else {
         /*
          * Updating the last property in a non-dictionary-mode object. Such
          * objects share their shapes via a tree rooted at a prototype
          * emptyShape, or perhaps a well-known compartment-wide singleton
          * emptyShape.
          *
          * If any shape in the tree has a property hashtable, it is shared and
@@ -951,16 +944,31 @@ JSObject::removeProperty(JSContext *cx, 
      * the object or table, so the remaining removal is infallible.
      */
     Shape *spare = NULL;
     if (inDictionaryMode()) {
         spare = js_NewGCShape(cx);
         if (!spare)
             return false;
         new (spare) Shape(shape->base()->unowned(), 0);
+        if (shape == lastProperty()) {
+            /*
+             * Get an up to date unowned base shape for the new last property
+             * when removing the dictionary's last property. Information in
+             * base shapes for non-last properties may be out of sync with the
+             * object's state.
+             */
+            Shape *previous = lastProperty()->parent;
+            BaseShape base(getClass(), getParent(), lastProperty()->getObjectFlags(),
+                           previous->attrs, previous->getter(), previous->setter());
+            BaseShape *nbase = BaseShape::getUnowned(cx, base);
+            if (!nbase)
+                return false;
+            previous->base_ = nbase;
+        }
     }
 
     /* If shape has a slot, free its slot number. */
     if (shape->hasSlot()) {
         freeSlot(cx, shape->slot());
         JS_ATOMIC_INCREMENT(&cx->runtime->propertyRemovals);
     }
 
@@ -1138,31 +1146,37 @@ JSObject::clearParent(JSContext *cx)
 
 bool
 JSObject::setParent(JSContext *cx, JSObject *parent)
 {
     if (parent && !parent->setDelegate(cx))
         return false;
 
     if (inDictionaryMode()) {
-        lastProperty()->base()->setParent(parent);
+        BaseShape base(*lastProperty()->base()->unowned());
+        base.setObjectParent(parent);
+        UnownedBaseShape *nbase = BaseShape::getUnowned(cx, base);
+        if (!nbase)
+            return false;
+
+        lastProperty()->base()->adoptUnowned(nbase);
         return true;
     }
 
     return Shape::setObjectParent(cx, parent, getProto(), &shape_);
 }
 
 /* static */ bool
 Shape::setObjectParent(JSContext *cx, JSObject *parent, JSObject *proto, HeapPtrShape *listp)
 {
     if ((*listp)->getObjectParent() == parent)
         return true;
 
     BaseShape base(*(*listp)->base()->unowned());
-    base.setParent(parent);
+    base.setObjectParent(parent);
 
     return replaceLastProperty(cx, base, proto, listp);
 }
 
 bool
 JSObject::preventExtensions(JSContext *cx, js::AutoIdVector *props)
 {
     JS_ASSERT(isExtensible());
@@ -1191,17 +1205,24 @@ JSObject::setFlag(JSContext *cx, /*BaseS
     BaseShape::Flag flag = (BaseShape::Flag) flag_;
 
     if (lastProperty()->getObjectFlags() & flag)
         return true;
 
     if (inDictionaryMode()) {
         if (generateShape == GENERATE_SHAPE && !generateOwnShape(cx))
             return false;
-        lastProperty()->base()->setObjectFlag(flag);
+
+        BaseShape base(*lastProperty()->base()->unowned());
+        base.setObjectFlag(flag);
+        UnownedBaseShape *nbase = BaseShape::getUnowned(cx, base);
+        if (!nbase)
+            return false;
+
+        lastProperty()->base()->adoptUnowned(nbase);
         return true;
     }
 
     return Shape::setObjectFlag(cx, flag, getProto(), &shape_);
 }
 
 /* static */ bool
 Shape::setObjectFlag(JSContext *cx, BaseShape::Flag flag, JSObject *proto, HeapPtrShape *listp)
--- a/js/src/jsscope.h
+++ b/js/src/jsscope.h
@@ -411,20 +411,21 @@ class BaseShape : public js::gc::Cell
     bool isOwned() const { return !!(flags & OWNED_SHAPE); }
 
     inline bool matchesGetterSetter(PropertyOp rawGetter,
                                     StrictPropertyOp rawSetter) const;
 
     inline void adoptUnowned(UnownedBaseShape *other);
     inline void setOwned(UnownedBaseShape *unowned);
 
-    inline void setParent(JSObject *obj);
+    inline void setObjectParent(JSObject *obj);
     JSObject *getObjectParent() { return parent; }
 
     void setObjectFlag(Flag flag) { JS_ASSERT(!(flag & ~OBJECT_FLAG_MASK)); flags |= flag; }
+    uint32_t getObjectFlags() const { return flags & OBJECT_FLAG_MASK; }
 
     bool hasGetterObject() const { return !!(flags & HAS_GETTER_OBJECT); }
     JSObject *getterObject() const { JS_ASSERT(hasGetterObject()); return getterObj; }
 
     bool hasSetterObject() const { return !!(flags & HAS_SETTER_OBJECT); }
     JSObject *setterObject() const { JS_ASSERT(hasSetterObject()); return setterObj; }
 
     bool hasTable() const { JS_ASSERT_IF(table_, isOwned()); return table_ != NULL; }
@@ -441,16 +442,19 @@ class BaseShape : public js::gc::Cell
     inline UnownedBaseShape *unowned();
 
     /* Get the canonical base shape for an owned one. */
     inline UnownedBaseShape *baseUnowned();
 
     /* Get the canonical base shape for an unowned one (i.e. identity). */
     inline UnownedBaseShape *toUnowned();
 
+    /* Check that an owned base shape is consistent with its unowned base. */
+    inline void assertConsistency();
+
     /* For JIT usage */
     static inline size_t offsetOfClass() { return offsetof(BaseShape, clasp); }
     static inline size_t offsetOfParent() { return offsetof(BaseShape, parent); }
     static inline size_t offsetOfFlags() { return offsetof(BaseShape, flags); }
 
     static inline void writeBarrierPre(BaseShape *shape);
     static inline void writeBarrierPost(BaseShape *shape, void *addr);
     static inline void readBarrier(BaseShape *shape);
@@ -626,17 +630,17 @@ struct Shape : public js::gc::Cell
     }
 
     Class *getObjectClass() const { return base()->clasp; }
     JSObject *getObjectParent() const { return base()->parent; }
 
     static bool setObjectParent(JSContext *cx, JSObject *obj, JSObject *proto, HeapPtrShape *listp);
     static bool setObjectFlag(JSContext *cx, BaseShape::Flag flag, JSObject *proto, HeapPtrShape *listp);
 
-    uint32_t getObjectFlags() const { return base()->flags & BaseShape::OBJECT_FLAG_MASK; }
+    uint32_t getObjectFlags() const { return base()->getObjectFlags(); }
     bool hasObjectFlag(BaseShape::Flag flag) const {
         JS_ASSERT(!(flag & ~BaseShape::OBJECT_FLAG_MASK));
         return !!(base()->flags & flag);
     }
 
   protected:
     /*
      * Implementation-private bits stored in shape->flags. See public: enum {}
--- a/js/src/jsscopeinlines.h
+++ b/js/src/jsscopeinlines.h
@@ -53,37 +53,16 @@
 
 #include "vm/ArgumentsObject.h"
 #include "vm/StringObject.h"
 
 #include "jscntxtinlines.h"
 #include "jsgcinlines.h"
 #include "jsobjinlines.h"
 
-inline bool
-JSObject::maybeSetIndexed(JSContext *cx, jsid id)
-{
-    jsuint index;
-    if (js_IdIsIndex(id, &index)) {
-        if (!setIndexed(cx))
-            return false;
-    }
-    return true;
-}
-
-inline bool
-JSObject::extend(JSContext *cx, const js::Shape *shape, bool isDefinitelyAtom)
-{
-    if (!isDefinitelyAtom && !maybeSetIndexed(cx, shape->propid()))
-        return false;
-    if (!setLastProperty(cx, shape))
-        return false;
-    return true;
-}
-
 namespace js {
 
 inline
 BaseShape::BaseShape(Class *clasp, JSObject *parent, uint32_t objectFlags)
 {
     JS_ASSERT(!(objectFlags & ~OBJECT_FLAG_MASK));
     PodZero(this);
     this->clasp = clasp;
@@ -114,51 +93,66 @@ BaseShape::BaseShape(Class *clasp, JSObj
 
 inline bool
 BaseShape::matchesGetterSetter(PropertyOp rawGetter, StrictPropertyOp rawSetter) const
 {
     return rawGetter == this->rawGetter && rawSetter == this->rawSetter;
 }
 
 inline void
-BaseShape::setParent(JSObject *obj)
+BaseShape::setObjectParent(JSObject *obj)
 {
     parent = obj;
 }
 
 inline void
 BaseShape::adoptUnowned(UnownedBaseShape *other)
 {
     /*
      * This is a base shape owned by a dictionary object, update it to reflect the
      * unowned base shape of a new last property.
      */
     JS_ASSERT(isOwned());
-
-    JSObject *parent = this->parent;
-    uint32_t flags = (this->flags & OBJECT_FLAG_MASK);
+    DebugOnly<uint32_t> flags = getObjectFlags();
+    JS_ASSERT((flags & other->getObjectFlags()) == flags);
 
     uint32_t span = slotSpan();
     PropertyTable *table = &this->table();
 
     *this = *other;
     setOwned(other);
-    this->parent = parent;
-    this->flags |= flags;
     setTable(table);
     setSlotSpan(span);
+
+    assertConsistency();
 }
 
 inline void
 BaseShape::setOwned(UnownedBaseShape *unowned)
 {
     flags |= OWNED_SHAPE;
     this->unowned_ = unowned;
 }
 
+inline void
+BaseShape::assertConsistency()
+{
+#ifdef DEBUG
+    if (isOwned()) {
+        UnownedBaseShape *unowned = baseUnowned();
+        JS_ASSERT(hasGetterObject() == unowned->hasGetterObject());
+        JS_ASSERT(hasSetterObject() == unowned->hasSetterObject());
+        JS_ASSERT_IF(hasGetterObject(), getterObject() == unowned->getterObject());
+        JS_ASSERT_IF(hasSetterObject(), setterObject() == unowned->setterObject());
+        JS_ASSERT(getObjectParent() == unowned->getObjectParent());
+        JS_ASSERT(getObjectFlags() == unowned->getObjectFlags());
+    }
+#endif
+}
+
 inline
 Shape::Shape(UnownedBaseShape *base, jsid propid, uint32_t slot, uint32_t nfixed,
              uintN attrs, uintN flags, intN shortid)
   : base_(base),
     propid_(propid),
     slotInfo(slot | (nfixed << FIXED_SLOTS_SHIFT)),
     attrs(uint8_t(attrs)),
     flags(uint8_t(flags)),