Avoid data races on initializing GCScope.

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 59 matching lines @@
     ThreadState* m_state;
 };
 
 class GCScope final {
 public:
     GCScope(ThreadState* state, BlinkGC::StackState stackState, BlinkGC::GCType gcType)
         : m_state(state)
         , 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:
             m_visitor = adoptPtr(new MarkingVisitor<Visitor::GlobalMarking>());
             break;
         case BlinkGC::TakeSnapshot:
             m_visitor = adoptPtr(new MarkingVisitor<Visitor::SnapshotMarking>());
             break;
         case BlinkGC::ThreadTerminationGC:
             m_visitor = adoptPtr(new MarkingVisitor<Visitor::ThreadLocalMarking>());
             break;
         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:
     ThreadState* m_state;
     // The ordering of the two scope objects matters: GCs must first be forbidden
     // before entering the safe point scope. Prior to reaching the safe point,
     // ThreadState::runScheduledGC() is called. See its comment why we need
     // 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()
 {
     s_heapDoesNotContainCache->flush();
 }
 
 void Heap::init()
 {
     ThreadState::init();
@@ skipping 223 matching lines @@
 }
 
 void Heap::collectGarbage(BlinkGC::StackState stackState, BlinkGC::GCType gcType, BlinkGC::GCReason reason)
 {
     ThreadState* state = ThreadState::current();
     // Nested collectGarbage() invocations aren't supported.
     RELEASE_ASSERT(!state->isGCForbidden());
     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();
 
     TRACE_EVENT2("blink_gc", "Heap::collectGarbage",
         "lazySweeping", gcType == BlinkGC::GCWithoutSweep,
         "gcReason", gcReasonString(reason));
     TRACE_EVENT_SCOPED_SAMPLING_STATE("blink_gc", "BlinkGC");
     double startTime = WTF::currentTimeMS();
 
     if (gcType == BlinkGC::TakeSnapshot)
@@ skipping 359 matching lines @@
 size_t Heap::s_wrapperCount = 0;
 size_t Heap::s_wrapperCountAtLastGC = 0;
 size_t Heap::s_collectedWrapperCount = 0;
 size_t Heap::s_partitionAllocSizeAtLastGC = 0;
 double Heap::s_estimatedMarkingTimePerByte = 0.0;
 #if ENABLE(ASSERT)
 uint16_t Heap::s_gcGeneration = 0;
 #endif
 
 } // namespace blink