Fix data race in blink::ThreadHeap::detach

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

Index: third_party/WebKit/Source/platform/heap/Heap.cpp
diff --git a/third_party/WebKit/Source/platform/heap/Heap.cpp b/third_party/WebKit/Source/platform/heap/Heap.cpp
index c2b0bb3e801c559c2a502c06c84bede3e602c5ac..6c8e4d4d678ff5611d48092ff7636bb98f314f9c 100644
--- a/third_party/WebKit/Source/platform/heap/Heap.cpp
+++ b/third_party/WebKit/Source/platform/heap/Heap.cpp
@@ -272,13 +272,14 @@ void ThreadHeap::detach(ThreadState* thread)
         thread->runTerminationGC();
         ASSERT(m_threads.contains(thread));
         m_threads.remove(thread);
+
+        // The main thread must be the last thread that gets detached.
+        DCHECK(!thread->isMainThread() || m_threads.isEmpty());

[sof, 2016/06/09 09:55:09]

This assert isn't accurate,

 DCHECK(thread->isMainThread() == m_threads.isEmpty());

[keishi, 2016/06/09 10:37:25]

The main thread must be the last thread that gets detached.
If the ThreadHeap is for a per-thread-heap enabled thread, the last thread being
detached is not the main thread.

[sof, 2016/06/09 10:44:57]

Yes, the assert as-is doesn't verify that last condition.

[keishi, 2016/06/09 11:47:34]

For a per-thread-heap enabled thread:
thread->isMainThread() will be false
m_threads.isEmpty() will be true
so thread->isMainThread() == m_threads.isEmpty() is not true.

this DCHECK is just trying to do
if (thread->isMainThread())
  DCHECK(m_threads.isEmpty());

Maybe I should be doing this?

if (thread->perThreadHeapEnabled() || thread->isMainThread())
  DCHECK(m_threads.isEmpty());

[sof, 2016/06/09 12:00:25]

Yes, that condition captures the "owner" of the ThreadHeap best, i think.

+        if (thread->isMainThread())
+            DCHECK_EQ(heapStats().allocatedSpace(), 0u);
+        if (m_threads.isEmpty())
+            delete this;

[sof, 2016/06/09 10:00:46]

Won't this UAF when the mutex locker stack object is destructed?

[keishi, 2016/06/09 10:37:25]

I guess you're right. Changed to  destruct out of the scope.

     }
-    // The main thread must be the last thread that gets detached.
-    ASSERT(!thread->isMainThread() || m_threads.isEmpty());
-    if (thread->isMainThread())
-        DCHECK_EQ(heapStats().allocatedSpace(), 0u);
-    if (m_threads.isEmpty())
-        delete this;
 }
 
 bool ThreadHeap::park()