Bug 991752 - Always check has[G|S]etterObject before updating the Shape; r=jonco
authorTerrence Cole <terrence@mozilla.com>
Tue, 07 Apr 2015 10:51:46 -0700
changeset 257320 2982f84319a6f2b9e5d7468a589c3f75902c8b38
parent 257319 30f3ac8076dd6ad5b2e414b4330ade43d3a4adae
child 257321 b91f61e2d950073eea81472c2f6c8c6c7f22b7ca
push id8007
push userraliiev@mozilla.com
push dateMon, 11 May 2015 19:23:16 +0000
treeherdermozilla-aurora@e2ce1aac996e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjonco
bugs991752
milestone40.0a1
Bug 991752 - Always check has[G|S]etterObject before updating the Shape; r=jonco
js/src/gc/Nursery.cpp
js/src/jspropertytree.cpp
js/src/vm/Shape.cpp
js/src/vm/Shape.h
--- a/js/src/gc/Nursery.cpp
+++ b/js/src/gc/Nursery.cpp
@@ -695,18 +695,20 @@ js::Nursery::moveObjectToTenured(MinorCo
     if (src->isNative()) {
         NativeObject* ndst = &dst->as<NativeObject>();
         NativeObject* nsrc = &src->as<NativeObject>();
         tenuredSize += moveSlotsToTenured(ndst, nsrc, dstKind);
         tenuredSize += moveElementsToTenured(ndst, nsrc, dstKind);
 
         // The shape's list head may point into the old object. This can only
         // happen for dictionaries, which are native objects.
-        if (&nsrc->shape_ == ndst->shape_->listp)
+        if (&nsrc->shape_ == ndst->shape_->listp) {
+            MOZ_ASSERT(nsrc->shape_->inDictionary());
             ndst->shape_->listp = &ndst->shape_;
+        }
     }
 
     if (src->is<InlineTypedObject>())
         InlineTypedObject::objectMovedDuringMinorGC(trc, dst, src);
 
     return tenuredSize;
 }
 
--- a/js/src/jspropertytree.cpp
+++ b/js/src/jspropertytree.cpp
@@ -305,42 +305,36 @@ Shape::fixupAfterMovingGC()
 {
     if (inDictionary())
         fixupDictionaryShapeAfterMovingGC();
     else
         fixupShapeTreeAfterMovingGC();
 }
 
 void
