Revert "[wasm] Run foreground compilation tasks as normal tasks"

This reverts commit 1520a851.

Reason for revert: This CL does not do what it should. All tasks which access the isolate have to be cancelable to guarantee that the isolate still exists when the task is executed. Foreground compilation tasks access the isolate, so they cannot be just normal tasks.

Original change's description:
> [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}

TBR=mtrofin@chromium.org,ahaas@chromium.org,clemensh@chromium.org

Change-Id: I60fab90b46d70c703d827816503e7e23b8c50251
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/558284Reviewed-by: Andreas Haas <ahaas@chromium.org>
Commit-Queue: Andreas Haas <ahaas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#46353}

src/wasm/compilation-manager.cc

@@ -19,13 +19,10 @@ void CompilationManager::StartAsyncCompileJob(
   job->Start();
 }
 
-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::RemoveJob(AsyncCompileJob* job) {
+  size_t num_removed = jobs_.erase(job);
+  USE(num_removed);
+  DCHECK_EQ(1, num_removed);
 }
 
 void CompilationManager::TearDown() { jobs_.clear(); }

src/wasm/compilation-manager.h

@@ -26,9 +26,8 @@ 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. It will be deleted when
-  // the returned shared pointer expires.
-  std::shared_ptr<AsyncCompileJob> RemoveJob(AsyncCompileJob* job);
+  // Removes {job} from the list of active compile jobs. This will delete {job}.
+  void RemoveJob(AsyncCompileJob* job);
 
   void TearDown();
 

src/wasm/module-compiler.cc

@@ -1916,15 +1916,13 @@ 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
@@ -1956,34 +1954,30 @@ class AsyncCompileJob::CompileStep {
   const size_t num_background_tasks_;
 };
 
-class AsyncCompileJob::ForegroundCompileTask : NON_EXPORTED_BASE(public Task) {
+class AsyncCompileJob::CompileTask : public CancelableTask {
  public:
-  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) {}
+  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) {}
 
   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 ForegroundCompileTask(this));
+      reinterpret_cast<v8::Isolate*>(isolate_), new CompileTask(this, true));
 }
 
 template <typename State, typename... Args>
@@ -1995,7 +1989,7 @@ void AsyncCompileJob::DoSync(Args&&... args) {
 
 void AsyncCompileJob::StartBackgroundTask() {
   V8::GetCurrentPlatform()->CallOnBackgroundThread(
-      new BackgroundCompileTask(this), v8::Platform::kShortRunningTask);
+      new CompileTask(this, false), v8::Platform::kShortRunningTask);
 }
 
 template <typename State, typename... Args>

src/wasm/module-compiler.h

@@ -286,8 +286,7 @@ class AsyncCompileJob {
   ~AsyncCompileJob();
 
  private:
-  class ForegroundCompileTask;
-  class BackgroundCompileTask;
+  class CompileTask;
   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);