[test] Clean up some incorrect use of ScopedRunTimeout in //chrome.

ScopedRunTimeout is intended to be used to fail tests if a loop is Run()
for too long, rather than to Run() the loop for a specific length of
time.

Clean up some tests which were using ScopedRunTimeout inappropriately.

Bug: 1021777
Change-Id: If6b2ee5f3eab498698d09fdee103e759fff51ead
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2033445
Auto-Submit: Wez <wez@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Commit-Queue: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#738180}

base/util/memory_pressure/system_memory_pressure_evaluator_chromeos_unittest.cc

@@ -19,6 +19,7 @@
 #include "base/test/test_timeouts.h"
 #include "base/threading/platform_thread.h"
 #include "base/threading/scoped_blocking_call.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace util {
@@ -61,8 +62,8 @@ void OnMemoryPressure(
 
 void RunLoopRunWithTimeout(base::TimeDelta timeout) {
   base::RunLoop run_loop;
-  base::RunLoop::ScopedRunTimeoutForTest run_timeout(timeout,
-                                                     run_loop.QuitClosure());
+  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+      FROM_HERE, run_loop.QuitClosure(), timeout);
   run_loop.Run();
 }
 }  // namespace

chrome/browser/chromeos/login/oobe_localization_browsertest.cc

@@ -44,9 +44,6 @@ namespace chromeos {
 
 namespace {
 
-// Timeout for RunLoop::Run() in this test.
-const int kTimeoutSeconds = 2;
-
 // OOBE constants.
 const char kLanguageSelect[] = "languageSelect";
 const char kKeyboardSelect[] = "keyboardSelect";
@@ -56,52 +53,12 @@ std::string GetGetSelectStatement(const std::string& selectId) {
 }
 
 const char kUSLayout[] = "xkb:us::eng";
-class TimedRunLoop {
- public:
-  TimedRunLoop(const base::TimeDelta& timeout,
-               const std::string& failure_message)
-      : timeout_(timeout), message_(failure_message) {}
-
-  // Returns true if Run() successfully finished,
-  // Returns false on timeout.
-  bool Run() {
-    base::OneShotTimer timer;
-    timer.Start(FROM_HERE, timeout_,
-                base::Bind(&TimedRunLoop::OnTimeout, base::Unretained(this)));
-    loop_.Run();
-    return result_;
-  }
-
-  void Quit() {
-    result_ = true;
-    loop_.Quit();
-  }
-
-  base::Closure QuitClosure() {
-    return base::Bind(&TimedRunLoop::Quit, base::Unretained(this));
-  }
-
- private:
-  void OnTimeout() {
-    LOG(ERROR) << "Timeout waiting for: " << message_;
-    result_ = false;
-    loop_.Quit();
-  }
-
-  bool result_ = false;
-  const base::TimeDelta timeout_;
-  const std::string message_;
-  base::RunLoop loop_;
-
-  DISALLOW_COPY_AND_ASSIGN(TimedRunLoop);
-};
 
 class LanguageListWaiter : public WelcomeScreen::Observer {
  public:
   LanguageListWaiter()
       : welcome_screen_(WelcomeScreen::Get(
-            WizardController::default_controller()->screen_manager())),
-        loop_(base::TimeDelta::FromSeconds(kTimeoutSeconds), "LanguageList") {
+            WizardController::default_controller()->screen_manager())) {
     welcome_screen_->AddObserver(this);
     CheckLanguageList();
   }
@@ -111,13 +68,8 @@ class LanguageListWaiter : public WelcomeScreen::Observer {
   // WelcomeScreen::Observer implementation:
   void OnLanguageListReloaded() override { CheckLanguageList(); }
 
-  // Returns true on success, false on timeout.
-  bool Wait() {
-    if (LanguageListReady())
-      return true;
-
-    return loop_.Run();
-  }
+  // Run the loop until the list is ready or the default Run() timeout expires.
+  void RunUntilLanguageListReady() { loop_.Run(); }
 
  private:
   bool LanguageListReady() const { return welcome_screen_->language_list(); }
@@ -128,7 +80,7 @@ class LanguageListWaiter : public WelcomeScreen::Observer {
   }
 
   WelcomeScreen* welcome_screen_;
-  TimedRunLoop loop_;
+  base::RunLoop loop_;
 };
 
 }  // namespace
@@ -255,23 +207,17 @@ class OobeLocalizationTest
   // Runs the test for the given locale and keyboard layout.
   void RunLocalizationTest();
 
-  // Returns true on success, false on error.
-  bool WaitUntilJSIsReady() {
+  // Runs until the OOBE UI JS is ready, or the default Run() timeout expires.
+  void RunUntilJSIsReady() {
     LoginDisplayHost* host = LoginDisplayHost::default_host();
-    if (!host)
-      return false;
+    ASSERT_TRUE(host);
 
     OobeUI* oobe_ui = host->GetOobeUI();
-    if (!oobe_ui)
-      return false;
-
-    TimedRunLoop run_loop(base::TimeDelta::FromSeconds(kTimeoutSeconds),
-                          "WaitUntilJSIsReady()");
-    const bool oobe_ui_ready = oobe_ui->IsJSReady(run_loop.QuitClosure());
-    if (oobe_ui_ready)
-      return true;
+    ASSERT_TRUE(oobe_ui);
 
-    return run_loop.Run();
+    base::RunLoop run_loop;
+    if (!oobe_ui->IsJSReady(run_loop.QuitClosure()))
+      run_loop.Run();
   }
 
  private:
@@ -402,9 +348,9 @@ void OobeLocalizationTest::RunLocalizationTest() {
   const std::string expected_keyboard_select =
       TranslateXKB2Extension(expected_keyboard_select_control);
 
-  ASSERT_TRUE(LanguageListWaiter().Wait());
+  ASSERT_NO_FATAL_FAILURE(LanguageListWaiter().RunUntilLanguageListReady());
 
-  ASSERT_TRUE(WaitUntilJSIsReady());
+  ASSERT_NO_FATAL_FAILURE(RunUntilJSIsReady());
 
   const std::string first_language =
       expected_locale.substr(0, expected_locale.find(','));

chrome/browser/net/network_context_configuration_browsertest.cc

@@ -22,6 +22,7 @@
 #include "base/test/bind_test_util.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/threading/thread_restrictions.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_content_browser_client.h"
@@ -2010,12 +2011,11 @@ class NetworkContextConfigurationProxySettingsBrowserTest
     expected_connections_run_loop.Run();
 
     // Then wait for any remaining connections that we should NOT get.
-    base::RunLoop unexpected_connections_run_loop;
-    base::RunLoop::ScopedRunTimeoutForTest run_timeout(
-        base::TimeDelta::FromMilliseconds(100),
-        base::BindLambdaForTesting(
-            [&]() { unexpected_connections_run_loop.Quit(); }));
-    unexpected_connections_run_loop.Run();
+    base::RunLoop ugly_100ms_wait;
+    base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+        FROM_HERE, ugly_100ms_wait.QuitClosure(),
+        base::TimeDelta::FromMilliseconds(100));
+    ugly_100ms_wait.Run();
 
     // Stop the server.
     ASSERT_TRUE(embedded_test_server()->ShutdownAndWaitUntilComplete());

chrome/browser/ui/ash/assistant/assistant_test_mixin.cc

@@ -11,6 +11,7 @@
 #include "ash/public/cpp/assistant/assistant_state.h"
 #include "ash/public/cpp/test/assistant_test_api.h"
 #include "ash/public/mojom/assistant_state_controller.mojom-shared.h"
+#include "base/auto_reset.h"
 #include "base/run_loop.h"
 #include "base/test/bind_test_util.h"
 #include "base/time/time.h"
@@ -45,23 +46,6 @@ bool Equals(const char* left, const char* right) {
   return strcmp(left, right) == 0;
 }
 
-// Run the loop until the timeout expires, or until it is quit through other
-// mechanisms.
-// Returns whether the loop finished successfully, i.e. |false| if the timeout
-// expired.
-bool RunWithTimeout(base::RunLoop* run_loop, base::TimeDelta timeout) {
-  bool success = true;
-
-  base::RunLoop::ScopedRunTimeoutForTest scoped_timeout(
-      timeout, base::BindLambdaForTesting([&success, run_loop] {
-        success = false;
-        run_loop->QuitClosure();
-      }));
-  run_loop->Run();
-
-  return success;
-}
-
 // Waiter that blocks in the |Wait| method until a given |mojom::AssistantState|
 // is reached, or until a timeout is hit.
 // On timeout this will abort the test with a useful error message.
@@ -75,31 +59,30 @@ class AssistantStatusWaiter : private ash::AssistantStateObserver {
 
   ~AssistantStatusWaiter() override { state_->RemoveObserver(this); }
 
-  void Wait(base::TimeDelta wait_timeout) {
+  void RunUntilExpectedStatus() {
     if (state_->assistant_state() == expected_status_)
       return;
 
     // Wait until we're ready or we hit the timeout.
-    run_loop_ = std::make_unique<base::RunLoop>();
-    bool success = RunWithTimeout(run_loop_.get(), wait_timeout);
-    run_loop_.reset();
-
-    EXPECT_TRUE(success)
-        << "Timeout waiting for AssistantStatus |" << expected_status_ << "|. "
+    base::RunLoop run_loop;
+    base::AutoReset<base::OnceClosure> quit_loop(&quit_loop_,
+                                                 run_loop.QuitClosure());
+    EXPECT_NO_FATAL_FAILURE(run_loop.Run())
+        << "Failed waiting for AssistantStatus |" << expected_status_ << "|. "
         << "Current status is |" << state_->assistant_state() << "|. "
         << "One possible cause is that you're using an expired access token.";
   }
 
  private:
   void OnAssistantStatusChanged(ash::mojom::AssistantState status) override {
-    if (status == expected_status_ && run_loop_ != nullptr)
-      run_loop_->Quit();
+    if (status == expected_status_ && quit_loop_)
+      std::move(quit_loop_).Run();
   }
 
   ash::AssistantState* const state_;
   ash::mojom::AssistantState const expected_status_;
 
-  std::unique_ptr<base::RunLoop> run_loop_;
+  base::OnceClosure quit_loop_;
 };
 
 // Base class that observes all new responses being displayed under the
@@ -120,19 +103,18 @@ class ResponseWaiter : private views::ViewObserver {
       parent_view_->RemoveObserver(this);
   }
 
-  void Wait(base::TimeDelta wait_timeout) {
+  void RunUntilResponseReceived() {
     if (HasExpectedResponse())
       return;
 
     // Wait until we're ready or we hit the timeout.
-    run_loop_ = std::make_unique<base::RunLoop>();
-    bool success = RunWithTimeout(run_loop_.get(), wait_timeout);
-    run_loop_.reset();
-
-    EXPECT_TRUE(success) << "Timeout waiting for Assistant response.\n"
-                         << "Expected any of " << FormatExpectedResponses()
-                         << ".\n"
-                         << "Got \"" << GetResponseText() << "\"";
+    base::RunLoop run_loop;
+    base::AutoReset<base::OnceClosure> quit_loop(&quit_loop_,
+                                                 run_loop.QuitClosure());
+    EXPECT_NO_FATAL_FAILURE(run_loop.Run())
+        << "Failed waiting for Assistant response.\n"
+        << "Expected any of " << FormatExpectedResponses() << ".\n"
+        << "Got \"" << GetResponseText() << "\"";
   }
 
  private:
@@ -140,19 +122,19 @@ class ResponseWaiter : private views::ViewObserver {
   void OnViewHierarchyChanged(
       views::View* observed_view,
       const views::ViewHierarchyChangedDetails& details) override {
-    if (run_loop_ && HasExpectedResponse())
-      run_loop_->Quit();
+    if (quit_loop_ && HasExpectedResponse())
+      std::move(quit_loop_).Run();
   }
 
   void OnViewIsDeleting(views::View* observed_view) override {
     DCHECK(observed_view == parent_view_);
 
-    if (run_loop_) {
+    if (quit_loop_) {
       FAIL() << parent_view_->GetClassName() << " is deleted "
              << "before receiving the Assistant response.\n"
              << "Expected any of " << FormatExpectedResponses() << ".\n"
              << "Got \"" << GetResponseText() << "\"";
-      run_loop_->Quit();
+      std::move(quit_loop_).Run();
     }
 
     parent_view_ = nullptr;
@@ -198,7 +180,7 @@ class ResponseWaiter : private views::ViewObserver {
   views::View* parent_view_;
   std::vector<std::string> expected_responses_;
 
-  std::unique_ptr<base::RunLoop> run_loop_;
+  base::OnceClosure quit_loop_;
 };
 
 class TextResponseWaiter : public ResponseWaiter {
@@ -339,6 +321,8 @@ void AssistantTestMixin::TearDownOnMainThread() {
 
 void AssistantTestMixin::StartAssistantAndWaitForReady(
     base::TimeDelta wait_timeout) {
+  const base::RunLoop::ScopedRunTimeoutForTest run_timeout(
+      wait_timeout, base::MakeExpectedNotRunClosure(FROM_HERE));
   // Note: You might be tempted to call this function from SetUpOnMainThread(),
   // but that will not work as the Assistant service can not start until
   // |BrowserTestBase| calls InitializeNetworkProcess(), which it only does
@@ -349,7 +333,7 @@ void AssistantTestMixin::StartAssistantAndWaitForReady(
 
   AssistantStatusWaiter waiter(test_api_->GetAssistantState(),
                                ash::mojom::AssistantState::NEW_READY);
-  waiter.Wait(wait_timeout);
+  waiter.RunUntilExpectedStatus();
 
   // With the warmer welcome enabled the Assistant service will start an
   // interaction that will never complete (as our tests finish too soon).
@@ -370,9 +354,11 @@ void AssistantTestMixin::SendTextQuery(const std::string& query) {
 void AssistantTestMixin::ExpectCardResponse(
     const std::string& expected_response,
     base::TimeDelta wait_timeout) {
+  const base::RunLoop::ScopedRunTimeoutForTest run_timeout(
+      wait_timeout, base::MakeExpectedNotRunClosure(FROM_HERE));
   CardResponseWaiter waiter(test_api_->ui_element_container(),
                             {expected_response});
-  waiter.Wait(wait_timeout);
+  waiter.RunUntilResponseReceived();
 }
 
 void AssistantTestMixin::ExpectTextResponse(
@@ -384,9 +370,11 @@ void AssistantTestMixin::ExpectTextResponse(
 void AssistantTestMixin::ExpectAnyOfTheseTextResponses(
     const std::vector<std::string>& expected_responses,
     base::TimeDelta wait_timeout) {
+  const base::RunLoop::ScopedRunTimeoutForTest run_timeout(
+      wait_timeout, base::MakeExpectedNotRunClosure(FROM_HERE));
   TextResponseWaiter waiter(test_api_->ui_element_container(),
                             expected_responses);
-  waiter.Wait(wait_timeout);
+  waiter.RunUntilResponseReceived();
 }
 
 void AssistantTestMixin::PressAssistantKey() {