SchedulerWorker Refcounting for Destruction in Production

base/task_scheduler/scheduler_worker_unittest.cc

Index: base/task_scheduler/scheduler_worker_unittest.cc
diff --git a/base/task_scheduler/scheduler_worker_unittest.cc b/base/task_scheduler/scheduler_worker_unittest.cc
index b65d50c07c3fb5db64ba8df08031bc6934ef10a5..4682368e7fe8c86deb99b9ea30c52f1448b4e547 100644
--- a/base/task_scheduler/scheduler_worker_unittest.cc
+++ b/base/task_scheduler/scheduler_worker_unittest.cc
@@ -13,13 +13,16 @@
 #include "base/bind_helpers.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
+#include "base/memory/ref_counted.h"
 #include "base/synchronization/condition_variable.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/task_scheduler/scheduler_lock.h"
 #include "base/task_scheduler/sequence.h"
 #include "base/task_scheduler/task.h"
 #include "base/task_scheduler/task_tracker.h"
+#include "base/test/test_timeouts.h"
 #include "base/threading/platform_thread.h"
+#include "base/threading/simple_thread.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -121,7 +124,7 @@ class TaskSchedulerWorkerTest : public testing::TestWithParam<size_t> {
     return re_enqueued_sequences_;
   }
 
-  std::unique_ptr<SchedulerWorker> worker_;
+  scoped_refptr<SchedulerWorker> worker_;
 
  private:
   class TestSchedulerWorkerDelegate : public SchedulerWorkerDefaultDelegate {
@@ -355,34 +358,92 @@ namespace {
 
 class ControllableDetachDelegate : public SchedulerWorkerDefaultDelegate {
  public:
-  ControllableDetachDelegate(TaskTracker* task_tracker)
-      : task_tracker_(task_tracker),
-        work_processed_(WaitableEvent::ResetPolicy::MANUAL,
-                        WaitableEvent::InitialState::NOT_SIGNALED),
-        detach_requested_(WaitableEvent::ResetPolicy::MANUAL,
+  class Controls : public RefCountedThreadSafe<Controls> {
+   public:
+    Controls()
+        : work_running_(WaitableEvent::ResetPolicy::MANUAL,
+                        WaitableEvent::InitialState::SIGNALED),
+          work_processed_(WaitableEvent::ResetPolicy::MANUAL,
                           WaitableEvent::InitialState::NOT_SIGNALED),
-        detached_(WaitableEvent::ResetPolicy::MANUAL,
-                  WaitableEvent::InitialState::NOT_SIGNALED) {
-    EXPECT_TRUE(task_tracker_);
-  }
+          detach_requested_(WaitableEvent::ResetPolicy::MANUAL,
+                            WaitableEvent::InitialState::NOT_SIGNALED),
+          detached_(WaitableEvent::ResetPolicy::MANUAL,
+                    WaitableEvent::InitialState::NOT_SIGNALED),
+          can_detach_block_(WaitableEvent::ResetPolicy::MANUAL,
+                            WaitableEvent::InitialState::SIGNALED),
+          destroyed_(WaitableEvent::ResetPolicy::MANUAL,
+                     WaitableEvent::InitialState::NOT_SIGNALED) {}
 
-  ~ControllableDetachDelegate() override = default;
+    void HaveWorkBlock() { work_running_.Reset(); }
 
-  // SchedulerWorker::Delegate:
-  MOCK_METHOD1(OnMainEntry, void(SchedulerWorker* worker));
+    void UnblockWork() { work_running_.Signal(); }
+
+    void MakeCanDetachBlock() { can_detach_block_.Reset(); }
+
+    void UnblockCanDetach() { can_detach_block_.Signal(); }
+
+    void WaitForWorkToRun() { work_processed_.Wait(); }
+
+    void WaitForDetachRequest() { detach_requested_.Wait(); }
+
+    void WaitForDetach() { detached_.Wait(); }
+
+    void WaitForDelegateDestroy() { destroyed_.Wait(); }
+
+    void ResetState() {
+      work_running_.Signal();
+      work_processed_.Reset();
+      detach_requested_.Reset();
+      can_detach_block_.Signal();
+      work_requested_ = false;
+    }
+
+    void set_can_detach(bool can_detach) { can_detach_ = can_detach; }
+
+   private:
+    friend class ControllableDetachDelegate;
+    friend class RefCountedThreadSafe<Controls>;
+    ~Controls() = default;
+
+    WaitableEvent work_running_;
+    WaitableEvent work_processed_;
+    WaitableEvent detach_requested_;
+    WaitableEvent detached_;
+    WaitableEvent can_detach_block_;
+    WaitableEvent destroyed_;
+
+    bool can_detach_ = false;
+    bool work_requested_ = false;
+
+    DISALLOW_COPY_AND_ASSIGN(Controls);
+  };
+
+  ControllableDetachDelegate(TaskTracker* task_tracker)
+      : task_tracker_(task_tracker), controls_(new Controls()) {}
+
+  ~ControllableDetachDelegate() override { controls_->destroyed_.Signal(); }
 
   scoped_refptr<Sequence> GetWork(SchedulerWorker* worker)
       override {
     // Sends one item of work to signal |work_processed_|. On subsequent calls,
     // sends nullptr to indicate there's no more work to be done.
-    if (work_requested_)
+    if (controls_->work_requested_)
       return nullptr;
 
-    work_requested_ = true;
+    controls_->work_requested_ = true;
     scoped_refptr<Sequence> sequence(new Sequence);
     std::unique_ptr<Task> task(new Task(
-        FROM_HERE, Bind(&WaitableEvent::Signal, Unretained(&work_processed_)),
-        TaskTraits(), TimeDelta()));
+        FROM_HERE,
+        Bind(
+            [](WaitableEvent* work_processed, WaitableEvent* work_running) {
+              work_processed->Signal();
+              work_running->Wait();
+            },
+            Unretained(&controls_->work_processed_),
+            Unretained(&controls_->work_running_)),
+        TaskTraits().WithBaseSyncPrimitives().WithShutdownBehavior(
+            TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN),
+        TimeDelta()));
     EXPECT_TRUE(task_tracker_->WillPostTask(task.get()));
     sequence->PushTask(std::move(task));
     return sequence;
@@ -391,43 +452,42 @@ class ControllableDetachDelegate : public SchedulerWorkerDefaultDelegate {
   void DidRunTask() override {}
 
   bool CanDetach(SchedulerWorker* worker) override {
-    detach_requested_.Signal();
-    return can_detach_;
+    // Saving |can_detach_| now so that callers waiting on |detach_requested_|
+    // have the thread go to sleep and then allow detachment.
+    bool can_detach = controls_->can_detach_;
+    controls_->detach_requested_.Signal();
+    controls_->can_detach_block_.Wait();
+    return can_detach;
   }
 
   void OnDetach() override {
-    EXPECT_TRUE(can_detach_);
-    EXPECT_TRUE(detach_requested_.IsSignaled());
-    detached_.Signal();
+    EXPECT_TRUE(controls_->can_detach_);
+    EXPECT_TRUE(controls_->detach_requested_.IsSignaled());
+    controls_->detached_.Signal();
   }
 
-  void WaitForWorkToRun() {
-    work_processed_.Wait();
-  }
+  // ControllableDetachDelegate:

[gab, 2017/02/21 19:01:04]

This usually means that it implements that interface which isn't the case here.

[robliao, 2017/02/21 22:26:57]

This was habit for me working in C++ interfaces for me so you could identify
which methods aren't part of any interface. Otherwise they all blend together
and you have no idea!

Any strong preference to removing this? There doesn't appear to be a style
guideline either way.

+  scoped_refptr<Controls> controls() { return controls_; }

[gab, 2017/02/21 19:01:04]

Does this need to expose the ref (usage seems to non-owning?). If not, can the
whole class not be refcounted?

[robliao, 2017/02/21 22:26:57]

This needs to be owned for cases where we race between Join/Shutdown and
Cleanup. In these cases, the delegate can be destroyed at anytime. Refcounting
the controls addresses this and is robust to whatever happens to the delegate.

 
-  void WaitForDetachRequest() {
-    detach_requested_.Wait();
-  }
+ private:
+  scoped_refptr<Sequence> work_sequence_;
+  TaskTracker* const task_tracker_;
+  scoped_refptr<Controls> controls_;
 
-  void WaitForDetach() { detached_.Wait(); }
+  DISALLOW_COPY_AND_ASSIGN(ControllableDetachDelegate);
+};
 
-  void ResetState() {
-    work_requested_ = false;
-    work_processed_.Reset();
-    detach_requested_.Reset();
-  }
+class MockedControllableDetachDelegate : public ControllableDetachDelegate {
+ public:
+  MockedControllableDetachDelegate(TaskTracker* task_tracker)
+      : ControllableDetachDelegate(task_tracker){};
+  ~MockedControllableDetachDelegate() = default;
 
-  void set_can_detach(bool can_detach) { can_detach_ = can_detach; }
+  // SchedulerWorker::Delegate:
+  MOCK_METHOD1(OnMainEntry, void(SchedulerWorker* worker));
 
  private:
-  TaskTracker* const task_tracker_;
-  bool work_requested_ = false;
-  bool can_detach_ = false;
-  WaitableEvent work_processed_;
-  WaitableEvent detach_requested_;
-  WaitableEvent detached_;
-
-  DISALLOW_COPY_AND_ASSIGN(ControllableDetachDelegate);
+  DISALLOW_COPY_AND_ASSIGN(MockedControllableDetachDelegate);
 };
 
 }  // namespace
@@ -435,50 +495,224 @@ class ControllableDetachDelegate : public SchedulerWorkerDefaultDelegate {
 TEST(TaskSchedulerWorkerTest, WorkerDetaches) {
   TaskTracker task_tracker;
   // Will be owned by SchedulerWorker.
-  ControllableDetachDelegate* delegate =
-      new StrictMock<ControllableDetachDelegate>(&task_tracker);
-  delegate->set_can_detach(true);
+  MockedControllableDetachDelegate* delegate =
+      new StrictMock<MockedControllableDetachDelegate>(&task_tracker);
+  scoped_refptr<ControllableDetachDelegate::Controls> controls =
+      delegate->controls();
+  controls->set_can_detach(true);
   EXPECT_CALL(*delegate, OnMainEntry(_));
-  std::unique_ptr<SchedulerWorker> worker =
-      SchedulerWorker::Create(
-          ThreadPriority::NORMAL, WrapUnique(delegate), &task_tracker,
-          SchedulerWorker::InitialState::ALIVE);
+  scoped_refptr<SchedulerWorker> worker = SchedulerWorker::Create(
+      ThreadPriority::NORMAL, WrapUnique(delegate), &task_tracker,
+      SchedulerWorker::InitialState::ALIVE);
   worker->WakeUp();
-  delegate->WaitForWorkToRun();
+  controls->WaitForWorkToRun();
   Mock::VerifyAndClear(delegate);
-  delegate->WaitForDetachRequest();
-  delegate->WaitForDetach();
+  controls->WaitForDetachRequest();
+  controls->WaitForDetach();
   ASSERT_FALSE(worker->ThreadAliveForTesting());
 }
 
+TEST(TaskSchedulerWorkerTest, WorkerReleasesBeforeDetach) {
+  TaskTracker task_tracker;
+  // Will be owned by SchedulerWorker.
+  // No mock here as that's reasonably covered by other tests and the delegate
+  // may destroy on a different thread. Mocks aren't designed with that in mind.
+  std::unique_ptr<ControllableDetachDelegate> delegate =
+      MakeUnique<ControllableDetachDelegate>(&task_tracker);
+  scoped_refptr<ControllableDetachDelegate::Controls> controls =
+      delegate->controls();
+
+  controls->set_can_detach(true);
+  controls->MakeCanDetachBlock();
+
+  scoped_refptr<SchedulerWorker> worker = SchedulerWorker::Create(
+      ThreadPriority::NORMAL, std::move(delegate), &task_tracker,
+      SchedulerWorker::InitialState::ALIVE);
+  worker->WakeUp();
+
+  controls->WaitForDetachRequest();
+  worker->Cleanup();
+  worker = nullptr;
+  controls->UnblockCanDetach();
+  controls->WaitForDelegateDestroy();
+}
+
+TEST(TaskSchedulerWorkerTest, WorkerDetachesAndSelfDestroysAfterDetach) {
+  TaskTracker task_tracker;
+  // Will be owned by SchedulerWorker.
+  // No mock here as that's reasonably covered by other tests and the delegate
+  // may destroy on a different thread. Mocks aren't designed with that in mind.
+  std::unique_ptr<ControllableDetachDelegate> delegate =
+      MakeUnique<ControllableDetachDelegate>(&task_tracker);
+  scoped_refptr<ControllableDetachDelegate::Controls> controls =
+      delegate->controls();
+
+  controls->set_can_detach(true);
+
+  scoped_refptr<SchedulerWorker> worker = SchedulerWorker::Create(
+      ThreadPriority::NORMAL, std::move(delegate), &task_tracker,
+      SchedulerWorker::InitialState::ALIVE);
+  worker->WakeUp();
+
+  controls->WaitForDetach();
+  worker->Cleanup();
+  worker = nullptr;
+  controls->WaitForDelegateDestroy();
+}
+
+TEST(TaskSchedulerWorkerTest, WorkerDetachesAndSelfDestroysDuringWork) {
+  TaskTracker task_tracker;
+  // Will be owned by SchedulerWorker.
+  // No mock here as that's reasonably covered by other tests and the delegate
+  // may destroy on a different thread. Mocks aren't designed with that in mind.
+  std::unique_ptr<ControllableDetachDelegate> delegate =
+      MakeUnique<ControllableDetachDelegate>(&task_tracker);
+  scoped_refptr<ControllableDetachDelegate::Controls> controls =
+      delegate->controls();
+
+  controls->set_can_detach(true);
+  controls->HaveWorkBlock();
+
+  scoped_refptr<SchedulerWorker> worker = SchedulerWorker::Create(
+      ThreadPriority::NORMAL, std::move(delegate), &task_tracker,
+      SchedulerWorker::InitialState::ALIVE);
+  worker->WakeUp();
+
+  controls->WaitForWorkToRun();
+  worker->Cleanup();
+  worker = nullptr;
+  controls->UnblockWork();
+  controls->WaitForDelegateDestroy();
+}
+
+TEST(TaskSchedulerWorkerTest, WorkerDetachesAndSelfDestroysDuringWait) {
+  TaskTracker task_tracker;
+  // Will be owned by SchedulerWorker.
+  // No mock here as that's reasonably covered by other tests and the delegate
+  // may destroy on a different thread. Mocks aren't designed with that in mind.
+  std::unique_ptr<ControllableDetachDelegate> delegate =
+      MakeUnique<ControllableDetachDelegate>(&task_tracker);
+  scoped_refptr<ControllableDetachDelegate::Controls> controls =
+      delegate->controls();
+
+  scoped_refptr<SchedulerWorker> worker = SchedulerWorker::Create(
+      ThreadPriority::NORMAL, std::move(delegate), &task_tracker,
+      SchedulerWorker::InitialState::ALIVE);
+  worker->WakeUp();
+
+  controls->WaitForDetachRequest();
+  controls->set_can_detach(true);

[gab, 2017/02/21 19:01:04]

What's the point of setting it after? (I guess I also don't understand why
|can_detach_| is saved before the wait and used as the return value?)

[robliao, 2017/02/21 22:26:57]

In this case, we just want to wait for a CanDetach() serviced at least once with
CanDetach() => false, forcing us to go into the Wait (there isn't a Wait
callback (coming attractions for COM).

+  worker->Cleanup();
+  worker = nullptr;
+  controls->WaitForDelegateDestroy();
+}
+
+TEST(TaskSchedulerWorkerTest, WorkerDetachesAndSelfDestroysDuringShutdown) {
+  TaskTracker task_tracker;
+  // Will be owned by SchedulerWorker.
+  // No mock here as that's reasonably covered by other tests and the delegate
+  // may destroy on a different thread. Mocks aren't designed with that in mind.
+  std::unique_ptr<ControllableDetachDelegate> delegate =
+      MakeUnique<ControllableDetachDelegate>(&task_tracker);
+  scoped_refptr<ControllableDetachDelegate::Controls> controls =
+      delegate->controls();
+
+  controls->HaveWorkBlock();
+
+  scoped_refptr<SchedulerWorker> worker = SchedulerWorker::Create(
+      ThreadPriority::NORMAL, std::move(delegate), &task_tracker,
+      SchedulerWorker::InitialState::ALIVE);
+  worker->WakeUp();
+
+  controls->WaitForWorkToRun();
+  task_tracker.Shutdown();
+  worker->Cleanup();
+  worker = nullptr;
+  controls->UnblockWork();
+  controls->WaitForDelegateDestroy();
+}
+
+namespace {
+
+class CallJoinFromDifferentThread : public SimpleThread {
+ public:
+  CallJoinFromDifferentThread(SchedulerWorker* worker_to_join)
+      : SimpleThread("SchedulerWorkerJoinThread"),
+        worker_to_join_(worker_to_join) {}
+
+  ~CallJoinFromDifferentThread() override = default;
+
+  void Run() override { worker_to_join_->JoinForTesting(); }
+
+ private:
+  SchedulerWorker* const worker_to_join_;
+  DISALLOW_COPY_AND_ASSIGN(CallJoinFromDifferentThread);
+};
+
+}  // namespace
+
+TEST(TaskSchedulerWorkerTest, WorkerDetachesAndSelfDestroysDuringJoin) {
+  TaskTracker task_tracker;
+  // Will be owned by SchedulerWorker.
+  // No mock here as that's reasonably covered by other tests and the
+  // delegate may destroy on a different thread. Mocks aren't designed with that
+  // in mind.
+  std::unique_ptr<ControllableDetachDelegate> delegate =
+      MakeUnique<ControllableDetachDelegate>(&task_tracker);
+  scoped_refptr<ControllableDetachDelegate::Controls> controls =
+      delegate->controls();
+
+  controls->HaveWorkBlock();
+
+  scoped_refptr<SchedulerWorker> worker = SchedulerWorker::Create(
+      ThreadPriority::NORMAL, std::move(delegate), &task_tracker,
+      SchedulerWorker::InitialState::ALIVE);
+  worker->WakeUp();
+
+  controls->WaitForWorkToRun();
+  CallJoinFromDifferentThread join_from_different_thread(worker.get());
+  join_from_different_thread.Start();
+  // While Start blocks until the thread starts, that doesn't cover the actual

[gab, 2017/02/21 19:01:04]

Start no longer blocks until thread starts (Francois just changed that -- or did
that land? anyways it's coming!)

[robliao, 2017/02/21 22:26:57]

SimpleThread::Start() waits.

Thread::Start() doesn't wait. 

[gab, 2017/02/22 18:02:24]

Ah, found it: https://codereview.chromium.org/2664953004/ (was reverted :( ).
SimpleThread::Start() shouldn't block either (and the reason is exactly as your
comment here describes -- that serves no purpose as it doesn't guarantee Run()
was invoked when it unblocks).

Hence I'd prefer to rephrase this comment to avoid making an explicit statement
about SimpleThread::Start()'s behavior. Perhaps just:

Why not add an event that is signaled by the worker thread right before it calls
JoinForTesting()? That explicitly highlights what you want (have a race in this
test which the impl has to handle).

[robliao, 2017/02/22 20:43:39]

Went with the event. Amended comment.

+  // join call. Yield here to give the other thread a chance to call
+  // JoinForTesting().
+  PlatformThread::YieldCurrentThread();
+  worker->Cleanup();
+  worker = nullptr;
+  controls->UnblockWork();
+  controls->WaitForDelegateDestroy();
+  join_from_different_thread.Join();
+}
+
 TEST(TaskSchedulerWorkerTest, WorkerDetachesAndWakes) {
   TaskTracker task_tracker;
   // Will be owned by SchedulerWorker.
-  ControllableDetachDelegate* delegate =
-      new StrictMock<ControllableDetachDelegate>(&task_tracker);
-  delegate->set_can_detach(true);
+  MockedControllableDetachDelegate* delegate =
+      new StrictMock<MockedControllableDetachDelegate>(&task_tracker);
+  scoped_refptr<ControllableDetachDelegate::Controls> controls =
+      delegate->controls();
+
+  controls->set_can_detach(true);
   EXPECT_CALL(*delegate, OnMainEntry(_));
-  std::unique_ptr<SchedulerWorker> worker =
-      SchedulerWorker::Create(
-          ThreadPriority::NORMAL, WrapUnique(delegate), &task_tracker,
-          SchedulerWorker::InitialState::ALIVE);
+  scoped_refptr<SchedulerWorker> worker = SchedulerWorker::Create(
+      ThreadPriority::NORMAL, WrapUnique(delegate), &task_tracker,
+      SchedulerWorker::InitialState::ALIVE);
   worker->WakeUp();
-  delegate->WaitForWorkToRun();
+  controls->WaitForWorkToRun();
   Mock::VerifyAndClear(delegate);
-  delegate->WaitForDetachRequest();
-  delegate->WaitForDetach();
+  controls->WaitForDetachRequest();
+  controls->WaitForDetach();
   ASSERT_FALSE(worker->ThreadAliveForTesting());
 
-  delegate->ResetState();
-  delegate->set_can_detach(false);
+  controls->ResetState();
+  controls->set_can_detach(false);
   // Expect OnMainEntry() to be called when SchedulerWorker recreates its
   // thread.
   EXPECT_CALL(*delegate, OnMainEntry(worker.get()));
   worker->WakeUp();
-  delegate->WaitForWorkToRun();
+  controls->WaitForWorkToRun();
   Mock::VerifyAndClear(delegate);
-  delegate->WaitForDetachRequest();
-  delegate->WaitForDetach();
+  controls->WaitForDetachRequest();
+  controls->WaitForDetach();
   ASSERT_TRUE(worker->ThreadAliveForTesting());
   worker->JoinForTesting();
 }
@@ -486,18 +720,19 @@ TEST(TaskSchedulerWorkerTest, WorkerDetachesAndWakes) {
 TEST(TaskSchedulerWorkerTest, CreateDetached) {
   TaskTracker task_tracker;
   // Will be owned by SchedulerWorker.
-  ControllableDetachDelegate* delegate =
-      new StrictMock<ControllableDetachDelegate>(&task_tracker);
-  std::unique_ptr<SchedulerWorker> worker =
-      SchedulerWorker::Create(
-          ThreadPriority::NORMAL, WrapUnique(delegate), &task_tracker,
-          SchedulerWorker::InitialState::DETACHED);
+  MockedControllableDetachDelegate* delegate =
+      new StrictMock<MockedControllableDetachDelegate>(&task_tracker);
+  scoped_refptr<ControllableDetachDelegate::Controls> controls =
+      delegate->controls();
+  scoped_refptr<SchedulerWorker> worker = SchedulerWorker::Create(
+      ThreadPriority::NORMAL, WrapUnique(delegate), &task_tracker,
+      SchedulerWorker::InitialState::DETACHED);
   ASSERT_FALSE(worker->ThreadAliveForTesting());
   EXPECT_CALL(*delegate, OnMainEntry(worker.get()));
   worker->WakeUp();
-  delegate->WaitForWorkToRun();
+  controls->WaitForWorkToRun();
   Mock::VerifyAndClear(delegate);
-  delegate->WaitForDetachRequest();
+  controls->WaitForDetachRequest();
   ASSERT_TRUE(worker->ThreadAliveForTesting());
   worker->JoinForTesting();
 }
@@ -560,7 +795,7 @@ TEST(TaskSchedulerWorkerTest, BumpPriorityOfAliveThreadDuringShutdown) {
           ? ThreadPriority::BACKGROUND
           : ThreadPriority::NORMAL);
 
-  std::unique_ptr<SchedulerWorker> worker = SchedulerWorker::Create(
+  scoped_refptr<SchedulerWorker> worker = SchedulerWorker::Create(
       ThreadPriority::BACKGROUND, std::move(delegate), &task_tracker,
       SchedulerWorker::InitialState::ALIVE);
 
@@ -587,7 +822,7 @@ TEST(TaskSchedulerWorkerTest, BumpPriorityOfDetachedThreadDuringShutdown) {
   delegate_raw->SetExpectedThreadPriority(ThreadPriority::NORMAL);
 
   // Create a DETACHED thread.
-  std::unique_ptr<SchedulerWorker> worker = SchedulerWorker::Create(
+  scoped_refptr<SchedulerWorker> worker = SchedulerWorker::Create(
       ThreadPriority::BACKGROUND, std::move(delegate), &task_tracker,
       SchedulerWorker::InitialState::DETACHED);