Avoid data races on initializing GCScope.

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 66850722296c66cd4bc2678d7f00a2c2ef34acc2..5f0fd7cf58431057eb6e63fbb324ff84fc0c11ce 100644
--- a/third_party/WebKit/Source/platform/heap/Heap.cpp
+++ b/third_party/WebKit/Source/platform/heap/Heap.cpp
@@ -77,26 +77,10 @@ public:
         , m_gcForbiddenScope(state)
         // See collectGarbageForTerminatingThread() comment on why a
         // safepoint scope isn't entered for its GCScope.
-        , m_safePointScope(stackState, gcType != BlinkGC::ThreadTerminationGC ? state : nullptr)
-        , m_gcType(gcType)
-        , m_parkedAllThreads(false)
+        , m_safePointScope(stackState, gcType != BlinkGC::ThreadTerminationGC ? state : nullptr, true)

[haraken, 2015/11/09 06:20:11]

It seems that it's no longer worth using a SafePoint"Scope" (since we need to
delay entering the safe point). Can we just directly call
state->enterSafePoint() and state->leaveSafePoint()?

[sof, 2015/11/09 06:33:56]

That's better, delineating the two scope objects. Done.

     {
-        TRACE_EVENT0("blink_gc", "Heap::GCScope");
-        const char* samplingState = TRACE_EVENT_GET_SAMPLING_STATE();
-        if (m_state->isMainThread())
-            TRACE_EVENT_SET_SAMPLING_STATE("blink_gc", "BlinkGCWaiting");
-
         ASSERT(m_state->checkThread());
 
-        double startTime = WTF::currentTimeMS();
-        // TODO(haraken): In an unlikely coincidence that two threads decide
-        // to collect garbage at the same time, avoid doing two GCs in
-        // a row.
-        if (LIKELY(gcType != BlinkGC::ThreadTerminationGC && ThreadState::stopThreads()))
-            m_parkedAllThreads = true;
-        double timeForStoppingThreads = WTF::currentTimeMS() - startTime;
-        Platform::current()->histogramCustomCounts("BlinkGC.TimeForStoppingThreads", timeForStoppingThreads, 1, 1000, 50);
-
         switch (gcType) {
         case BlinkGC::GCWithSweep:
         case BlinkGC::GCWithoutSweep:
@@ -111,20 +95,35 @@ public:
         default:
             ASSERT_NOT_REACHED();
         }
+    }
+
+    bool parkAllThreads(BlinkGC::StackState stackState, BlinkGC::GCType gcType)
+    {
+        TRACE_EVENT0("blink_gc", "Heap::GCScope");
+        const char* samplingState = TRACE_EVENT_GET_SAMPLING_STATE();
+        if (m_state->isMainThread())
+            TRACE_EVENT_SET_SAMPLING_STATE("blink_gc", "BlinkGCWaiting");
+
+        m_safePointScope.enter(stackState);
+
+        // TODO(haraken): In an unlikely coincidence that two threads decide
+        // to collect garbage at the same time, avoid doing two GCs in
+        // a row and return false.
+        double startTime = WTF::currentTimeMS();
+        bool allParked = gcType != BlinkGC::ThreadTerminationGC && ThreadState::stopThreads();
+        double timeForStoppingThreads = WTF::currentTimeMS() - startTime;
+        Platform::current()->histogramCustomCounts("BlinkGC.TimeForStoppingThreads", timeForStoppingThreads, 1, 1000, 50);
 
         if (m_state->isMainThread())
             TRACE_EVENT_SET_NONCONST_SAMPLING_STATE(samplingState);
+
+        return allParked;
     }
 
-    bool allThreadsParked() const { return m_parkedAllThreads; }
     Visitor* visitor() const { return m_visitor.get(); }
 
     ~GCScope()
     {
-        // Only cleanup if we parked all threads in which case the GC happened
-        // and we need to resume the other threads.
-        if (LIKELY(m_gcType != BlinkGC::ThreadTerminationGC && m_parkedAllThreads))
-            ThreadState::resumeThreads();
     }
 
 private:
@@ -135,9 +134,24 @@ private:
     // to be in a GC forbidden scope when doing so.
     GCForbiddenScope m_gcForbiddenScope;
     SafePointScope m_safePointScope;
-    BlinkGC::GCType m_gcType;
     OwnPtr<Visitor> m_visitor;
-    bool m_parkedAllThreads; // False if we fail to park all threads
+};
+
+class ResumeThreadScope {
+public:
+    explicit ResumeThreadScope(BlinkGC::GCType gcType)
+        : m_resumeThreads(gcType != BlinkGC::ThreadTerminationGC)
+    {
+    }
+    ~ResumeThreadScope()
+    {
+        // Only cleanup if we parked all threads in which case the GC happened
+        // and we need to resume the other threads.
+        if (m_resumeThreads)
+            ThreadState::resumeThreads();
+    }
+private:
+    bool m_resumeThreads;
 };
 
 void Heap::flushHeapDoesNotContainCache()
@@ -381,11 +395,14 @@ void Heap::collectGarbage(BlinkGC::StackState stackState, BlinkGC::GCType gcType
     state->completeSweep();
 
     GCScope gcScope(state, stackState, gcType);
-    // Check if we successfully parked the other threads.  If not we bail out of
-    // the GC.
-    if (!gcScope.allThreadsParked())
+
+    // Try to park the other threads. If we're unable to, bail out of the GC.
+    if (!gcScope.parkAllThreads(stackState, gcType))
         return;
 
+    // Resume all parked threads upon leaving this scope.
+    ResumeThreadScope resumeThreads(gcType);
+
     if (state->isMainThread())
         ScriptForbiddenScope::enter();