Bug 759991 - Fix infinite loop in rekeyFront with fully collided Table; r=luke

This hooks up the same path to putNew, because it is slightly more efficient and
fixes an OOM failure introduced in c9024bcb8da0.

js/public/HashTable.h

@@ -220,10 +220,10 @@ class HashTable : private AllocPolicy
             JS_ASSERT(&k != &HashPolicy::getKey(this->cur->t));
             if (match(*this->cur, l))
                 return;
-            Entry e = *this->cur;
+            typename HashTableEntry<T>::NonConstT t = this->cur->t;
-            HashPolicy::setKey(e.t, const_cast<Key &>(k));
+            HashPolicy::setKey(t, const_cast<Key &>(k));
             table.remove(*this->cur);
-            table.add(l, e);
+            table.putNewInfallible(l, t);
             added = true;
             this->validEntry = false;
         }
@@ -234,22 +234,27 @@ class HashTable : private AllocPolicy
 
         /* Potentially rehashes the table. */
         ~Enum() {
-            if (added)
+            JS_ASSERT(!added);
-                table.checkOverloaded();
             if (removed)
                 table.checkUnderloaded();
         }
 
-        /* Can be used to end the enumeration before the destructor. */
+        /*
-        void endEnumeration() {
+         * Can be used to end the enumeration before the destructor. Unlike
+         * |~Enum()|, this can report OOM on resize, so must be called if
+         * |rekeyFront()| is used during enumeration.
+         */
+        bool endEnumeration() {
             if (added) {
-                table.checkOverloaded();
                 added = false;
+                if (table.checkOverloaded() == RehashFailed)
+                    return false;
             }
             if (removed) {
-                table.checkUnderloaded();
                 removed = false;
+                table.checkUnderloaded();
             }
+            return true;
         }
     };
 
@@ -519,7 +524,7 @@ class HashTable : private AllocPolicy
         Entry *entry = &table[h1];
 
         /* Miss: return space for a new entry. */
-        if (entry->isFree()) {
+        if (!entry->isLive()) {
             METER(stats.misses++);
             return *entry;
         }
@@ -535,14 +540,16 @@ class HashTable : private AllocPolicy
             h1 = applyDoubleHash(h1, dh);
 
             entry = &table[h1];
-            if (entry->isFree()) {
+            if (!entry->isLive()) {
                 METER(stats.misses++);
                 return *entry;
             }
         }
     }
 
-    bool changeTableSize(int deltaLog2)
+    enum RebuildStatus { NotOverloaded, Rehashed, RehashFailed };
+
+    RebuildStatus changeTableSize(int deltaLog2)
     {
         /* Look, but don't touch, until we succeed in getting new entry store. */
         Entry *oldTable = table;
@@ -551,12 +558,12 @@ class HashTable : private AllocPolicy
         uint32_t newCapacity = JS_BIT(newLog2);
         if (newCapacity > sMaxCapacity) {
             this->reportAllocOverflow();
-            return false;
+            return RehashFailed;
         }
 
         Entry *newTable = createTable(*this, newCapacity);
         if (!newTable)
-            return false;
+            return RehashFailed;
 
         /* We can't fail from here on, so update table parameters. */
         setTableSizeLog2(newLog2);
@@ -573,32 +580,13 @@ class HashTable : private AllocPolicy
         }
 
         destroyTable(*this, oldTable, oldCap);
-        return true;
+        return Rehashed;
     }
 
-    void add(const Lookup &l, const Entry &e)
+    RebuildStatus checkOverloaded()
-    {
-        JS_ASSERT(table);
-
-        HashNumber keyHash = prepareHash(l);
-        Entry &entry = lookup(l, keyHash, sCollisionBit);
-
-        if (entry.isRemoved()) {
-            METER(stats.addOverRemoved++);
-            removedCount--;
-            keyHash |= sCollisionBit;
-        }
-
-        entry.t = e.t;
-        entry.setLive(keyHash);
-        entryCount++;
-        mutationCount++;
-    }
-
-    bool checkOverloaded()
     {
         if (!overloaded())
-            return false;
+            return NotOverloaded;
 
         /* Compress if a quarter or more of all entries are removed. */
         int deltaLog2;
@@ -732,8 +720,11 @@ class HashTable : private AllocPolicy
             removedCount--;
             p.keyHash |= sCollisionBit;
         } else {
-            if (checkOverloaded())
+            /* Preserve the validity of |p.entry|. */
-                /* Preserve the validity of |p.entry|. */
+            RebuildStatus status = checkOverloaded();
+            if (status == RehashFailed)
+                return false;
+            if (status == Rehashed)
                 p.entry = &findFreeEntry(p.keyHash);
         }
 
@@ -764,6 +755,34 @@ class HashTable : private AllocPolicy
         return true;
     }
 
