Make RunLoop::Quit() thread-safe.

base/run_loop.cc

Index: base/run_loop.cc
diff --git a/base/run_loop.cc b/base/run_loop.cc
index 6de1707b8f6885e9b2eb2e9be5882311f2752de5..87128c99d16087d03067af47f477f0628d21497f 100644
--- a/base/run_loop.cc
+++ b/base/run_loop.cc
@@ -5,8 +5,11 @@
 #include "base/run_loop.h"
 
 #include "base/bind.h"
+#include "base/callback.h"
 #include "base/lazy_instance.h"
+#include "base/single_thread_task_runner.h"
 #include "base/threading/thread_local.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "base/tracked_objects.h"
 #include "build/build_config.h"
 
@@ -17,6 +20,17 @@ namespace {
 LazyInstance<ThreadLocalPointer<RunLoop::Delegate>>::Leaky tls_delegate =
     LAZY_INSTANCE_INITIALIZER;
 
+// Runs |closure| immediately if this is called on |task_runner|, otherwise
+// forwards |closure| to it.
+void ProxyToTaskRunner(scoped_refptr<SequencedTaskRunner> task_runner,
+                       OnceClosure closure) {
+  if (task_runner->RunsTasksInCurrentSequence()) {

[danakj, 2017/05/19 18:11:30]

This if (on thread) A else post A pattern always irks me a bit.

What do you think of a QuitSoon() that is thread safe, vs Quit() that is not.
And QuitSoon always posts? Then you get consistent behaviour about leaving the
current stack frame from each method.

[gab, 2017/05/19 19:38:16]

Discussed offline, won't do that because the goal of this CL is to make
migration from FILE thread to TaskScheduler as seamless as possible and
requiring to update tests unnecessarily doesn't fit that requirement.

+    std::move(closure).Run();
+    return;
+  }
+  task_runner->PostTask(FROM_HERE, std::move(closure));
+}
+
 }  // namespace
 
 RunLoop::Delegate::Delegate() {
@@ -63,9 +77,13 @@ RunLoop::Delegate::Client* RunLoop::RegisterDelegateForCurrentThread(
   return &delegate->client_interface_;
 }
 
-RunLoop::RunLoop() : delegate_(tls_delegate.Get().Get()), weak_factory_(this) {
+RunLoop::RunLoop()
+    : delegate_(tls_delegate.Get().Get()),
+      origin_task_runner_(ThreadTaskRunnerHandle::Get()),
+      weak_factory_(this) {
   // A RunLoop::Delegate must be bound to this thread prior to using RunLoop.
   DCHECK(delegate_);
+  DCHECK(origin_task_runner_);
 }
 
 RunLoop::~RunLoop() {
@@ -108,7 +126,16 @@ void RunLoop::RunUntilIdle() {
 }
 
 void RunLoop::Quit() {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  // Thread-safe.
+
+  // This can only be hit if run_loop->Quit() is called directly (QuitClosure()
+  // proxies through ProxyToTaskRunner() as it can only deref its WeakPtr on
+  // |origin_task_runner_|).
+  if (!origin_task_runner_->RunsTasksInCurrentSequence()) {
+    origin_task_runner_->PostTask(FROM_HERE,
+                                  base::Bind(&RunLoop::Quit, Unretained(this)));
+    return;
+  }
 
   quit_called_ = true;
   if (running_ && delegate_->active_run_loops_.top() == this) {
@@ -118,20 +145,41 @@ void RunLoop::Quit() {
 }
 
 void RunLoop::QuitWhenIdle() {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  // Thread-safe.
+
+  // This can only be hit if run_loop->QuitWhenIdle() is called directly
+  // (QuitWhenIdleClosure() proxies through ProxyToTaskRunner() as it can only
+  // deref its WeakPtr on |origin_task_runner_|).
+  if (!origin_task_runner_->RunsTasksInCurrentSequence()) {
+    origin_task_runner_->PostTask(
+        FROM_HERE, base::Bind(&RunLoop::QuitWhenIdle, Unretained(this)));
+    return;
+  }
+
   quit_when_idle_received_ = true;
 }
 
 base::Closure RunLoop::QuitClosure() {
   // TODO(gab): Fix bad usage and enable this check, http://crbug.com/715235.
   // DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  return base::Bind(&RunLoop::Quit, weak_factory_.GetWeakPtr());
+
+  // Need to use ProxyToTaskRunner() as WeakPtrs vended from
+  // |weak_factory_| may only be accessed on |origin_task_runner_|.
+  // TODO(gab): It feels wrong that QuitClosure() is bound to a WeakPtr.
+  return base::Bind(&ProxyToTaskRunner, origin_task_runner_,
+                    base::Bind(&RunLoop::Quit, weak_factory_.GetWeakPtr()));
 }
 
 base::Closure RunLoop::QuitWhenIdleClosure() {
   // TODO(gab): Fix bad usage and enable this check, http://crbug.com/715235.
   // DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  return base::Bind(&RunLoop::QuitWhenIdle, weak_factory_.GetWeakPtr());
+
+  // Need to use ProxyToTaskRunner() as WeakPtrs vended from
+  // |weak_factory_| may only be accessed on |origin_task_runner_|.
+  // TODO(gab): It feels wrong that QuitWhenIdleClosure() is bound to a WeakPtr.
+  return base::Bind(
+      &ProxyToTaskRunner, origin_task_runner_,
+      base::Bind(&RunLoop::QuitWhenIdle, weak_factory_.GetWeakPtr()));
 }
 
 // static
@@ -175,8 +223,10 @@ void RunLoop::DisallowNestingOnCurrentThread() {
 bool RunLoop::BeforeRun() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
+#if DCHECK_IS_ON()
   DCHECK(!run_called_);
   run_called_ = true;
+#endif  // DCHECK_IS_ON()
 
   // Allow Quit to be called before Run.
   if (quit_called_)