Revert "Make interactive tests have unlimited timeout on Windows."

This reverts commit 3adf99c6.

Reason for revert: This CL is causing a hang on base_unittests on iOS internal device bots, and the tests consistently hang on RunLoopDelegateTest.NestableTasksDontRunInDefaultNestedLoops test.

Original change's description:
> Make interactive tests have unlimited timeout on Windows.
> 
> This upgrades --interactive to a test-launcher-scope switch (necessarily
> renaming it to --test-launcher-interactive) and treats it as a timeout
> specifier.  For browser UI tests on Windows, this means the parent process will
> now get an (effectively) unlimited timeout and not just the child process, so
> the test won't terminate early.
> 
> BUG=none
> TEST=none
> 
> Change-Id: I56b02ba30ad0df6617b0f1f95d9986724c5035a0
> Reviewed-on: https://chromium-review.googlesource.com/909920
> Commit-Queue: Peter Kasting <pkasting@chromium.org>
> Reviewed-by: Gabriel Charette <gab@chromium.org>
> Reviewed-by: Scott Violet <sky@chromium.org>
> Reviewed-by: Trent Apted <tapted@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#536634}

TBR=sky@chromium.org,pkasting@chromium.org,gab@chromium.org,tapted@chromium.org

Change-Id: I75837514518ee8ac3fd846151c4dc1385ebf385c
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: none
Reviewed-on: https://chromium-review.googlesource.com/919741Reviewed-by: Yuke Liao <liaoyuke@chromium.org>
Commit-Queue: Yuke Liao <liaoyuke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#536780}

base/test/test_switches.cc

@@ -26,10 +26,6 @@ const char switches::kTestLauncherForceRunBrokenTests[] =
 // Path to file containing test filter (one pattern per line).
 const char switches::kTestLauncherFilterFile[] = "test-launcher-filter-file";
 
-// Whether the test launcher should launch in "interactive mode", which disables
-// timeouts (and may have other effects for specific test types).
-const char switches::kTestLauncherInteractive[] = "test-launcher-interactive";
-
 // Number of parallel test launcher jobs.
 const char switches::kTestLauncherJobs[] = "test-launcher-jobs";
 

base/test/test_switches.h

@@ -14,7 +14,6 @@ extern const char kTestLauncherBotMode[];
 extern const char kTestLauncherDebugLauncher[];
 extern const char kTestLauncherForceRunBrokenTests[];
 extern const char kTestLauncherFilterFile[];
-extern const char kTestLauncherInteractive[];
 extern const char kTestLauncherJobs[];
 extern const char kTestLauncherListTests[];
 extern const char kTestLauncherOutput[];

base/test/test_timeouts.cc

