Oilpan: Support nested pre-finalizers

Source/platform/heap/ThreadState.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 1278 matching lines @@
 void ThreadState::lockThreadAttachMutex()
 {
     threadAttachMutex().lock();
 }
 
 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();
     ASSERT(!sweepForbidden());
     TRACE_EVENT0("blink_gc", "ThreadState::invokePreFinalizers");
 
     if (isMainThread())
         ScriptForbiddenScope::enter();
 
     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);
 
     if (isMainThread())
         ScriptForbiddenScope::exit();
 }
 
 void ThreadState::clearHeapAges()
 {
@@ skipping 136 matching lines @@
         json->beginArray(it->key.ascii().data());
         for (size_t age = 0; age <= maxHeapObjectAge; ++age)
             json->pushInteger(it->value.ages[age]);
         json->endArray();
     }
     TRACE_EVENT_OBJECT_SNAPSHOT_WITH_ID(TRACE_DISABLED_BY_DEFAULT("blink_gc"), statsName, this, json.release());
 }
 #endif
 
 } // namespace blink