Commit d642ca5d authored by Hiroki Nakagawa's avatar Hiroki Nakagawa Committed by Commit Bot

Worker: Stop using WorkerThread::TerminateAllWorkersForTesting() in unit tests

TerminateAllWorkersForTesting() causes deadlock depending on timing. This CL
replaces the function with a combination of TerminateForTesting() and
WaitForShutdownForTesting() that are equivalent to
TerminateAllWorkersForTesting() but don't acquire the lock.

TerminateAllWorkersForTesting() on the main thread calls WaitableEvent::Wait()
while that is holding the ThreadSetMutex(). The mutex is also referenced from
the worker thread during worker startup in InitializeOnWorkerThread(). Deadlock
happens when the main thread starts termination immediately before the worker
thread tries to acquire the lock to insert itself in the thread set.

Note: TerminateAllWorkersForTesting() is still used from LeakDetector. I'll try
to remove/change it in a separate CL.

Bug: 1003217
Change-Id: I7896057c1c65a830c621e120123558403a94c951
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1839224
Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: default avatarKenichi Ishibashi <bashi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#702767}
parent e5f8f85c
...@@ -276,7 +276,8 @@ TEST_F(WorkerThreadTest, SyncTerminate_OnIdle) { ...@@ -276,7 +276,8 @@ TEST_F(WorkerThreadTest, SyncTerminate_OnIdle) {
// idle. // idle.
worker_thread_->WaitForInit(); worker_thread_->WaitForInit();
WorkerThread::TerminateAllWorkersForTesting(); worker_thread_->TerminateForTesting();
worker_thread_->WaitForShutdownForTesting();
// The worker thread may gracefully shut down before forcible termination // The worker thread may gracefully shut down before forcible termination
// runs. // runs.
...@@ -296,19 +297,19 @@ TEST_F(WorkerThreadTest, AsyncTerminate_ImmediatelyAfterStart) { ...@@ -296,19 +297,19 @@ TEST_F(WorkerThreadTest, AsyncTerminate_ImmediatelyAfterStart) {
EXPECT_EQ(ExitCode::kGracefullyTerminated, GetExitCode()); EXPECT_EQ(ExitCode::kGracefullyTerminated, GetExitCode());
} }
// Disabled due to flakiness: https://crbug.com/1003217. TEST_F(WorkerThreadTest, SyncTerminate_ImmediatelyAfterStart) {
TEST_F(WorkerThreadTest, DISABLED_SyncTerminate_ImmediatelyAfterStart) {
ExpectReportingCallsForWorkerPossiblyTerminatedBeforeInitialization(); ExpectReportingCallsForWorkerPossiblyTerminatedBeforeInitialization();
Start(); Start();
// There are two possible cases depending on timing: // There are two possible cases depending on timing:
// (1) If the thread hasn't been initialized on the worker thread yet, // (1) If the thread hasn't been initialized on the worker thread yet,
// TerminateAllWorkersForTesting() should wait for initialization and shut // TerminateForTesting() should wait for initialization and shut down the
// down the thread immediately after that. // thread immediately after that.
// (2) If the thread has already been initialized on the worker thread, // (2) If the thread has already been initialized on the worker thread,
// TerminateAllWorkersForTesting() should synchronously forcibly terminates // TerminateForTesting() should synchronously forcibly terminates the worker
// the worker execution. // script execution.
WorkerThread::TerminateAllWorkersForTesting(); worker_thread_->TerminateForTesting();
worker_thread_->WaitForShutdownForTesting();
ExitCode exit_code = GetExitCode(); ExitCode exit_code = GetExitCode();
EXPECT_TRUE(ExitCode::kGracefullyTerminated == exit_code || EXPECT_TRUE(ExitCode::kGracefullyTerminated == exit_code ||
ExitCode::kSyncForciblyTerminated == exit_code); ExitCode::kSyncForciblyTerminated == exit_code);
...@@ -343,9 +344,9 @@ TEST_F(WorkerThreadTest, SyncTerminate_WhileTaskIsRunning) { ...@@ -343,9 +344,9 @@ TEST_F(WorkerThreadTest, SyncTerminate_WhileTaskIsRunning) {
StartWithSourceCodeNotToFinish(); StartWithSourceCodeNotToFinish();
reporting_proxy_->WaitUntilScriptEvaluation(); reporting_proxy_->WaitUntilScriptEvaluation();
// TerminateAllWorkersForTesting() synchronously terminates the worker // TerminateForTesting() synchronously terminates the worker script execution.
// execution. worker_thread_->TerminateForTesting();
WorkerThread::TerminateAllWorkersForTesting(); worker_thread_->WaitForShutdownForTesting();
EXPECT_EQ(ExitCode::kSyncForciblyTerminated, GetExitCode()); EXPECT_EQ(ExitCode::kSyncForciblyTerminated, GetExitCode());
} }
...@@ -362,9 +363,10 @@ TEST_F(WorkerThreadTest, ...@@ -362,9 +363,10 @@ TEST_F(WorkerThreadTest,
EXPECT_TRUE(IsForcibleTerminationTaskScheduled()); EXPECT_TRUE(IsForcibleTerminationTaskScheduled());
EXPECT_EQ(ExitCode::kNotTerminated, GetExitCode()); EXPECT_EQ(ExitCode::kNotTerminated, GetExitCode());
// TerminateAllWorkersForTesting() should overtake the scheduled forcible // TerminateForTesting() should overtake the scheduled forcible termination
// termination task. // task.
WorkerThread::TerminateAllWorkersForTesting(); worker_thread_->TerminateForTesting();
worker_thread_->WaitForShutdownForTesting();
EXPECT_FALSE(IsForcibleTerminationTaskScheduled()); EXPECT_FALSE(IsForcibleTerminationTaskScheduled());
EXPECT_EQ(ExitCode::kSyncForciblyTerminated, GetExitCode()); EXPECT_EQ(ExitCode::kSyncForciblyTerminated, GetExitCode());
} }
......
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