Unify and generalize thread static persistent finalization.

third_party/WebKit/Source/platform/heap/ThreadState.cpp

Index: third_party/WebKit/Source/platform/heap/ThreadState.cpp
diff --git a/third_party/WebKit/Source/platform/heap/ThreadState.cpp b/third_party/WebKit/Source/platform/heap/ThreadState.cpp
index d8882ad5babb73e58f7d680967f1331d4e2c1940..ef6b720da68e14a078e6af12898be3b28bcccf36 100644
--- a/third_party/WebKit/Source/platform/heap/ThreadState.cpp
+++ b/third_party/WebKit/Source/platform/heap/ThreadState.cpp
@@ -220,7 +220,7 @@ void ThreadState::runTerminationGC()
     // pointers into the heap owned by this thread.
     m_isTerminating = true;
 
-    ThreadState::callThreadShutdownHooks();
+    releaseStaticPersistentNodes();
 
     // Set the terminate flag on all heap pages of this thread. This is used to
     // ensure we don't trace pages on other threads that are not part of the
@@ -255,15 +255,17 @@ void ThreadState::cleanupMainThread()
 {
     ASSERT(isMainThread());
 
+    releaseStaticPersistentNodes();
+
 #if defined(LEAK_SANITIZER)
-    // If LSan is about to perform leak detection, release all the registered
-    // static Persistent<> root references to global caches that Blink keeps,
-    // followed by GCs to clear out all they referred to.
+    // If LSan is about to perform leak detection, after having released all
+    // the registered static Persistent<> root references to global caches
+    // that Blink keeps, follow up with a round of GCs to clear out all
+    // what they referred to.
     //
     // This is not needed for caches over non-Oilpan objects, as they're
     // not scanned by LSan due to being held in non-global storage
     // ("static" references inside functions/methods.)
-    releaseStaticPersistentNodes();
     ThreadHeap::collectAllGarbage();
 #endif
 
@@ -290,15 +292,6 @@ void ThreadState::detachMainThread()
     state->~ThreadState();
 }
 
-void ThreadState::callThreadShutdownHooks()
-{
-    // Invoke the cleanup hooks. This gives an opportunity to release any
-    // persistent handles that may exist, e.g. in thread-specific static
-    // locals.
-    for (const OwnPtr<SameThreadClosure>& hook : m_threadShutdownHooks)
-        (*hook)();
-}
-
 void ThreadState::detachCurrentThread()
 {
     ThreadState* state = current();
@@ -1316,31 +1309,40 @@ void ThreadState::addInterruptor(PassOwnPtr<BlinkGCInterruptor> interruptor)
     }
 }
 
-void ThreadState::registerThreadShutdownHook(PassOwnPtr<SameThreadClosure> hook)
+void ThreadState::registerStaticPersistentNode(PersistentNode* node, PersistentClearCallback callback)
 {
-    ASSERT(checkThread());
-    ASSERT(!isTerminating());
-    m_threadShutdownHooks.append(hook);
-}
-
 #if defined(LEAK_SANITIZER)
-void ThreadState::registerStaticPersistentNode(PersistentNode* node)
-{
     if (m_disabledStaticPersistentsRegistration)
         return;
+#endif
 
     ASSERT(!m_staticPersistents.contains(node));
-    m_staticPersistents.add(node);
+    m_staticPersistents.add(node, callback);
 }
 
 void ThreadState::releaseStaticPersistentNodes()
 {
-    for (PersistentNode* node : m_staticPersistents)
-        getPersistentRegion()->freePersistentNode(node);
+    HashMap<PersistentNode*, ThreadState::PersistentClearCallback> staticPersistents;
+    staticPersistents.swap(m_staticPersistents);
+
+    PersistentRegion* persistentRegion = getPersistentRegion();
+    for (const auto& it : staticPersistents)
+        persistentRegion->releasePersistentNode(it.key, it.value);
+}
 
-    m_staticPersistents.clear();
+void ThreadState::freePersistentNode(PersistentNode* persistentNode)
+{
+    PersistentRegion* persistentRegion = getPersistentRegion();
+    persistentRegion->freePersistentNode(persistentNode);
+    // Do not allow static persistents to be freed before
+    // they're all released in releaseStaticPersistentNodes().
+    //
+    // There's no fundamental reason why this couldn't be supported,
+    // but no known use for it.

[haraken, 2016/04/25 08:39:23]

Yeah, I prefer adding this restriction too. registerStaticPersistentNode should
be used for only "static" persistent nodes (i.e., never freed).

[sof, 2016/04/25 08:53:05]

Agreed; a thread needing to free a persistent while running would be it, but I'd
like to see where & when that is needed first. 

+    ASSERT(!m_staticPersistents.contains(persistentNode));
 }
 
+#if defined(LEAK_SANITIZER)
 void ThreadState::enterStaticReferenceRegistrationDisabledScope()
 {
     m_disabledStaticPersistentsRegistration++;