@@ -15,6 +15,25 @@
 
 namespace {
 
+// ASan/TSan/MSan instrument each memory access. This may slow the execution
+// down significantly.
+#if defined(MEMORY_SANITIZER)
+// For MSan the slowdown depends heavily on the value of msan_track_origins GYP
+// flag. The multiplier below corresponds to msan_track_origins=1.
+static const int kTimeoutMultiplier = 6;
+#elif defined(ADDRESS_SANITIZER) && defined(OS_WIN)
+// Asan/Win has not been optimized yet, give it a higher
+// timeout multiplier. See http://crbug.com/412471
+static const int kTimeoutMultiplier = 3;
+#elif defined(ADDRESS_SANITIZER) || defined(THREAD_SANITIZER) || \
+    defined(SYZYASAN)
+static const int kTimeoutMultiplier = 2;
+#else
+static const int kTimeoutMultiplier = 1;
+#endif
+
+const int kAlmostInfiniteTimeoutMs = 100000000;
+
 // Sets value to the greatest of:
 // 1) value's current value multiplied by kTimeoutMultiplier (assuming
 // InitializeTimeout is called only once per value).
@@ -23,43 +42,34 @@ namespace {
 // by kTimeoutMultiplier.
 void InitializeTimeout(const char* switch_name, int min_value, int* value) {
   DCHECK(value);
-  int timeout = 0;
-  if (base::debug::BeingDebugged() ||
-      base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kTestLauncherInteractive)) {
-    constexpr int kVeryLargeTimeoutMs = 100'000'000;
-    timeout = kVeryLargeTimeoutMs;
-  } else if (base::CommandLine::ForCurrentProcess()->HasSwitch(switch_name)) {
+  if (base::CommandLine::ForCurrentProcess()->HasSwitch(switch_name)) {
     std::string string_value(base::CommandLine::ForCurrentProcess()->
          GetSwitchValueASCII(switch_name));
-    if (!base::StringToInt(string_value, &timeout)) {
-      LOG(ERROR) << "Timeout value \"" << string_value << "\" was parsed as "
-                 << timeout;
-    }
+    int timeout;
+    if (string_value == TestTimeouts::kNoTimeoutSwitchValue)
+      timeout = kAlmostInfiniteTimeoutMs;
+    else
+      base::StringToInt(string_value, &timeout);
+    *value = std::max(*value, timeout);
   }
+  *value *= kTimeoutMultiplier;
+  *value = std::max(*value, min_value);
+}
 
-#if defined(MEMORY_SANITIZER)
-  // ASan/TSan/MSan instrument each memory access. This may slow the execution
-  // down significantly.
-  // For MSan the slowdown depends heavily on the value of msan_track_origins
-  // build flag. The multiplier below corresponds to msan_track_origins = 1.
-  constexpr int kTimeoutMultiplier = 6;
-#elif defined(ADDRESS_SANITIZER) && defined(OS_WIN)
-  // ASan/Win has not been optimized yet, give it a higher
-  // timeout multiplier. See http://crbug.com/412471
-  constexpr int kTimeoutMultiplier = 3;
-#elif defined(ADDRESS_SANITIZER) || defined(THREAD_SANITIZER) || \
-    defined(SYZYASAN)
-  constexpr int kTimeoutMultiplier = 2;
-#else
-  constexpr int kTimeoutMultiplier = 1;
-#endif
-
-  *value = std::max(std::max(*value, timeout) * kTimeoutMultiplier, min_value);
+// Sets value to the greatest of:
+// 1) value's current value multiplied by kTimeoutMultiplier.
+// 2) 0
+// 3) the numerical value given by switch_name on the command line multiplied
+// by kTimeoutMultiplier.
+void InitializeTimeout(const char* switch_name, int* value) {
+  InitializeTimeout(switch_name, 0, value);
 }
 
 }  // namespace
 
+// static
+constexpr const char TestTimeouts::kNoTimeoutSwitchValue[];
+
 // static
 bool TestTimeouts::initialized_ = false;
 
@@ -77,7 +87,10 @@ int TestTimeouts::test_launcher_timeout_ms_ = 45000;
 
 // static
 void TestTimeouts::Initialize() {
-  DCHECK(!initialized_);
+  if (initialized_) {
+    NOTREACHED();
+    return;
+  }
   initialized_ = true;
 
   if (base::debug::BeingDebugged()) {
@@ -87,8 +100,10 @@ void TestTimeouts::Initialize() {
 
   // Note that these timeouts MUST be initialized in the correct order as
   // per the CHECKS below.
-  InitializeTimeout(switches::kTestTinyTimeout, 0, &tiny_timeout_ms_);
-  InitializeTimeout(switches::kUiTestActionTimeout, tiny_timeout_ms_,
+  InitializeTimeout(switches::kTestTinyTimeout, &tiny_timeout_ms_);
+  InitializeTimeout(switches::kUiTestActionTimeout,
+                    base::debug::BeingDebugged() ? kAlmostInfiniteTimeoutMs
+                                                 : tiny_timeout_ms_,
                     &action_timeout_ms_);
   InitializeTimeout(switches::kUiTestActionMaxTimeout, action_timeout_ms_,
                     &action_max_timeout_ms_);
@@ -98,7 +113,8 @@ void TestTimeouts::Initialize() {
                     &test_launcher_timeout_ms_);
 
   // The timeout values should be increasing in the right order.
-  CHECK_LE(tiny_timeout_ms_, action_timeout_ms_);
-  CHECK_LE(action_timeout_ms_, action_max_timeout_ms_);
-  CHECK_LE(action_timeout_ms_, test_launcher_timeout_ms_);
+  CHECK(tiny_timeout_ms_ <= action_timeout_ms_);
+  CHECK(action_timeout_ms_ <= action_max_timeout_ms_);
+
+  CHECK(action_timeout_ms_ <= test_launcher_timeout_ms_);
 }

base/test/test_timeouts.h

@@ -13,6 +13,9 @@
 // the timeouts for different environments (like TSan).
 class TestTimeouts {
  public:
+  // Argument that can be passed on the command line to indicate "no timeout".
+  static constexpr const char kNoTimeoutSwitchValue[] = "-1";
+
   // Initializes the timeouts. Non thread-safe. Should be called exactly once
   // by the test suite.
   static void Initialize();

chrome/browser/ui/test/browser_ui_browsertest.cc

@@ -4,11 +4,11 @@
 
 #include "base/command_line.h"
 #include "base/process/launch.h"
-#include "base/stl_util.h"
 #include "base/test/launcher/test_launcher.h"
 #include "base/test/test_switches.h"
 #include "base/test/test_timeouts.h"
 #include "build/build_config.h"
+#include "chrome/browser/ui/test/test_browser_ui.h"
 #include "content/public/common/content_switches.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/compositor/compositor_switches.h"
@@ -68,8 +68,12 @@ TEST(BrowserUiTest, Invoke) {
 
   base::LaunchOptions options;
 
-  // Generate screen output if --test-launcher-interactive was specified.
-  if (command.HasSwitch(switches::kTestLauncherInteractive)) {
+  // Disable timeouts and generate screen output if --interactive was specified.
+  if (command.HasSwitch(internal::kInteractiveSwitch)) {
+    command.AppendSwitchASCII(switches::kUiTestActionMaxTimeout,
+                              TestTimeouts::kNoTimeoutSwitchValue);
+    command.AppendSwitchASCII(switches::kTestLauncherTimeout,
+                              TestTimeouts::kNoTimeoutSwitchValue);
     command.AppendSwitch(switches::kEnablePixelOutputInTests);
 #if defined(OS_WIN)
     // Under Windows, the child process won't launch without the wait option.

chrome/browser/ui/test/test_browser_ui.cc

@@ -7,7 +7,6 @@
 #include "base/command_line.h"
 #include "base/test/gtest_util.h"
 #include "base/test/scoped_feature_list.h"
-#include "base/test/test_switches.h"
 #include "build/build_config.h"
 #include "chrome/common/chrome_features.h"
 #include "ui/base/ui_base_features.h"
@@ -34,7 +33,7 @@ void TestBrowserUi::ShowAndVerifyUi() {
   ShowUi(NameFromTestCase());
   ASSERT_TRUE(VerifyUi());
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kTestLauncherInteractive))
+          internal::kInteractiveSwitch))
     WaitForUserDismissal();
   else
     DismissUi();

chrome/browser/ui/test/test_browser_ui.h

@@ -68,8 +68,8 @@ class ScopedFeatureList;
 //
 // UI listed can be shown interactively using the --ui argument. E.g.
 //
-//   browser_tests --gtest_filter=BrowserUiTest.Invoke
-//       --test-launcher-interactive --ui=FooUiTest.InvokeUi_name
+//   browser_tests --gtest_filter=BrowserUiTest.Invoke --interactive
+//       --ui=FooUiTest.InvokeUi_name
 class TestBrowserUi {
  protected:
   TestBrowserUi();
@@ -126,4 +126,11 @@ class SupportsTestUi : public Base, public TestUi {
 
 using UiBrowserTest = SupportsTestUi<InProcessBrowserTest, TestBrowserUi>;
 
+namespace internal {
+
+// When present on the command line, runs the test in an interactive mode.
+constexpr const char kInteractiveSwitch[] = "interactive";
+
+}  // namespace internal
+
 #endif  // CHROME_BROWSER_UI_TEST_TEST_BROWSER_UI_H_

chrome/test/base/browser_tests_main.cc

@@ -9,8 +9,8 @@
 #include "chrome/test/base/chrome_test_suite.h"
 
 #if defined(OS_WIN)
-#include "base/test/test_switches.h"
 #include "base/win/win_util.h"
+#include "chrome/browser/ui/test/test_browser_ui.h"
 #endif  // defined(OS_WIN)
 
 int main(int argc, char** argv) {
@@ -26,7 +26,7 @@ int main(int argc, char** argv) {
   // Enable high-DPI for interactive tests where the user is expected to
   // manually verify results.
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kTestLauncherInteractive)) {
+          internal::kInteractiveSwitch)) {
     base::win::EnableHighDPISupport();
   }
 #endif  // defined(OS_WIN)