Check that TaskScheduler is not leaked between tests, or test-cases.

This is a reland of a2a4e75d with a
work-around for cronet_tests' sharing libbase.so with libcronet.so in
component builds.

- Move some APIs off of base::TestSuite, that we unnecessarily public.
- Rename the various internal TestEventListeners to express purpose.
- Add CheckForLeakedGlobals check, and DisableCheckForLeakedGlobals API.
  The latter is called by the content::ContentTestSuiteBase, since the
  various browser- and ui-tests running under that suite base run more
  like the browser itself, and so are expected to let global singletons
  leak.

If tests fail or flake due to this patch, then this indicates that the
test is actually leaking global state that may break other tests if they
happen to run in the same process, leading to hard-to-diagnose flakes.

There are two main failure cases:
If the test directly or indirectly uses TaskScheduler, but does not
create a ScopedTaskEnvironment, then please add one to the test, or to
the test base-class, as appropriate, to fix it.
If the test suite cannot be fixed in this way then please add a call to
DisableCheckForLeakedGlobals(), with a comment linking to a bug for the
issue.

TBR: gab, sky, mef
Bug: 875486, 877355, 744567
Change-Id: Iaea38d24ede271c248a3abb0b3f7ee931c2538f5
Reviewed-on: https://chromium-review.googlesource.com/1187783Reviewed-by: Wez <wez@chromium.org>
Commit-Queue: Wez <wez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586065}

base/test/test_suite.cc

@@ -26,6 +26,7 @@
 #include "base/path_service.h"
 #include "base/process/launch.h"
 #include "base/process/memory.h"
+#include "base/task/task_scheduler/task_scheduler.h"
 #include "base/test/gtest_xml_unittest_result_printer.h"
 #include "base/test/gtest_xml_util.h"
 #include "base/test/icu_test_util.h"
