Make sure all destructors are called in HeapHashMap and HeapHashSet

Source/heap/HeapTest.cpp

Index: Source/heap/HeapTest.cpp
diff --git a/Source/heap/HeapTest.cpp b/Source/heap/HeapTest.cpp
index 005556e562003385ac64f26ec5a6ad3928879e74..2d38948111e9a16ce3aae030775fe6a86e95c3c5 100644
--- a/Source/heap/HeapTest.cpp
+++ b/Source/heap/HeapTest.cpp
@@ -708,6 +708,11 @@ public:
         ++s_destructorCalls;
     }
 
+    // These are here with their default implementations so you can break in
+    // them in the debugger.
+    void ref() { RefCountedGarbageCollected<RefCountedAndGarbageCollected>::ref(); }
+    void deref() { RefCountedGarbageCollected<RefCountedAndGarbageCollected>::deref(); }
+
     void trace(Visitor*) { }
 
     static int s_destructorCalls;
@@ -2220,7 +2225,8 @@ void SetIteratorCheck(T& it, const T& end, int expected)
 
 TEST(HeapTest, HeapWeakCollectionSimple)
 {
-
+    HeapStats initialHeapStats;
+    clearOutOldGarbage(&initialHeapStats);
     IntWrapper::s_destructorCalls = 0;
 
     PersistentHeapVector<Member<IntWrapper> > keepNumbersAlive;
@@ -2265,6 +2271,120 @@ TEST(HeapTest, HeapWeakCollectionSimple)
     EXPECT_EQ(2u, weakSet->size());
 }
 
+class ThingWithDestructor {
+public:
+    ThingWithDestructor() : m_x(emptyValue) { }
+    ThingWithDestructor(int x) : m_x(x) { ASSERT(x != emptyValue); }
+
+    ~ThingWithDestructor()
+    {
+        if (m_x != emptyValue)

[wibling-chromium, 2014/03/28 10:35:23]

Does the constructor get called more than once?

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

Thanks for pointing this out.  There was a bug in the destructor in HashTable.h
that was skipping deleted buckets, but calling the destructor for empty objects.
 Now fixed, and the test has been changed to keep track of both births and
deaths of objects.

+            s_destructorCalls++;
+        m_x = emptyValue;
+    }
+
+    int value() { return m_x; }
+
+    static int s_destructorCalls;
+
+    unsigned hash() { return IntHash<int>::hash(m_x); }
+
+private:
+    static const int emptyValue = 0;
+    int m_x;
+};
+
+int ThingWithDestructor::s_destructorCalls;
+
+struct ThingWithDestructorTraits : public HashTraits<ThingWithDestructor> {
+    static const bool needsDestruction = true;
+};
+
+static void heapMapDestructorHelper(bool clearMaps)
+{
+    HeapStats initialHeapStats;
+    clearOutOldGarbage(&initialHeapStats);
+    ThingWithDestructor::s_destructorCalls = 0;
+
+    typedef HeapHashMap<WeakMember<IntWrapper>, RefPtr<RefCountedAndGarbageCollected> > RefMap;
+
+    typedef HeapHashMap<
+        WeakMember<IntWrapper>,
+        ThingWithDestructor,
+        DefaultHash<WeakMember<IntWrapper> >::Hash,
+        HashTraits<WeakMember<IntWrapper> >,
+        ThingWithDestructorTraits> Map;
+
+    Persistent<Map> map(new Map());
+    Persistent<RefMap> refMap(new RefMap());
+
+    Persistent<IntWrapper> luck(IntWrapper::create(103));
+
+    int baseLine, refBaseLine;
+
+    {
+        Map stackMap;
+        RefMap stackRefMap;
+
+        Heap::collectGarbage(ThreadState::NoHeapPointersOnStack);
+        Heap::collectGarbage(ThreadState::NoHeapPointersOnStack);
+
+        stackMap.add(IntWrapper::create(42), ThingWithDestructor(1729));
+        stackMap.add(luck, ThingWithDestructor(8128));
+        stackRefMap.add(IntWrapper::create(42), RefCountedAndGarbageCollected::create());
+        stackRefMap.add(luck, RefCountedAndGarbageCollected::create());
+
+        baseLine = ThingWithDestructor::s_destructorCalls;
+        refBaseLine = RefCountedAndGarbageCollected::s_destructorCalls;
+
+        // Although the heap maps are on-stack, we can't expect prompt
+        // finalization of the elements, so when they go out of scope here we
+        // will not necessarily have called the relevant destructors.
+    }
+
+    // The RefCountedAndGarbageCollected things need an extra GC to discover
+    // that they are no longer ref counted.
+    Heap::collectGarbage(ThreadState::NoHeapPointersOnStack);
+    Heap::collectGarbage(ThreadState::NoHeapPointersOnStack);
+    EXPECT_EQ(baseLine + 2, ThingWithDestructor::s_destructorCalls);
+    EXPECT_EQ(refBaseLine + 2, RefCountedAndGarbageCollected::s_destructorCalls);
+
+    // Now use maps kept alive with persistents. Here we don't expect any
+    // destructors to be called before there have been GCs.
+
+    map->add(IntWrapper::create(42), ThingWithDestructor(1729));
+    map->add(luck, ThingWithDestructor(8128));
+    refMap->add(IntWrapper::create(42), RefCountedAndGarbageCollected::create());
+    refMap->add(luck, RefCountedAndGarbageCollected::create());
+
+    baseLine  =  ThingWithDestructor::s_destructorCalls;
+    refBaseLine = RefCountedAndGarbageCollected::s_destructorCalls;
+
+    luck.clear();
+    if (clearMaps) {
+        map->clear(); // Clear map.
+        refMap->clear(); // Clear map.
+    } else {
+        map.clear(); // Clear Persistent handle, not map.
+        refMap.clear(); // Clear Persistent handle, not map.
+        Heap::collectGarbage(ThreadState::NoHeapPointersOnStack);
+        Heap::collectGarbage(ThreadState::NoHeapPointersOnStack);
+    }
+
+    EXPECT_EQ(baseLine + 2, ThingWithDestructor::s_destructorCalls);
+
+    // Need a GC to make sure that the RefCountedAndGarbageCollected thing
+    // noticies it's been decremented to zero.
+    Heap::collectGarbage(ThreadState::NoHeapPointersOnStack);
+    EXPECT_EQ(refBaseLine + 2, RefCountedAndGarbageCollected::s_destructorCalls);
+}
+
+TEST(HeapTest, HeapMapDestructor)
+{
+    heapMapDestructorHelper(true);
+    heapMapDestructorHelper(false);
+}
+
 typedef HeapHashSet<PairWeakStrong> WeakStrongSet;
 typedef HeapHashSet<PairWeakUnwrapped> WeakUnwrappedSet;
 typedef HeapHashSet<PairStrongWeak> StrongWeakSet;
