Worker: Use lock annotations in WorkerThread

Lock annotations mechanism was ported from Abseil in this CL:
https://chromium-review.googlesource.com/c/chromium/src/+/885506

This mechanism provides Clang's built-in support for static lock analysis. This
would be really useful for workers, especially WorkerThread.

Bug: 812222
Change-Id: I88d4e24ea8a2d93edf126d61148d4c7aec060901
Reviewed-on: https://chromium-review.googlesource.com/919321Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537294}

third_party/WebKit/Source/core/workers/WorkerThread.cpp

@@ -151,7 +151,7 @@ void WorkerThread::Terminate() {
   DCHECK(IsMainThread());
 
   {
-    MutexLocker lock(thread_state_mutex_);
+    MutexLocker lock(mutex_);
     if (requested_to_terminate_)
       return;
     requested_to_terminate_ = true;
@@ -241,13 +241,13 @@ ThreadableLoadingContext* WorkerThread::GetLoadingContext() {
 
 void WorkerThread::AppendDebuggerTask(CrossThreadClosure task) {
   DCHECK(IsMainThread());
-  if (requested_to_terminate_)
+  if (CheckRequestedToTerminate())
     return;
   inspector_task_runner_->AppendTask(CrossThreadBind(
       &WorkerThread::PerformDebuggerTaskOnWorkerThread,
       CrossThreadUnretained(this), WTF::Passed(std::move(task))));
   {
-    MutexLocker lock(thread_state_mutex_);
+    MutexLocker lock(mutex_);
     if (GetIsolate() && thread_state_ != ThreadState::kReadyToShutdown)
       inspector_task_runner_->InterruptAndRunAllTasksDontWait(GetIsolate());
   }
@@ -305,7 +305,7 @@ PlatformThreadId WorkerThread::GetPlatformThreadId() {
 }
 
 bool WorkerThread::IsForciblyTerminated() {
-  MutexLocker lock(thread_state_mutex_);
+  MutexLocker lock(mutex_);
   switch (exit_code_) {
     case ExitCode::kNotTerminated:
     case ExitCode::kGracefullyTerminated:
@@ -322,7 +322,7 @@ bool WorkerThread::IsForciblyTerminated() {
 }
 
 ExitCode WorkerThread::GetExitCodeForTesting() {
-  MutexLocker lock(thread_state_mutex_);
+  MutexLocker lock(mutex_);
   return exit_code_;
 }
 
@@ -353,10 +353,8 @@ void WorkerThread::ScheduleToTerminateScriptExecution() {
       forcible_termination_delay_);
 }
 
-bool WorkerThread::ShouldTerminateScriptExecution(const MutexLocker& lock) {
+bool WorkerThread::ShouldTerminateScriptExecution() {
   DCHECK(IsMainThread());
-  DCHECK(IsThreadStateMutexLocked(lock));
-
   switch (thread_state_) {
     case ThreadState::kNotStarted:
       // Shutdown sequence will surely start during initialization sequence
@@ -378,13 +376,13 @@ bool WorkerThread::ShouldTerminateScriptExecution(const MutexLocker& lock) {
 
 void WorkerThread::EnsureScriptExecutionTerminates(ExitCode exit_code) {
   DCHECK(IsMainThread());
-  MutexLocker lock(thread_state_mutex_);
-  if (!ShouldTerminateScriptExecution(lock))
+  MutexLocker lock(mutex_);
+  if (!ShouldTerminateScriptExecution())
     return;
 
   DCHECK(exit_code == ExitCode::kSyncForciblyTerminated ||
          exit_code == ExitCode::kAsyncForciblyTerminated);
-  SetExitCode(lock, exit_code);
+  SetExitCode(exit_code);
 
   GetIsolate()->TerminateExecution();
   forcible_termination_task_handle_.Cancel();
@@ -409,12 +407,9 @@ void WorkerThread::InitializeOnWorkerThread(
     const WTF::Optional<WorkerBackingThreadStartupData>& thread_startup_data,
     WorkerInspectorProxy::PauseOnWorkerStart pause_on_start) {
   DCHECK(IsCurrentThread());
-  DCHECK_EQ(ThreadState::kNotStarted, thread_state_);
-
-  KURL script_url = global_scope_creation_params->script_url;
-
   {
-    MutexLocker lock(thread_state_mutex_);
+    MutexLocker lock(mutex_);
+    DCHECK_EQ(ThreadState::kNotStarted, thread_state_);
 
     if (IsOwningBackingThread()) {
       DCHECK(thread_startup_data.has_value());
@@ -440,13 +435,13 @@ void WorkerThread::InitializeOnWorkerThread(
           GlobalScope()->ScriptController()->GetContext());
     }
 
-    SetThreadState(lock, ThreadState::kRunning);
+    SetThreadState(ThreadState::kRunning);
   }
 
   if (pause_on_start == WorkerInspectorProxy::PauseOnWorkerStart::kPause)
     StartRunningDebuggerTasksOnPauseOnWorkerThread();
 
-  if (CheckRequestedToTerminateOnWorkerThread()) {
+  if (CheckRequestedToTerminate()) {
     // Stop further worker tasks from running after this point. WorkerThread
     // was requested to terminate before initialization or during running
     // debugger tasks. PerformShutdownOnWorkerThread() will be called soon.
@@ -481,12 +476,12 @@ void WorkerThread::ImportModuleScriptOnWorkerThread(
 void WorkerThread::PrepareForShutdownOnWorkerThread() {
   DCHECK(IsCurrentThread());
   {
-    MutexLocker lock(thread_state_mutex_);
+    MutexLocker lock(mutex_);
     if (thread_state_ == ThreadState::kReadyToShutdown)
       return;
-    SetThreadState(lock, ThreadState::kReadyToShutdown);
+    SetThreadState(ThreadState::kReadyToShutdown);
     if (exit_code_ == ExitCode::kNotTerminated)
-      SetExitCode(lock, ExitCode::kGracefullyTerminated);
+      SetExitCode(ExitCode::kGracefullyTerminated);
   }
 
   inspector_task_runner_->Kill();
@@ -509,8 +504,13 @@ void WorkerThread::PrepareForShutdownOnWorkerThread() {
 
 void WorkerThread::PerformShutdownOnWorkerThread() {
   DCHECK(IsCurrentThread());
-  DCHECK(CheckRequestedToTerminateOnWorkerThread());
-  DCHECK_EQ(ThreadState::kReadyToShutdown, thread_state_);
+#if DCHECK_IS_ON()
+  {
+    MutexLocker lock(mutex_);
+    DCHECK(requested_to_terminate_);
+    DCHECK_EQ(ThreadState::kReadyToShutdown, thread_state_);
+  }
+#endif
 
   if (IsOwningBackingThread())
     GetWorkerBackingThread().ShutdownOnBackingThread();
@@ -529,7 +529,7 @@ void WorkerThread::PerformDebuggerTaskOnWorkerThread(CrossThreadClosure task) {
   InspectorTaskRunner::IgnoreInterruptsScope scope(
       inspector_task_runner_.get());
   {
-    MutexLocker lock(thread_state_mutex_);
+    MutexLocker lock(mutex_);
     DCHECK_EQ(ThreadState::kRunning, thread_state_);
     running_debugger_task_ = true;
   }
@@ -537,7 +537,7 @@ void WorkerThread::PerformDebuggerTaskOnWorkerThread(CrossThreadClosure task) {
   std::move(task).Run();
   ThreadDebugger::IdleStarted(GetIsolate());
   {
-    MutexLocker lock(thread_state_mutex_);
+    MutexLocker lock(mutex_);
     running_debugger_task_ = false;
   }
 }
@@ -550,9 +550,7 @@ void WorkerThread::PerformDebuggerTaskDontWaitOnWorkerThread() {
     std::move(task).Run();
 }
 
-void WorkerThread::SetThreadState(const MutexLocker& lock,
-                                  ThreadState next_thread_state) {
-  DCHECK(IsThreadStateMutexLocked(lock));
+void WorkerThread::SetThreadState(ThreadState next_thread_state) {
   switch (next_thread_state) {
     case ThreadState::kNotStarted:
       NOTREACHED();
@@ -568,24 +566,13 @@ void WorkerThread::SetThreadState(const MutexLocker& lock,
   }
 }
 
-void WorkerThread::SetExitCode(const MutexLocker& lock, ExitCode exit_code) {
-  DCHECK(IsThreadStateMutexLocked(lock));
+void WorkerThread::SetExitCode(ExitCode exit_code) {
   DCHECK_EQ(ExitCode::kNotTerminated, exit_code_);
   exit_code_ = exit_code;
 }
 
-bool WorkerThread::IsThreadStateMutexLocked(const MutexLocker& /* unused */) {
-#if DCHECK_IS_ON()
-  // Mutex::locked() is available only if DCHECK_IS_ON() is true.
-  return thread_state_mutex_.Locked();
-#else
-  // Otherwise, believe the given MutexLocker holds |m_threadStateMutex|.
-  return true;
-#endif
-}
-
-bool WorkerThread::CheckRequestedToTerminateOnWorkerThread() {
-  MutexLocker lock(thread_state_mutex_);
+bool WorkerThread::CheckRequestedToTerminate() {
+  MutexLocker lock(mutex_);
   return requested_to_terminate_;
 }
 

third_party/WebKit/Source/core/workers/WorkerThread.h

@@ -31,6 +31,7 @@
 
 #include "base/memory/scoped_refptr.h"
 #include "base/single_thread_task_runner.h"
+#include "base/thread_annotations.h"
 #include "base/unguessable_token.h"
 #include "core/CoreExport.h"
 #include "core/frame/csp/ContentSecurityPolicy.h"
@@ -118,7 +119,7 @@ class CORE_EXPORT WorkerThread : public WebThread::TaskObserver {
   // the underlying thread. This task may be blocked by JavaScript execution on
   // the worker thread, so this function also forcibly terminates JavaScript
   // execution after a certain grace period.
-  void Terminate();
+  void Terminate() LOCKS_EXCLUDED(mutex_);
 
   // Terminates the worker thread. Subclasses of WorkerThread can override this
   // to do cleanup. The default behavior is to call Terminate() and
@@ -154,7 +155,7 @@ class CORE_EXPORT WorkerThread : public WebThread::TaskObserver {
   }
 
   // Only callable on the main thread.
-  void AppendDebuggerTask(CrossThreadClosure);
+  void AppendDebuggerTask(CrossThreadClosure) LOCKS_EXCLUDED(mutex_);
 
   // Only callable on the main thread.
   const base::UnguessableToken& GetDevToolsWorkerToken() const {
@@ -187,10 +188,10 @@ class CORE_EXPORT WorkerThread : public WebThread::TaskObserver {
 
   PlatformThreadId GetPlatformThreadId();
 
-  bool IsForciblyTerminated();
+  bool IsForciblyTerminated() LOCKS_EXCLUDED(mutex_);
 
   void WaitForShutdownForTesting() { shutdown_event_->Wait(); }
-  ExitCode GetExitCodeForTesting();
+  ExitCode GetExitCodeForTesting() LOCKS_EXCLUDED(mutex_);
 
   ParentFrameTaskRunners* GetParentFrameTaskRunners() const {
     return parent_frame_task_runners_.Get();
@@ -224,11 +225,7 @@ class CORE_EXPORT WorkerThread : public WebThread::TaskObserver {
   FRIEND_TEST_ALL_PREFIXES(WorkerThreadTest,
                            Terminate_WhileDebuggerTaskIsRunning);
 
-  // Represents the state of this worker thread. A caller may need to acquire
-  // a lock |m_threadStateMutex| before accessing this:
-  //   - Only the worker thread can set this with the lock.
-  //   - The worker thread can read this without the lock.
-  //   - The main thread can read this with the lock.
+  // Represents the state of this worker thread.
   enum class ThreadState {
     kNotStarted,
     kRunning,
@@ -252,23 +249,22 @@ class CORE_EXPORT WorkerThread : public WebThread::TaskObserver {
   void ScheduleToTerminateScriptExecution();
 
   // Returns true if we should synchronously terminate the script execution so
-  // that a shutdown task can be handled by the thread event loop. This must be
-  // called with |m_threadStateMutex| acquired.
-  bool ShouldTerminateScriptExecution(const MutexLocker&);
+  // that a shutdown task can be handled by the thread event loop.
+  bool ShouldTerminateScriptExecution() EXCLUSIVE_LOCKS_REQUIRED(mutex_);
 
   // Terminates worker script execution if the worker thread is running and not
   // already shutting down. Does not terminate if a debugger task is running,
   // because the debugger task is guaranteed to finish and it heavily uses V8
   // API calls which would crash after forcible script termination. Called on
   // the main thread.
-  void EnsureScriptExecutionTerminates(ExitCode);
+  void EnsureScriptExecutionTerminates(ExitCode) LOCKS_EXCLUDED(mutex_);
 
   // These are called in this order during worker thread startup.
   void InitializeSchedulerOnWorkerThread(WaitableEvent*);
   void InitializeOnWorkerThread(
       std::unique_ptr<GlobalScopeCreationParams>,
       const WTF::Optional<WorkerBackingThreadStartupData>&,
-      WorkerInspectorProxy::PauseOnWorkerStart);
+      WorkerInspectorProxy::PauseOnWorkerStart) LOCKS_EXCLUDED(mutex_);
 
   void EvaluateClassicScriptOnWorkerThread(
       const KURL& script_url,
@@ -279,36 +275,32 @@ class CORE_EXPORT WorkerThread : public WebThread::TaskObserver {
                                         network::mojom::FetchCredentialsMode);
 
   // These are called in this order during worker thread termination.
-  void PrepareForShutdownOnWorkerThread();
-  void PerformShutdownOnWorkerThread();
+  void PrepareForShutdownOnWorkerThread() LOCKS_EXCLUDED(mutex_);
+  void PerformShutdownOnWorkerThread() LOCKS_EXCLUDED(mutex_);
 
-  void PerformDebuggerTaskOnWorkerThread(CrossThreadClosure);
+  void PerformDebuggerTaskOnWorkerThread(CrossThreadClosure)
+      LOCKS_EXCLUDED(mutex_);
   void PerformDebuggerTaskDontWaitOnWorkerThread();
 
-  // These must be called with |m_threadStateMutex| acquired.
-  void SetThreadState(const MutexLocker&, ThreadState);
-  void SetExitCode(const MutexLocker&, ExitCode);
-  bool IsThreadStateMutexLocked(const MutexLocker&);
+  void SetThreadState(ThreadState) EXCLUSIVE_LOCKS_REQUIRED(mutex_);
+  void SetExitCode(ExitCode) EXCLUSIVE_LOCKS_REQUIRED(mutex_);
 
-  // This internally acquires |m_threadStateMutex|. If you already have the
-  // lock or you're on the main thread, you should consider directly accessing
-  // |m_requestedToTerminate|.
-  bool CheckRequestedToTerminateOnWorkerThread();
+  bool CheckRequestedToTerminate() LOCKS_EXCLUDED(mutex_);
 
   // A unique identifier among all WorkerThreads.
   const int worker_thread_id_;
 
-  // Set on the main thread and checked on both the main and worker threads.
-  bool requested_to_terminate_ = false;
+  // Set on the main thread.
+  bool requested_to_terminate_ GUARDED_BY(mutex_) = false;
 
   // Accessed only on the worker thread.
   bool paused_in_debugger_ = false;
 
-  // Set on the worker thread and checked on both the main and worker threads.
-  bool running_debugger_task_ = false;
+  // Set on the worker thread.
+  bool running_debugger_task_ GUARDED_BY(mutex_) = false;
 
-  ThreadState thread_state_ = ThreadState::kNotStarted;
-  ExitCode exit_code_ = ExitCode::kNotTerminated;
+  ThreadState thread_state_ GUARDED_BY(mutex_) = ThreadState::kNotStarted;
+  ExitCode exit_code_ GUARDED_BY(mutex_) = ExitCode::kNotTerminated;
 
   TimeDelta forcible_termination_delay_;
 
@@ -328,9 +320,10 @@ class CORE_EXPORT WorkerThread : public WebThread::TaskObserver {
   std::unique_ptr<scheduler::WorkerGlobalScopeScheduler>
       global_scope_scheduler_;
 
-  // This lock protects |m_globalScope|, |m_requestedToTerminate|,
-  // |m_threadState|, |m_runningDebuggerTask| and |m_exitCode|.
-  Mutex thread_state_mutex_;
+  // This lock protects shared states between the main thread and the worker
+  // thread. See thread-safety annotations (e.g., GUARDED_BY) in this header
+  // file.
+  Mutex mutex_;
 
   CrossThreadPersistent<ConsoleMessageStorage> console_message_storage_;
   CrossThreadPersistent<WorkerOrWorkletGlobalScope> global_scope_;

third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp

@@ -146,23 +146,25 @@ class WorkerThreadTest : public ::testing::Test {
 
 TEST_F(WorkerThreadTest, ShouldTerminateScriptExecution) {
   using ThreadState = WorkerThread::ThreadState;
-  MutexLocker dummy_lock(worker_thread_->thread_state_mutex_);
+
+  // SetExitCode() and ShouldTerminateScriptExecution() require the lock.
+  MutexLocker dummy_lock(worker_thread_->mutex_);
 
   EXPECT_EQ(ThreadState::kNotStarted, worker_thread_->thread_state_);
-  EXPECT_FALSE(worker_thread_->ShouldTerminateScriptExecution(dummy_lock));
+  EXPECT_FALSE(worker_thread_->ShouldTerminateScriptExecution());
 
-  worker_thread_->SetThreadState(dummy_lock, ThreadState::kRunning);
+  worker_thread_->SetThreadState(ThreadState::kRunning);
   EXPECT_FALSE(worker_thread_->running_debugger_task_);
-  EXPECT_TRUE(worker_thread_->ShouldTerminateScriptExecution(dummy_lock));
+  EXPECT_TRUE(worker_thread_->ShouldTerminateScriptExecution());
 
   worker_thread_->running_debugger_task_ = true;
-  EXPECT_FALSE(worker_thread_->ShouldTerminateScriptExecution(dummy_lock));
+  EXPECT_FALSE(worker_thread_->ShouldTerminateScriptExecution());
 
-  worker_thread_->SetThreadState(dummy_lock, ThreadState::kReadyToShutdown);
-  EXPECT_FALSE(worker_thread_->ShouldTerminateScriptExecution(dummy_lock));
+  worker_thread_->SetThreadState(ThreadState::kReadyToShutdown);
+  EXPECT_FALSE(worker_thread_->ShouldTerminateScriptExecution());
 
   // This is necessary to satisfy DCHECK in the dtor of WorkerThread.
-  worker_thread_->SetExitCode(dummy_lock, ExitCode::kGracefullyTerminated);
+  worker_thread_->SetExitCode(ExitCode::kGracefullyTerminated);
 }
 
 TEST_F(WorkerThreadTest, AsyncTerminate_OnIdle) {