Synchronize memory in TaskTracker::Flush().

base/task_scheduler/task_tracker.cc

Index: base/task_scheduler/task_tracker.cc
diff --git a/base/task_scheduler/task_tracker.cc b/base/task_scheduler/task_tracker.cc
index a8fbfcac856d1ccca41dc61a0921841d52102b7f..a209a25e4ad8cc9ece50db6aa46a385309398d7c 100644
--- a/base/task_scheduler/task_tracker.cc
+++ b/base/task_scheduler/task_tracker.cc
@@ -215,7 +215,7 @@ void TaskTracker::Shutdown() {
 
 void TaskTracker::Flush() {
   AutoSchedulerLock auto_lock(flush_lock_);
-  while (subtle::NoBarrier_Load(&num_pending_undelayed_tasks_) != 0 &&
+  while (subtle::Acquire_Load(&num_pending_undelayed_tasks_) != 0 &&

[robliao, 2017/05/02 14:23:20]

What was the failure that caused this?

[robliao, 2017/05/02 15:44:47]

Offline Discussion: The Acquire here is probably not necessary due to the
flush_lock_. We also aren't concerned about the effects of writes reordered
within this thread for visibility to other threads.

task_tracker.h will need to be updated too.
  // Number of undelayed tasks that haven't completed their execution. Is
  // incremented and decremented without a barrier. When it reaches zero,
  // |flush_lock_| is acquired (forcing memory synchronization) and |flush_cv_|
  // is signaled.
  subtle::Atomic32 num_pending_undelayed_tasks_ = 0;

[gab, 2017/05/02 20:26:27]

My thoughts (no action required):

So the problem is that it's possible that this sees 0 before |flush_lock_| is
acquired because it dropped it to 0 in DecrementNumPendingUndelayedTasks()

So what needs to happen is DecrementNumPendingUndelayedTasks() not only calls
flush_cv_->Signal() inside the lock but also sets another piece of state that
the conditional here will depend on (e.g "bool flushed_ = true;").

The atomic |num_pending_undelayed_tasks_| is still useful to make sure we only
grab the lock on sync points (0 -> 1 and 1 -> 0)...


But meh... overall the barriers are simpler I think...

PS: I do think we need the Acquire here as this is what forces this to
happen-after the Release semantics of the Barrier_AtomicIncrement.

[fdoray, 2017/05/03 13:17:28]

Updating comment in header file: Done.

          !IsShutdownComplete()) {
     flush_cv_->Wait();
   }
@@ -484,7 +484,7 @@ void TaskTracker::OnBlockingShutdownTasksComplete() {
 
 void TaskTracker::DecrementNumPendingUndelayedTasks() {
   const auto new_num_pending_undelayed_tasks =
-      subtle::NoBarrier_AtomicIncrement(&num_pending_undelayed_tasks_, -1);
+      subtle::Barrier_AtomicIncrement(&num_pending_undelayed_tasks_, -1);

[robliao, 2017/05/02 14:23:20]

Is this barrier actually necessary? What other memory was impacted?

[robliao, 2017/05/02 15:44:48]

Offline discussion: The memory writes of the task finishing could be reordered
after this atomic decrement, which is why we need a barrier here so that when
another thread sees 0, it's guaranteed that all writes are visible.

   DCHECK_GE(new_num_pending_undelayed_tasks, 0);
   if (new_num_pending_undelayed_tasks == 0) {
     AutoSchedulerLock auto_lock(flush_lock_);