@@ -2844,6 +2964,10 @@ TEST(HeapTest, CollectionNesting)
     Heap::collectGarbage(ThreadState::NoHeapPointersOnStack);
     EXPECT_EQ(1u, map->get(key).size());
     EXPECT_EQ(0, IntWrapper::s_destructorCalls);
+
+    keepAlive = nullptr;
+    Heap::collectGarbage(ThreadState::NoHeapPointersOnStack);
+    EXPECT_EQ(1, IntWrapper::s_destructorCalls);
 }
 
 TEST(heap, GarbageCollectedMixin)
@@ -3062,4 +3186,27 @@ TEST(HeapTest, AllocationDuringFinalization)
     EXPECT_EQ(512, OneKiloByteObject::s_destructorCalls);
 }
 
+class Fluff {

[Mads Ager (chromium), 2014/03/28 11:12:39]

Ehm, move meaningful names, please?

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

Done.

+public:
+    Fluff() { }
+    ~Fluff()
+    {
+        ASSERT(!s_wasDestructed);
+        s_wasDestructed = true;
+    }
+    static bool s_wasDestructed;
+};
+
+bool Fluff::s_wasDestructed;
+
+TEST(HeapTest, Flaf)

[Mads Ager (chromium), 2014/03/28 11:12:39]

DestructorsCalledOnMapClear?

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

Done.

+{
+    HeapHashMap<Fluff*, OwnPtr<Fluff> > map;
+    Fluff* fluff = new Fluff();
+    map.add(fluff, adoptPtr(fluff));
+    Fluff::s_wasDestructed = false;
+    map.clear();
+    ASSERT(Fluff::s_wasDestructed);
+}
+
 } // WebCore namespace