[wasm] Remove the state from tasks of an AsyncCompileJob

There exists a hidden assumption in V8 that neither foreground nor
background tasks own any memory. For asynchronous WebAssembly
compilation this assumption was wrong, which causes crashes when V8 shut
down before the compilation finished.

With this CL I change the way asynchrous compilation happens. In the
existing implementation each compilation stage provided its own task
which could be spawned either in foreground or background. With this CL
each stage only provides a state, and a generic CompileTask executes on
that state. There exists exactly one state at a time.

To have exactly one state at a time I combined the stages
ExecuteCompilationUnits and FinishCompilationUnits to a single stage. In
addition I removed the WaitForBackgroundTasks stage and added a
CancelableTaskManager to the AsyncCompileJob instead to do the waiting.

BUG=v8:6436
R=clemensh@chromium.org, mtrofin@chromium.org

Change-Id: I2eb61f74235c65524ce720c474eaf99ae7472c81
Reviewed-on: https://chromium-review.googlesource.com/532993
Commit-Queue: Andreas Haas <ahaas@chromium.org>
Reviewed-by: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#45908}

src/wasm/module-compiler.cc

@@ -4,9 +4,11 @@
 
 #include <src/wasm/module-compiler.h>
 
-#include <src/counters.h>
+#include <atomic>
+
 #include "src/asmjs/asm-js.h"
 #include "src/assembler-inl.h"
+#include "src/counters.h"
 #include "src/property-descriptor.h"
 #include "src/wasm/compilation-manager.h"
 #include "src/wasm/module-decoder.h"
@@ -1853,6 +1855,7 @@ void AsyncCompileJob::Start() {
 }
 
 AsyncCompileJob::~AsyncCompileJob() {
+  background_task_manager_.CancelAndWait();
   for (auto d : deferred_handles_) delete d;
 }
 
