[wasm] Run foreground compilation tasks as normal tasks

This CL makes foreground compilation tasks normal (i.e. not cancelable)
again, because otherwise a deadlock can happen. I think the reason why
the foreground tasks were cancelable was to make sure that all tasks
either finish correctly or get canceled. However, since the isolate can
only shut down on the main thread, this means that the foreground task
should have already finished when the isolate shuts down, or it should
not have started at all. I reordered the deletion of the AsyncCompileJob
though to make sure that an AsyncCompileJob is removed from
CompilationManager before its promise is resolved.

Here is the deadlock: The JS code which is executed after a promise is
resolved is executed within the task which resolves the promise. In case
of async compilation this means that some JS code is executed within a
CompileTask. In JS, the shutdown of the isolate can be triggered. During
the shutdown of the isolate, the CancelableTaskManager waits for all
registered cancelable tasks to complete, including the CompileTask of
async compilation. This means that the CancelableTaskManager waits for
itself to finish, which is a deadlock.

R=clemensh@chromium.org, mtrofin@chromium.org

Change-Id: I9f8c7fb2cfc5b9bfc53c761010b1590293bb82c9
Reviewed-on: https://chromium-review.googlesource.com/554733
Commit-Queue: Andreas Haas <ahaas@chromium.org>
Reviewed-by: Mircea Trofin <mtrofin@chromium.org>
Reviewed-by: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#46343}

src/wasm/compilation-manager.cc

@@ -19,10 +19,13 @@ void CompilationManager::StartAsyncCompileJob(
   job->Start();
 }
 
-void CompilationManager::RemoveJob(AsyncCompileJob* job) {
-  size_t num_removed = jobs_.erase(job);
-  USE(num_removed);
-  DCHECK_EQ(1, num_removed);
+std::shared_ptr<AsyncCompileJob> CompilationManager::RemoveJob(
+    AsyncCompileJob* job) {
+  auto item = jobs_.find(job);
+  DCHECK(item != jobs_.end());
+  std::shared_ptr<AsyncCompileJob> result = item->second;
+  jobs_.erase(item);
+  return result;
 }
 
 void CompilationManager::TearDown() { jobs_.clear(); }

src/wasm/compilation-manager.h

@@ -26,8 +26,9 @@ class CompilationManager {
                             std::unique_ptr<byte[]> bytes_copy, size_t length,
                             Handle<Context> context, Handle<JSPromise> promise);
 
-  // Removes {job} from the list of active compile jobs. This will delete {job}.
-  void RemoveJob(AsyncCompileJob* job);
+  // Removes {job} from the list of active compile jobs. It will be deleted when
+  // the returned shared pointer expires.
+  std::shared_ptr<AsyncCompileJob> RemoveJob(AsyncCompileJob* job);
 
   void TearDown();
 

src/wasm/module-compiler.cc

@@ -1916,13 +1916,15 @@ void AsyncCompileJob::ReopenHandlesInDeferredScope() {
 }
 
 void AsyncCompileJob::AsyncCompileFailed(ErrorThrower& thrower) {
+  std::shared_ptr<AsyncCompileJob> job =
+      isolate_->wasm_compilation_manager()->RemoveJob(this);
   RejectPromise(isolate_, context_, thrower, module_promise_);
-  isolate_->wasm_compilation_manager()->RemoveJob(this);
 }
 
 void AsyncCompileJob::AsyncCompileSucceeded(Handle<Object> result) {
+  std::shared_ptr<AsyncCompileJob> job =
+      isolate_->wasm_compilation_manager()->RemoveJob(this);
   ResolvePromise(isolate_, context_, module_promise_, result);
-  isolate_->wasm_compilation_manager()->RemoveJob(this);
 }
 
 // A closure to run a compilation step (either as foreground or background
@@ -1954,30 +1956,34 @@ class AsyncCompileJob::CompileStep {
   const size_t num_background_tasks_;
 };
 
-class AsyncCompileJob::CompileTask : public CancelableTask {
+class AsyncCompileJob::ForegroundCompileTask : NON_EXPORTED_BASE(public Task) {
  public:
-  CompileTask(AsyncCompileJob* job, bool on_foreground)
-      // We only manage the background tasks with the {CancelableTaskManager} of
-      // the {AsyncCompileJob}. Foreground tasks are managed by the system's
-      // {CancelableTaskManager}. Background tasks cannot spawn tasks managed by
-      // their own task manager.
-      : CancelableTask(on_foreground ? job->isolate_->cancelable_task_manager()
-                                     : &job->background_task_manager_),
-        job_(job),
-        on_foreground_(on_foreground) {}
+  explicit ForegroundCompileTask(AsyncCompileJob* job) : job_(job) {}
+  void Run() override { job_->step_->Run(on_foreground_); }
+
+ private:
+  static constexpr bool on_foreground_ = true;
+  AsyncCompileJob* job_;
+};
+
+class AsyncCompileJob::BackgroundCompileTask : public CancelableTask {
+ public:
+  explicit BackgroundCompileTask(AsyncCompileJob* job)
+      : CancelableTask(&job->background_task_manager_), job_(job) {}
 
   void RunInternal() override { job_->step_->Run(on_foreground_); }
 
  private:
+  static constexpr bool on_foreground_ = false;
   AsyncCompileJob* job_;
-  bool on_foreground_;
 };
 
 void AsyncCompileJob::StartForegroundTask() {
   DCHECK_EQ(0, num_pending_foreground_tasks_++);
 
   V8::GetCurrentPlatform()->CallOnForegroundThread(
-      reinterpret_cast<v8::Isolate*>(isolate_), new CompileTask(this, true));
+      reinterpret_cast<v8::Isolate*>(isolate_),
+      new ForegroundCompileTask(this));
 }
 
 template <typename State, typename... Args>
@@ -1989,7 +1995,7 @@ void AsyncCompileJob::DoSync(Args&&... args) {
 
 void AsyncCompileJob::StartBackgroundTask() {
   V8::GetCurrentPlatform()->CallOnBackgroundThread(
-      new CompileTask(this, false), v8::Platform::kShortRunningTask);
+      new BackgroundCompileTask(this), v8::Platform::kShortRunningTask);
 }
 
 template <typename State, typename... Args>

src/wasm/module-compiler.h

@@ -286,7 +286,8 @@ class AsyncCompileJob {
   ~AsyncCompileJob();
 
  private:
-  class CompileTask;
+  class ForegroundCompileTask;
+  class BackgroundCompileTask;
   class CompileStep;
 
   // States of the AsyncCompileJob.

test/mjsunit/regress/wasm/compile-then-quit.js

+// Copyright 2017 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --expose-wasm --allow-natives-syntax --wasm-async-compilation
+
+load('test/mjsunit/wasm/wasm-constants.js');
+load('test/mjsunit/wasm/wasm-module-builder.js');
+
+let builder = new WasmModuleBuilder();
+builder.addFunction('f', kSig_i_v).addBody([kExprI32Const, 42]).exportAs('f');
+let buffer = builder.toBuffer();
+
+// This test is meaningless if quit does not exist, e.g. with "d8 --omit-quit".
+assertPromiseResult(
+    WebAssembly.compile(buffer), typeof quit !== 'undefined' ? quit : _ => {
+      print('No quit() available');
+    }, assertUnreachable);