Make sure all destructors are called in HeapHashMap and HeapHashSet

Source/heap/HeapTest.cpp

 /*
  * Copyright (C) 2013 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 690 matching lines @@
     static PassRefPtr<RefCountedAndGarbageCollected> create()
     {
         return adoptRef(new RefCountedAndGarbageCollected());
     }
 
     ~RefCountedAndGarbageCollected()
     {
         ++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;
 
 private:
     RefCountedAndGarbageCollected()
     {
     }
 };
 
@@ skipping 1492 matching lines @@
         found++;
         int value = (*it)->value();
         EXPECT_TRUE(value >= 0 && value < 1100);
         ++it;
     }
     EXPECT_EQ(expected, found);
 }
 
 TEST(HeapTest, HeapWeakCollectionSimple)
 {
-
+    HeapStats initialHeapStats;
+    clearOutOldGarbage(&initialHeapStats);
     IntWrapper::s_destructorCalls = 0;
 
     PersistentHeapVector<Member<IntWrapper> > keepNumbersAlive;
 
     typedef HeapHashMap<WeakMember<IntWrapper>, Member<IntWrapper> > WeakStrong;
     typedef HeapHashMap<Member<IntWrapper>, WeakMember<IntWrapper> > StrongWeak;
     typedef HeapHashMap<WeakMember<IntWrapper>, WeakMember<IntWrapper> > WeakWeak;
     typedef HeapHashSet<WeakMember<IntWrapper> > WeakSet;
 
     Persistent<WeakStrong> weakStrong = new WeakStrong();
@@ skipping 24 matching lines @@
     keepNumbersAlive[0] = nullptr;
 
     Heap::collectGarbage(ThreadState::NoHeapPointersOnStack);
 
     EXPECT_EQ(0u, weakStrong->size());
     EXPECT_EQ(0u, strongWeak->size());
     EXPECT_EQ(0u, weakWeak->size());
     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;
 typedef HeapHashSet<PairUnwrappedWeak> UnwrappedWeakSet;
 
 void checkPairSets(
     Persistent<WeakStrongSet>& weakStrong,
     Persistent<StrongWeakSet>& strongWeak,
     Persistent<WeakUnwrappedSet>& weakUnwrapped,
     Persistent<UnwrappedWeakSet>& unwrappedWeak,
@@ skipping 559 matching lines @@
     HeapHashMap<void*, IntVector>::iterator it = map->find(key);
     EXPECT_EQ(0u, map->get(key).size());
 
     it->value.append(IntWrapper::create(42));
     EXPECT_EQ(1u, map->get(key).size());
 
     Persistent<HeapHashMap<void*, IntVector> > keepAlive(map);
     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)
 {
     HeapStats initialHeapStats;
     clearOutOldGarbage(&initialHeapStats);
 
     Persistent<UseMixin> usemixin = UseMixin::create();
     ASSERT_EQ(0, UseMixin::s_traceCount);
     Heap::collectGarbage(ThreadState::NoHeapPointersOnStack);
@@ skipping 198 matching lines @@
     // Check that the wrapper allocated during finalization is not
     // swept away and zapped later in the same sweeping phase.
     EXPECT_EQ(42, wrapper->value());
 
     wrapper.clear();
     Heap::collectGarbage(ThreadState::NoHeapPointersOnStack);
     EXPECT_EQ(10, IntWrapper::s_destructorCalls);
     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