Add TaskScheduler::JoinForTesting().

base/task_scheduler/scheduler_worker.cc

Index: base/task_scheduler/scheduler_worker.cc
diff --git a/base/task_scheduler/scheduler_worker.cc b/base/task_scheduler/scheduler_worker.cc
index 970a7d4cf908a9b4aa8249f988e1ebeaed1e41ba..0ed3c96abad51538f47945a3d21f336b251fdc63 100644
--- a/base/task_scheduler/scheduler_worker.cc
+++ b/base/task_scheduler/scheduler_worker.cc
@@ -229,15 +229,17 @@ void SchedulerWorker::JoinForTesting() {
 
   WakeUp();
 
-  // Normally holding a lock and joining is dangerous. However, since this is
-  // only for testing, we're okay since the only scenario that could impact this
-  // is a call to Detach, which is disallowed by having the delegate always
-  // return false for the CanDetach call.
-  AutoSchedulerLock auto_lock(thread_lock_);
-  if (thread_)
-    thread_->Join();
+  std::unique_ptr<Thread> thread;
+
+  {
+    // Take ownership of |thread_|. This prevents it from detaching.
+    AutoSchedulerLock auto_lock(thread_lock_);
+    thread = std::move(thread_);
+  }
 
-  thread_.reset();
+  // Join outside the lock.

[robliao, 2017/01/17 19:01:27]

Nit: Remove this comment as it is self evident.

[fdoray, 2017/01/17 20:43:13]

Done.

+  if (thread)
+    thread->Join();

[robliao, 2017/01/17 19:01:27]

Previously, JoinForTesting holding the lock would ensure that thread_ was valid
for the entirety of the join.

Now, WakeUp could plausibly be called during the join, creating a new thread in
the process. Seems like WakeUp should check should_exit_for_testing_? 

[fdoray, 2017/01/17 20:43:13]

Done.

 }
 
 bool SchedulerWorker::ThreadAliveForTesting() const {
@@ -256,8 +258,14 @@ SchedulerWorker::SchedulerWorker(ThreadPriority priority_hint,
 }
 
 std::unique_ptr<SchedulerWorker::Thread> SchedulerWorker::Detach() {
-  DCHECK(!should_exit_for_testing_.IsSet()) << "Worker was already joined";
   AutoSchedulerLock auto_lock(thread_lock_);
+
+  // Do not detach if the thread is being joined.
+  if (!thread_) {
+    DCHECK(should_exit_for_testing_.IsSet());
+    return nullptr;
+  }
+
   // If a wakeup is pending, then a WakeUp() came in while we were deciding to
   // detach. This means we can't go away anymore since we would break the
   // guarantee that we call GetWork() after a successful wakeup.