Postpone interrupts while dipatching debugger events to listeners

The interrupts are already postponed in message handlers [1]. This CL aligns debug event listener (the mechanism that is actually used in Chrome DevTools) implementation with that. Handling interrupts on events like v8::AfterCompile leads to crashes like the one in the lined bug. This happens because in the interrupt handler we may change debugger state.

[1] https://codereview.chromium.org/309533009/diff/40001/src/debug.cc

BUG=chromium:520702
LOG=Y

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

Cr-Commit-Position: refs/heads/master@{#30488}

diff --git a/src/debug/debug.cc b/src/debug/debug.cc
index de7cc23..297156f 100644 (file)
--- a/src/debug/debug.cc
+++ b/src/debug/debug.cc
@@ -1815,6 +1815,7 @@ void Debug::OnException(Handle<Object> exception, Handle<Object> promise) {
 
 void Debug::OnCompileError(Handle<Script> script) {
   if (ignore_events()) return;
+  SuppressDebug while_processing(this);
 
   if (in_debug_scope()) {
     ProcessCompileEventInDebugScope(v8::CompileError, script);
@@ -1857,6 +1858,7 @@ void Debug::OnDebugBreak(Handle<Object> break_points_hit,
 
 void Debug::OnBeforeCompile(Handle<Script> script) {
   if (in_debug_scope() || ignore_events()) return;
+  SuppressDebug while_processing(this);
 
   HandleScope scope(isolate_);
   DebugScope debug_scope(this);
@@ -1878,6 +1880,7 @@ void Debug::OnBeforeCompile(Handle<Script> script) {
 // Handle debugger actions when a new script is compiled.
 void Debug::OnAfterCompile(Handle<Script> script) {
   if (ignore_events()) return;
+  SuppressDebug while_processing(this);
 
   if (in_debug_scope()) {
     ProcessCompileEventInDebugScope(v8::AfterCompile, script);
@@ -1974,6 +1977,9 @@ void Debug::CallEventCallback(v8::DebugEvent event,
                               Handle<Object> exec_state,
                               Handle<Object> event_data,
                               v8::Debug::ClientData* client_data) {
+  // Prevent other interrupts from triggering, for example API callbacks,
+  // while dispatching event listners.
+  PostponeInterruptsScope postpone(isolate_);
   bool previous = in_debug_event_listener_;
   in_debug_event_listener_ = true;
   if (event_listener_->IsForeign()) {
@@ -2007,7 +2013,6 @@ void Debug::ProcessCompileEventInDebugScope(v8::DebugEvent event,
                                             Handle<Script> script) {
   if (event_listener_.is_null()) return;
 
-  SuppressDebug while_processing(this);
   DebugScope debug_scope(this);
   if (debug_scope.failed()) return;
 

diff --git a/test/cctest/test-debug.cc b/test/cctest/test-debug.cc
index 1e3f0ab..e2ba3f5 100644 (file)
--- a/test/cctest/test-debug.cc
+++ b/test/cctest/test-debug.cc
@@ -7626,3 +7626,27 @@ TEST(DebugBreakInLexicalScopes) {
       "x * y",
       30);
 }
+
+static int after_compile_handler_depth = 0;
+static void HandleInterrupt(v8::Isolate* isolate, void* data) {
+  CHECK_EQ(0, after_compile_handler_depth);
+}
+
+static void NoInterruptsOnDebugEvent(
+    const v8::Debug::EventDetails& event_details) {
+  if (event_details.GetEvent() != v8::AfterCompile) return;
+  ++after_compile_handler_depth;
+  // Do not allow nested AfterCompile events.
+  CHECK(after_compile_handler_depth <= 1);
+  v8::Isolate* isolate = event_details.GetEventContext()->GetIsolate();
+  isolate->RequestInterrupt(&HandleInterrupt, nullptr);
+  CompileRun("function foo() {}; foo();");
+  --after_compile_handler_depth;
+}
+
+
+TEST(NoInterruptsInDebugListener) {
+  DebugLocalContext env;
+  v8::Debug::SetDebugEventListener(NoInterruptsOnDebugEvent);
+  CompileRun("void(0);");
+}