Automatically cleanup TestGpuServiceHolder between tests.

There are threading problems if TestGpuServiceHolder is reused between
tests that reset the global time override. There is a race between
reading the current time and base::subtle::ScopedTimeClockOverrides
changing.

Bug: 1000251
Change-Id: I74e7776ea3dc0854d40fcd0fc7bb4bf0ecc5b5f8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1800483
Commit-Queue: kylechar <kylechar@chromium.org>
Reviewed-by: Jonathan Backer <backer@chromium.org>
Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
Cr-Commit-Position: refs/heads/master@{#702443}

cc/test/cc_test_suite.cc

@@ -9,7 +9,6 @@
 #include "base/threading/thread_id_name_manager.h"
 #include "cc/base/histograms.h"
 #include "components/viz/test/paths.h"
-#include "components/viz/test/test_gpu_service_holder.h"
 #include "gpu/ipc/test_gpu_thread_holder.h"
 #include "ui/gl/test/gl_surface_test_support.h"
 
@@ -25,7 +24,6 @@ void CCTestSuite::Initialize() {
   message_loop_ = std::make_unique<base::MessageLoop>();
 
   gl::GLSurfaceTestSupport::InitializeOneOff();
-  viz::TestGpuServiceHolder::DestroyInstanceAfterEachTest();
 
   viz::Paths::RegisterPathProvider();
 

components/exo/test/DEPS

-specific_include_rules = {
-  "run_all_unittests\.cc": [
-    "+components/viz/test/test_gpu_service_holder.h",
-  ],
-}

components/exo/test/run_all_unittests.cc

@@ -4,7 +4,6 @@
 
 #include "base/bind.h"
 #include "base/test/launcher/unit_test_launcher.h"
-#include "components/viz/test/test_gpu_service_holder.h"
 
 #if defined(OS_CHROMEOS)
 #include "ash/test/ash_test_suite.h"
@@ -27,8 +26,6 @@ int main(int argc, char** argv) {
   mojo::core::Init();
 #endif
 
-  viz::TestGpuServiceHolder::DestroyInstanceAfterEachTest();
-
   return base::LaunchUnitTests(
       argc, argv,
       base::BindOnce(&base::TestSuite::Run, base::Unretained(&test_suite)));

components/exo/wayland/clients/test/DEPS

 specific_include_rules = {
   "run_all_client_perftests\.cc": [
+    "+components/viz/test/test_gpu_service_holder.h",
     "+mojo/core/embedder/embedder.h",
   ],
 }

components/exo/wayland/clients/test/run_all_client_perftests.cc

@@ -14,6 +14,7 @@
 #include "base/threading/thread_task_runner_handle.h"
 #include "build/build_config.h"
 #include "components/exo/wayland/clients/test/wayland_client_test.h"
+#include "components/viz/test/test_gpu_service_holder.h"
 #include "mojo/core/embedder/embedder.h"
 
 namespace exo {
@@ -108,6 +109,12 @@ class ExoClientPerfTestSuite : public ash::AshTestSuite {
 int main(int argc, char** argv) {
   mojo::core::Init();
 
+  // The TaskEnvironment and UI thread don't get reset between tests so don't
+  // reset the GPU thread either. Destroying the GPU service is problematic
+  // because tasks on the UI thread can live on past the end of the test and
+  // keep references to GPU thread objects.
+  viz::TestGpuServiceHolder::DoNotResetOnTestExit();
+
   exo::ExoClientPerfTestSuite test_suite(argc, argv);
 
   return base::LaunchUnitTestsSerially(

components/viz/test/test_gpu_service_holder.cc

@@ -43,8 +43,10 @@ base::Lock& GetLock() {
   return *lock;
 }
 
-// We expect |GetLock()| to be acquired before accessing this variable.
+// We expect GetLock() to be acquired before accessing these variables.
 TestGpuServiceHolder* g_holder = nullptr;
+bool g_should_register_listener = true;
+bool g_registered_listener = false;
 
 class InstanceResetter : public testing::EmptyTestEventListener {
  public:
@@ -65,14 +67,22 @@ class InstanceResetter : public testing::EmptyTestEventListener {
 TestGpuServiceHolder* TestGpuServiceHolder::GetInstance() {
   base::AutoLock locked(GetLock());
 
-  // Make sure all TestGpuServiceHolders are deleted at process exit.
+  // Make sure the global TestGpuServiceHolder is delete after each test. The
+  // listener will always be registered with gtest even if gtest isn't
+  // otherwised used. This should do nothing in the non-gtest case.
+  if (!g_registered_listener && g_should_register_listener) {
+    g_registered_listener = true;
+    testing::TestEventListeners& listeners =
+        testing::UnitTest::GetInstance()->listeners();
+    listeners.Append(new InstanceResetter);
+  }
+
+  // Make sure the global TestGpuServiceHolder is deleted at process exit.
   static bool registered_cleanup = false;
   if (!registered_cleanup) {
     registered_cleanup = true;
-    base::AtExitManager::RegisterTask(base::BindOnce([]() {
-      if (g_holder)
-        delete g_holder;
-    }));
+    base::AtExitManager::RegisterTask(
+        base::BindOnce(&TestGpuServiceHolder::ResetInstance));
   }
 
   if (!g_holder) {
@@ -92,14 +102,12 @@ void TestGpuServiceHolder::ResetInstance() {
 }
 
 // static
-void TestGpuServiceHolder::DestroyInstanceAfterEachTest() {
-  static bool registered_listener = false;
-  if (!registered_listener) {
-    registered_listener = true;
-    testing::TestEventListeners& listeners =
-        testing::UnitTest::GetInstance()->listeners();
-    listeners.Append(new InstanceResetter);
-  }
+void TestGpuServiceHolder::DoNotResetOnTestExit() {
+  base::AutoLock locked(GetLock());
+
+  // This must be called before GetInstance() is ever called.
+  DCHECK(!g_registered_listener);
+  g_should_register_listener = false;
 }
 
 TestGpuServiceHolder::TestGpuServiceHolder(

components/viz/test/test_gpu_service_holder.h

@@ -43,17 +43,19 @@ class TestGpuServiceHolder {
   //
   // If specific feature flags or GpuPreferences are needed for a specific test,
   // a separate instance of this class can be created.
+  //
+  // By default the instance created by GetInstance() is destroyed after each
+  // gtest completes -- it only applies to gtest because it uses gtest hooks. If
+  // this isn't desired call DoNotResetOnTestExit() before first use.
   static TestGpuServiceHolder* GetInstance();
 
   // Resets the singleton instance, joining the GL thread. This is useful for
   // tests that individually initialize and tear down GL.
   static void ResetInstance();
 
-  // Calling this method ensures that GetInstance() is destroyed after each
-  // gtest completes -- it only applies to gtest because it uses gtest hooks. A
-  // subsequent call to GetInstance() will create a new instance. Safe to call
-  // more than once.
-  static void DestroyInstanceAfterEachTest();
+  // Don't reset global instance on gtest exit. Must be called before
+  // GetInstance().
+  static void DoNotResetOnTestExit();
 
   explicit TestGpuServiceHolder(const gpu::GpuPreferences& preferences);
   ~TestGpuServiceHolder();

components/viz/test/viz_test_suite.cc

@@ -6,7 +6,6 @@
 
 #include "base/threading/thread_id_name_manager.h"
 #include "components/viz/test/paths.h"
-#include "components/viz/test/test_gpu_service_holder.h"
 #include "ui/gl/test/gl_surface_test_support.h"
 
 namespace viz {
@@ -23,7 +22,6 @@ void VizTestSuite::Initialize() {
       base::test::TaskEnvironment::MainThreadType::UI);
 
   gl::GLSurfaceTestSupport::InitializeOneOff();
-  TestGpuServiceHolder::DestroyInstanceAfterEachTest();
   Paths::RegisterPathProvider();
 
   base::ThreadIdNameManager::GetInstance()->SetName("Main");