Oilpan: Support nested pre-finalizers

Source/platform/heap/ThreadState.cpp

Index: Source/platform/heap/ThreadState.cpp
diff --git a/Source/platform/heap/ThreadState.cpp b/Source/platform/heap/ThreadState.cpp
index 73afe55fb9d405a2dd345e901dbcd85afe6609e6..574ea1ab75abf6a34cd65bcd5bad02e9e3d88142 100644
--- a/Source/platform/heap/ThreadState.cpp
+++ b/Source/platform/heap/ThreadState.cpp
@@ -1296,16 +1296,6 @@ void ThreadState::unlockThreadAttachMutex()
     threadAttachMutex().unlock();
 }
 
-void ThreadState::unregisterPreFinalizerInternal(void* target)
-{
-    checkThread();
-    if (sweepForbidden())
-        return;
-    auto it = m_preFinalizers.find(target);
-    ASSERT(it != m_preFinalizers.end());
-    m_preFinalizers.remove(it);
-}
-
 void ThreadState::invokePreFinalizers()
 {
     checkThread();
@@ -1317,9 +1307,17 @@ void ThreadState::invokePreFinalizers()
 
     SweepForbiddenScope forbiddenScope(this);
     Vector<void*> deadObjects;
-    for (auto& entry : m_preFinalizers) {
-        if (entry.value(entry.key))
-            deadObjects.append(entry.key);
+    for (auto& it : m_preFinalizers) {
+        void* object = it.key;
+        Vector<PreFinalizerCallback>* callbackVector = it.value.get();
+        ASSERT(callbackVector->size() >= 1);
+        if (!(callbackVector->at(0))(object))

[sof, 2015/06/24 11:21:56]

I wonder if it would be tidier to just check if 'object' is unmarked here?

[haraken, 2015/06/24 12:54:01]

Unfortunately we can't do this because isHeapObjectIsAlive requires T*.

[sof, 2015/06/24 13:40:31]

..and we have to support mixin prefinalizers.

+            continue;
+        deadObjects.append(object);
+        for (size_t i = 1; i < callbackVector->size(); i++) {

[sof, 2015/06/24 11:21:56]

Don't you want to do this the other way around? the vector will have the
ordering as given by the constructors (base classes first). I think you want to
run the prefinalizers in the other direction.

[haraken, 2015/06/24 12:54:01]

Nice catch! Done.

However, we cannot guarantee the order between Mixin::dispose and
SubClass::dispose, because |this| pointer is different between them and thus
they are registered in a different slot of the hash map of m_preFinalizers.

[sof, 2015/06/24 13:40:31]

It would have been preferable to avoid that non-determinism -- the worry is that
a mixin instance would somehow touch fields of the mixin when prefinalizing. And
find them disposed of..or not, potentially.

I suppose it wouldn't work out to do a 'this' pointer adjustment to its leftmost
base when registering the prefinalizer either, as the vptr hasn't been
initialized to the mixin instance's vtbl yet (i.e., perform a similar trick to
isHeapObjectAlive().)

[haraken, 2015/06/24 13:47:50]

Maybe can we use a LinkedListHashSet instead of a Vector?

+            bool ret = (callbackVector->at(i))(object);
+            ASSERT_UNUSED(ret, ret);
+        }
     }
     // FIXME: removeAll is inefficient.  It can shrink repeatedly.
     m_preFinalizers.removeAll(deadObjects);