Fix incorrect memory barrier usage I previously asked to be introduced.

base/task_scheduler/task_tracker.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/task_scheduler/task_tracker.h"
 
 #include <limits>
 
 #include "base/atomicops.h"
 #include "base/callback.h"
@@ skipping 23 matching lines @@
 
 void RecordNumBlockShutdownTasksPostedDuringShutdown(
     HistogramBase::Sample value) {
   UMA_HISTOGRAM_CUSTOM_COUNTS(
       "TaskScheduler.BlockShutdownTasksPostedDuringShutdown", value, 1,
       kMaxBlockShutdownTasksPostedDuringShutdown, 50);
 }
 
 }  // namespace
 
+// Atomic internal state used by TaskTracker. Sequential consistency shouldn't
+// be assumed from these calls (i.e. a thread reading
+// |HasShutdownStarted() == true| isn't guaranteed to see all writes made before
+// |StartShutdown()| on the thread that invoked it).
 class TaskTracker::State {
  public:
   State() = default;
 
   // Sets a flag indicating that shutdown has started. Returns true if there are
   // tasks blocking shutdown. Can only be called once.
   bool StartShutdown() {
     const auto new_value =
-        subtle::Barrier_AtomicIncrement(&bits_, kShutdownHasStartedMask);
+        subtle::NoBarrier_AtomicIncrement(&bits_, kShutdownHasStartedMask);
 
     // Check that the "shutdown has started" bit isn't zero. This would happen
     // if it was incremented twice.
     DCHECK(new_value & kShutdownHasStartedMask);
 
     const auto num_tasks_blocking_shutdown =
         new_value >> kNumTasksBlockingShutdownBitOffset;
     return num_tasks_blocking_shutdown != 0;
   }
 
   // Returns true if shutdown has started.
   bool HasShutdownStarted() const {
-    return subtle::Acquire_Load(&bits_) & kShutdownHasStartedMask;
+    return subtle::NoBarrier_Load(&bits_) & kShutdownHasStartedMask;
   }
 
   // Returns true if there are tasks blocking shutdown.
   bool AreTasksBlockingShutdown() const {
     const auto num_tasks_blocking_shutdown =
-        subtle::Acquire_Load(&bits_) >> kNumTasksBlockingShutdownBitOffset;
+        subtle::NoBarrier_Load(&bits_) >> kNumTasksBlockingShutdownBitOffset;
     DCHECK_GE(num_tasks_blocking_shutdown, 0);
     return num_tasks_blocking_shutdown != 0;
   }
 
   // Increments the number of tasks blocking shutdown. Returns true if shutdown
   // has started.
   bool IncrementNumTasksBlockingShutdown() {
 #if DCHECK_IS_ON()
     // Verify that no overflow will occur.
     const auto num_tasks_blocking_shutdown =
-        subtle::Acquire_Load(&bits_) >> kNumTasksBlockingShutdownBitOffset;
+        subtle::NoBarrier_Load(&bits_) >> kNumTasksBlockingShutdownBitOffset;
     DCHECK_LT(num_tasks_blocking_shutdown,
               std::numeric_limits<subtle::Atomic32>::max() -
                   kNumTasksBlockingShutdownIncrement);
 #endif
 
-    const auto new_bits = subtle::Barrier_AtomicIncrement(
+    const auto new_bits = subtle::NoBarrier_AtomicIncrement(
         &bits_, kNumTasksBlockingShutdownIncrement);
     return new_bits & kShutdownHasStartedMask;
   }
 
   // Decrements the number of tasks blocking shutdown. Returns true if shutdown
   // has started and the number of tasks blocking shutdown becomes zero.
   bool DecrementNumTasksBlockingShutdown() {
-    const auto new_bits = subtle::Barrier_AtomicIncrement(
+    const auto new_bits = subtle::NoBarrier_AtomicIncrement(
         &bits_, -kNumTasksBlockingShutdownIncrement);
     const bool shutdown_has_started = new_bits & kShutdownHasStartedMask;
     const auto num_tasks_blocking_shutdown =
         new_bits >> kNumTasksBlockingShutdownBitOffset;
     DCHECK_GE(num_tasks_blocking_shutdown, 0);
     return shutdown_has_started && num_tasks_blocking_shutdown == 0;
   }
 
  private:
   static constexpr subtle::Atomic32 kShutdownHasStartedMask = 1;
   static constexpr subtle::Atomic32 kNumTasksBlockingShutdownBitOffset = 1;
   static constexpr subtle::Atomic32 kNumTasksBlockingShutdownIncrement =
       1 << kNumTasksBlockingShutdownBitOffset;
 
   // The LSB indicates whether shutdown has started. The other bits count the
   // number of tasks blocking shutdown.
+  // No barriers are required to read/write |bits_| as this class is only used
+  // as an atomic state checker, it doesn't provide sequential consistency
+  // guarantees w.r.t. external state. Sequencing of the TaskTracker::State
+  // operations themselves is guaranteed by the AtomicIncrement RMW (read-
+  // modify-write) semantics however. For example, if two threads are racing to
+  // call IncrementNumTasksBlockingShutdown() and StartShutdown() respectively,
+  // either the first thread will win and the StartShutdown() call will see the
+  // blocking task or the second thread will win and
+  // IncrementNumTasksBlockingShutdown() will know that shutdown has started.
   subtle::Atomic32 bits_ = 0;
 
   DISALLOW_COPY_AND_ASSIGN(State);
 };
 
 TaskTracker::TaskTracker() : state_(new State) {}
 TaskTracker::~TaskTracker() = default;
 
 void TaskTracker::Shutdown() {
   {
@@ skipping 207 matching lines @@
 
   // This method can only be called after shutdown has started.
   DCHECK(state_->HasShutdownStarted());
   DCHECK(shutdown_event_);
 
   shutdown_event_->Signal();
 }
 
 }  // namespace internal
 }  // namespace base