-ShapeGetterSetterRef::mark(JSTracer* trc)
+Shape::fixupGetterSetterForBarrier(JSTracer *trc)
 {
-    // Update the current shape's entry in the parent KidsHash table if needed.
-    // This is necessary as the computed hash includes the getter/setter
-    // pointers.
-
-    JSObject* obj = *objp;
-    JSObject* prior = obj;
-    if (!prior)
-        return;
-
-    trc->setTracingLocation(&*prior);
-    TraceManuallyBarrieredEdge(trc, &obj, "AccessorShape getter or setter");
-    if (obj == *objp)
-        return;
-
-    Shape* parent = shape->parent;
-    if (shape->inDictionary() || !parent->kids.isHash()) {
-        *objp = obj;
-        return;
+    // Relocating the getterObj or setterObj will change our location in our
+    // parent's KidsHash, so remove ourself first if we're going to get moved.
+    if (parent && !parent->inDictionary() && parent->kids.isHash()) {
+        KidsHash* kh = parent->kids.toHash();
+        MOZ_ASSERT(kh->lookup(StackShape(this)));
+        kh->remove(StackShape(this));
     }
 
-    KidsHash* kh = parent->kids.toHash();
-    kh->remove(StackShape(shape));
-    *objp = obj;
-    MOZ_ALWAYS_TRUE(kh->putNew(StackShape(shape), shape));
+    if (hasGetterObject())
+        TraceManuallyBarrieredEdge(trc, &asAccessorShape().getterObj, "getterObj");
+    if (hasSetterObject())
+        TraceManuallyBarrieredEdge(trc, &asAccessorShape().setterObj, "setterObj");
+
+    if (parent && !parent->inDictionary() && parent->kids.isHash()) {
+        KidsHash* kh = parent->kids.toHash();
+        MOZ_ASSERT(!kh->lookup(StackShape(this)));
+        MOZ_ALWAYS_TRUE(kh->putNew(StackShape(this), this));
+    }
 }
 
 #ifdef DEBUG
 
 void
 KidsPointer::checkConsistency(Shape* aKid) const
 {
     if (isShape()) {
--- a/js/src/vm/Shape.cpp
+++ b/js/src/vm/Shape.cpp
@@ -807,21 +807,18 @@ NativeObject::putProperty(ExclusiveConte
         MOZ_ASSERT_IF(attrs & (JSPROP_GETTER | JSPROP_SETTER), attrs & JSPROP_SHARED);
 
         shape->setSlot(slot);
         shape->attrs = uint8_t(attrs);
         shape->flags = flags | Shape::IN_DICTIONARY | (accessorShape ? Shape::ACCESSOR_SHAPE : 0);
         if (shape->isAccessorShape()) {
             AccessorShape& accShape = shape->asAccessorShape();
             accShape.rawGetter = getter;
-            if (accShape.hasGetterObject())
-                GetterSetterWriteBarrierPost(&accShape, &accShape.getterObj);
             accShape.rawSetter = setter;
-            if (accShape.hasSetterObject())
-                GetterSetterWriteBarrierPost(&accShape, &accShape.setterObj);
+            GetterSetterWriteBarrierPost(&accShape);
         } else {
             MOZ_ASSERT(!getter);
             MOZ_ASSERT(!setter);
         }
     } else {
         /*
          * Updating the last property in a non-dictionary-mode object. Find an
          * alternate shared child of the last property's previous shape.
--- a/js/src/vm/Shape.h
+++ b/js/src/vm/Shape.h
@@ -314,49 +314,16 @@ class ShapeTable {
  * property, however.
  */
 
 class AccessorShape;
 class Shape;
 class UnownedBaseShape;
 struct StackBaseShape;
 
-// This class is used to add a post barrier on the AccessorShape's getter/setter
-// objects. It updates the shape's entry in the parent's KidsHash table.
-class ShapeGetterSetterRef : public gc::BufferableRef
-{
-    AccessorShape* shape;
-    JSObject** objp;
-
-  public:
-    ShapeGetterSetterRef(AccessorShape* shape, JSObject** objp)
-      : shape(shape), objp(objp)
-    {}
-
-    void mark(JSTracer* trc);
-};
-
-static inline void
-GetterSetterWriteBarrierPost(AccessorShape* shape, JSObject** objp)
-{
-    MOZ_ASSERT(shape);
-    MOZ_ASSERT(objp);
-    MOZ_ASSERT(*objp);
-    gc::Cell** cellp = reinterpret_cast<gc::Cell**>(objp);
-    if (gc::StoreBuffer* sb = (*cellp)->storeBuffer())
-        sb->putGeneric(ShapeGetterSetterRef(shape, objp));
-}
-
-static inline void
-GetterSetterWriteBarrierPostRemove(JSRuntime* rt, JSObject** objp)
-{
-    JS::shadow::Runtime* shadowRuntime = JS::shadow::Runtime::asShadowRuntime(rt);
-    shadowRuntime->gcStoreBufferPtr()->removeRelocatableCellFromAnyThread(reinterpret_cast<gc::Cell**>(objp));
-}
-
 class BaseShape : public gc::TenuredCell
 {
   public:
     friend class Shape;
     friend struct StackBaseShape;
     friend struct StackShape;
     friend void gc::MergeCompartments(JSCompartment* source, JSCompartment* target);
 
@@ -595,17 +562,16 @@ class Shape : public gc::TenuredCell
 {
     friend class ::JSObject;
     friend class ::JSFunction;
     friend class Bindings;
     friend class Nursery;
     friend class NativeObject;
     friend class PropertyTree;
     friend class StaticBlockObject;
-    friend class ShapeGetterSetterRef;
     friend struct StackShape;
     friend struct StackBaseShape;
 
   protected:
     HeapPtrBaseShape    base_;
     PreBarrieredId      propid_;
 
     enum SlotInfo : uint32_t
@@ -1000,16 +966,17 @@ class Shape : public gc::TenuredCell
     static inline ThingRootKind rootKind() { return THING_ROOT_SHAPE; }
 
     inline void markChildren(JSTracer* trc);
 
     inline Shape* search(ExclusiveContext* cx, jsid id);
     inline Shape* searchLinear(jsid id);
 
     void fixupAfterMovingGC();
+    void fixupGetterSetterForBarrier(JSTracer *trc);
 
     /* For JIT usage */
     static inline size_t offsetOfBase() { return offsetof(Shape, base_); }
     static inline size_t offsetOfSlotInfo() { return offsetof(Shape, slotInfo); }
     static inline uint32_t fixedSlotsMask() { return FIXED_SLOTS_MASK; }
 
   private:
     void fixupDictionaryShapeAfterMovingGC();
@@ -1021,17 +988,16 @@ class Shape : public gc::TenuredCell
         JS_STATIC_ASSERT(FIXED_SLOTS_SHIFT == js::shadow::Shape::FIXED_SLOTS_SHIFT);
     }
 };
 
 /* Fat Shape used for accessor properties. */
 class AccessorShape : public Shape
 {
     friend class Shape;
-    friend class ShapeGetterSetterRef;
     friend class NativeObject;
 
     union {
         GetterOp rawGetter;     /* getter hook for shape */
         JSObject* getterObj;    /* user-defined callable "get" object or
                                    null if shape->hasGetterValue() */
     };
     union {
@@ -1265,28 +1231,56 @@ Shape::Shape(const StackShape& other, ui
     MOZ_ASSERT_IF(other.isAccessorShape(), allocKind == gc::AllocKind::ACCESSOR_SHAPE);
     MOZ_ASSERT_IF(allocKind == gc::AllocKind::SHAPE, !other.isAccessorShape());
 #endif
 
     MOZ_ASSERT_IF(attrs & (JSPROP_GETTER | JSPROP_SETTER), attrs & JSPROP_SHARED);
     kids.setNull();
 }
 
+// This class is used to add a post barrier on the AccessorShape's getter/setter
+// objects. It updates the pointers and the shape's entry in the parent's
+// KidsHash table.
+class ShapeGetterSetterRef : public gc::BufferableRef
+{
+    AccessorShape* shape_;
+
+  public:
+    explicit ShapeGetterSetterRef(AccessorShape* shape) : shape_(shape) {}
+    void mark(JSTracer* trc) { shape_->fixupGetterSetterForBarrier(trc); }
+};
+
+static inline void
+GetterSetterWriteBarrierPost(AccessorShape* shape)
+{
+    MOZ_ASSERT(shape);
+    if (shape->hasGetterObject()) {
+        gc::StoreBuffer *sb = reinterpret_cast<gc::Cell*>(shape->getterObject())->storeBuffer();
+        if (sb) {
+            sb->putGeneric(ShapeGetterSetterRef(shape));
+            return;
+        }
+    }
+    if (shape->hasSetterObject()) {
+        gc::StoreBuffer *sb = reinterpret_cast<gc::Cell*>(shape->setterObject())->storeBuffer();
+        if (sb) {
+            sb->putGeneric(ShapeGetterSetterRef(shape));
+            return;
+        }
+    }
+}
+
 inline
 AccessorShape::AccessorShape(const StackShape& other, uint32_t nfixed)
   : Shape(other, nfixed),
     rawGetter(other.rawGetter),
     rawSetter(other.rawSetter)
 {
     MOZ_ASSERT(getAllocKind() == gc::AllocKind::ACCESSOR_SHAPE);
-
-    if ((attrs & JSPROP_GETTER) && rawGetter)
-        GetterSetterWriteBarrierPost(this, &this->getterObj);
-    if ((attrs & JSPROP_SETTER) && rawSetter)
-        GetterSetterWriteBarrierPost(this, &this->setterObj);
+    GetterSetterWriteBarrierPost(this);
 }
 
 inline
 Shape::Shape(UnownedBaseShape* base, uint32_t nfixed)
   : base_(base),
     propid_(JSID_EMPTY),
     slotInfo(SHAPE_INVALID_SLOT | (nfixed << FIXED_SLOTS_SHIFT)),
     attrs(JSPROP_SHARED),