Oilpan:Allow custom handling of classes that contain weak pointers that are embedded in collections.

Source/platform/heap/HeapTest.cpp

Index: Source/platform/heap/HeapTest.cpp
diff --git a/Source/platform/heap/HeapTest.cpp b/Source/platform/heap/HeapTest.cpp
index 5c93ea68e5a3d0207641dd5d8846a9288692f6cd..dd70f34a6569098d31437fc736bd9f4e70c6bda1 100644
--- a/Source/platform/heap/HeapTest.cpp
+++ b/Source/platform/heap/HeapTest.cpp
@@ -44,6 +44,8 @@
 
 namespace WebCore {
 
+class IntWrapper;

[Mads Ager (chromium), 2014/06/06 09:59:17]

Can we just move IntWrapper to right after ThreadMarker? IntWrapper should be
very simple and not really rely on anything else and we use it in most of the
tests so it makes sense to have it at the top.

[Erik Corry, 2014/06/10 11:21:18]

Done.

+
 class ThreadMarker {
 public:
     ThreadMarker() : m_creatingThread(reinterpret_cast<ThreadState*>(0)), m_num(0) { }
@@ -75,6 +77,49 @@ struct ThreadMarkerHash {
     static const bool safeToCompareToEmptyOrDeleted = false;
 };
 
+typedef std::pair<Member<IntWrapper>, WeakMember<IntWrapper> > StrongWeakPair;
+
+struct PairWithWeakHandling : public StrongWeakPair {

[Mads Ager (chromium), 2014/06/06 09:59:17]

This is clever since it shuts up the gc plugin. ;-)

I think there is some work to do here to make this work properly with the gc
plugin if you do this manually instead:

class PairWithWeakHandling {
    ALLOW_ONLY_INLINE_ALLOCATION();
public:
    ...
private:
    Member<IntWrapper> m_first;
    WeakMember<IntWrapper> m_second;
};

In this case the gc plugin needs to treat the
traceInCollection/shouldRemoveFromCollection pair as a trace method. That is, it
needs to require either a trace method or both of traceInCollection and
shouldRemoveFromCollection. traceInCollection should have the normal trace
requirements that all members and weak members are mentioned. That seems like a
fairly simple change to the plugin. To make it safe, the plugin would have to
also check that if you use traceInCollection and not trace you need to
specialize the trait. Not sure how easy that will be to check. Maybe it should
just require that you always have a trace method and then if you have these
methods it should check that they do the right thing as well. That is something
to work out after this change though.

[Erik Corry, 2014/06/10 11:21:18]

OK

+    ALLOW_ONLY_INLINE_ALLOCATION();
+
+public:
+    // Regular constructor.
+    PairWithWeakHandling(IntWrapper* one, IntWrapper* two)
+        : StrongWeakPair(one, two)
+    {
+        ASSERT(one); // We use null first field to indicate empty slots in the hash table.
+    }
+
+    // The HashTable (via the HashTrait) calls this constructor with a
+    // placement new to mark slots in the hash table as being deleted. We will
+    // never call trace or the destructor on these slots. We mark ourselves deleted
+    // with a pointer to -1 in the first field.
+    PairWithWeakHandling(WTF::HashTableDeletedValueType)
+        : StrongWeakPair(reinterpret_cast<IntWrapper*>(-1), nullptr)
+    {
+    }
+
+    // Used by the HashTable (via the HashTrait) to skip deleted slots in the
+    // table. Recognizes objects that were 'constructed' using the above
+    // constructor.
+    bool isHashTableDeletedValue() const { return first == reinterpret_cast<IntWrapper*>(-1); }
+
+    // Since we don't allocate independent objects of this type, we don't need
+    // a regular trace method. Instead, we use a traceInCollection method.
+    void traceInCollection(Visitor* visitor, ShouldWeakPointersBeMarkedStrongly strongify)
+    {
+        visitor->trace(first);
+        visitor->traceInCollection(second, strongify);
+    }
+    // The traceInCollection may not trace the weak members, so it is vital
+    // that shouldRemoveFromCollection checks them so the entry can be removed
+    // from the collection (otherwise it would contain a dangling pointer).
+    bool shouldRemoveFromCollection(Visitor* visitor)
+    {
+        return !visitor->isAlive(second);
+    }
+};
+
 }
 
 namespace WTF {
@@ -91,6 +136,29 @@ template<> struct HashTraits<WebCore::ThreadMarker> : GenericHashTraits<WebCore:
     static bool isDeletedValue(const WebCore::ThreadMarker& slot) { return slot.isHashTableDeletedValue(); }
 };
 
