Destroy TestGpuServiceHolder before TaskEnvironment

TestGpuServiceHolder has several threads which pick up a mock clock
created by TaskEnvironment. This mock clock is used by
TestGpuServiceHolder when posting delayed tasks. This CL adds hooks so
that when a TaskEnvironment is non-trivially destroyed (e.g. not as a
result of a move), TestGpuServiceHolder will be notified and tear down
first to avoid data races and potential use after free.

Bug: 1014790
Change-Id: Ibeabf93d66d3f7bb73f2aee085130b599029e3c0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1929531
Commit-Queue: Jonathan Backer <backer@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#718224}

base/test/task_environment.cc

@@ -13,6 +13,7 @@
 #include "base/memory/ptr_util.h"
 #include "base/message_loop/message_pump.h"
 #include "base/message_loop/message_pump_type.h"
+#include "base/no_destructor.h"
 #include "base/run_loop.h"
 #include "base/synchronization/condition_variable.h"
 #include "base/synchronization/lock.h"
@@ -45,6 +46,12 @@ namespace test {
 
 namespace {
 
+ObserverList<TaskEnvironment::DestructionObserver>& GetDestructionObservers() {
+  static NoDestructor<ObserverList<TaskEnvironment::DestructionObserver>>
+      instance;
+  return *instance;
+}
+
 base::MessagePumpType GetMessagePumpTypeForMainThreadType(
     TaskEnvironment::MainThreadType main_thread_type) {
   switch (main_thread_type) {
@@ -471,6 +478,8 @@ TaskEnvironment::~TaskEnvironment() {
   // If we've been moved then bail out.
   if (!owns_instance_)
     return;
+  for (auto& observer : GetDestructionObservers())
+    observer.WillDestroyCurrentTaskEnvironment();
   DestroyThreadPool();
   task_queue_ = nullptr;
   NotifyDestructionObserversAndReleaseSequenceManager();
@@ -717,6 +726,16 @@ void TaskEnvironment::DescribePendingMainThreadTasks() const {
   LOG(INFO) << sequence_manager_->DescribeAllPendingTasks();
 }
 
+// static
+void TaskEnvironment::AddDestructionObserver(DestructionObserver* observer) {
+  GetDestructionObservers().AddObserver(observer);
+}
+
+// static
+void TaskEnvironment::RemoveDestructionObserver(DestructionObserver* observer) {
+  GetDestructionObservers().RemoveObserver(observer);
+}
+
 TaskEnvironment::TestTaskTracker::TestTaskTracker()
     : internal::ThreadPoolImpl::TaskTrackerImpl(std::string()),
       can_run_tasks_cv_(&lock_),

base/test/task_environment.h

@@ -10,6 +10,7 @@
 #include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
+#include "base/observer_list.h"
 #include "base/run_loop.h"
 #include "base/single_thread_task_runner.h"
 #include "base/task/lazy_task_runner.h"
@@ -270,6 +271,23 @@ class TaskEnvironment {
   // thread.
   void DescribePendingMainThreadTasks() const;
 
+  class DestructionObserver : public CheckedObserver {
+   public:
+    DestructionObserver() = default;
+    ~DestructionObserver() override = default;
+
+    DestructionObserver(const DestructionObserver&) = delete;
+    DestructionObserver& operator=(const DestructionObserver&) = delete;
+
+    virtual void WillDestroyCurrentTaskEnvironment() = 0;
+  };
+
+  // Adds/removes a DestructionObserver to any TaskEnvironment. Observers are
+  // notified when any TaskEnvironment goes out of scope (other than with a move
+  // operation). Must be called on the main thread.
+  static void AddDestructionObserver(DestructionObserver* observer);
+  static void RemoveDestructionObserver(DestructionObserver* observer);
+
  protected:
   explicit TaskEnvironment(TaskEnvironment&& other);
 

components/viz/test/test_gpu_service_holder.cc

@@ -15,6 +15,7 @@
 #include "base/no_destructor.h"
 #include "base/synchronization/lock.h"
 #include "base/synchronization/waitable_event.h"
+#include "base/test/task_environment.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "components/viz/service/gl/gpu_service_impl.h"
 #include "gpu/command_buffer/service/service_utils.h"
@@ -48,16 +49,43 @@ TestGpuServiceHolder* g_holder = nullptr;
 bool g_should_register_listener = true;
 bool g_registered_listener = false;
 
-class InstanceResetter : public testing::EmptyTestEventListener {
+class InstanceResetter
+    : public testing::EmptyTestEventListener,
+      public base::test::TaskEnvironment::DestructionObserver {
  public:
-  InstanceResetter() = default;
-  ~InstanceResetter() override = default;
+  InstanceResetter() {
+    base::test::TaskEnvironment::AddDestructionObserver(this);
+  }
+
+  ~InstanceResetter() override {
+    base::test::TaskEnvironment::RemoveDestructionObserver(this);
+  }
 
+  // testing::EmptyTestEventListener:
   void OnTestEnd(const testing::TestInfo& test_info) override {
+    {
+      base::AutoLock locked(GetLock());
+      // Make sure the TestGpuServiceHolder instance is not re-created after
+      // WillDestroyCurrentTaskEnvironment().
+      // Otherwise we'll end up with GPU tasks weirdly running in a different
+      // context after the test.
+      DCHECK(!(reset_by_task_env && g_holder))
+          << "TestGpuServiceHolder was re-created after "
+             "base::test::TaskEnvironment was destroyed.";
+    }
+    reset_by_task_env = false;
+    TestGpuServiceHolder::ResetInstance();
+  }
+
+  // base::test::TaskEnvironment::DestructionObserver:
+  void WillDestroyCurrentTaskEnvironment() override {
+    reset_by_task_env = true;
     TestGpuServiceHolder::ResetInstance();
   }
 
  private:
+  bool reset_by_task_env = false;
+
   DISALLOW_COPY_AND_ASSIGN(InstanceResetter);
 };
 
@@ -74,6 +102,7 @@ TestGpuServiceHolder* TestGpuServiceHolder::GetInstance() {
     g_registered_listener = true;
     testing::TestEventListeners& listeners =
         testing::UnitTest::GetInstance()->listeners();
+    // |listeners| assumes ownership of InstanceResetter.
     listeners.Append(new InstanceResetter);
   }