From 390aaf4f792b1ffc2415ec9c591e829bb329b7dd Mon Sep 17 00:00:00 2001 From: "aroben@apple.com" Date: Mon, 12 Nov 2007 06:44:26 +0000 Subject: [PATCH] Fix ASSERT in HashTable::checkTableConsistencyExceptSize beneath WebNotificationCenter The bug was due to a mismatch between HashMap::remove and HashTable::checkTableConsistency. HashMap::remove can delete the value stored in the HashTable (by derefing it), which is not normally allowed by HashTable. It's OK in this case because the value is about to be removed from the table, but HashTable wasn't aware of this. HashMap::remove now performs the consistency check itself before derefing the value. Darin noticed that the same bug would occur in HashSet, so I've fixed it there as well. Reviewed by Darin. * wtf/HashMap.h: (WTF::HashMap::remove): Perform the HashTable consistency check manually before calling deref. * wtf/HashSet.h: (WTF::HashSet::remove): Ditto. * wtf/HashTable.h: Made checkTableConsistency public so that HashMap and HashSet can call it. (WTF::HashTable::removeAndInvalidateWithoutEntryConsistencyCheck): Added. (WTF::HashTable::removeAndInvalidate): Added. (WTF::HashTable::remove): (WTF::HashTable::removeWithoutEntryConsistencyCheck): Added. git-svn-id: http://svn.webkit.org/repository/webkit/trunk@27710 268f45cc-cd09-0410-ab3c-d52691b4dbfc --- JavaScriptCore/ChangeLog | 31 +++++++++++++++++++++++++++++ JavaScriptCore/wtf/HashMap.h | 3 ++- JavaScriptCore/wtf/HashSet.h | 3 ++- JavaScriptCore/wtf/HashTable.h | 36 ++++++++++++++++++++++++++++++---- 4 files changed, 67 insertions(+), 6 deletions(-) diff --git a/JavaScriptCore/ChangeLog b/JavaScriptCore/ChangeLog index 9278714d3c9..08b7aa824bc 100644 --- a/JavaScriptCore/ChangeLog +++ b/JavaScriptCore/ChangeLog @@ -1,3 +1,34 @@ +2007-11-11 Adam Roben + + Fix ASSERT in HashTable::checkTableConsistencyExceptSize beneath WebNotificationCenter + + The bug was due to a mismatch between HashMap::remove and + HashTable::checkTableConsistency. HashMap::remove can delete the value + stored in the HashTable (by derefing it), which is not normally + allowed by HashTable. It's OK in this case because the value is about + to be removed from the table, but HashTable wasn't aware of this. + + HashMap::remove now performs the consistency check itself before + derefing the value. + + Darin noticed that the same bug would occur in HashSet, so I've fixed + it there as well. + + Reviewed by Darin. + + * wtf/HashMap.h: + (WTF::HashMap::remove): Perform the HashTable consistency check + manually before calling deref. + * wtf/HashSet.h: + (WTF::HashSet::remove): Ditto. + * wtf/HashTable.h: Made checkTableConsistency public so that HashMap + and HashSet can call it. + (WTF::HashTable::removeAndInvalidateWithoutEntryConsistencyCheck): + Added. + (WTF::HashTable::removeAndInvalidate): Added. + (WTF::HashTable::remove): + (WTF::HashTable::removeWithoutEntryConsistencyCheck): Added. + 2007-11-11 Mark Rowe Build fix. Use the correct filename case. diff --git a/JavaScriptCore/wtf/HashMap.h b/JavaScriptCore/wtf/HashMap.h index 3397d60f5f7..7d83694d923 100644 --- a/JavaScriptCore/wtf/HashMap.h +++ b/JavaScriptCore/wtf/HashMap.h @@ -307,8 +307,9 @@ namespace WTF { { if (it.m_impl == m_impl.end()) return; + m_impl.checkTableConsistency(); RefCounter::deref(*it.m_impl); - m_impl.remove(it.m_impl); + m_impl.removeWithoutEntryConsistencyCheck(it.m_impl); } template diff --git a/JavaScriptCore/wtf/HashSet.h b/JavaScriptCore/wtf/HashSet.h index de67513cefd..7da41f67300 100644 --- a/JavaScriptCore/wtf/HashSet.h +++ b/JavaScriptCore/wtf/HashSet.h @@ -284,8 +284,9 @@ namespace WTF { { if (it.m_impl == m_impl.end()) return; + m_impl.checkTableConsistency(); RefCounter::deref(*it.m_impl); - m_impl.remove(it.m_impl); + m_impl.removeWithoutEntryConsistencyCheck(it.m_impl); } template diff --git a/JavaScriptCore/wtf/HashTable.h b/JavaScriptCore/wtf/HashTable.h index a64fed44150..c49716d368d 100644 --- a/JavaScriptCore/wtf/HashTable.h +++ b/JavaScriptCore/wtf/HashTable.h @@ -321,6 +321,7 @@ namespace WTF { void remove(const KeyType&); void remove(iterator); + void removeWithoutEntryConsistencyCheck(iterator); void clear(); static bool isEmptyBucket(const ValueType& value) { return Extractor::extract(value) == KeyTraits::emptyValue(); } @@ -329,6 +330,12 @@ namespace WTF { ValueType* lookup(const Key& key) { return lookup(key); } +#if CHECK_HASHTABLE_CONSISTENCY + void checkTableConsistency() const; +#else + static void checkTableConsistency() { } +#endif + private: static ValueType* allocateTable(int size); static void deallocateTable(ValueType* table, int size); @@ -341,6 +348,8 @@ namespace WTF { template FullLookupType fullLookupForWriting(const T&); template LookupType lookupForWriting(const T&); + void removeAndInvalidateWithoutEntryConsistencyCheck(ValueType*); + void removeAndInvalidate(ValueType*); void remove(ValueType*); bool shouldExpand() const { return (m_keyCount + m_deletedCount) * m_maxLoad >= m_tableSize; } @@ -364,10 +373,8 @@ namespace WTF { const_iterator makeKnownGoodConstIterator(ValueType* pos) const { return const_iterator(this, pos, m_table + m_tableSize, HashItemKnownGood); } #if CHECK_HASHTABLE_CONSISTENCY - void checkTableConsistency() const; void checkTableConsistencyExceptSize() const; #else - static void checkTableConsistency() { } static void checkTableConsistencyExceptSize() { } #endif @@ -758,11 +765,23 @@ namespace WTF { } template - void HashTable::remove(ValueType* pos) + void HashTable::removeAndInvalidateWithoutEntryConsistencyCheck(ValueType* pos) + { + invalidateIterators(); + remove(pos); + } + + template + void HashTable::removeAndInvalidate(ValueType* pos) { invalidateIterators(); checkTableConsistency(); + remove(pos); + } + template + void HashTable::remove(ValueType* pos) + { #if DUMP_HASHTABLE_STATS ++HashTableStats::numRemoves; #endif @@ -783,7 +802,16 @@ namespace WTF { if (it == end()) return; - remove(const_cast(it.m_iterator.m_position)); + removeAndInvalidate(const_cast(it.m_iterator.m_position)); + } + + template + inline void HashTable::removeWithoutEntryConsistencyCheck(iterator it) + { + if (it == end()) + return; + + removeAndInvalidateWithoutEntryConsistencyCheck(const_cast(it.m_iterator.m_position)); } template -- GitLab