@@ -70,21 +71,26 @@ namespace base {
 
 namespace {
 
-class MaybeTestDisabler : public testing::EmptyTestEventListener {
+// Returns true if the test is marked as "MAYBE_".
+// When using different prefixes depending on platform, we use MAYBE_ and
+// preprocessor directives to replace MAYBE_ with the target prefix.
+bool IsMarkedMaybe(const testing::TestInfo& test) {
+  return strncmp(test.name(), "MAYBE_", 6) == 0;
+}
+
+class DisableMaybeTests : public testing::EmptyTestEventListener {
  public:
   void OnTestStart(const testing::TestInfo& test_info) override {
-    ASSERT_FALSE(TestSuite::IsMarkedMaybe(test_info))
+    ASSERT_FALSE(IsMarkedMaybe(test_info))
         << "Probably the OS #ifdefs don't include all of the necessary "
            "platforms.\nPlease ensure that no tests have the MAYBE_ prefix "
            "after the code is preprocessed.";
   }
 };
 
-class TestClientInitializer : public testing::EmptyTestEventListener {
+class ResetCommandLineBetweenTests : public testing::EmptyTestEventListener {
  public:
-  TestClientInitializer()
-      : old_command_line_(CommandLine::NO_PROGRAM) {
-  }
+  ResetCommandLineBetweenTests() : old_command_line_(CommandLine::NO_PROGRAM) {}
 
   void OnTestStart(const testing::TestInfo& test_info) override {
     old_command_line_ = *CommandLine::ForCurrentProcess();
@@ -97,7 +103,36 @@ class TestClientInitializer : public testing::EmptyTestEventListener {
  private:
   CommandLine old_command_line_;
 
-  DISALLOW_COPY_AND_ASSIGN(TestClientInitializer);
+  DISALLOW_COPY_AND_ASSIGN(ResetCommandLineBetweenTests);
+};
+
+class CheckForLeakedGlobals : public testing::EmptyTestEventListener {
+ public:
+  CheckForLeakedGlobals() = default;
+
+  // Check for leaks in individual tests.
+  void OnTestStart(const testing::TestInfo& test) override {
+    scheduler_set_before_test_ = TaskScheduler::GetInstance();
+  }
+  void OnTestEnd(const testing::TestInfo& test) override {
+    DCHECK_EQ(scheduler_set_before_test_, TaskScheduler::GetInstance())
+        << " in test " << test.test_case_name() << "." << test.name();
+  }
+
+  // Check for leaks in test cases (consisting of one or more tests).
+  void OnTestCaseStart(const testing::TestCase& test_case) override {
+    scheduler_set_before_case_ = TaskScheduler::GetInstance();
+  }
+  void OnTestCaseEnd(const testing::TestCase& test_case) override {
+    DCHECK_EQ(scheduler_set_before_case_, TaskScheduler::GetInstance())
+        << " in case " << test_case.name();
+  }
+
+ private:
+  TaskScheduler* scheduler_set_before_test_ = nullptr;
+  TaskScheduler* scheduler_set_before_case_ = nullptr;
+
+  DISALLOW_COPY_AND_ASSIGN(CheckForLeakedGlobals);
 };
 
 std::string GetProfileName() {
@@ -140,7 +175,7 @@ int RunUnitTestsUsingBaseTestSuite(int argc, char **argv) {
                          Bind(&TestSuite::Run, Unretained(&test_suite)));
 }
 
-TestSuite::TestSuite(int argc, char** argv) : initialized_command_line_(false) {
+TestSuite::TestSuite(int argc, char** argv) {
   PreInitialize();
   InitializeFromCommandLine(argc, argv);
   // Logging must be initialized before any thread has a chance to call logging
@@ -149,8 +184,7 @@ TestSuite::TestSuite(int argc, char** argv) : initialized_command_line_(false) {
 }
 
 #if defined(OS_WIN)
-TestSuite::TestSuite(int argc, wchar_t** argv)
-    : initialized_command_line_(false) {
+TestSuite::TestSuite(int argc, wchar_t** argv) {
   PreInitialize();
   InitializeFromCommandLine(argc, argv);
   // Logging must be initialized before any thread has a chance to call logging
@@ -184,6 +218,8 @@ void TestSuite::InitializeFromCommandLine(int argc, wchar_t** argv) {
 #endif  // defined(OS_WIN)
 
 void TestSuite::PreInitialize() {
+  DCHECK(!is_initialized_);
+
 #if defined(OS_WIN)
   testing::GTEST_FLAG(catch_exceptions) = false;
 #endif
@@ -205,24 +241,6 @@ void TestSuite::PreInitialize() {
   // Initialize().  See bug 6436.
 }
 
-
-// static
-bool TestSuite::IsMarkedMaybe(const testing::TestInfo& test) {
-  return strncmp(test.name(), "MAYBE_", 6) == 0;
-}
-
-void TestSuite::CatchMaybeTests() {
-  testing::TestEventListeners& listeners =
-      testing::UnitTest::GetInstance()->listeners();
-  listeners.Append(new MaybeTestDisabler);
-}
-
-void TestSuite::ResetCommandLine() {
-  testing::TestEventListeners& listeners =
-      testing::UnitTest::GetInstance()->listeners();
-  listeners.Append(new TestClientInitializer);
-}
-
 void TestSuite::AddTestLauncherResultPrinter() {
   // Only add the custom printer if requested.
   if (!CommandLine::ForCurrentProcess()->HasSwitch(
@@ -288,6 +306,11 @@ int TestSuite::Run() {
   return result;
 }
 
+void TestSuite::DisableCheckForLeakedGlobals() {
+  DCHECK(!is_initialized_);
+  check_for_leaked_globals_ = false;
+}
+
 void TestSuite::UnitTestAssertHandler(const char* file,
                                       int line,
                                       const base::StringPiece summary,
@@ -384,6 +407,8 @@ void TestSuite::SuppressErrorDialogs() {
 }
 
 void TestSuite::Initialize() {
+  DCHECK(!is_initialized_);
+
   const CommandLine* command_line = CommandLine::ForCurrentProcess();
 #if !defined(OS_IOS)
   if (command_line->HasSwitch(switches::kWaitForDebugger)) {
@@ -468,8 +493,14 @@ void TestSuite::Initialize() {
   SetUpFontconfig();
 #endif
 
-  CatchMaybeTests();
-  ResetCommandLine();
+  // Add TestEventListeners to enforce certain properties across tests.
+  testing::TestEventListeners& listeners =
+      testing::UnitTest::GetInstance()->listeners();
+  listeners.Append(new DisableMaybeTests);
+  listeners.Append(new ResetCommandLineBetweenTests);
+  if (check_for_leaked_globals_)
+    listeners.Append(new CheckForLeakedGlobals);
+
   AddTestLauncherResultPrinter();
 
   TestTimeouts::Initialize();
@@ -477,9 +508,12 @@ void TestSuite::Initialize() {
   trace_to_file_.BeginTracingFromCommandLineOptions();
 
   base::debug::StartProfiling(GetProfileName());
+
+  is_initialized_ = true;
 }
 
 void TestSuite::Shutdown() {
+  DCHECK(is_initialized_);
   base::debug::StopProfiling();
 }
 

base/test/test_suite.h

@@ -41,19 +41,11 @@ class TestSuite {
 #endif  // defined(OS_WIN)
   virtual ~TestSuite();
 
-  // Returns true if the test is marked as "MAYBE_".
-  // When using different prefixes depending on platform, we use MAYBE_ and
-  // preprocessor directives to replace MAYBE_ with the target prefix.
-  static bool IsMarkedMaybe(const testing::TestInfo& test);
-
-  void CatchMaybeTests();
-
-  void ResetCommandLine();
-
-  void AddTestLauncherResultPrinter();
-
   int Run();
 
+  // Disables checks for certain global objects being leaked across tests.
+  void DisableCheckForLeakedGlobals();
+
  protected:
   // By default fatal log messages (e.g. from DCHECKs) result in error dialogs
   // which gum up buildbots. Use a minimalistic assert handler which just
@@ -77,6 +69,8 @@ class TestSuite {
   std::unique_ptr<base::AtExitManager> at_exit_manager_;
 
  private:
+  void AddTestLauncherResultPrinter();
+
   void InitializeFromCommandLine(int argc, char** argv);
 #if defined(OS_WIN)
   void InitializeFromCommandLine(int argc, wchar_t** argv);
@@ -87,7 +81,7 @@ class TestSuite {
 
   test::TraceToFile trace_to_file_;
 
-  bool initialized_command_line_;
+  bool initialized_command_line_ = false;
 
   test::ScopedFeatureList scoped_feature_list_;
 
@@ -95,6 +89,10 @@ class TestSuite {
 
   std::unique_ptr<logging::ScopedLogAssertHandler> assert_handler_;
 
+  bool check_for_leaked_globals_ = true;
+
+  bool is_initialized_ = false;
+
   DISALLOW_COPY_AND_ASSIGN(TestSuite);
 };
 

chrome/test/base/chrome_test_launcher.cc

@@ -62,7 +62,10 @@ ChromeTestSuiteRunner::ChromeTestSuiteRunner() {}
 ChromeTestSuiteRunner::~ChromeTestSuiteRunner() {}
 
 int ChromeTestSuiteRunner::RunTestSuite(int argc, char** argv) {
-  return ChromeTestSuite(argc, argv).Run();
+  ChromeTestSuite test_suite(argc, argv);
+  // Browser tests are expected not to tear-down various globals.
+  test_suite.DisableCheckForLeakedGlobals();
+  return test_suite.Run();
 }
 
 ChromeTestLauncherDelegate::ChromeTestLauncherDelegate(

chrome/test/base/interactive_ui_tests_main.cc

@@ -37,6 +37,9 @@ class InteractiveUITestSuite : public ChromeTestSuite {
  protected:
   // ChromeTestSuite overrides:
   void Initialize() override {
+    // Browser tests are expected not to tear-down various globals.
+    base::TestSuite::DisableCheckForLeakedGlobals();
+
     ChromeTestSuite::Initialize();
 
     // Only allow ui_controls to be used in interactive_ui_tests, since they

chromecast/app/cast_test_launcher.cc

@@ -21,7 +21,10 @@ class CastTestLauncherDelegate : public content::TestLauncherDelegate {
   ~CastTestLauncherDelegate() override {}
 
   int RunTestSuite(int argc, char** argv) override {
-    return base::TestSuite(argc, argv).Run();
+    base::TestSuite test_suite(argc, argv);
+    // Browser tests are expected not to tear-down various globals.
+    test_suite.DisableCheckForLeakedGlobals();
+    return test_suite.Run();
   }
 
   bool AdjustChildProcessCommandLine(

components/cronet/BUILD.gn

@@ -116,6 +116,8 @@ if (!is_ios && !is_android) {
       "run_all_unittests.cc",
     ]
 
+    defines = [ "CRONET_TESTS_IMPLEMENTATION" ]
+
     if (is_linux && !is_component_build) {
       public_configs = [ "//build/config/gcc:rpath_for_built_shared_libraries" ]
     }

components/cronet/run_all_unittests.cc

@@ -8,6 +8,11 @@
 
 int main(int argc, char** argv) {
   base::TestSuite test_suite(argc, argv);
+#if defined(CRONET_TESTS_IMPLEMENTATION) && defined(COMPONENT_BUILD)
+  // In component builds cronet_tests and libcronet.so share various libraries,
+  // so globals initialized by libcronet.so are visible to the TestSuite.
+  test_suite.DisableCheckForLeakedGlobals();
+#endif
   return base::LaunchUnitTests(
       argc, argv,
       base::BindOnce(&base::TestSuite::Run, base::Unretained(&test_suite)));

content/test/content_test_launcher.cc

@@ -98,6 +98,9 @@ class ContentBrowserTestSuite : public ContentTestSuiteBase {
         content::CreateContentBrowserTestsCatalog());
 #endif
 
+    // Browser tests are expected not to tear-down various globals.
+    base::TestSuite::DisableCheckForLeakedGlobals();
+
     ContentTestSuiteBase::Initialize();
   }
 

extensions/shell/test/shell_test_launcher_delegate.cc

@@ -12,7 +12,10 @@
 namespace extensions {
 
 int AppShellTestLauncherDelegate::RunTestSuite(int argc, char** argv) {
-  return base::TestSuite(argc, argv).Run();
+  base::TestSuite test_suite(argc, argv);
+  // Browser tests are expected not to tear-down various globals.
+  test_suite.DisableCheckForLeakedGlobals();
+  return test_suite.Run();
 }
 
 bool AppShellTestLauncherDelegate::AdjustChildProcessCommandLine(

headless/test/headless_test_launcher.cc

@@ -37,7 +37,10 @@ class HeadlessTestLauncherDelegate : public content::TestLauncherDelegate {
 
   // content::TestLauncherDelegate implementation:
   int RunTestSuite(int argc, char** argv) override {
-    return base::TestSuite(argc, argv).Run();
+    base::TestSuite test_suite(argc, argv);
+    // Browser tests are expected not to tear-down various globals.
+    test_suite.DisableCheckForLeakedGlobals();
+    return test_suite.Run();
   }
 
   bool AdjustChildProcessCommandLine(

webrunner/browser/webrunner_test_launcher.cc

@@ -25,7 +25,10 @@ class WebRunnerTestLauncherDelegate : public content::TestLauncherDelegate {
 
   // content::TestLauncherDelegate implementation:
   int RunTestSuite(int argc, char** argv) override {
-    return base::TestSuite(argc, argv).Run();
+    base::TestSuite test_suite(argc, argv);
+    // Browser tests are expected not to tear-down various globals.
+    test_suite.DisableCheckForLeakedGlobals();
+    return test_suite.Run();
   }
 
   bool AdjustChildProcessCommandLine(