+// The hash algorithm for our custom pair class is just the standard double
+// hash for pairs. Note that this means you can't mutate either of the parts of
+// the pair while they are in the hash table, as that would change their hash
+// code and thus their preferred placement in the table.
+template<> struct DefaultHash<WebCore::PairWithWeakHandling> {
+    typedef PairHash<WebCore::Member<WebCore::IntWrapper>, WebCore::WeakMember<WebCore::IntWrapper> > Hash;
+};
+
+// Custom traits for the pair. These are weakness handling traits, which means
+// PairWithWeakHandling must implement the traceInCollection and
+// shouldRemoveFromCollection methods. In addition, these traits are concerned
+// with the two magic values for the object, that represent empty and deleted
+// slots in the hash table. The SimpleClassHashTraits allow empty slots in the
+// table to be initialzed with memset to zero, and we use -1 in the first part
+// of the pair to represent deleted slots.
+template<> struct HashTraits<WebCore::PairWithWeakHandling> : WebCore::WeakHandlingHashTraits<WebCore::PairWithWeakHandling> {
+    static const bool needsDestruction = false;
+    static const bool hasIsEmptyValueFunction = true;
+    static bool isEmptyValue(const WebCore::PairWithWeakHandling& value) { return !value.first; }
+    static void constructDeletedValue(WebCore::PairWithWeakHandling& slot) { new (NotNull, &slot) WebCore::PairWithWeakHandling(HashTableDeletedValue); }
+    static bool isDeletedValue(const WebCore::PairWithWeakHandling& value) { return value.isHashTableDeletedValue(); }
+};
+
 }
 
 namespace WebCore {
@@ -3316,12 +3384,19 @@ TEST(HeapTest, CheckAndMarkPointer)
         EXPECT_TRUE(scope.allThreadsParked());
         Heap::makeConsistentForGC();
         for (size_t i = 0; i < objectAddresses.size(); i++) {
-            EXPECT_FALSE(Heap::checkAndMarkPointer(&visitor, objectAddresses[i]));
-            EXPECT_FALSE(Heap::checkAndMarkPointer(&visitor, endAddresses[i]));
+            // We would like to assert that checkAndMarkPointer returned false
+            // here because the pointers no longer point into a valid object
+            // (it's been freed by the GCs. But checkAndMarkPointer will return
+            // true for any pointer that points into a heap page, regardless of
+            // whether it points at a valid object (this ensures the
+            // correctness of the page-based on-heap address caches), so we
+            // can't make that assert.
+            Heap::checkAndMarkPointer(&visitor, objectAddresses[i]);
+            Heap::checkAndMarkPointer(&visitor, endAddresses[i]);
         }
         EXPECT_EQ(0ul, visitor.count());
-        EXPECT_FALSE(Heap::checkAndMarkPointer(&visitor, largeObjectAddress));
-        EXPECT_FALSE(Heap::checkAndMarkPointer(&visitor, largeObjectEndAddress));
+        Heap::checkAndMarkPointer(&visitor, largeObjectAddress);
+        Heap::checkAndMarkPointer(&visitor, largeObjectEndAddress);
         EXPECT_EQ(0ul, visitor.count());
     }
     // This round of GC is important to make sure that the object start
@@ -3978,4 +4053,219 @@ TEST(HeapTest, NeedsAdjustAndMark)
     EXPECT_FALSE(NeedsAdjustAndMark<const UseMixin>::value);
 }
 
