Add TaskScheduler::JoinForTesting().

base/task_scheduler/scheduler_worker.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/scheduler_worker.h"
 
 #include <stddef.h>
 
 #include <utility>
 
@@ skipping 211 matching lines @@
   if (thread_)
     thread_->WakeUp();
 }
 
 void SchedulerWorker::JoinForTesting() {
   DCHECK(!should_exit_for_testing_.IsSet());
   should_exit_for_testing_.Set();
 
   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;
 
-  thread_.reset();
+  {
+    // Take ownership of |thread_|. This prevents it from detaching.
+    AutoSchedulerLock auto_lock(thread_lock_);
+    thread = std::move(thread_);
+  }
+
+  // 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 {
   AutoSchedulerLock auto_lock(thread_lock_);
   return !!thread_;
 }
 
 SchedulerWorker::SchedulerWorker(ThreadPriority priority_hint,
                                  std::unique_ptr<Delegate> delegate,
                                  TaskTracker* task_tracker)
     : priority_hint_(priority_hint),
       delegate_(std::move(delegate)),
       task_tracker_(task_tracker) {
   DCHECK(delegate_);
   DCHECK(task_tracker_);
 }
 
 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.
   if (thread_->IsWakeUpPending())
     return nullptr;
 
   // Call OnDetach() within the scope of |thread_lock_| to prevent the delegate
   // from being used concurrently from an old and a new thread.
   delegate_->OnDetach();
 
   return std::move(thread_);
 }
 
 void SchedulerWorker::CreateThread() {
   thread_ = Thread::Create(this);
 }
 
 void SchedulerWorker::CreateThreadAssertSynchronized() {
   thread_lock_.AssertAcquired();
   CreateThread();
 }
 
 }  // namespace internal
 }  // namespace base