[d8] Avoid lock-order-inversion warning in the DefaultPlatform

This CL fixes two lock-order-inversion warning in the DefaultPlatform.
The problem was that during shutdown locks get taken in the oposite
order than during initialization.

The first two conflicting locks were the WasmEngine lock and the
lock of the DefaultTaskRunner. During WasmEngine initialization, when
the WasmEngine lock is hold, a foreground task is scheduled, which
requires the TaskRunner lock. During shutdown, the task queue of the
TaskRunner gets drained while holding the TaskRunner lock. Thereby
the destructors of the tasks get executed, and the LogCode task of
the WasmEngine thereby acquires the WasmEngine lock.

The second conflict happens between the WasmEngine lock and the
DefaultPlatform lock, where the DefaultPlatform lock is taken during
WasmEngine initialization when the ForegroundTaskrunner is acquired.
During Shutdown, the DefaultPlatform lock was hold while the task
queue was drained, as described above.

Bug: chromium:1346250
Change-Id: Ib67d0c6cad1372e7c592f40bbe68b0ae31b2976b
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3782796
Commit-Queue: Andreas Haas <ahaas@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#81911}

src/libplatform/default-foreground-task-runner.cc

@@ -27,11 +27,37 @@ DefaultForegroundTaskRunner::DefaultForegroundTaskRunner(
     : idle_task_support_(idle_task_support), time_function_(time_function) {}
 
 void DefaultForegroundTaskRunner::Terminate() {
-  base::MutexGuard guard(&lock_);
-  terminated_ = true;
-
+  {
+    base::MutexGuard guard(&lock_);
+    terminated_ = true;
+  }
   // Drain the task queues.
-  while (!task_queue_.empty()) task_queue_.pop_front();
+  // We have to do this lock dance here to avoid a lock order inversion warning
+  // of TSAN: during initialization, the WasmEngine lock gets taken before the
+  // TaskRunner lock. The TaskRunner lock is taken during WasmEngine
+  // initialization when the ForegroundTaskRunner gets acquire. During shutdown,
+  // the TaskRunner lock gets taken before the WasmEngine lock. The WasmEngine
+  // lock gets taken in the destructor of the LogCode task.
+  // In the code below we pop one task at a time from the task queue while
+  // holding the TaskRunner lock, but delete the task only after releasing the
+  // TaskRunner lock. Thereby we avoid holding the TaskRunner lock when the
+  // destructor of the task may take other locks.
+  while (true) {
+    std::unique_ptr<Task> task;
+    {
+      base::MutexGuard guard(&lock_);
+      if (task_queue_.empty()) break;
+      task = std::move(task_queue_.front().second);
+      task_queue_.pop_front();
+    }
+    // Reset the unique_ptr just for readability, to show that the task gets
+    // deallocated here without holding the lock.
+    task.reset();
+  }
+
+  // TODO(v8): If it ever becomes necessary, do the same lock dance for delayed
+  // tasks and idle tasks as above.
+  base::MutexGuard guard(&lock_);
   while (!delayed_task_queue_.empty()) delayed_task_queue_.pop();
   while (!idle_task_queue_.empty()) idle_task_queue_.pop();
 }

src/libplatform/default-platform.cc

@@ -273,12 +273,16 @@ v8::PageAllocator* DefaultPlatform::GetPageAllocator() {
 }
 
 void DefaultPlatform::NotifyIsolateShutdown(Isolate* isolate) {
-  base::MutexGuard guard(&lock_);
-  auto it = foreground_task_runner_map_.find(isolate);
-  if (it != foreground_task_runner_map_.end()) {
-    it->second->Terminate();
-    foreground_task_runner_map_.erase(it);
+  std::shared_ptr<DefaultForegroundTaskRunner> taskrunner;
+  {
+    base::MutexGuard guard(&lock_);
+    auto it = foreground_task_runner_map_.find(isolate);
+    if (it != foreground_task_runner_map_.end()) {
+      taskrunner = it->second;
+      foreground_task_runner_map_.erase(it);
+    }
   }
+  taskrunner->Terminate();
 }
 
 }  // namespace platform