+template<typename Set>
+void setWithCustomWeaknessHandling()
+{
+    Set set;
+    Set* set2 = new Set();
+    Persistent<Set> set3(new Set());
+    Persistent<IntWrapper> livingInt(IntWrapper::create(42));
+    set.add(PairWithWeakHandling(IntWrapper::create(0), IntWrapper::create(1)));
+    set2->add(PairWithWeakHandling(IntWrapper::create(2), IntWrapper::create(3)));
+    set3->add(PairWithWeakHandling(IntWrapper::create(4), IntWrapper::create(5)));
+    Heap::collectGarbage(ThreadState::HeapPointersOnStack);
+    typedef typename Set::iterator Iterator;
+    // The first set is on-stack, so its backing store must be referenced from
+    // the stack. That makes the weak references strong.
+    Iterator i = set.begin();
+    EXPECT_EQ(0, i->first->value());
+    EXPECT_EQ(1, i->second->value());
+    // The second set is pointed to from the stack, so it's referenced, but the
+    // weak processing may have taken place.
+    if (set2->size()) {
+        Iterator i2 = set2->begin();
+        EXPECT_EQ(2, i2->first->value());
+        EXPECT_EQ(3, i2->second->value());
+    }
+    // The third set is pointed to from a persistent, so it's referenced, but
+    // the weak processing may have taken place.
+    if (set3->size()) {
+        Iterator i3 = set3->begin();
+        EXPECT_EQ(4, i3->first->value());
+        EXPECT_EQ(5, i3->second->value());
+    }
+    Heap::collectGarbage(ThreadState::NoHeapPointersOnStack);
+    // No looking at the set and set2 now, they are busted by our lying about the heap pointers on the stack!

[Mads Ager (chromium), 2014/06/06 09:59:17]

How about scoping them so we can't touch them?

[Erik Corry, 2014/06/10 11:21:18]

Done.

+    if (set3->size()) {

[Mads Ager (chromium), 2014/06/06 09:59:17]

Since we ignore all heap pointers on the stack this should be guaranteed to
clear out the element, right? So this should be EXPECT_EQ(0, set3->size())?

[Erik Corry, 2014/06/10 11:21:18]

Done.

+        Iterator i3 = set3->begin();
+        EXPECT_EQ(4, i3->first->value());
+        EXPECT_EQ(5, i3->second->value());
+    }
+    set3->clear();

[Mads Ager (chromium), 2014/06/06 09:59:17]

And then this is not needed.

[Erik Corry, 2014/06/10 11:21:18]

Done.

+    set3->add(PairWithWeakHandling(IntWrapper::create(103), livingInt));
+    set3->add(PairWithWeakHandling(livingInt, IntWrapper::create(103))); // This one gets zapped at GC time because nothing holds the 103 alive.
+    set3->add(PairWithWeakHandling(IntWrapper::create(103), IntWrapper::create(103))); // This one gets zapped too.
+    set3->add(PairWithWeakHandling(livingInt, livingInt));
+    set3->add(PairWithWeakHandling(livingInt, livingInt)); // This one is identical to the previous and doesn't add anything.
+    EXPECT_EQ(4u, set3->size());
+    Heap::collectGarbage(ThreadState::NoHeapPointersOnStack);
+    EXPECT_EQ(2u, set3->size());
+    Iterator i3 = set3->begin();
+    EXPECT_TRUE(i3->first->value() == 103 || i3->first == livingInt);
+    EXPECT_EQ(livingInt, i3->second);
+    ++i3;
+    EXPECT_TRUE(i3->first->value() == 103 || i3->first == livingInt);
+    EXPECT_EQ(livingInt, i3->second);
+}
+
+TEST(HeapTest, SetWithCustomWeaknessHandling)
+{
+    setWithCustomWeaknessHandling<HeapHashSet<PairWithWeakHandling> >();
+    setWithCustomWeaknessHandling<HeapLinkedHashSet<PairWithWeakHandling> >();
+}
+
+TEST(HeapTest, MapWithCustomWeaknessHandling)
+{
+    typedef HeapHashMap<PairWithWeakHandling, RefPtr<OffHeapInt> > Map;
+    HeapStats initialHeapSize;
+    clearOutOldGarbage(&initialHeapSize);
+    OffHeapInt::s_destructorCalls = 0;
+
+    Map map;
+    Map* map2 = new Map();
+    Persistent<Map> map3(new Map());
+    Persistent<IntWrapper> livingInt(IntWrapper::create(42));
+    map.add(PairWithWeakHandling(IntWrapper::create(0), IntWrapper::create(1)), OffHeapInt::create(1001));
+    map2->add(PairWithWeakHandling(IntWrapper::create(2), IntWrapper::create(3)), OffHeapInt::create(1002));
+    map3->add(PairWithWeakHandling(IntWrapper::create(4), IntWrapper::create(5)), OffHeapInt::create(1003));
+    EXPECT_EQ(0, OffHeapInt::s_destructorCalls);
+
+    Heap::collectGarbage(ThreadState::HeapPointersOnStack);
+    typedef typename Map::iterator Iterator;
+    // The first map is on-stack, so its backing store must be referenced from
+    // the stack. That makes the weak references strong.
+    Iterator i = map.begin();
+    EXPECT_EQ(0, i->key.first->value());
+    EXPECT_EQ(1, i->key.second->value());
+    EXPECT_EQ(1001, i->value->value());
+    // The second map is pointed to from the stack, so it's referenced, but the
+    // weak processing may have taken place.
+    if (map2->size()) {
+        Iterator i2 = map2->begin();
+        EXPECT_EQ(2, i2->key.first->value());
+        EXPECT_EQ(3, i2->key.second->value());
+        EXPECT_EQ(1002, i2->value->value());
+    }
+    // The third map is pointed to from a persistent, so it's referenced, but
+    // the weak processing may have taken place.
+    if (map3->size()) {
+        Iterator i3 = map3->begin();
+        EXPECT_EQ(4, i3->key.first->value());
+        EXPECT_EQ(5, i3->key.second->value());
+        EXPECT_EQ(1003, i3->value->value());
+    }
+    Heap::collectGarbage(ThreadState::NoHeapPointersOnStack);
+    // No looking at the map and map2 now, they are busted by our lying about the heap pointers on the stack!
+    if (map3->size()) {

[Mads Ager (chromium), 2014/06/06 09:59:16]

Same here. This should be reduced to the EXPECT_EQ in the else branch?

[Erik Corry, 2014/06/10 11:21:18]

Done.

+        Iterator i3 = map3->begin();
+        EXPECT_EQ(4, i3->key.first->value());
+        EXPECT_EQ(5, i3->key.second->value());
+        EXPECT_EQ(1003, i3->value->value());
+        EXPECT_EQ(2, OffHeapInt::s_destructorCalls);
+    } else {
+        EXPECT_EQ(3, OffHeapInt::s_destructorCalls);
+    }
+    map3->clear();
+
+    EXPECT_EQ(3, OffHeapInt::s_destructorCalls);
+    OffHeapInt::s_destructorCalls = 0;
+
+    map3->add(PairWithWeakHandling(IntWrapper::create(103), livingInt), OffHeapInt::create(2000));
+    map3->add(PairWithWeakHandling(livingInt, IntWrapper::create(103)), OffHeapInt::create(2001)); // This one gets zapped at GC time because nothing holds the 103 alive.
+    map3->add(PairWithWeakHandling(IntWrapper::create(103), IntWrapper::create(103)), OffHeapInt::create(2002)); // This one gets zapped too.
+    RefPtr<OffHeapInt> dupeInt(OffHeapInt::create(2003));
+    map3->add(PairWithWeakHandling(livingInt, livingInt), dupeInt);
+    map3->add(PairWithWeakHandling(livingInt, livingInt), dupeInt); // This one is identical to the previous and doesn't add anything.
+    dupeInt.clear();
+
+    EXPECT_EQ(0, OffHeapInt::s_destructorCalls);
+    EXPECT_EQ(4u, map3->size());
+    Heap::collectGarbage(ThreadState::NoHeapPointersOnStack);
+    EXPECT_EQ(2, OffHeapInt::s_destructorCalls);
+    EXPECT_EQ(2u, map3->size());
+    Iterator i3 = map3->begin();
+    EXPECT_TRUE(i3->key.first->value() == 103 || i3->key.first == livingInt);
+    EXPECT_EQ(livingInt, i3->key.second);
+    ++i3;
+    EXPECT_TRUE(i3->key.first->value() == 103 || i3->key.first == livingInt);
+    EXPECT_EQ(livingInt, i3->key.second);
+}
+
+TEST(HeapTest, MapWithCustomWeaknessHandling2)
+{
+    typedef HeapHashMap<RefPtr<OffHeapInt>, PairWithWeakHandling> Map;
+    HeapStats initialHeapSize;
+    clearOutOldGarbage(&initialHeapSize);
+    OffHeapInt::s_destructorCalls = 0;
+
+    Map map;
+    Map* map2 = new Map();
+    Persistent<Map> map3(new Map());
+    Persistent<IntWrapper> livingInt(IntWrapper::create(42));
+    map.add(OffHeapInt::create(1001), PairWithWeakHandling(IntWrapper::create(0), IntWrapper::create(1)));
+    map2->add(OffHeapInt::create(1002), PairWithWeakHandling(IntWrapper::create(2), IntWrapper::create(3)));
+    map3->add(OffHeapInt::create(1003), PairWithWeakHandling(IntWrapper::create(4), IntWrapper::create(5)));
+    EXPECT_EQ(0, OffHeapInt::s_destructorCalls);
+
+    Heap::collectGarbage(ThreadState::HeapPointersOnStack);
+    typedef typename Map::iterator Iterator;
+    // The first map is on-stack, so its backing store must be referenced from
+    // the stack. That makes the weak references strong.
+    Iterator i = map.begin();
+    EXPECT_EQ(0, i->value.first->value());
+    EXPECT_EQ(1, i->value.second->value());
+    EXPECT_EQ(1001, i->key->value());
+    // The second map is pointed to from the stack, so it's referenced, but the
+    // weak processing may have taken place.
+    if (map2->size()) {
+        Iterator i2 = map2->begin();
+        EXPECT_EQ(2, i2->value.first->value());
+        EXPECT_EQ(3, i2->value.second->value());
+        EXPECT_EQ(1002, i2->key->value());
+    }
+    // The third map is pointed to from a persistent, so it's referenced, but
+    // the weak processing may have taken place.
+    if (map3->size()) {
+        Iterator i3 = map3->begin();
+        EXPECT_EQ(4, i3->value.first->value());
+        EXPECT_EQ(5, i3->value.second->value());
+        EXPECT_EQ(1003, i3->key->value());
+    }
+    Heap::collectGarbage(ThreadState::NoHeapPointersOnStack);
+    // No looking at the map and map2 now, they are busted by our lying about the heap pointers on the stack!
+    if (map3->size()) {

[Mads Ager (chromium), 2014/06/06 09:59:17]

This one too.

[Erik Corry, 2014/06/10 11:21:18]

Done.

+        Iterator i3 = map3->begin();
+        EXPECT_EQ(4, i3->value.first->value());
+        EXPECT_EQ(5, i3->value.second->value());
+        EXPECT_EQ(1003, i3->key->value());
+        EXPECT_EQ(2, OffHeapInt::s_destructorCalls);
+    } else {
+        EXPECT_EQ(3, OffHeapInt::s_destructorCalls);
+    }
+    map3->clear();
+
+    EXPECT_EQ(3, OffHeapInt::s_destructorCalls);
+    OffHeapInt::s_destructorCalls = 0;
+
+    map3->add(OffHeapInt::create(2000), PairWithWeakHandling(IntWrapper::create(103), livingInt));
+    map3->add(OffHeapInt::create(2001), PairWithWeakHandling(livingInt, IntWrapper::create(103))); // This one gets zapped at GC time because nothing holds the 103 alive.
+    map3->add(OffHeapInt::create(2002), PairWithWeakHandling(IntWrapper::create(103), IntWrapper::create(103))); // This one gets zapped too.
+    RefPtr<OffHeapInt> dupeInt(OffHeapInt::create(2003));
+    map3->add(dupeInt, PairWithWeakHandling(livingInt, livingInt));
+    map3->add(dupeInt, PairWithWeakHandling(livingInt, livingInt)); // This one is identical to the previous and doesn't add anything.
+    dupeInt.clear();
+
+    EXPECT_EQ(0, OffHeapInt::s_destructorCalls);
+    EXPECT_EQ(4u, map3->size());
+    Heap::collectGarbage(ThreadState::NoHeapPointersOnStack);
+    EXPECT_EQ(2, OffHeapInt::s_destructorCalls);
+    EXPECT_EQ(2u, map3->size());
+    Iterator i3 = map3->begin();
+    EXPECT_TRUE(i3->value.first->value() == 103 || i3->value.first == livingInt);
+    EXPECT_EQ(livingInt, i3->value.second);
+    ++i3;
+    EXPECT_TRUE(i3->value.first->value() == 103 || i3->value.first == livingInt);
+    EXPECT_EQ(livingInt, i3->value.second);
+}
+
 } // WebCore namespace