Fix minor Scheduler data races.

Ensure that Scheduler::{is_failed_,has_been_shutdown_}
are properly guarded by a lock since they are accessed
from multiple threads concurrently.

In Scheduler::FailWithError(), ensure the PostTask()
call happens under the lock.

In particular, this can happen when several background worker
threads are posting tasks to the queue, and one of them calls
Scheduler::FailWithError() which will implicitly call PostQuit().

Change-Id: Ie0b70be0838b9fdcf4f41c3ae53bc5bd8e6d71d3
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/27080
Commit-Queue: David Turner <digit@google.com>
Reviewed-by: Takuto Ikuta <tikuta@google.com>

src/gn/scheduler.cc

diff --git a/src/gn/scheduler.cc b/src/gn/scheduler.cc
index b40f29c..c6de66e 100644
--- a/src/gn/scheduler.cc
+++ b/src/gn/scheduler.cc
@@ -28,21 +28,29 @@
   // Reentrancy check.
   CHECK(!is_running_);
 
-  // Ensure there is at least one task, (or else this will wait forever).
-  if (is_failed_ || (work_count_.IsZero() && pool_work_count_.IsZero())) {
-    // Flush any posted tasks (that were posted from the UI thread).
-    main_thread_run_loop_->PostQuit();
-    main_thread_run_loop_->Run();
-    return !is_failed_;
-  }
-
-  has_been_shutdown_ = false;
-  is_running_ = true;
-  main_thread_run_loop_->Run();
+  // has_been_shutdown_ and is_failed_ are read or modified
+  // from other threads in Scheduler::FailWithError.
   bool local_is_failed;
   {
     std::lock_guard<std::mutex> lock(lock_);
-    local_is_failed = is_failed();
+    has_been_shutdown_ = false;
+    local_is_failed = is_failed_;
+  }
+
+  // Ensure there is at least one task, (or else this will wait forever).
+  if (local_is_failed || (work_count_.IsZero() && pool_work_count_.IsZero())) {
+    // Flush any posted tasks (that were posted from the UI thread).
+    main_thread_run_loop_->PostQuit();
+    main_thread_run_loop_->Run();
+    WaitForPoolTasks();
+    return !is_failed();
+  }
+
+  is_running_ = true;
+  main_thread_run_loop_->Run();
+  {
+    std::lock_guard<std::mutex> lock(lock_);
+    local_is_failed = is_failed_;
     has_been_shutdown_ = true;
   }
   // Don't do this while holding |lock_|, since it will block on the workers,
@@ -63,10 +71,10 @@
 
     if (is_failed_ || has_been_shutdown_)
       return;  // Ignore errors once we see one.
-    is_failed_ = true;
-  }
 
-  task_runner()->PostTask([this, err]() { FailWithErrorOnMainThread(err); });
+    is_failed_ = true;
+    task_runner()->PostTask([this, err]() { FailWithErrorOnMainThread(err); });
+  }
 }
 
 void Scheduler::ScheduleWork(std::function<void()> work) {

src/gn/scheduler.h

diff --git a/src/gn/scheduler.h b/src/gn/scheduler.h
index 08c4eb6..0db3726 100644
--- a/src/gn/scheduler.h
+++ b/src/gn/scheduler.h
@@ -39,8 +39,10 @@
   bool verbose_logging() const { return verbose_logging_; }
   void set_verbose_logging(bool v) { verbose_logging_ = v; }
 
-  // TODO(brettw) data race on this access (benign?).
-  bool is_failed() const { return is_failed_; }
+  bool is_failed() const {
+    std::lock_guard<std::mutex> lock(lock_);
+    return is_failed_;
+  }
 
   void Log(const std::string& verb, const std::string& msg);
   void FailWithError(const Err& err);

src/util/msg_loop.cc

diff --git a/src/util/msg_loop.cc b/src/util/msg_loop.cc
index 117daf8..ec88aab 100644
--- a/src/util/msg_loop.cc
+++ b/src/util/msg_loop.cc
@@ -38,8 +38,13 @@
         return (!task_queue_.empty()) || should_quit_;
       });
 
-      if (should_quit_)
+      if (should_quit_) {
+        // Clear any remaninig items in the queue to avoid
+        // running them on the next Run() invocation.
+        while (!task_queue_.empty())
+          task_queue_.pop();
         return;
+      }
 
       task = std::move(task_queue_.front());
       task_queue_.pop();