From f6ddf5df9750577ad6111e5a853fa32ff5085e23 Mon Sep 17 00:00:00 2001 From: Terrence Cole Date: Wed, 6 Jun 2012 16:40:56 -0700 Subject: [PATCH] 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 | 107 +++++++++++++++------------ js/src/jsapi-tests/testHashTable.cpp | 16 +++- 2 files changed, 72 insertions(+), 51 deletions(-) diff --git a/js/public/HashTable.h b/js/public/HashTable.h index b427240357f..11b83df83ac 100644 --- a/js/public/HashTable.h +++ b/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; - HashPolicy::setKey(e.t, const_cast(k)); + typename HashTableEntry::NonConstT t = this->cur->t; + HashPolicy::setKey(t, const_cast(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) - table.checkOverloaded(); + JS_ASSERT(!added); 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) - { - 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() + RebuildStatus 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); - JS_ASSERT(!p); - return add(p, k, v); + return impl.putNew(k, Entry(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); - JS_ASSERT(!p); - return add(p, t); + return impl.putNew(t, t); } bool putNew(const Lookup &l, const T &t) { - AddPtr p = lookupForAdd(l); - JS_ASSERT(!p); - return add(p, t); + return impl.putNew(l, t); } void remove(const Lookup &l) { diff --git a/js/src/jsapi-tests/testHashTable.cpp b/js/src/jsapi-tests/testHashTable.cpp index c3c062cfa01..58210f86b9b 100644 --- a/js/src/jsapi-tests/testHashTable.cpp +++ b/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(&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(&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(&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(&bs)); CHECK(SetsAreEqual(as, bs));