Bug 939993 - Check that AddPtrs are used only with matching Lookup values r=sfink
☠☠ backed out by 05a0228c2caa ☠ ☠
authorJon Coppeard <jcoppeard@mozilla.com>
Tue, 19 Nov 2013 22:53:32 +0000
changeset 156538 1b720320ccf43e2f88a4a4eba46222b1ac0ec6f9
parent 156537 51a006875f2497f8c1225b78c4778d675e47ab4f
child 156539 b75c703f2e7e502abb3be1facffba48787bfbe77
push id36451
push userjcoppeard@mozilla.com
push dateWed, 20 Nov 2013 15:55:58 +0000
treeherdermozilla-inbound@db0f8a5eeb33 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssfink
bugs939993
milestone28.0a1
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 939993 - Check that AddPtrs are used only with matching Lookup values r=sfink
js/public/HashTable.h
js/src/vm/Shape.cpp
--- a/js/public/HashTable.h
+++ b/js/public/HashTable.h
@@ -1506,16 +1506,17 @@ class HashTable : private AllocPolicy
     // Note: |l| may be a reference to a piece of |u|, so this function
     // must take care not to use |l| after moving |u|.
     template <class U>
     bool relookupOrAdd(AddPtr& p, const Lookup &l, U &&u)
     {
         p.mutationCount = mutationCount;
         {
             mozilla::ReentrancyGuard g(*this);
+            JS_ASSERT(prepareHash(l) == p.keyHash); // l has not been destroyed
             p.entry_ = &lookup(l, p.keyHash, sCollisionBit);
         }
         return p.found() || add(p, mozilla::Forward<U>(u));
     }
 
     void remove(Ptr p)
     {
         JS_ASSERT(table);
--- a/js/src/vm/Shape.cpp
+++ b/js/src/vm/Shape.cpp
@@ -1461,32 +1461,40 @@ StackBaseShape::AutoRooter::trace(JSTrac
 /* static */ UnownedBaseShape*
 BaseShape::getUnowned(ExclusiveContext *cx, const StackBaseShape &base)
 {
     BaseShapeSet &table = cx->compartment()->baseShapes;
 
     if (!table.initialized() && !table.init())
         return nullptr;
 
+    uint64_t originalGcNumber = cx->zone()->gcNumber();
     BaseShapeSet::AddPtr p = table.lookupForAdd(&base);
 
     if (p)
         return *p;
 
     StackBaseShape::AutoRooter root(cx, &base);
 
     BaseShape *nbase_ = js_NewGCBaseShape<CanGC>(cx);
     if (!nbase_)
         return nullptr;
 
     new (nbase_) BaseShape(base);
 
     UnownedBaseShape *nbase = static_cast<UnownedBaseShape *>(nbase_);
 
-    if (!table.relookupOrAdd(p, &base, nbase))
+    /*
+     * If a GC has occurred then the hash we calculated may be invalid, as it is
+     * based on the objects inside StackBaseShape, which may have been moved.
+     */
+    bool gcHappened = cx->zone()->gcNumber() != originalGcNumber;
+    bool added = gcHappened ? table.putNew(&base, nbase)
+                            : table.relookupOrAdd(p, &base, nbase);
+    if (!added)
         return nullptr;
 
     return nbase;
 }
 
 /* static */ UnownedBaseShape *
 BaseShape::lookupUnowned(ThreadSafeContext *cx, const StackBaseShape &base)
 {
@@ -1588,16 +1596,17 @@ EmptyShape::getInitialShape(ExclusiveCon
     JS_ASSERT_IF(parent, cx->isInsideCurrentCompartment(parent));
 
     InitialShapeSet &table = cx->compartment()->initialShapes;
 
     if (!table.initialized() && !table.init())
         return nullptr;
 
     typedef InitialShapeEntry::Lookup Lookup;
+    uint64_t originalGcNumber = cx->zone()->gcNumber();
     InitialShapeSet::AddPtr p =
         table.lookupForAdd(Lookup(clasp, proto, parent, metadata, nfixed, objectFlags));
 
     if (p)
         return p->shape;
 
     SkipRoot skip(cx, &p); /* The hash may look like a GC pointer and get poisoned. */
     Rooted<TaggedProto> protoRoot(cx, proto);
@@ -1609,21 +1618,27 @@ EmptyShape::getInitialShape(ExclusiveCon
     if (!nbase)
         return nullptr;
 
     Shape *shape = cx->compartment()->propertyTree.newShape(cx);
     if (!shape)
         return nullptr;
     new (shape) EmptyShape(nbase, nfixed);
 
-    if (!table.relookupOrAdd(p, Lookup(clasp, protoRoot, parentRoot, metadataRoot, nfixed, objectFlags),
-                             InitialShapeEntry(shape, protoRoot)))
-    {
+    /*
+     * If a GC has occurred, then the hash we calculated may be invalid, as it
+     * is based on objects which may have been moved.
+     */
+    Lookup lookup(clasp, protoRoot, parentRoot, metadataRoot, nfixed, objectFlags);
+    InitialShapeEntry entry(shape, protoRoot);
+    bool gcHappened = cx->zone()->gcNumber() != originalGcNumber;
+    bool added = gcHappened ? table.putNew(lookup, entry)
+                            : table.relookupOrAdd(p, lookup, entry);
+    if (!added)
         return nullptr;
-    }
 
     return shape;
 }
 
 /* static */ Shape *
 EmptyShape::getInitialShape(ExclusiveContext *cx, const Class *clasp, TaggedProto proto,
                             JSObject *parent, JSObject *metadata,
                             AllocKind kind, uint32_t objectFlags)