From be1b2d66c088da3e191956bd49ac07f55fb4e929 Mon Sep 17 00:00:00 2001
From: Clemens Hammacher <clemensh@chromium.org>
Date: Thu, 22 Mar 2018 11:36:56 +0100
Subject: [PATCH] [wasm] Fix deadlock on async compilation

See referenced bug: Async compilation can deadlock if a background task
queues the last compilation unit to be finished while the finisher
is already exiting because there was no more work.
This CL fixes this by making the finisher task check for new work after
setting the finisher_is_running_ flag to false.

R=ahaas@chromium.org
CC=kimanh@google.com

Bug: chromium:824681
Change-Id: If1f5700a9fdd5d150b36e37a5d14b692c2b0f3fb
Reviewed-on: https://chromium-review.googlesource.com/975301
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Reviewed-by: Andreas Haas <ahaas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#52139}
---
 src/wasm/module-compiler.cc                 | 30 ++++++++++++++++-----
 test/mjsunit/regress/wasm/regress-824681.js | 20 ++++++++++++++
 2 files changed, 43 insertions(+), 7 deletions(-)
 create mode 100644 test/mjsunit/regress/wasm/regress-824681.js

diff --git a/src/wasm/module-compiler.cc b/src/wasm/module-compiler.cc
index ce54162761..d0ea69dba6 100644
--- a/src/wasm/module-compiler.cc
+++ b/src/wasm/module-compiler.cc
@@ -113,6 +113,7 @@ class CompilationState {
       std::vector<std::unique_ptr<compiler::WasmCompilationUnit>>& units);
   std::unique_ptr<compiler::WasmCompilationUnit> GetNextCompilationUnit();
   std::unique_ptr<compiler::WasmCompilationUnit> GetNextExecutedUnit();
+  bool HasCompilationUnitToFinish();
 
   void OnError(Handle<Object> error, NotifyCompilationCallback notify);
   void OnFinishedUnit(NotifyCompilationCallback notify);
@@ -123,7 +124,8 @@ class CompilationState {
   void OnBackgroundTaskStopped();
   void RestartBackgroundTasks();
   // Only one foreground thread (finisher) is allowed to run at a time.
-  void SetFinisherIsRunning(bool value);
+  // {SetFinisherIsRunning} returns whether the flag changed its state.
+  bool SetFinisherIsRunning(bool value);
   void ScheduleFinisherTask();
 
   bool CanAcceptWork() const { return executed_units_.CanAcceptWork(); }
@@ -1488,10 +1490,21 @@ class FinishCompileTask : public CancelableTask {
         USE(result);
         Handle<Object> error = thrower.Reify();
         compilation_state_->OnError(error, NotifyCompilationCallback::kNotify);
+        compilation_state_->SetFinisherIsRunning(false);
         break;
       }
 
-      if (func_index < 0) break;
+      if (func_index < 0) {
+        // It might happen that a background task just scheduled a unit to be
+        // finished, but did not start a finisher task since the flag was still
+        // set. Check for this case, and continue if there is more work.
+        compilation_state_->SetFinisherIsRunning(false);
+        if (compilation_state_->HasCompilationUnitToFinish() &&
+            compilation_state_->SetFinisherIsRunning(true)) {
+          continue;
+        }
+        break;
+      }
 
       // Update the compilation state, and possibly notify
       // threads waiting for events.
@@ -1505,10 +1518,6 @@ class FinishCompileTask : public CancelableTask {
         return;
       }
     }
-
-    // This task finishes without being rescheduled. Therefore we set the
-    // FinisherIsRunning flag to false.
-    compilation_state_->SetFinisherIsRunning(false);
   }
 
  private:
@@ -3483,6 +3492,11 @@ CompilationState::GetNextExecutedUnit() {
   return std::unique_ptr<compiler::WasmCompilationUnit>();
 }
 
+bool CompilationState::HasCompilationUnitToFinish() {
+  base::LockGuard<base::Mutex> guard(&result_mutex_);
+  return !executed_units_.IsEmpty();
+}
+
 void CompilationState::OnError(Handle<Object> error,
                                NotifyCompilationCallback notify) {
   failed_ = true;
@@ -3538,9 +3552,11 @@ void CompilationState::RestartBackgroundTasks() {
   }
 }
 
-void CompilationState::SetFinisherIsRunning(bool value) {
+bool CompilationState::SetFinisherIsRunning(bool value) {
   base::LockGuard<base::Mutex> guard(&result_mutex_);
+  if (finisher_is_running_ == value) return false;
   finisher_is_running_ = value;
+  return true;
 }
 
 void CompilationState::ScheduleFinisherTask() {
diff --git a/test/mjsunit/regress/wasm/regress-824681.js b/test/mjsunit/regress/wasm/regress-824681.js
new file mode 100644
index 0000000000..18ca3d0b5d
--- /dev/null
+++ b/test/mjsunit/regress/wasm/regress-824681.js
@@ -0,0 +1,20 @@
+// Copyright 2018 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.
+
+load('test/mjsunit/wasm/wasm-constants.js');
+load('test/mjsunit/wasm/wasm-module-builder.js');
+
+let chain = Promise.resolve();
+const builder = new WasmModuleBuilder();
+for (let i = 0; i < 50; ++i) {
+  builder.addFunction('fun' + i, kSig_i_v)
+      .addBody([...wasmI32Const(i)])
+      .exportFunc();
+}
+const buffer = builder.toBuffer();
+for (let i = 0; i < 100; ++i) {
+  chain = chain.then(() => WebAssembly.instantiate(buffer));
+}
+chain.then(({module, instance}) => instance.exports.fun1155())
+    .then(res => print('Result of executing fun1155: ' + res));