Commit 68c9f104 authored by ahest's avatar ahest Committed by Commit bot

Fix cases where RunLoop is created without a MessageLoop.

The tests in these cases can pass only if Quit is called before Run, or
if Run is never called at all. If this is guaranteed to hold true, then
RunLoop is not needed (such as in timer_unittest.cc). Other cases (where
it is not guaranteed) are tests that can crash occasionally.

BUG=668707

Review-Url: https://codereview.chromium.org/2571473002
Cr-Commit-Position: refs/heads/master@{#438126}
parent 2c7d13f7
......@@ -19,6 +19,7 @@ RunLoop::RunLoop()
running_(false),
quit_when_idle_received_(false),
weak_factory_(this) {
DCHECK(loop_);
}
RunLoop::~RunLoop() {
......
......@@ -54,17 +54,58 @@ class Receiver {
int count_;
};
class OneShotTimerTester {
// A basic helper class that can start a one-shot timer and signal a
// WaitableEvent when this timer fires.
class OneShotTimerTesterBase {
public:
// |did_run|, if provided, will be signaled when Run() fires.
explicit OneShotTimerTesterBase(
WaitableEvent* did_run = nullptr,
const TimeDelta& delay = TimeDelta::FromMilliseconds(10))
: did_run_(did_run), delay_(delay) {}
virtual ~OneShotTimerTesterBase() = default;
void Start() {
started_time_ = TimeTicks::Now();
timer_->Start(FROM_HERE, delay_, this, &OneShotTimerTesterBase::Run);
}
bool IsRunning() { return timer_->IsRunning(); }
TimeTicks started_time() const { return started_time_; }
TimeDelta delay() const { return delay_; }
protected:
virtual void Run() {
if (did_run_) {
EXPECT_FALSE(did_run_->IsSignaled());
did_run_->Signal();
}
}
std::unique_ptr<OneShotTimer> timer_ = MakeUnique<OneShotTimer>();
private:
WaitableEvent* const did_run_;
const TimeDelta delay_;
TimeTicks started_time_;
DISALLOW_COPY_AND_ASSIGN(OneShotTimerTesterBase);
};
// Extends functionality of OneShotTimerTesterBase with the abilities to wait
// until the timer fires and to change task runner for the timer.
class OneShotTimerTester : public OneShotTimerTesterBase {
public:
// |did_run|, if provided, will be signaled when Run() fires.
explicit OneShotTimerTester(
WaitableEvent* did_run = nullptr,
const TimeDelta& delay = TimeDelta::FromMilliseconds(10))
: quit_closure_(run_loop_.QuitClosure()),
did_run_(did_run),
delay_(delay) {}
: OneShotTimerTesterBase(did_run, delay),
quit_closure_(run_loop_.QuitClosure()) {}
virtual ~OneShotTimerTester() = default;
~OneShotTimerTester() override = default;
void SetTaskRunner(scoped_refptr<SingleThreadTaskRunner> task_runner) {
timer_->SetTaskRunner(std::move(task_runner));
......@@ -76,45 +117,29 @@ class OneShotTimerTester {
ThreadTaskRunnerHandle::Get(), FROM_HERE, run_loop_.QuitClosure());
}
void Start() {
started_time_ = TimeTicks::Now();
timer_->Start(FROM_HERE, delay_, this, &OneShotTimerTester::Run);
}
// Blocks until Run() executes and confirms that Run() didn't fire before
// |delay_| expired.
void WaitAndConfirmTimerFiredAfterDelay() {
run_loop_.Run();
EXPECT_NE(TimeTicks(), started_time_);
EXPECT_GE(TimeTicks::Now() - started_time_, delay_);
EXPECT_NE(TimeTicks(), started_time());
EXPECT_GE(TimeTicks::Now() - started_time(), delay());
}
bool IsRunning() { return timer_->IsRunning(); }
protected:
// Overridable method to do things on Run() before signaling events/closures
// managed by this helper.
virtual void OnRun() {}
std::unique_ptr<OneShotTimer> timer_ = MakeUnique<OneShotTimer>();
private:
void Run() {
void Run() override {
OnRun();
if (did_run_) {
EXPECT_FALSE(did_run_->IsSignaled());
did_run_->Signal();
}
OneShotTimerTesterBase::Run();
quit_closure_.Run();
}
RunLoop run_loop_;
Closure quit_closure_;
WaitableEvent* const did_run_;
const TimeDelta delay_;
TimeTicks started_time_;
DISALLOW_COPY_AND_ASSIGN(OneShotTimerTester);
};
......@@ -517,10 +542,10 @@ TEST(TimerTest, MessageLoopShutdown) {
WaitableEvent did_run(WaitableEvent::ResetPolicy::MANUAL,
WaitableEvent::InitialState::NOT_SIGNALED);
{
OneShotTimerTester a(&did_run);
OneShotTimerTester b(&did_run);
OneShotTimerTester c(&did_run);
OneShotTimerTester d(&did_run);
OneShotTimerTesterBase a(&did_run);
OneShotTimerTesterBase b(&did_run);
OneShotTimerTesterBase c(&did_run);
OneShotTimerTesterBase d(&did_run);
{
MessageLoop loop;
a.Start();
......
......@@ -10,6 +10,7 @@
#include "ash/shell.h"
#include "base/command_line.h"
#include "base/files/file_util.h"
#include "base/run_loop.h"
#include "chrome/browser/chromeos/login/screenshot_testing/SkDiffPixelsMetric.h"
#include "chrome/browser/chromeos/login/screenshot_testing/SkImageDiffer.h"
#include "chrome/browser/chromeos/login/screenshot_testing/SkPMetric.h"
......
......@@ -8,11 +8,11 @@
#include <string>
#include <vector>
#include "base/callback.h"
#include "base/files/file_path.h"
#include "base/macros.h"
#include "base/memory/ref_counted_memory.h"
#include "base/memory/weak_ptr.h"
#include "base/run_loop.h"
#include "third_party/skia/include/core/SkBitmap.h"
namespace chromeos {
......@@ -137,9 +137,8 @@ class ScreenshotTester {
// and difference between them and golden ones will be stored.
base::FilePath artifacts_dir_;
// |run_loop_| and |run_loop_quitter_| are used to synchronize
// with ui::GrabWindowSnapshotAsync.
base::RunLoop run_loop_;
// |run_loop_quitter_| is used to stop waiting when
// ui::GrabWindowSnapshotAsync completes.
base::Closure run_loop_quitter_;
// Is true when we're in test mode:
......
......@@ -4,6 +4,7 @@
#include "chrome/browser/chromeos/login/screenshot_testing/screenshot_testing_mixin.h"
#include "base/run_loop.h"
#include "chrome/browser/browser_process.h"
#include "chrome/browser/chrome_notification_types.h"
#include "content/public/browser/browser_thread.h"
......
......@@ -49,6 +49,7 @@ class DeviceIDTest : public OobeBaseTest,
}
void SetUpOnMainThread() override {
user_removal_loop_.reset(new base::RunLoop);
OobeBaseTest::SetUpOnMainThread();
LoadRefreshTokenToDeviceIdMap();
}
......@@ -125,7 +126,7 @@ class DeviceIDTest : public OobeBaseTest,
void RemoveUser(const AccountId& account_id) {
user_manager::UserManager::Get()->RemoveUser(account_id, this);
user_removal_loop_.Run();
user_removal_loop_->Run();
}
private:
......@@ -133,7 +134,7 @@ class DeviceIDTest : public OobeBaseTest,
void OnBeforeUserRemoved(const AccountId& account_id) override {}
void OnUserRemoved(const AccountId& account_id) override {
user_removal_loop_.Quit();
user_removal_loop_->Quit();
}
base::FilePath GetRefreshTokenToDeviceIdMapFilePath() const {
......@@ -170,7 +171,7 @@ class DeviceIDTest : public OobeBaseTest,
json.c_str(), json.length()));
}
base::RunLoop user_removal_loop_;
std::unique_ptr<base::RunLoop> user_removal_loop_;
};
// Add the first user and check that device ID is consistent.
......
......@@ -59,7 +59,7 @@ class DeviceDisablingTest
std::string GetCurrentScreenName(content::WebContents* web_contents);
protected:
base::RunLoop network_state_change_wait_run_loop_;
std::unique_ptr<base::RunLoop> network_state_change_wait_run_loop_;
private:
// OobeBaseTest:
......@@ -121,6 +121,8 @@ void DeviceDisablingTest::SetUpInProcessBrowserTestFixture() {
}
void DeviceDisablingTest::SetUpOnMainThread() {
network_state_change_wait_run_loop_.reset(new base::RunLoop);
OobeBaseTest::SetUpOnMainThread();
// Set up fake networks.
......@@ -129,7 +131,7 @@ void DeviceDisablingTest::SetUpOnMainThread() {
}
void DeviceDisablingTest::UpdateState(NetworkError::ErrorReason reason) {
network_state_change_wait_run_loop_.Quit();
network_state_change_wait_run_loop_->Quit();
}
IN_PROC_BROWSER_TEST_F(DeviceDisablingTest, DisableDuringNormalOperation) {
......@@ -206,7 +208,7 @@ IN_PROC_BROWSER_TEST_F(DeviceDisablingTest, DisableWithEphemeralUsers) {
ASSERT_TRUE(signin_screen_handler);
signin_screen_handler->ZeroOfflineTimeoutForTesting();
SimulateNetworkOffline();
network_state_change_wait_run_loop_.Run();
network_state_change_wait_run_loop_->Run();
network_state_informer->RemoveObserver(this);
base::RunLoop().RunUntilIdle();
......
......@@ -48,7 +48,16 @@ class AsyncRevalidationManagerBrowserTest : public ContentBrowserTest {
ContentBrowserTest::SetUp();
}
base::RunLoop* run_loop() { return &run_loop_; }
void SetUpOnMainThread() override {
ContentBrowserTest::SetUpOnMainThread();
run_loop_.reset(new base::RunLoop);
}
base::RunLoop* run_loop() {
DCHECK(run_loop_);
return run_loop_.get();
}
int requests_counted() const { return requests_counted_; }
// This method lacks diagnostics for the failure case because TitleWatcher
......@@ -92,7 +101,7 @@ class AsyncRevalidationManagerBrowserTest : public ContentBrowserTest {
// use this for synchronisation.
if (version == 2) {
BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
run_loop_.QuitClosure());
run_loop()->QuitClosure());
}
return std::move(http_response);
}
......@@ -142,7 +151,7 @@ class AsyncRevalidationManagerBrowserTest : public ContentBrowserTest {
return http_response;
}
base::RunLoop run_loop_;
std::unique_ptr<base::RunLoop> run_loop_;
int requests_counted_ = 0;
base::test::ScopedFeatureList scoped_feature_list_;
......
......@@ -124,6 +124,7 @@ class AudioRendererMixerInputTest : public testing::Test,
protected:
virtual ~AudioRendererMixerInputTest() {}
base::MessageLoop message_loop_;
AudioParameters audio_parameters_;
scoped_refptr<MockAudioRendererSink> sinks_[2];
std::unique_ptr<AudioRendererMixer> mixers_[2];
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment