For unit tests, track additions to AtExitManager and warn.

While trying to bullet proof a unit test, I had trouble getting very far when
running all tests in shuffle mode. Tracked that back to a few other tests doing
stuff that accessed Singleton()s outside of a test-scoped
ShadowingAtExitManager. Seemed to me that should be an invariant around any
unit test, so created this towards that end, hopefully helping stabilize out
unit_tests a bit more.

BUG=133403


Review URL: https://chromiumcodereview.appspot.com/10582012

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@144488 0039d316-1c4b-4281-b951-d872f2087c98

base/at_exit.cc

@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -79,4 +79,13 @@ AtExitManager::AtExitManager(bool shadow) : next_manager_(g_top_manager) {
   g_top_manager = this;
 }
 
+// static
+AtExitManager* AtExitManager::current() {
+  return g_top_manager;
+}
+
+size_t AtExitManager::CallbackStackSize() const {
+  return stack_.size();
+}
+
 }  // namespace base

base/at_exit.h

@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -15,6 +15,8 @@
 
 namespace base {
 
+class TestWatchAtExitManager;
+
 // This class provides a facility similar to the CRT atexit(), except that
 // we control when the callbacks are executed. Under Windows for a DLL they
 // happen at a really bad time and under the loader lock. This facility is
@@ -58,6 +60,11 @@ class BASE_EXPORT AtExitManager {
   explicit AtExitManager(bool shadow);
 
  private:
+  friend class TestWatchAtExitManager;
+
+  static AtExitManager* current();
+  size_t CallbackStackSize() const;
+
   base::Lock lock_;
   std::stack<base::Closure> stack_;
   AtExitManager* next_manager_;  // Stack of managers to allow shadowing.

base/test/test_suite.cc

@@ -17,6 +17,7 @@
 #include "base/path_service.h"
 #include "base/process_util.h"
 #include "base/test/multiprocess_test.h"
+#include "base/test/test_switches.h"
 #include "base/test/test_timeouts.h"
 #include "base/time.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -69,6 +70,46 @@ class TestClientInitializer : public testing::EmptyTestEventListener {
 
 }  // namespace
 
+namespace base {
+
+class TestWatchAtExitManager : public testing::EmptyTestEventListener {
+ public:
+  TestWatchAtExitManager() { }
+  ~TestWatchAtExitManager() { }
+
+  virtual void OnTestStart(const testing::TestInfo& test_info) OVERRIDE {
+    initial_top_manager_ = AtExitManager::current();
+    at_exit_stack_size_ = initial_top_manager_->CallbackStackSize();
+  }
+
+  virtual void OnTestEnd(const testing::TestInfo& test_info) OVERRIDE {
+    AtExitManager* new_top_manager = AtExitManager::current();
+    size_t new_stack_size = new_top_manager->CallbackStackSize();
+
+    if (initial_top_manager_ != new_top_manager) {
+      ADD_FAILURE() << "The current AtExitManager has changed across the "
+          "test " << test_info.test_case_name() << "." << test_info.name() <<
+          " most likely because one was created without being destroyed.";
+    } else if (new_stack_size != at_exit_stack_size_) {
+      // TODO(scottbyer): clean up all the errors that result from this and
+      // turn this into a test failure with
+      // ADD_FAILURE(). http://crbug.com/133403
+      LOG(WARNING) <<
+          "AtExitManager: items were added to the callback list by " <<
+          test_info.test_case_name() << "." << test_info.name() <<
+          ". Global state should be cleaned up before a test exits.";
+    }
+  }
+
+ private:
+  AtExitManager* initial_top_manager_;
+  size_t at_exit_stack_size_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestWatchAtExitManager);
+};
+
+}  // namespace base
+
 const char TestSuite::kStrictFailureHandling[] = "strict_failure_handling";
 
 TestSuite::TestSuite(int argc, char** argv) : initialized_command_line_(false) {
@@ -170,6 +211,12 @@ void TestSuite::ResetCommandLine() {
   listeners.Append(new TestClientInitializer);
 }
 
+void TestSuite::WatchAtExitManager() {
+  testing::TestEventListeners& listeners =
+      testing::UnitTest::GetInstance()->listeners();
+  listeners.Append(new TestWatchAtExitManager);
+}
+
 // Don't add additional code to this method.  Instead add it to
 // Initialize().  See bug 6436.
 int TestSuite::Run() {
@@ -287,6 +334,15 @@ void TestSuite::Initialize() {
   CatchMaybeTests();
   ResetCommandLine();
 
+  // Don't watch for AtExit items being added if we're running as a child
+  // process (e.g., browser_tests or interactive_ui_tests).
+  if (!CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kSingleProcessTestsFlag) &&
+      !CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kSingleProcessChromeFlag)) {
+    WatchAtExitManager();
+  }
+
   TestTimeouts::Initialize();
 }
 

base/test/test_suite.h

