Release execution lock before dispatching interrupt handling.

R=yurys@chromium.org

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

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

src/debug.cc

@@ -2541,6 +2541,8 @@ MaybeHandle<Object> Debug::MakeJSObject(const char* constructor_name,
       isolate_, isolate_->global_object(), constructor_name).ToHandleChecked();
   ASSERT(constructor->IsJSFunction());
   if (!constructor->IsJSFunction()) return MaybeHandle<Object>();
+  // We do not handle interrupts here.  In particular, termination interrupts.
+  PostponeInterruptsScope no_interrupts(isolate_);
   return Execution::TryCall(Handle<JSFunction>::cast(constructor),
                             Handle<JSObject>(debug_context()->global_object()),
                             argc,
@@ -2852,6 +2854,9 @@ void Debug::NotifyMessageHandler(v8::DebugEvent event,
                                  Handle<JSObject> exec_state,
                                  Handle<JSObject> event_data,
                                  bool auto_continue) {
+  // Prevent other interrupts from triggering, for example API callbacks,
+  // while dispatching message handler callbacks.
+  PostponeInterruptsScope no_interrupts(isolate_);
   ASSERT(is_active_);
   HandleScope scope(isolate_);
   // Process the individual events.

src/execution.cc

@@ -366,13 +366,13 @@ void StackGuard::ClearInterrupt(int flagbit) {
 }
 
 
-bool StackGuard::CheckAndClearInterrupt(InterruptFlag flag,
-                                        const ExecutionAccess& lock) {
+bool StackGuard::CheckAndClearInterrupt(InterruptFlag flag) {
+  ExecutionAccess access(isolate_);
   int flagbit = 1 << flag;
   bool result = (thread_local_.interrupt_flags_ & flagbit);
   thread_local_.interrupt_flags_ &= ~flagbit;
-  if (!should_postpone_interrupts(lock) && !has_pending_interrupts(lock)) {
-    reset_limits(lock);
+  if (!should_postpone_interrupts(access) && !has_pending_interrupts(access)) {
+    reset_limits(access);
   }
   return result;
 }
@@ -655,14 +655,14 @@ Handle<String> Execution::GetStackTraceLine(Handle<Object> recv,
 
 
 Object* StackGuard::HandleInterrupts() {
-  bool has_api_interrupt = false;
   {
     ExecutionAccess access(isolate_);
     if (should_postpone_interrupts(access)) {
       return isolate_->heap()->undefined_value();
     }
+  }
 
-    if (CheckAndClearInterrupt(GC_REQUEST, access)) {
+  if (CheckAndClearInterrupt(GC_REQUEST)) {
     isolate_->heap()->CollectAllGarbage(Heap::kNoGCFlags, "GC interrupt");
   }
 
@@ -670,30 +670,27 @@ Object* StackGuard::HandleInterrupts() {
     isolate_->debug()->DebugBreakHelper();
   }
 
-    if (CheckAndClearInterrupt(TERMINATE_EXECUTION, access)) {
+  if (CheckAndClearInterrupt(TERMINATE_EXECUTION)) {
     return isolate_->TerminateExecution();
   }
 
-    if (CheckAndClearInterrupt(DEOPT_MARKED_ALLOCATION_SITES, access)) {
+  if (CheckAndClearInterrupt(DEOPT_MARKED_ALLOCATION_SITES)) {
     isolate_->heap()->DeoptMarkedAllocationSites();
   }
 
-    if (CheckAndClearInterrupt(INSTALL_CODE, access)) {
+  if (CheckAndClearInterrupt(INSTALL_CODE)) {
     ASSERT(isolate_->concurrent_recompilation_enabled());
     isolate_->optimizing_compiler_thread()->InstallOptimizedFunctions();
   }
 
-    has_api_interrupt = CheckAndClearInterrupt(API_INTERRUPT, access);
+  if (CheckAndClearInterrupt(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();
-  }
-
-  if (has_api_interrupt) {
-    // Callback must be invoked outside of ExecusionAccess lock.
-    isolate_->InvokeApiInterruptCallback();
-  }
 
   return isolate_->heap()->undefined_value();
 }

src/execution.h

@@ -203,7 +203,7 @@ enum InterruptFlag {
   bool CheckInterrupt(int flagbit);
   void RequestInterrupt(int flagbit);
   void ClearInterrupt(int flagbit);
-  bool CheckAndClearInterrupt(InterruptFlag flag, const ExecutionAccess& lock);
+  bool CheckAndClearInterrupt(InterruptFlag flag);
 
   // You should hold the ExecutionAccess lock when calling this method.
   bool has_pending_interrupts(const ExecutionAccess& lock) {

test/cctest/test-debug.cc

@@ -7425,3 +7425,49 @@ TEST(DebugBreakStackTrace) {
              "  }"
              "})()");
 }
+
+
+v8::internal::Semaphore terminate_requested_semaphore(0);
+v8::internal::Semaphore terminate_fired_semaphore(0);
+bool terminate_already_fired = false;
+
+
+static void DebugBreakTriggerTerminate(
+    const v8::Debug::EventDetails& event_details) {
+  if (event_details.GetEvent() != v8::Break || terminate_already_fired) return;
+  terminate_requested_semaphore.Signal();
+  // Wait for at most 2 seconds for the terminate request.
+  CHECK(terminate_fired_semaphore.WaitFor(i::TimeDelta::FromSeconds(2)));
+  terminate_already_fired = true;
+  v8::internal::Isolate* isolate =
+      v8::Utils::OpenHandle(*event_details.GetEventContext())->GetIsolate();
+  CHECK(isolate->stack_guard()->CheckTerminateExecution());
+}
+
+
+class TerminationThread : public v8::internal::Thread {
+ public:
+  explicit TerminationThread(v8::Isolate* isolate) : Thread("terminator"),
+                                                     isolate_(isolate) { }
+
+  virtual void Run() {
+    terminate_requested_semaphore.Wait();
+    v8::V8::TerminateExecution(isolate_);
+    terminate_fired_semaphore.Signal();
+  }
+
+ private:
+  v8::Isolate* isolate_;
+};
+
+
+TEST(DebugBreakOffThreadTerminate) {
+  DebugLocalContext env;
+  v8::Isolate* isolate = env->GetIsolate();
+  v8::HandleScope scope(isolate);
+  v8::Debug::SetDebugEventListener(DebugBreakTriggerTerminate);
+  TerminationThread terminator(isolate);
+  terminator.Start();
+  v8::Debug::DebugBreak(isolate);
+  CompileRun("while (true);");
+}