Make sure all destructors are called in HeapHashMap and HeapHashSet

Source/wtf/HashTable.h

Index: Source/wtf/HashTable.h
diff --git a/Source/wtf/HashTable.h b/Source/wtf/HashTable.h
index 566fda8164554a8c67cf060641d6f9965f5e03b9..7a03395afbb7c3205030d06e9add6ed62633b900 100644
--- a/Source/wtf/HashTable.h
+++ b/Source/wtf/HashTable.h
@@ -241,6 +241,8 @@ namespace WTF {
         typedef typename KeyTraits::PeekInType KeyPeekInType;
         typedef typename KeyTraits::PassInType KeyPassInType;
         typedef Value ValueType;
+        typedef Extractor ExtractorType;
+        typedef KeyTraits KeyTraitsType;
         typedef typename Traits::PeekInType ValuePeekInType;
         typedef IdentityHashTranslator<HashFunctions> IdentityTranslatorType;
         typedef HashTableAddResult<ValueType> AddResult;
@@ -296,7 +298,7 @@ namespace WTF {
             ASSERT(!Allocator::isGarbageCollected);
             if (LIKELY(!m_table))
                 return;
-            deallocateTable(m_table, m_tableSize);
+            deleteAllBucketsAndDeallocate(m_table, m_tableSize);
             m_table = 0;
         }
 
@@ -351,7 +353,7 @@ namespace WTF {
 
     private:
         static ValueType* allocateTable(unsigned size);
-        static void deallocateTable(ValueType* table, unsigned size);
+        static void deleteAllBucketsAndDeallocate(ValueType* table, unsigned size);
 
         typedef std::pair<ValueType*, bool> LookupType;
         typedef std::pair<LookupType, unsigned> FullLookupType;
@@ -884,7 +886,7 @@ namespace WTF {
     template<typename Key, typename Value, typename Extractor, typename HashFunctions, typename Traits, typename KeyTraits, typename Allocator>
     Value* HashTable<Key, Value, Extractor, HashFunctions, Traits, KeyTraits, Allocator>::allocateTable(unsigned size)
     {
-        typedef typename Allocator::template HashTableBackingHelper<Key, Value, Extractor, Traits, KeyTraits>::Type HashTableBacking;
+        typedef typename Allocator::template HashTableBackingHelper<HashTable>::Type HashTableBacking;
 
         size_t allocSize = size * sizeof(ValueType);
         ValueType* result;
@@ -899,14 +901,21 @@ namespace WTF {
     }
 
     template<typename Key, typename Value, typename Extractor, typename HashFunctions, typename Traits, typename KeyTraits, typename Allocator>
-    void HashTable<Key, Value, Extractor, HashFunctions, Traits, KeyTraits, Allocator>::deallocateTable(ValueType* table, unsigned size)
+    void HashTable<Key, Value, Extractor, HashFunctions, Traits, KeyTraits, Allocator>::deleteAllBucketsAndDeallocate(ValueType* table, unsigned size)
     {
-        if (Allocator::isGarbageCollected)
-            return;
         if (Traits::needsDestruction) {
             for (unsigned i = 0; i < size; ++i) {
-                if (!isDeletedBucket(table[i]))
-                    table[i].~ValueType();
+                // If we are GCing we need to both call the destructor and mark
+                // the bucket as deleted, otherwise the destructor gets called
+                // again when the GC finds the backing store. With the default

[haraken, 2014/03/28 10:43:36]

This comment was a bit confusing to me at first. Maybe slightly better: "A
deleted bucket is marked as deleted in the weak processing phase and finalized
in the sweeping phase."

[Erik Corry, 2014/03/30 20:11:36]

This code is not related to weakness.  It is called when the hashtable is
cleared or rehashed due to size changes.  At this point the backing store is
freed.  In the GC heap, freeing it just done by no longer pointing at this
backing store, which means it will eventually be collected.  We want to run the
destructors of the entries eagerly on clear() and rehash, but we have to make
sure they are not run again when hte backing is eventually collected.

I'll try to make this clear in the comment.

[haraken, 2014/03/31 00:34:54]

Thanks, now I understand.

BTW, currently we don't allow rehashing in a destructor of on-heap hash
maps/sets, because allocation was not allowed in the destructor. However, now
that allocation is allowed in the destructor, shall we enable the rehashing
again?

+                // allocator it's enough to call the destructor, since we won't
+                // see the bucket again.
+                if (!isDeletedBucket(table[i])) {
+                    if (Allocator::isGarbageCollected)
+                        deleteBucket(table[i]);
+                    else
+                        table[i].~ValueType();
+                }
             }
         }
         Allocator::backingFree(table);
@@ -954,7 +963,7 @@ namespace WTF {
 
         m_deletedCount = 0;
 
-        deallocateTable(oldTable, oldTableSize);
+        deleteAllBucketsAndDeallocate(oldTable, oldTableSize);
     }
 
     template<typename Key, typename Value, typename Extractor, typename HashFunctions, typename Traits, typename KeyTraits, typename Allocator>
@@ -963,7 +972,7 @@ namespace WTF {
         if (!m_table)
             return;
 
-        deallocateTable(m_table, m_tableSize);
+        deleteAllBucketsAndDeallocate(m_table, m_tableSize);
         m_table = 0;
         m_tableSize = 0;
         m_tableSizeMask = 0;