@@ -1877,52 +1880,80 @@ void AsyncCompileJob::AsyncCompileSucceeded(Handle<Object> result) {
 
 // A closure to run a compilation step (either as foreground or background
 // task) and schedule the next step(s), if any.
-class AsyncCompileJob::CompileTask : NON_EXPORTED_BASE(public v8::Task) {
+class AsyncCompileJob::CompileState {
  public:
-  AsyncCompileJob* job_ = nullptr;
-  CompileTask() {}
-  void Run() override = 0;  // Force sub-classes to override Run().
-};
+  virtual ~CompileState() {}
+
+  void Run(bool on_foreground) {
+    if (on_foreground) {
+      DCHECK_EQ(1, job_->num_pending_foreground_tasks_--);
+      SaveContext saved_context(job_->isolate_);
+      job_->isolate_->set_context(*job_->context_);
+      RunInForeground();
+    } else {
+      RunInBackground();
+    }
+  }
 
-class AsyncCompileJob::AsyncCompileTask : public AsyncCompileJob::CompileTask {
+  virtual void RunInForeground() { UNREACHABLE(); }
+  virtual void RunInBackground() { UNREACHABLE(); }
+
+  virtual size_t NumberOfBackgroundTasks() { return 0; }
+
+  AsyncCompileJob* job_ = nullptr;
 };
 
-class AsyncCompileJob::SyncCompileTask : public AsyncCompileJob::CompileTask {
+class AsyncCompileJob::CompileTask : public CancelableTask {
  public:
-  void Run() final {
-    SaveContext saved_context(job_->isolate_);
-    job_->isolate_->set_context(*job_->context_);
-    RunImpl();
-  }
+  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_->state_->Run(on_foreground_); }
 
- protected:
-  virtual void RunImpl() = 0;
+ private:
+  AsyncCompileJob* job_;
+  bool on_foreground_;
 };
 
-template <typename Task, typename... Args>
-void AsyncCompileJob::DoSync(Args&&... args) {
-  static_assert(std::is_base_of<SyncCompileTask, Task>::value,
-                "Scheduled type must be sync");
-  Task* task = new Task(std::forward<Args>(args)...);
-  task->job_ = this;
+void AsyncCompileJob::StartForegroundTask() {
+  DCHECK_EQ(0, num_pending_foreground_tasks_++);
+
   V8::GetCurrentPlatform()->CallOnForegroundThread(
-      reinterpret_cast<v8::Isolate*>(isolate_), task);
+      reinterpret_cast<v8::Isolate*>(isolate_), new CompileTask(this, true));
+}
+
+template <typename State, typename... Args>
+void AsyncCompileJob::DoSync(Args&&... args) {
+  state_.reset(new State(std::forward<Args>(args)...));
+  state_->job_ = this;
+  StartForegroundTask();
 }
 
-template <typename Task, typename... Args>
+template <typename State, typename... Args>
 void AsyncCompileJob::DoAsync(Args&&... args) {
-  static_assert(std::is_base_of<AsyncCompileTask, Task>::value,
-                "Scheduled type must be async");
-  Task* task = new Task(std::forward<Args>(args)...);
-  task->job_ = this;
-  V8::GetCurrentPlatform()->CallOnBackgroundThread(
-      task, v8::Platform::kShortRunningTask);
+  state_.reset(new State(std::forward<Args>(args)...));
+  state_->job_ = this;
+  size_t end = state_->NumberOfBackgroundTasks();
+  for (size_t i = 0; i < end; ++i) {
+    V8::GetCurrentPlatform()->CallOnBackgroundThread(
+        new CompileTask(this, false), v8::Platform::kShortRunningTask);
+  }
 }
+
 //==========================================================================
 // Step 1: (async) Decode the module.
 //==========================================================================
-class AsyncCompileJob::DecodeModule : public AsyncCompileJob::AsyncCompileTask {
-  void Run() override {
+class AsyncCompileJob::DecodeModule : public AsyncCompileJob::CompileState {
+  size_t NumberOfBackgroundTasks() override { return 1; }
+
+  void RunInBackground() override {
     ModuleResult result;
     {
       DisallowHandleAllocation no_handle;
@@ -1947,13 +1978,13 @@ class AsyncCompileJob::DecodeModule : public AsyncCompileJob::AsyncCompileTask {
 //==========================================================================
 // Step 1b: (sync) Fail decoding the module.
 //==========================================================================
-class AsyncCompileJob::DecodeFail : public SyncCompileTask {
+class AsyncCompileJob::DecodeFail : public CompileState {
  public:
   explicit DecodeFail(ModuleResult result) : result_(std::move(result)) {}
 
  private:
   ModuleResult result_;
-  void RunImpl() override {
+  void RunInForeground() override {
     TRACE_COMPILE("(1b) Decoding failed.\n");
     HandleScope scope(job_->isolate_);
     ErrorThrower thrower(job_->isolate_, "AsyncCompile");
@@ -1966,14 +1997,14 @@ class AsyncCompileJob::DecodeFail : public SyncCompileTask {
 //==========================================================================
 // Step 2 (sync): Create heap-allocated data and start compile.
 //==========================================================================
-class AsyncCompileJob::PrepareAndStartCompile : public SyncCompileTask {
+class AsyncCompileJob::PrepareAndStartCompile : public CompileState {
  public:
   explicit PrepareAndStartCompile(std::unique_ptr<WasmModule> module)
       : module_(std::move(module)) {}
 
  private:
   std::unique_ptr<WasmModule> module_;
-  void RunImpl() override {
+  void RunInForeground() override {
     TRACE_COMPILE("(2) Prepare and start compile...\n");
     HandleScope scope(job_->isolate_);
 
@@ -2039,7 +2070,7 @@ class AsyncCompileJob::PrepareAndStartCompile : public SyncCompileTask {
     }
 
     // Start asynchronous compilation tasks.
-    job_->num_background_tasks_ =
+    size_t num_background_tasks =
         Max(static_cast<size_t>(1),
             Min(num_functions,
                 Min(static_cast<size_t>(FLAG_wasm_num_compilation_tasks),
@@ -2052,23 +2083,28 @@ class AsyncCompileJob::PrepareAndStartCompile : public SyncCompileTask {
 
     // Reopen all handles which should survive in the DeferredHandleScope.
     job_->ReopenHandlesInDeferredScope();
-    for (size_t i = 0; i < job_->num_background_tasks_; ++i) {
-      job_->DoAsync<ExecuteCompilationUnits>();
-    }
+    job_->DoAsync<ExecuteAndFinishCompilationUnits>(num_background_tasks);
   }
 };
 
 //==========================================================================
 // Step 3 (async x K tasks): Execute compilation units.
 //==========================================================================
-class AsyncCompileJob::ExecuteCompilationUnits : public AsyncCompileTask {
-  void Run() override {
+class AsyncCompileJob::ExecuteAndFinishCompilationUnits : public CompileState {
+ public:
+  explicit ExecuteAndFinishCompilationUnits(size_t num_compile_tasks)
+      : num_compile_tasks_(num_compile_tasks) {}
+
+  size_t NumberOfBackgroundTasks() override { return num_compile_tasks_; }
+
+  void RunInBackground() override {
     std::function<void()> StartFinishCompilationUnit = [this]() {
-      job_->DoSync<FinishCompilationUnits>();
+      if (!failed_) job_->StartForegroundTask();
     };
 
     TRACE_COMPILE("(3) Compiling...\n");
     for (;;) {
+      if (failed_) break;
       DisallowHandleAllocation no_handle;
       DisallowHeapAllocation no_allocation;
       if (!job_->compiler_->FetchAndExecuteCompilationUnit(
@@ -2076,17 +2112,11 @@ class AsyncCompileJob::ExecuteCompilationUnits : public AsyncCompileTask {
         break;
       }
     }
-    job_->compiler_->module_->pending_tasks.get()->Signal();
   }
-};
 
-//==========================================================================
-// Step 4 (sync): Finish compilation units.
-//==========================================================================
-class AsyncCompileJob::FinishCompilationUnits : public SyncCompileTask {
-  void RunImpl() override {
+  void RunInForeground() override {
     TRACE_COMPILE("(4a) Finishing compilation units...\n");
-    if (job_->failed_) {
+    if (failed_) {
       // The job failed already, no need to do more work.
       job_->compiler_->SetFinisherIsRunning(false);
       return;
@@ -2106,7 +2136,7 @@ class AsyncCompileJob::FinishCompilationUnits : public SyncCompileTask {
       if (thrower.error()) {
         // An error was detected, we stop compiling and wait for the
         // background tasks to finish.
-        job_->failed_ = true;
+        failed_ = true;
         break;
       } else if (result.is_null()) {
         // The working queue was empty, we break the loop. If new work units
@@ -2123,74 +2153,35 @@ class AsyncCompileJob::FinishCompilationUnits : public SyncCompileTask {
         // We reached the deadline. We reschedule this task and return
         // immediately. Since we rescheduled this task already, we do not set
         // the FinisherIsRunning flat to false.
-        job_->DoSync<FinishCompilationUnits>();
+        job_->StartForegroundTask();
         return;
       }
     }
     // This task finishes without being rescheduled. Therefore we set the
     // FinisherIsRunning flag to false.
     job_->compiler_->SetFinisherIsRunning(false);
-    if (thrower.error() || job_->outstanding_units_ == 0) {
-      // All compilation units are done. We still need to wait for the
-      // background tasks to shut down and only then is it safe to finish the
-      // compile and delete this job. We can wait for that to happen also
-      // in a background task.
-      job_->DoAsync<WaitForBackgroundTasks>(std::move(thrower));
-    }
-  }
-};
-
-//==========================================================================
-// Step 4b (async): Wait for all background tasks to finish.
-//==========================================================================
-class AsyncCompileJob::WaitForBackgroundTasks
-    : public AsyncCompileJob::AsyncCompileTask {
- public:
-  explicit WaitForBackgroundTasks(ErrorThrower thrower)
-      : thrower_(std::move(thrower)) {}
-
- private:
-  ErrorThrower thrower_;
-
-  void Run() override {
-    TRACE_COMPILE("(4b) Waiting for background tasks...\n");
-    // Bump next_unit_, such that background tasks stop processing the queue.
-    job_->compiler_->next_unit_.SetValue(
-        job_->compiler_->compilation_units_.size());
-    for (size_t i = 0; i < job_->num_background_tasks_; ++i) {
-      // We wait for it to finish.
-      job_->compiler_->module_->pending_tasks.get()->Wait();
+    if (thrower.error()) {
+      // Make sure all compilation tasks stopped running.
+      job_->background_task_manager_.CancelAndWait();
+      return job_->AsyncCompileFailed(thrower);
     }
-    if (thrower_.error()) {
-      job_->DoSync<FailCompile>(std::move(thrower_));
-    } else {
+    if (job_->outstanding_units_ == 0) {
+      // Make sure all compilation tasks stopped running.
+      job_->background_task_manager_.CancelAndWait();
       job_->DoSync<FinishCompile>();
     }
   }
-};
-
-//==========================================================================
-// Step 5a (sync): Fail compilation (reject promise).
-//==========================================================================
-class AsyncCompileJob::FailCompile : public SyncCompileTask {
- public:
-  explicit FailCompile(ErrorThrower thrower) : thrower_(std::move(thrower)) {}
 
  private:
-  ErrorThrower thrower_;
-
-  void RunImpl() override {
-    TRACE_COMPILE("(5a) Fail compilation...\n");
-    HandleScope scope(job_->isolate_);
-    return job_->AsyncCompileFailed(thrower_);
-  }
+  size_t num_compile_tasks_;
+  std::atomic<bool> failed_{false};
 };
 
 //==========================================================================
-// Step 5b (sync): Finish heap-allocated data structures.
+// Step 5 (sync): Finish heap-allocated data structures.
 //==========================================================================
-class AsyncCompileJob::FinishCompile : public SyncCompileTask {
-  void RunImpl() override {
+class AsyncCompileJob::FinishCompile : public CompileState {
+  void RunInForeground() override {
     TRACE_COMPILE("(5b) Finish compile...\n");
     HandleScope scope(job_->isolate_);
     // At this point, compilation has completed. Update the code table.
@@ -2255,8 +2246,8 @@ class AsyncCompileJob::FinishCompile : public SyncCompileTask {
 //==========================================================================
 // Step 6 (sync): Compile JS->wasm wrappers.
 //==========================================================================
-class AsyncCompileJob::CompileWrappers : public SyncCompileTask {
-  void RunImpl() override {
+class AsyncCompileJob::CompileWrappers : public CompileState {
+  void RunInForeground() override {
     TRACE_COMPILE("(6) Compile wrappers...\n");
     // Compile JS->wasm wrappers for exported functions.
     HandleScope scope(job_->isolate_);
@@ -2284,8 +2275,8 @@ class AsyncCompileJob::CompileWrappers : public SyncCompileTask {
 //==========================================================================
 // Step 7 (sync): Finish the module and resolve the promise.
 //==========================================================================
-class AsyncCompileJob::FinishModule : public SyncCompileTask {
-  void RunImpl() override {
+class AsyncCompileJob::FinishModule : public CompileState {
+  void RunInForeground() override {
     TRACE_COMPILE("(7) Finish module...\n");
     HandleScope scope(job_->isolate_);
     Handle<WasmModuleObject> result =

src/wasm/module-compiler.h

@@ -257,6 +257,20 @@ class AsyncCompileJob {
   ~AsyncCompileJob();
 
  private:
+  class CompileTask;
+  class CompileState;
+
+  // States of the AsyncCompileJob.
+  class DecodeModule;
+  class DecodeFail;
+  class PrepareAndStartCompile;
+  class ExecuteAndFinishCompilationUnits;
+  class WaitForBackgroundTasks;
+  class FinishCompilationUnits;
+  class FinishCompile;
+  class CompileWrappers;
+  class FinishModule;
+
   Isolate* isolate_;
   std::shared_ptr<Counters> counters_shared_;
   Counters* counters_;
@@ -267,7 +281,6 @@ class AsyncCompileJob {
   std::unique_ptr<ModuleCompiler> compiler_;
   std::unique_ptr<ModuleBytesEnv> module_bytes_env_;
 
-  bool failed_ = false;
   std::vector<DeferredHandles*> deferred_handles_;
   Handle<WasmModuleObject> module_object_;
   Handle<FixedArray> function_tables_;
@@ -276,7 +289,12 @@ class AsyncCompileJob {
   Handle<FixedArray> code_table_;
   std::unique_ptr<WasmInstance> temp_instance_ = nullptr;
   size_t outstanding_units_ = 0;
-  size_t num_background_tasks_ = 0;
+  std::unique_ptr<CompileState> state_;
+  CancelableTaskManager background_task_manager_;
+#if DEBUG
+  // Counts the number of pending foreground tasks.
+  int32_t num_pending_foreground_tasks_ = 0;
+#endif
 
   void ReopenHandlesInDeferredScope();
 
@@ -287,22 +305,10 @@ class AsyncCompileJob {
   template <typename Task, typename... Args>
   void DoSync(Args&&... args);
 
+  void StartForegroundTask();
+
   template <typename Task, typename... Args>
   void DoAsync(Args&&... args);
-
-  class CompileTask;
-  class AsyncCompileTask;
-  class SyncCompileTask;
-  class DecodeModule;
-  class DecodeFail;
-  class PrepareAndStartCompile;
-  class ExecuteCompilationUnits;
-  class WaitForBackgroundTasks;
-  class FinishCompilationUnits;
-  class FailCompile;
-  class FinishCompile;
-  class CompileWrappers;
-  class FinishModule;
 };
 
 }  // namespace wasm

test/mjsunit/wasm/graceful_shutdown.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 --wasm-async-compilation
+
+load("test/mjsunit/wasm/wasm-constants.js");
+load("test/mjsunit/wasm/wasm-module-builder.js");
+
+(function CompileFunctionsTest() {
+  // Create a big module.
+  var builder = new WasmModuleBuilder();
+
+  builder.addMemory(1, 1, true);
+  for (i = 0; i < 100; i++) {
+    builder.addFunction("sub" + i, kSig_i_i)
+      .addBody([                // --
+        kExprGetLocal, 0,       // --
+        kExprI32Const, i % 61,  // --
+        kExprI32Sub])           // --
+      .exportFunc()
+  }
+
+  var buffer = builder.toBuffer();
+  // Start the compilation but do not wait for the promise to resolve
+  // with assertPromiseResult. This should not cause a crash.
+  WebAssembly.compile(buffer).then(
+    () => { print("success")},
+    () => { print("failed"); });
+})();