Changeset 27710 in webkit for trunk/JavaScriptCore

Timestamp:

Nov 11, 2007, 10:44:26 PM (18 years ago)

Author:

Adam Roben

Message:

Fix <rdar://5578982> 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.

Location:

trunk/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• wtf/HashMap.h (modified) (1 diff)
• wtf/HashSet.h (modified) (1 diff)
• wtf/HashTable.h (modified) (6 diffs)

trunk/JavaScriptCore/ChangeLog

@@ +1 @@
+2007-11-11  Adam Roben  <[email protected]>
+
+        Fix <rdar://5578982> 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  <[email protected]>
 

trunk/JavaScriptCore/wtf/HashMap.h

@@ -308 +308 @@
         if (it.m_impl == m_impl.end())
             return;
+        m_impl.checkTableConsistency();
         RefCounter<ValueTraits, ValueStorageTraits>::deref(*it.m_impl);
-        m_impl.remove(it.m_impl);
+        m_impl.removeWithoutEntryConsistencyCheck(it.m_impl);
     }
 

trunk/JavaScriptCore/wtf/HashSet.h

@@ -285 +285 @@
         if (it.m_impl == m_impl.end())
             return;
+        m_impl.checkTableConsistency();
         RefCounter<ValueTraits, StorageTraits>::deref(*it.m_impl);
-        m_impl.remove(it.m_impl);
+        m_impl.removeWithoutEntryConsistencyCheck(it.m_impl);
     }
 

trunk/JavaScriptCore/wtf/HashTable.h

@@ -322 +322 @@
         void remove(const KeyType&);
         void remove(iterator);
+        void removeWithoutEntryConsistencyCheck(iterator);
         void clear();
 
@@ -329 +330 @@
 
         ValueType* lookup(const Key& key) { return lookup<Key, IdentityTranslatorType>(key); }
+
+#if CHECK_HASHTABLE_CONSISTENCY
+        void checkTableConsistency() const;
+#else
+        static void checkTableConsistency() { }
+#endif
 
     private:
@@ -342 +349 @@
         template<typename T, typename HashTranslator> LookupType lookupForWriting(const T&);
 
+        void removeAndInvalidateWithoutEntryConsistencyCheck(ValueType*);
+        void removeAndInvalidate(ValueType*);
         void remove(ValueType*);
 
@@ -365 +374 @@
 
 #if CHECK_HASHTABLE_CONSISTENCY
-        void checkTableConsistency() const;
         void checkTableConsistencyExceptSize() const;
 #else
-        static void checkTableConsistency() { }
         static void checkTableConsistencyExceptSize() { }
 #endif
@@ -759 +766 @@
 
     template<typename Key, typename Value, typename Extractor, typename HashFunctions, typename Traits, typename KeyTraits>
-    void HashTable<Key, Value, Extractor, HashFunctions, Traits, KeyTraits>::remove(ValueType* pos)
+    void HashTable<Key, Value, Extractor, HashFunctions, Traits, KeyTraits>::removeAndInvalidateWithoutEntryConsistencyCheck(ValueType* pos)
+    {
+        invalidateIterators();
+        remove(pos);
+    }
+
+    template<typename Key, typename Value, typename Extractor, typename HashFunctions, typename Traits, typename KeyTraits>
+    void HashTable<Key, Value, Extractor, HashFunctions, Traits, KeyTraits>::removeAndInvalidate(ValueType* pos)
     {
         invalidateIterators();
         checkTableConsistency();
-
+        remove(pos);
+    }
+
+    template<typename Key, typename Value, typename Extractor, typename HashFunctions, typename Traits, typename KeyTraits>
+    void HashTable<Key, Value, Extractor, HashFunctions, Traits, KeyTraits>::remove(ValueType* pos)
+    {
 #if DUMP_HASHTABLE_STATS
         ++HashTableStats::numRemoves;
@@ -784 +803 @@
             return;
 
-        remove(const_cast<ValueType*>(it.m_iterator.m_position));
+        removeAndInvalidate(const_cast<ValueType*>(it.m_iterator.m_position));
+    }
+
+    template<typename Key, typename Value, typename Extractor, typename HashFunctions, typename Traits, typename KeyTraits>
+    inline void HashTable<Key, Value, Extractor, HashFunctions, Traits, KeyTraits>::removeWithoutEntryConsistencyCheck(iterator it)
+    {
+        if (it == end())
+            return;
+
+        removeAndInvalidateWithoutEntryConsistencyCheck(const_cast<ValueType*>(it.m_iterator.m_position));
     }