Try to rule out problems with TaskQueueImpl and TimeDomain destruction

Add a CHECK to assert UnregisterTaskQueue is called before
TaskQueueImpl's destructor and fix a few tests where that wasn't the case.

Change TimeDomain's destructor to assert registered_task_queues_ is
empty and fix a few test where it wasn't.

BUG=

Review URL: https://codereview.chromium.org/1681963002

Cr-Commit-Position: refs/heads/master@{#374390}

6 files changed, 41 insertions, 9 deletions

diff --git a/components/scheduler/base/task_queue_impl.cc b/components/scheduler/base/task_queue_impl.cc
index e013804..5f0b4a1 100644
--- a/components/scheduler/base/task_queue_impl.cc
+++ b/components/scheduler/base/task_queue_impl.cc
@@ -39,9 +39,15 @@ TaskQueueImpl::TaskQueueImpl(
 }
 
 TaskQueueImpl::~TaskQueueImpl() {
+#if DCHECK_IS_ON()
   base::AutoLock lock(any_thread_lock_);
-  if (any_thread().time_domain)
-    any_thread().time_domain->UnregisterQueue(this);
+  // NOTE this check shouldn't fire because |TaskQueueManager::queues_|
+  // contains a strong reference to this TaskQueueImpl and the TaskQueueManager
+  // destructor calls UnregisterTaskQueue on all task queues.
+  DCHECK(any_thread().task_queue_manager == nullptr)
+      << "UnregisterTaskQueue must be called first!";
+
+#endif
 }
 
 TaskQueueImpl::Task::Task()
@@ -109,10 +115,10 @@ TaskQueueImpl::MainThreadOnly::~MainThreadOnly() {}
 
 void TaskQueueImpl::UnregisterTaskQueue() {
   base::AutoLock lock(any_thread_lock_);
-  if (!any_thread().task_queue_manager)
-    return;
   if (main_thread_only().time_domain)
     main_thread_only().time_domain->UnregisterQueue(this);
+  if (!any_thread().task_queue_manager)
+    return;
   any_thread().time_domain = nullptr;
   main_thread_only().time_domain = nullptr;
   any_thread().task_queue_manager->UnregisterTaskQueue(this);
@@ -607,12 +613,19 @@ void TaskQueueImpl::NotifyDidProcessTask(
 
 void TaskQueueImpl::SetTimeDomain(TimeDomain* time_domain) {
   base::AutoLock lock(any_thread_lock_);
+  DCHECK(time_domain);
+  // NOTE this is similar to checking |any_thread().task_queue_manager| but the
+  // TaskQueueSelectorTests constructs TaskQueueImpl directly with a null
+  // task_queue_manager.  Instead we check |any_thread().time_domain| which is
+  // another way of asserting that UnregisterTaskQueue has not been called.
+  DCHECK(any_thread().time_domain);
+  if (!any_thread().time_domain)
+    return;
   DCHECK(main_thread_checker_.CalledOnValidThread());
   if (time_domain == main_thread_only().time_domain)
     return;
 
-  if (time_domain)
-    main_thread_only().time_domain->MigrateQueue(this, time_domain);
+  main_thread_only().time_domain->MigrateQueue(this, time_domain);
   main_thread_only().time_domain = time_domain;
   any_thread().time_domain = time_domain;
 }
diff --git a/components/scheduler/base/task_queue_manager_unittest.cc b/components/scheduler/base/task_queue_manager_unittest.cc
index e944f9a..3da5f0d 100644
--- a/components/scheduler/base/task_queue_manager_unittest.cc
+++ b/components/scheduler/base/task_queue_manager_unittest.cc
@@ -1511,6 +1511,9 @@ TEST_F(TaskQueueManagerTest, TimeDomainsAreIndependant) {
   test_task_runner_->RunUntilIdle();
   EXPECT_THAT(run_order, ElementsAre(4, 5, 6, 1, 2, 3));
 
+  runners_[0]->UnregisterTaskQueue();
+  runners_[1]->UnregisterTaskQueue();
+
   manager_->UnregisterTimeDomain(domain_a.get());
   manager_->UnregisterTimeDomain(domain_b.get());
 }
@@ -1550,6 +1553,8 @@ TEST_F(TaskQueueManagerTest, TimeDomainMigration) {
   test_task_runner_->RunUntilIdle();
   EXPECT_THAT(run_order, ElementsAre(1, 2, 3, 4));
 
+  runners_[0]->UnregisterTaskQueue();
+
   manager_->UnregisterTimeDomain(domain_a.get());
   manager_->UnregisterTimeDomain(domain_b.get());
 }
diff --git a/components/scheduler/base/task_queue_selector_unittest.cc b/components/scheduler/base/task_queue_selector_unittest.cc
index c8aa943..755e97e0 100644
--- a/components/scheduler/base/task_queue_selector_unittest.cc
+++ b/components/scheduler/base/task_queue_selector_unittest.cc
@@ -123,6 +123,12 @@ class TaskQueueSelectorTest : public testing::Test {
     }
   }
 
+  void TearDown() final {
+    for (scoped_refptr<TaskQueueImpl>& task_queue : task_queues_) {
+      task_queue->UnregisterTaskQueue();
+    }
+  }
+
   scoped_refptr<TaskQueueImpl> NewTaskQueueWithBlockReporting() {
     return make_scoped_refptr(new TaskQueueImpl(
         nullptr, virtual_time_domain_.get(),
@@ -382,6 +388,8 @@ TEST_F(TaskQueueSelectorTest, TestObserverWithOneBlockedQueue) {
   WorkQueue* chosen_work_queue;
   EXPECT_CALL(mock_observer, OnTriedToSelectBlockedWorkQueue(_)).Times(1);
   EXPECT_FALSE(selector.SelectWorkQueueToService(&chosen_work_queue));
+
+  task_queue->UnregisterTaskQueue();
 }
 
 TEST_F(TaskQueueSelectorTest, TestObserverWithTwoBlockedQueues) {
@@ -417,6 +425,9 @@ TEST_F(TaskQueueSelectorTest, TestObserverWithTwoBlockedQueues) {
   selector.RemoveQueue(task_queue.get());
   EXPECT_CALL(mock_observer, OnTriedToSelectBlockedWorkQueue(_)).Times(1);
   EXPECT_FALSE(selector.SelectWorkQueueToService(&chosen_work_queue));
+
+  task_queue->UnregisterTaskQueue();
+  task_queue2->UnregisterTaskQueue();
 }
 
 struct ChooseOldestWithPriorityTestParam {
diff --git a/components/scheduler/base/time_domain.cc b/components/scheduler/base/time_domain.cc
index 0ebee4e..6063798 100644
--- a/components/scheduler/base/time_domain.cc
+++ b/components/scheduler/base/time_domain.cc
@@ -17,9 +17,7 @@ TimeDomain::TimeDomain(Observer* observer) : observer_(observer) {}
 
 TimeDomain::~TimeDomain() {
   DCHECK(main_thread_checker_.CalledOnValidThread());
-  for (internal::TaskQueueImpl* queue : registered_task_queues_) {
-    queue->SetTimeDomain(nullptr);
-  }
+  DCHECK(registered_task_queues_.empty());
 }
 
 void TimeDomain::RegisterQueue(internal::TaskQueueImpl* queue) {
diff --git a/components/scheduler/base/time_domain_unittest.cc b/components/scheduler/base/time_domain_unittest.cc
index 85b34b2..c29f0ed 100644
--- a/components/scheduler/base/time_domain_unittest.cc
+++ b/components/scheduler/base/time_domain_unittest.cc
@@ -68,6 +68,10 @@ class TimeDomainTest : public testing::Test {
         "test.category", "test.category"));
   }
 
+  void TearDown() final {
+    task_queue_->UnregisterTaskQueue();
+  }
+
   virtual MockTimeDomain* CreateMockTimeDomain() {
     return new MockTimeDomain(nullptr);
   }
diff --git a/components/scheduler/renderer/auto_advancing_virtual_time_domain_unittest.cc b/components/scheduler/renderer/auto_advancing_virtual_time_domain_unittest.cc
index 62437bb..3a81f9f 100644
--- a/components/scheduler/renderer/auto_advancing_virtual_time_domain_unittest.cc
+++ b/components/scheduler/renderer/auto_advancing_virtual_time_domain_unittest.cc
@@ -41,6 +41,7 @@ class AutoAdvancingVirtualTimeDomainTest : public testing::Test {
   }
 
   void TearDown() override {
+    task_runner_->UnregisterTaskQueue();
     manager_->UnregisterTimeDomain(auto_advancing_time_domain_.get());
   }