Fix data race in blink::ThreadHeap::detach

third_party/WebKit/Source/platform/heap/Heap.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 254 matching lines @@
         // Grab the threadAttachMutex to ensure only one thread can shutdown at
         // a time and that no other thread can do a global GC. It also allows
         // safe iteration of the m_threads set which happens as part of
         // thread local GC asserts. We enter a safepoint while waiting for the
         // lock to avoid a dead-lock where another thread has already requested
         // GC.
         SafePointAwareMutexLocker locker(m_threadAttachMutex, BlinkGC::NoHeapPointersOnStack);
         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()
 {
     return m_safePointBarrier->parkOthers();
 }
 
 void ThreadHeap::resume()
 {
     m_safePointBarrier->resumeOthers();
@@ skipping 497 matching lines @@
     ProcessHeap::decreaseTotalMarkedObjectSize(m_stats.markedObjectSize());
 
     m_stats.reset();
     for (ThreadState* state : m_threads)
         state->resetHeapCounters();
 }
 
 ThreadHeap* ThreadHeap::s_mainThreadHeap = nullptr;
 
 } // namespace blink