Worker: Fix possible race condition in worker thread termination

In WorkerThread::PerformShutdownOnWorkerThread(), DidTerminateWorkerThread() may
induce the main thread to destroy the instance of WorkerThread, so accessing
|this| after the function call is dangerous. This CL avoids it.

Bug: 853520
Change-Id: If0b57ceb05fce97fa4d28d7ca9defb76e39d1c27
Reviewed-on: https://chromium-review.googlesource.com/1183005Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585769}

third_party/blink/renderer/core/workers/worker_thread.cc

@@ -86,6 +86,28 @@ static int GetNextWorkerThreadId() {
   return next_worker_thread_id;
 }
 
+// RefCountedWaitableEvent makes WaitableEvent thread-safe refcounted.
+// WorkerThread retains references to the event from both the parent context
+// thread and the worker thread with this wrapper. See
+// WorkerThread::PerformShutdownOnWorkerThread() for details.
+class WorkerThread::RefCountedWaitableEvent
+    : public WTF::ThreadSafeRefCounted<RefCountedWaitableEvent> {
+ public:
+  static scoped_refptr<RefCountedWaitableEvent> Create() {
+    return base::AdoptRef<RefCountedWaitableEvent>(new RefCountedWaitableEvent);
+  }
+
+  void Wait() { event_.Wait(); }
+  void Signal() { event_.Signal(); }
+
+ private:
+  RefCountedWaitableEvent() = default;
+
+  base::WaitableEvent event_;
+
+  DISALLOW_COPY_AND_ASSIGN(RefCountedWaitableEvent);
+};
+
 WorkerThread::~WorkerThread() {
   MutexLocker lock(ThreadSetMutex());
   DCHECK(WorkerThreads().Contains(this));
@@ -111,7 +133,7 @@ void WorkerThread::Start(
 
   // Synchronously initialize the per-global-scope scheduler to prevent someone
   // from posting a task to the thread before the scheduler is ready.
-  WaitableEvent waitable_event;
+  base::WaitableEvent waitable_event;
   GetWorkerBackingThread().BackingThread().PostTask(
       FROM_HERE,
       CrossThreadBind(&WorkerThread::InitializeSchedulerOnWorkerThread,
@@ -200,7 +222,7 @@ void WorkerThread::TerminateAllWorkersForTesting() {
   }
 
   for (WorkerThread* thread : threads)
-    thread->shutdown_event_->Wait();
+    thread->WaitForShutdownForTesting();
 
   // Destruct base::Thread and join the underlying system threads.
   for (WorkerThread* thread : threads)
@@ -302,6 +324,11 @@ bool WorkerThread::IsForciblyTerminated() {
   return false;
 }
 
+void WorkerThread::WaitForShutdownForTesting() {
+  DCHECK_CALLED_ON_VALID_THREAD(parent_thread_checker_);
+  shutdown_event_->Wait();
+}
+
 ExitCode WorkerThread::GetExitCodeForTesting() {
   MutexLocker lock(mutex_);
   return exit_code_;
@@ -336,9 +363,7 @@ WorkerThread::WorkerThread(WorkerReportingProxy& worker_reporting_proxy)
       forcible_termination_delay_(kForcibleTerminationDelay),
       devtools_worker_token_(base::UnguessableToken::Create()),
       worker_reporting_proxy_(worker_reporting_proxy),
-      shutdown_event_(std::make_unique<WaitableEvent>(
-          WaitableEvent::ResetPolicy::kManual,
-          WaitableEvent::InitialState::kNonSignaled)) {
+      shutdown_event_(RefCountedWaitableEvent::Create()) {
   MutexLocker lock(ThreadSetMutex());
   WorkerThreads().insert(this);
 }
@@ -389,7 +414,7 @@ void WorkerThread::EnsureScriptExecutionTerminates(ExitCode exit_code) {
 }
 
 void WorkerThread::InitializeSchedulerOnWorkerThread(
-    WaitableEvent* waitable_event) {
+    base::WaitableEvent* waitable_event) {
   DCHECK(IsCurrentThread());
   DCHECK(!worker_scheduler_);
   scheduler::WebThreadImplForWorkerScheduler& web_thread_for_worker =
@@ -550,14 +575,22 @@ void WorkerThread::PerformShutdownOnWorkerThread() {
 
   if (IsOwningBackingThread())
     GetWorkerBackingThread().ShutdownOnBackingThread();
-  // We must not touch workerBackingThread() from now on.
+  // We must not touch GetWorkerBackingThread() from now on.
+
+  // Keep the reference to the shutdown event in a local variable so that the
+  // worker thread can signal it even after calling DidTerminateWorkerThread(),
+  // which may destroy |this|.
+  scoped_refptr<RefCountedWaitableEvent> shutdown_event = shutdown_event_;
 
   // Notify the proxy that the WorkerOrWorkletGlobalScope has been disposed
   // of. This can free this thread object, hence it must not be touched
   // afterwards.
   GetWorkerReportingProxy().DidTerminateWorkerThread();
 
-  shutdown_event_->Signal();
+  // This should be signaled at the end because this may induce the main thread
+  // to clear the worker backing thread and stop thread execution in the system
+  // level.
+  shutdown_event->Signal();
 }
 
 void WorkerThread::SetThreadState(ThreadState next_thread_state) {

third_party/blink/renderer/core/workers/worker_thread.h

@@ -32,6 +32,7 @@
 #include "base/memory/scoped_refptr.h"
 #include "base/optional.h"
 #include "base/single_thread_task_runner.h"
+#include "base/synchronization/waitable_event.h"
 #include "base/thread_annotations.h"
 #include "base/unguessable_token.h"
 #include "services/network/public/mojom/fetch_api.mojom-shared.h"
@@ -43,7 +44,6 @@
 #include "third_party/blink/renderer/core/workers/worker_backing_thread_startup_data.h"
 #include "third_party/blink/renderer/core/workers/worker_inspector_proxy.h"
 #include "third_party/blink/renderer/platform/scheduler/public/worker_scheduler.h"
-#include "third_party/blink/renderer/platform/waitable_event.h"
 #include "third_party/blink/renderer/platform/web_task_runner.h"
 #include "third_party/blink/renderer/platform/wtf/forward.h"
 #include "third_party/blink/renderer/platform/wtf/functional.h"
@@ -182,7 +182,7 @@ class CORE_EXPORT WorkerThread : public WebThread::TaskObserver {
 
   bool IsForciblyTerminated() LOCKS_EXCLUDED(mutex_);
 
-  void WaitForShutdownForTesting() { shutdown_event_->Wait(); }
+  void WaitForShutdownForTesting();
   ExitCode GetExitCodeForTesting() LOCKS_EXCLUDED(mutex_);
 
   ParentExecutionContextTaskRunners* GetParentExecutionContextTaskRunners()
@@ -262,7 +262,7 @@ class CORE_EXPORT WorkerThread : public WebThread::TaskObserver {
   void EnsureScriptExecutionTerminates(ExitCode) LOCKS_EXCLUDED(mutex_);
 
   // These are called in this order during worker thread startup.
-  void InitializeSchedulerOnWorkerThread(WaitableEvent*);
+  void InitializeSchedulerOnWorkerThread(base::WaitableEvent*);
   void InitializeOnWorkerThread(
       std::unique_ptr<GlobalScopeCreationParams>,
       const base::Optional<WorkerBackingThreadStartupData>&,
@@ -323,8 +323,11 @@ class CORE_EXPORT WorkerThread : public WebThread::TaskObserver {
   CrossThreadPersistent<WorkerOrWorkletGlobalScope> global_scope_;
   CrossThreadPersistent<WorkerInspectorController> worker_inspector_controller_;
 
-  // Signaled when the thread completes termination on the worker thread.
-  std::unique_ptr<WaitableEvent> shutdown_event_;
+  // Signaled when the thread completes termination on the worker thread. Only
+  // the parent context thread should wait on this event after calling
+  // Terminate().
+  class RefCountedWaitableEvent;
+  scoped_refptr<RefCountedWaitableEvent> shutdown_event_;
 
   // Used to cancel a scheduled forcible termination task. See
   // mayForciblyTerminateExecution() for details.