@@ -53,6 +53,8 @@ class TestSuite {
 
   void ResetCommandLine();
 
+  void WatchAtExitManager();
+
   int Run();
 
   // A command-line flag that makes a test failure always result in a non-zero

base/test/test_switches.cc

@@ -4,9 +4,20 @@
 
 #include "base/test/test_switches.h"
 
+namespace switches {
+
+// Run Chromium in single process mode.
+const char kSingleProcessChromeFlag[]   = "single-process";
+
+// Run the tests and the launcher in the same process. Useful for debugging a
+// specific test in a debugger.
+const char kSingleProcessTestsFlag[]   = "single_process";
+
 // Time (in milliseconds) that the tests should wait before timing out.
 // TODO(phajdan.jr): Clean up the switch names.
-const char switches::kTestLargeTimeout[] = "test-large-timeout";
-const char switches::kTestTinyTimeout[] = "test-tiny-timeout";
-const char switches::kUiTestActionTimeout[] = "ui-test-action-timeout";
-const char switches::kUiTestActionMaxTimeout[] = "ui-test-action-max-timeout";
+const char kTestLargeTimeout[] = "test-large-timeout";
+const char kTestTinyTimeout[] = "test-tiny-timeout";
+const char kUiTestActionTimeout[] = "ui-test-action-timeout";
+const char kUiTestActionMaxTimeout[] = "ui-test-action-max-timeout";
+
+}  // namespace switches

base/test/test_switches.h

@@ -9,6 +9,8 @@ namespace switches {
 
 // All switches in alphabetical order. The switches should be documented
 // alongside the definition of their values in the .cc file.
+extern const char kSingleProcessChromeFlag[];
+extern const char kSingleProcessTestsFlag[];
 extern const char kTestLargeTimeout[];
 extern const char kTestTinyTimeout[];
 extern const char kUiTestActionTimeout[];

chrome/test/base/in_process_browser_test.cc

@@ -13,6 +13,7 @@
 #include "base/path_service.h"
 #include "base/string_number_conversions.h"
 #include "base/test/test_file_util.h"
+#include "base/test/test_switches.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/io_thread.h"
 #include "chrome/browser/lifetime/application_lifetime.h"
@@ -297,8 +298,8 @@ CommandLine InProcessBrowserTest::GetCommandLineForRelaunch() {
   CommandLine::SwitchMap switches =
       CommandLine::ForCurrentProcess()->GetSwitches();
   switches.erase(switches::kUserDataDir);
-  switches.erase(test_launcher::kSingleProcessTestsFlag);
-  switches.erase(test_launcher::kSingleProcessTestsAndChromeFlag);
+  switches.erase(switches::kSingleProcessTestsFlag);
+  switches.erase(switches::kSingleProcessChromeFlag);
   new_command_line.AppendSwitch(ChromeTestSuite::kLaunchAsBrowser);
 
 #if defined(USE_AURA)

content/public/test/test_launcher.h

@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -20,8 +20,6 @@ extern const char kGTestListTestsFlag[];
 extern const char kGTestRepeatFlag[];
 extern const char kGTestRunDisabledTestsFlag[];
 extern const char kGTestOutputFlag[];
-extern const char kSingleProcessTestsFlag[];
-extern const char kSingleProcessTestsAndChromeFlag[];
 extern const char kHelpFlag[];
 
 class TestLauncherDelegate {

content/test/test_launcher.cc

@@ -19,6 +19,7 @@
 #include "base/string_number_conversions.h"
 #include "base/string_util.h"
 #include "base/test/test_suite.h"
+#include "base/test/test_switches.h"
 #include "base/test/test_timeouts.h"
 #include "base/time.h"
 #include "base/utf_string_conversions.h"
@@ -342,7 +343,7 @@ int RunTest(TestLauncherDelegate* launcher_delegate,
   // tests unless this flag was specified to the browser test executable.
   new_cmd_line.AppendSwitch("gtest_also_run_disabled_tests");
   new_cmd_line.AppendSwitchASCII("gtest_filter", test_name);
-  new_cmd_line.AppendSwitch(kSingleProcessTestsFlag);
+  new_cmd_line.AppendSwitch(switches::kSingleProcessTestsFlag);
 
   // Do not let the child ignore failures.  We need to propagate the
   // failure status back to the parent.
@@ -566,8 +567,6 @@ const char kGTestRepeatFlag[] = "gtest_repeat";
 const char kGTestRunDisabledTestsFlag[] = "gtest_also_run_disabled_tests";
 const char kGTestOutputFlag[] = "gtest_output";
 
-const char kSingleProcessTestsFlag[]   = "single_process";
-const char kSingleProcessTestsAndChromeFlag[]   = "single-process";
 // The following is kept for historical reasons (so people that are used to
 // using it don't get surprised).
 const char kChildProcessFlag[]   = "child";
@@ -594,12 +593,12 @@ int LaunchTests(TestLauncherDelegate* launcher_delegate,
   // terrible UI.  Instead, there should be some sort of signal flag on the
   // command line, with all subsequent arguments passed through to the
   // underlying browser.
-  if (command_line->HasSwitch(kSingleProcessTestsFlag) ||
-      command_line->HasSwitch(kSingleProcessTestsAndChromeFlag) ||
+  if (command_line->HasSwitch(switches::kSingleProcessTestsFlag) ||
+      command_line->HasSwitch(switches::kSingleProcessChromeFlag) ||
       command_line->HasSwitch(kGTestListTestsFlag) ||
       command_line->HasSwitch(kGTestHelpFlag)) {
 #if defined(OS_WIN)
-    if (command_line->HasSwitch(kSingleProcessTestsFlag)) {
+    if (command_line->HasSwitch(switches::kSingleProcessTestsFlag)) {
       sandbox::SandboxInterfaceInfo sandbox_info;
       content::InitializeSandboxInfo(&sandbox_info);
       content::InitializeSandbox(&sandbox_info);