+    void putNewInfallible(const Lookup &l, const T &t)
+    {
+        JS_ASSERT(table);
+
+        HashNumber keyHash = prepareHash(l);
+        Entry *entry = &findFreeEntry(keyHash);
+
+        if (entry->isRemoved()) {
+            METER(stats.addOverRemoved++);
+            removedCount--;
+            keyHash |= sCollisionBit;
+        }
+
+        entry->t = t;
+        entry->setLive(keyHash);
+        entryCount++;
+        mutationCount++;
+    }
+
+    bool putNew(const Lookup &l, const T &t)
+    {
+        if (checkOverloaded() == RehashFailed)
+            return false;
+
+        putNewInfallible(l, t);
+        return true;
+    }
+
     bool relookupOrAdd(AddPtr& p, const Lookup &l, const T& t)
     {
         p.mutationCount = mutationCount;
@@ -1149,9 +1168,7 @@ class HashMap
 
     /* Like put, but assert that the given key is not already present. */
     bool putNew(const Key &k, const Value &v) {
-        AddPtr p = lookupForAdd(k);
+        return impl.putNew(k, Entry(k, v));
-        JS_ASSERT(!p);
-        return add(p, k, v);
     }
 
     /* Add (k,defaultValue) if k no found. Return false-y Ptr on oom. */
@@ -1357,15 +1374,11 @@ class HashSet
 
     /* Like put, but assert that the given key is not already present. */
     bool putNew(const T &t) {
-        AddPtr p = lookupForAdd(t);
+        return impl.putNew(t, t);
-        JS_ASSERT(!p);
-        return add(p, t);
     }
 
     bool putNew(const Lookup &l, const T &t) {
-        AddPtr p = lookupForAdd(l);
+        return impl.putNew(l, t);
-        JS_ASSERT(!p);
-        return add(p, t);
     }
 
     void remove(const Lookup &l) {

js/src/jsapi-tests/testHashTable.cpp

@@ -193,11 +193,13 @@ BEGIN_TEST(testHashRekeyManual)
         CHECK(AddLowKeys(&am, &bm, i));
         CHECK(MapsAreEqual(am, bm));
 
-        for (IntMap::Enum e(am); !e.empty(); e.popFront()) {
+        IntMap::Enum e(am);
+        for (; !e.empty(); e.popFront()) {
             uint32_t tmp = LowToHigh::rekey(e.front().key);
             if (tmp != e.front().key)
                 e.rekeyFront(tmp);
         }
+        CHECK(e.endEnumeration());
         CHECK(SlowRekey<LowToHigh>(&bm));
 
         CHECK(MapsAreEqual(am, bm));
@@ -215,11 +217,13 @@ BEGIN_TEST(testHashRekeyManual)
         CHECK(AddLowKeys(&as, &bs, i));
         CHECK(SetsAreEqual(as, bs));
 
-        for (IntSet::Enum e(as); !e.empty(); e.popFront()) {
+        IntSet::Enum e(as);
+        for (; !e.empty(); e.popFront()) {
             uint32_t tmp = LowToHigh::rekey(e.front());
             if (tmp != e.front())
                 e.rekeyFront(tmp);
         }
+        CHECK(e.endEnumeration());
         CHECK(SlowRekey<LowToHigh>(&bs));
 
         CHECK(SetsAreEqual(as, bs));
@@ -243,7 +247,8 @@ BEGIN_TEST(testHashRekeyManualRemoval)
         CHECK(AddLowKeys(&am, &bm, i));
         CHECK(MapsAreEqual(am, bm));
 
-        for (IntMap::Enum e(am); !e.empty(); e.popFront()) {
+        IntMap::Enum e(am);
+        for (; !e.empty(); e.popFront()) {
             if (LowToHighWithRemoval::shouldBeRemoved(e.front().key)) {
                 e.removeFront();
             } else {
@@ -252,6 +257,7 @@ BEGIN_TEST(testHashRekeyManualRemoval)
                     e.rekeyFront(tmp);
             }
         }
+        CHECK(e.endEnumeration());
         CHECK(SlowRekey<LowToHighWithRemoval>(&bm));
 
         CHECK(MapsAreEqual(am, bm));
@@ -269,7 +275,8 @@ BEGIN_TEST(testHashRekeyManualRemoval)
         CHECK(AddLowKeys(&as, &bs, i));
         CHECK(SetsAreEqual(as, bs));
 
-        for (IntSet::Enum e(as); !e.empty(); e.popFront()) {
+        IntSet::Enum e(as);
+        for (; !e.empty(); e.popFront()) {
             if (LowToHighWithRemoval::shouldBeRemoved(e.front())) {
                 e.removeFront();
             } else {
@@ -278,6 +285,7 @@ BEGIN_TEST(testHashRekeyManualRemoval)
                     e.rekeyFront(tmp);
             }
         }
+        CHECK(e.endEnumeration());
         CHECK(SlowRekey<LowToHighWithRemoval>(&bs));
 
         CHECK(SetsAreEqual(as, bs));