Ensure that interruptor callback registered through API is called outside of Executio...

Such a coarse locking can cause a dead-lock when another thread is attempting to clear an interrupt while we are waiting in the interrupt callback.

Add test that verifies this API invariant.

BUG=chromium:374978
LOG=N
R=yangguo@chromium.org

Review URL: https://codereview.chromium.org/291123002

git-svn-id: https://v8.googlecode.com/svn/branches/bleeding_edge@21376 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

diff --git a/src/execution.cc b/src/execution.cc
index b550415..62a4cbe 100644 (file)
--- a/src/execution.cc
+++ b/src/execution.cc
@@ -713,43 +713,50 @@ void Execution::ProcessDebugMessages(Isolate* isolate,
 
 
 Object* StackGuard::HandleInterrupts() {
-  ExecutionAccess access(isolate_);
-  if (should_postpone_interrupts(access)) {
-    return isolate_->heap()->undefined_value();
-  }
+  bool has_api_interrupt = false;
+  {
+    ExecutionAccess access(isolate_);
+    if (should_postpone_interrupts(access)) {
+      return isolate_->heap()->undefined_value();
+    }
 
-  if (CheckAndClearInterrupt(API_INTERRUPT, access)) {
-    isolate_->InvokeApiInterruptCallback();
-  }
+    if (CheckAndClearInterrupt(GC_REQUEST, access)) {
+      isolate_->heap()->CollectAllGarbage(Heap::kNoGCFlags, "GC interrupt");
+    }
 
-  if (CheckAndClearInterrupt(GC_REQUEST, access)) {
-    isolate_->heap()->CollectAllGarbage(Heap::kNoGCFlags, "GC interrupt");
-  }
+    if (CheckDebugBreak() || CheckDebugCommand()) {
+      Execution::DebugBreakHelper(isolate_);
+    }
 
-  if (CheckDebugBreak() || CheckDebugCommand()) {
-    Execution::DebugBreakHelper(isolate_);
-  }
+    if (CheckAndClearInterrupt(TERMINATE_EXECUTION, access)) {
+      return isolate_->TerminateExecution();
+    }
 
-  if (CheckAndClearInterrupt(TERMINATE_EXECUTION, access)) {
-    return isolate_->TerminateExecution();
-  }
+    if (CheckAndClearInterrupt(FULL_DEOPT, access)) {
+      Deoptimizer::DeoptimizeAll(isolate_);
+    }
 
-  if (CheckAndClearInterrupt(FULL_DEOPT, access)) {
-    Deoptimizer::DeoptimizeAll(isolate_);
-  }
+    if (CheckAndClearInterrupt(DEOPT_MARKED_ALLOCATION_SITES, access)) {
+      isolate_->heap()->DeoptMarkedAllocationSites();
+    }
 
-  if (CheckAndClearInterrupt(DEOPT_MARKED_ALLOCATION_SITES, access)) {
-    isolate_->heap()->DeoptMarkedAllocationSites();
+    if (CheckAndClearInterrupt(INSTALL_CODE, access)) {
+      ASSERT(isolate_->concurrent_recompilation_enabled());
+      isolate_->optimizing_compiler_thread()->InstallOptimizedFunctions();
+    }
+
+    has_api_interrupt = CheckAndClearInterrupt(API_INTERRUPT, access);
+
+    isolate_->counters()->stack_interrupts()->Increment();
+    isolate_->counters()->runtime_profiler_ticks()->Increment();
+    isolate_->runtime_profiler()->OptimizeNow();
   }
 
-  if (CheckAndClearInterrupt(INSTALL_CODE, access)) {
-    ASSERT(isolate_->concurrent_recompilation_enabled());
-    isolate_->optimizing_compiler_thread()->InstallOptimizedFunctions();
+  if (has_api_interrupt) {
+    // Callback must be invoked outside of ExecusionAccess lock.
+    isolate_->InvokeApiInterruptCallback();
   }
 
-  isolate_->counters()->stack_interrupts()->Increment();
-  isolate_->counters()->runtime_profiler_ticks()->Increment();
-  isolate_->runtime_profiler()->OptimizeNow();
   return isolate_->heap()->undefined_value();
 }
 

diff --git a/src/isolate.cc b/src/isolate.cc
index 46f6d71..82ace29 100644 (file)
--- a/src/isolate.cc
+++ b/src/isolate.cc
@@ -842,10 +842,16 @@ void Isolate::CancelTerminateExecution() {
 
 
 void Isolate::InvokeApiInterruptCallback() {
-  InterruptCallback callback = api_interrupt_callback_;
-  void* data = api_interrupt_callback_data_;
-  api_interrupt_callback_ = NULL;
-  api_interrupt_callback_data_ = NULL;
+  // Note: callback below should be called outside of execution access lock.
+  InterruptCallback callback = NULL;
+  void* data = NULL;
+  {
+    ExecutionAccess access(this);
+    callback = api_interrupt_callback_;
+    data = api_interrupt_callback_data_;
+    api_interrupt_callback_ = NULL;
+    api_interrupt_callback_data_ = NULL;
+  }
 
   if (callback != NULL) {
     VMState<EXTERNAL> state(this);

diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc
index cbead0f..d7f6602 100644 (file)
--- a/test/cctest/test-api.cc
+++ b/test/cctest/test-api.cc
@@ -21819,11 +21819,12 @@ class RequestInterruptTestBase {
 
   virtual ~RequestInterruptTestBase() { }
 
+  virtual void StartInterruptThread() = 0;
+
   virtual void TestBody() = 0;
 
   void RunTest() {
-    InterruptThread i_thread(this);
-    i_thread.Start();
+    StartInterruptThread();
 
     v8::HandleScope handle_scope(isolate_);
 
@@ -21852,7 +21853,6 @@ class RequestInterruptTestBase {
     return should_continue_;
   }
 
- protected:
   static void ShouldContinueCallback(
       const v8::FunctionCallbackInfo<Value>& info) {
     RequestInterruptTestBase* test =
@@ -21861,6 +21861,24 @@ class RequestInterruptTestBase {
     info.GetReturnValue().Set(test->ShouldContinue());
   }
 
+  LocalContext env_;
+  v8::Isolate* isolate_;
+  i::Semaphore sem_;
+  int warmup_;
+  bool should_continue_;
+};
+
+
+class RequestInterruptTestBaseWithSimpleInterrupt
+    : public RequestInterruptTestBase {
+ public:
+  RequestInterruptTestBaseWithSimpleInterrupt() : i_thread(this) { }
+
+  virtual void StartInterruptThread() {
+    i_thread.Start();
+  }
+
+ private:
   class InterruptThread : public i::Thread {
    public:
     explicit InterruptThread(RequestInterruptTestBase* test)
@@ -21880,15 +21898,12 @@ class RequestInterruptTestBase {
      RequestInterruptTestBase* test_;
   };
 
-  LocalContext env_;
-  v8::Isolate* isolate_;
-  i::Semaphore sem_;
-  int warmup_;
-  bool should_continue_;
+  InterruptThread i_thread;
 };
 
 
-class RequestInterruptTestWithFunctionCall : public RequestInterruptTestBase {
+class RequestInterruptTestWithFunctionCall
+    : public RequestInterruptTestBaseWithSimpleInterrupt {
  public:
   virtual void TestBody() {
     Local<Function> func = Function::New(
@@ -21900,7 +21915,8 @@ class RequestInterruptTestWithFunctionCall : public RequestInterruptTestBase {
 };
 
 
-class RequestInterruptTestWithMethodCall : public RequestInterruptTestBase {
+class RequestInterruptTestWithMethodCall
+    : public RequestInterruptTestBaseWithSimpleInterrupt {
  public:
   virtual void TestBody() {
     v8::Local<v8::FunctionTemplate> t = v8::FunctionTemplate::New(isolate_);
@@ -21914,7 +21930,8 @@ class RequestInterruptTestWithMethodCall : public RequestInterruptTestBase {
 };
 
 
-class RequestInterruptTestWithAccessor : public RequestInterruptTestBase {
+class RequestInterruptTestWithAccessor
+    : public RequestInterruptTestBaseWithSimpleInterrupt {
  public:
   virtual void TestBody() {
     v8::Local<v8::FunctionTemplate> t = v8::FunctionTemplate::New(isolate_);
@@ -21928,7 +21945,8 @@ class RequestInterruptTestWithAccessor : public RequestInterruptTestBase {
 };
 
 
-class RequestInterruptTestWithNativeAccessor : public RequestInterruptTestBase {
+class RequestInterruptTestWithNativeAccessor
+    : public RequestInterruptTestBaseWithSimpleInterrupt {
  public:
   virtual void TestBody() {
     v8::Local<v8::FunctionTemplate> t = v8::FunctionTemplate::New(isolate_);
@@ -21955,7 +21973,7 @@ class RequestInterruptTestWithNativeAccessor : public RequestInterruptTestBase {
 
 
 class RequestInterruptTestWithMethodCallAndInterceptor
-    : public RequestInterruptTestBase {
+    : public RequestInterruptTestBaseWithSimpleInterrupt {
  public:
   virtual void TestBody() {
     v8::Local<v8::FunctionTemplate> t = v8::FunctionTemplate::New(isolate_);
@@ -21978,7 +21996,8 @@ class RequestInterruptTestWithMethodCallAndInterceptor
 };
 
 
-class RequestInterruptTestWithMathAbs : public RequestInterruptTestBase {
+class RequestInterruptTestWithMathAbs
+    : public RequestInterruptTestBaseWithSimpleInterrupt {
  public:
   virtual void TestBody() {
     env_->Global()->Set(v8_str("WakeUpInterruptor"), Function::New(
@@ -22062,6 +22081,61 @@ TEST(RequestInterruptTestWithMathAbs) {
 }
 
 
+class ClearInterruptFromAnotherThread
+    : public RequestInterruptTestBase {
+ public:
+  ClearInterruptFromAnotherThread() : i_thread(this), sem2_(0) { }
+
+  virtual void StartInterruptThread() {
+    i_thread.Start();
+  }
+
+  virtual void TestBody() {
+    Local<Function> func = Function::New(
+        isolate_, ShouldContinueCallback, v8::External::New(isolate_, this));
+    env_->Global()->Set(v8_str("ShouldContinue"), func);
+
+    CompileRun("while (ShouldContinue()) { }");
+  }
+
+ private:
+  class InterruptThread : public i::Thread {
+   public:
+    explicit InterruptThread(ClearInterruptFromAnotherThread* test)
+        : Thread("RequestInterruptTest"), test_(test) {}
+
+    virtual void Run() {
+      test_->sem_.Wait();
+      test_->isolate_->RequestInterrupt(&OnInterrupt, test_);
+      test_->sem_.Wait();
+      test_->isolate_->ClearInterrupt();
+      test_->sem2_.Signal();
+    }
+
+    static void OnInterrupt(v8::Isolate* isolate, void* data) {
+      ClearInterruptFromAnotherThread* test =
+          reinterpret_cast<ClearInterruptFromAnotherThread*>(data);
+      test->sem_.Signal();
+      bool success = test->sem2_.WaitFor(i::TimeDelta::FromSeconds(2));
+      // Crash instead of timeout to make this failure more prominent.
+      CHECK(success);
+      test->should_continue_ = false;
+    }
+
+   private:
+     ClearInterruptFromAnotherThread* test_;
+  };
+
+  InterruptThread i_thread;
+  i::Semaphore sem2_;
+};
+
+
+TEST(ClearInterruptFromAnotherThread) {
+  ClearInterruptFromAnotherThread().RunTest();
+}
+
+
 static Local<Value> function_new_expected_env;
 static void FunctionNewCallback(const v8::FunctionCallbackInfo<Value>& info) {
   CHECK_EQ(function_new_expected_env, info.Data());