[inspector] Introduce debugger session stop API

We introduce V8InspectorSession::stop API to enable safe
detach from the session. In particular, after calling 'stop',
the session will leave any instrumentation pause it might
be in and disarm all its instrumentation breakpoints.

This is useful when the session disconnect request is registered
on V8 interrupt (so it is unsafe to disconnect at that point),
and the execution should first get to the message loop
where the disconnect can be handled safely.

Bug: chromium:1354043
Change-Id: I3caab12a21b123229835e8374efadc1f4c9954c2
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4085143
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Kim-Anh Tran <kimanh@chromium.org>
Commit-Queue: Jaroslav Sevcik <jarin@chromium.org>
Cr-Commit-Position: refs/heads/main@{#84753}

include/v8-inspector.h

@@ -212,6 +212,9 @@ class V8_EXPORT V8InspectorSession {
                             std::unique_ptr<StringBuffer>* objectGroup) = 0;
   virtual void releaseObjectGroup(StringView) = 0;
   virtual void triggerPreciseCoverageDeltaUpdate(StringView occasion) = 0;
+
+  // Prepare for shutdown (disables debugger pausing, etc.).
+  virtual void stop() = 0;
 };
 
 class V8_EXPORT WebDriverValue {

src/debug/debug-interface.h

@@ -287,14 +287,15 @@ class DebugDelegate {
       v8::Local<v8::Context> paused_context,
       const std::vector<debug::BreakpointId>& inspector_break_points_hit,
       base::EnumSet<BreakReason> break_reasons = {}) {}
-  enum PauseAfterInstrumentation {
-    kPauseAfterInstrumentationRequested,
-    kNoPauseAfterInstrumentationRequested
+  enum class ActionAfterInstrumentation {
+    kPause,
+    kPauseIfBreakpointsHit,
+    kContinue
   };
-  virtual PauseAfterInstrumentation BreakOnInstrumentation(
+  virtual ActionAfterInstrumentation BreakOnInstrumentation(
       v8::Local<v8::Context> paused_context,
       const debug::BreakpointId instrumentationId) {
-    return kNoPauseAfterInstrumentationRequested;
+    return ActionAfterInstrumentation::kPauseIfBreakpointsHit;
   }
   virtual void ExceptionThrown(v8::Local<v8::Context> paused_context,
                                v8::Local<v8::Value> exception,

src/debug/debug.cc

@@ -480,11 +480,12 @@ void Debug::Unload() {
   debug_delegate_ = nullptr;
 }
 
-debug::DebugDelegate::PauseAfterInstrumentation
+debug::DebugDelegate::ActionAfterInstrumentation
 Debug::OnInstrumentationBreak() {
   RCS_SCOPE(isolate_, RuntimeCallCounterId::kDebugger);
   if (!debug_delegate_) {
-    return debug::DebugDelegate::kNoPauseAfterInstrumentationRequested;
+    return debug::DebugDelegate::ActionAfterInstrumentation::
+        kPauseIfBreakpointsHit;
   }
   DCHECK(in_debug_scope());
   HandleScope scope(isolate_);
@@ -517,11 +518,19 @@ void Debug::Break(JavaScriptFrame* frame, Handle<JSFunction> break_target) {
       IsBreakOnInstrumentation(debug_info, location);
   bool shouldPauseAfterInstrumentation = false;
   if (hitInstrumentationBreak) {
-    debug::DebugDelegate::PauseAfterInstrumentation pauseDuringInstrumentation =
+    debug::DebugDelegate::ActionAfterInstrumentation action =
         OnInstrumentationBreak();
-    shouldPauseAfterInstrumentation =
-        pauseDuringInstrumentation ==
-        debug::DebugDelegate::kPauseAfterInstrumentationRequested;
+    switch (action) {
+      case debug::DebugDelegate::ActionAfterInstrumentation::kPause:
+        shouldPauseAfterInstrumentation = true;
+        break;
+      case debug::DebugDelegate::ActionAfterInstrumentation::
+          kPauseIfBreakpointsHit:
+        shouldPauseAfterInstrumentation = false;
+        break;
+      case debug::DebugDelegate::ActionAfterInstrumentation::kContinue:
+        return;
+    }
   }
 
   // Find actual break points, if any, and trigger debug break event.

src/debug/debug.h

@@ -222,7 +222,7 @@ class V8_EXPORT_PRIVATE Debug {
   // Debug event triggers.
   void OnDebugBreak(Handle<FixedArray> break_points_hit, StepAction stepAction,
                     debug::BreakReasons break_reasons = {});
-  debug::DebugDelegate::PauseAfterInstrumentation OnInstrumentationBreak();
+  debug::DebugDelegate::ActionAfterInstrumentation OnInstrumentationBreak();
 
   base::Optional<Object> OnThrow(Handle<Object> exception)
       V8_WARN_UNUSED_RESULT;

src/inspector/v8-debugger-agent-impl.cc

@@ -371,7 +371,7 @@ V8DebuggerAgentImpl::V8DebuggerAgentImpl(
     : m_inspector(session->inspector()),
       m_debugger(m_inspector->debugger()),
       m_session(session),
-      m_enabled(false),
+      m_enableState(kDisabled),
       m_state(state),
       m_frontend(frontendChannel),
       m_isolate(m_inspector->isolate()) {}
@@ -379,7 +379,7 @@ V8DebuggerAgentImpl::V8DebuggerAgentImpl(
 V8DebuggerAgentImpl::~V8DebuggerAgentImpl() = default;
 
 void V8DebuggerAgentImpl::enableImpl() {
-  m_enabled = true;
+  m_enableState = kEnabled;
   m_state->setBoolean(DebuggerAgentState::debuggerEnabled, true);
   m_debugger->enable();
 
@@ -401,6 +401,8 @@ void V8DebuggerAgentImpl::enableImpl() {
 
 Response V8DebuggerAgentImpl::enable(Maybe<double> maxScriptsCacheSize,
                                      String16* outDebuggerId) {
+  if (m_enableState == kStopping)
+    return Response::ServerError("Debugger is stopping");
   m_maxScriptCacheSize = v8::base::saturated_cast<size_t>(
       maxScriptsCacheSize.fromMaybe(std::numeric_limits<double>::max()));
   *outDebuggerId =
@@ -449,14 +451,14 @@ Response V8DebuggerAgentImpl::disable() {
   m_skipAllPauses = false;
   m_state->setBoolean(DebuggerAgentState::skipAllPauses, false);
   m_state->remove(DebuggerAgentState::blackboxPattern);
-  m_enabled = false;
+  m_enableState = kDisabled;
   m_state->setBoolean(DebuggerAgentState::debuggerEnabled, false);
   m_debugger->disable();
   return Response::Success();
 }
 
 void V8DebuggerAgentImpl::restore() {
-  DCHECK(!m_enabled);
+  DCHECK(m_enableState == kDisabled);
   if (!m_state->booleanProperty(DebuggerAgentState::debuggerEnabled, false))
     return;
   if (!m_inspector->client()->canExecuteScripts(m_session->contextGroupId()))
@@ -2197,4 +2199,9 @@ Response V8DebuggerAgentImpl::processSkipList(
   m_skipList = std::move(skipListInit);
   return Response::Success();
 }
+
+void V8DebuggerAgentImpl::stop() {
+  disable();
+  m_enableState = kStopping;
+}
 }  // namespace v8_inspector

src/inspector/v8-debugger-agent-impl.h

@@ -43,6 +43,7 @@ class V8DebuggerAgentImpl : public protocol::Debugger::Backend {
   V8DebuggerAgentImpl(const V8DebuggerAgentImpl&) = delete;
   V8DebuggerAgentImpl& operator=(const V8DebuggerAgentImpl&) = delete;
   void restore();
+  void stop();
 
   // Part of the protocol.
   Response enable(Maybe<double> maxScriptsCacheSize,
@@ -144,7 +145,7 @@ class V8DebuggerAgentImpl : public protocol::Debugger::Backend {
       std::unique_ptr<protocol::Array<protocol::Debugger::ScriptPosition>>
           positions) override;
 
-  bool enabled() const { return m_enabled; }
+  bool enabled() const { return m_enableState == kEnabled; }
 
   void setBreakpointFor(v8::Local<v8::Function> function,
                         v8::Local<v8::String> condition,
@@ -222,10 +223,17 @@ class V8DebuggerAgentImpl : public protocol::Debugger::Backend {
   using DebuggerBreakpointIdToBreakpointIdMap =
       std::unordered_map<v8::debug::BreakpointId, String16>;
 
+  enum EnableState {
+    kDisabled,
+    kEnabled,
+    kStopping,  // This is the same as 'disabled', but it cannot become enabled
+                // again.
+  };
+
   V8InspectorImpl* m_inspector;
   V8Debugger* m_debugger;
   V8InspectorSessionImpl* m_session;
-  bool m_enabled;
+  EnableState m_enableState;
   protocol::DictionaryValue* m_state;
   protocol::Debugger::Frontend m_frontend;
   v8::Isolate* m_isolate;

src/inspector/v8-debugger.cc

@@ -545,11 +545,11 @@ void V8Debugger::ScriptCompiled(v8::Local<v8::debug::Script> script,
       });
 }
 
-V8Debugger::PauseAfterInstrumentation V8Debugger::BreakOnInstrumentation(
+V8Debugger::ActionAfterInstrumentation V8Debugger::BreakOnInstrumentation(
     v8::Local<v8::Context> pausedContext,
     v8::debug::BreakpointId instrumentationId) {
   // Don't allow nested breaks.
-  if (isPaused()) return kNoPauseAfterInstrumentationRequested;
+  if (isPaused()) return ActionAfterInstrumentation::kPauseIfBreakpointsHit;
 
   int contextGroupId = m_inspector->contextGroupId(pausedContext);
   bool hasAgents = false;
@@ -558,7 +558,7 @@ V8Debugger::PauseAfterInstrumentation V8Debugger::BreakOnInstrumentation(
         if (session->debuggerAgent()->acceptsPause(false /* isOOMBreak */))
           hasAgents = true;
       });
-  if (!hasAgents) return kNoPauseAfterInstrumentationRequested;
+  if (!hasAgents) return ActionAfterInstrumentation::kPauseIfBreakpointsHit;
 
   m_pausedContextGroupId = contextGroupId;
   m_instrumentationPause = true;
@@ -580,14 +580,21 @@ V8Debugger::PauseAfterInstrumentation V8Debugger::BreakOnInstrumentation(
   m_pausedContextGroupId = 0;
   m_instrumentationPause = false;
 
-  m_inspector->forEachSession(contextGroupId,
-                              [](V8InspectorSessionImpl* session) {
-                                if (session->debuggerAgent()->enabled())
-                                  session->debuggerAgent()->didContinue();
-                              });
-  return requestedPauseAfterInstrumentation
-             ? kPauseAfterInstrumentationRequested
-             : kNoPauseAfterInstrumentationRequested;
+  hasAgents = false;
+  m_inspector->forEachSession(
+      contextGroupId, [&hasAgents](V8InspectorSessionImpl* session) {
+        if (session->debuggerAgent()->enabled())
+          session->debuggerAgent()->didContinue();
+        if (session->debuggerAgent()->acceptsPause(false /* isOOMBreak */))
+          hasAgents = true;
+      });
+  if (!hasAgents) {
+    return ActionAfterInstrumentation::kContinue;
+  } else if (requestedPauseAfterInstrumentation) {
+    return ActionAfterInstrumentation::kPause;
+  } else {
+    return ActionAfterInstrumentation::kPauseIfBreakpointsHit;
+  }
 }
 
 void V8Debugger::BreakProgramRequested(

src/inspector/v8-debugger.h

@@ -194,7 +194,7 @@ class V8Debugger : public v8::debug::DebugDelegate,
       v8::Local<v8::Context> paused_context,
       const std::vector<v8::debug::BreakpointId>& break_points_hit,
       v8::debug::BreakReasons break_reasons) override;
-  PauseAfterInstrumentation BreakOnInstrumentation(
+  ActionAfterInstrumentation BreakOnInstrumentation(
       v8::Local<v8::Context> paused_context, v8::debug::BreakpointId) override;
   void ExceptionThrown(v8::Local<v8::Context> paused_context,
                        v8::Local<v8::Value> exception,

src/inspector/v8-inspector-session-impl.cc

@@ -511,4 +511,6 @@ void V8InspectorSessionImpl::triggerPreciseCoverageDeltaUpdate(
   m_profilerAgent->triggerPreciseCoverageDeltaUpdate(toString16(occasion));
 }
 
+void V8InspectorSessionImpl::stop() { m_debuggerAgent->stop(); }
+
 }  // namespace v8_inspector

src/inspector/v8-inspector-session-impl.h

@@ -100,6 +100,8 @@ class V8InspectorSessionImpl : public V8InspectorSession,
   static const unsigned kInspectedObjectBufferSize = 5;
 
   void triggerPreciseCoverageDeltaUpdate(StringView occasion) override;
+  void stop() override;
+
   V8Inspector::ClientTrustLevel clientTrustLevel() {
     return m_clientTrustLevel;
   }

test/inspector/debugger/session-stop-expected.txt

@@ -0,0 +1,15 @@
+Checks V8InspectorSession::stop
+
+Running test: testSessionStopResumesPause
+Evaluation returned: 42
+
+Running test: testSessionStopResumesInstrumentationPause
+Paused: instrumentation
+Evaluation returned: 42
+
+Running test: testSessionStopDisablesDebugger
+Pause error(?): Debugger agent is not enabled
+
+Running test: testSessionStopDisallowsReenabling
+Pause error(?) after stop: Debugger agent is not enabled
+Pause error(?) after re-enable: Debugger agent is not enabled

test/inspector/debugger/session-stop.js

@@ -0,0 +1,60 @@
+// Copyright 2022 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.
+
+InspectorTest.log('Checks V8InspectorSession::stop');
+
+InspectorTest.runAsyncTestSuite([
+  async function testSessionStopResumesPause() {
+    let contextGroup = new InspectorTest.ContextGroup();
+    let session = contextGroup.connect();
+    let Protocol = session.Protocol;
+
+    Protocol.Debugger.enable();
+    await Protocol.Debugger.pause();
+    const result = Protocol.Runtime.evaluate({expression: '42'});
+    contextGroup.stop();
+    InspectorTest.log(
+        `Evaluation returned: ${(await result).result.result.value}`);
+  },
+  async function testSessionStopResumesInstrumentationPause() {
+    let contextGroup = new InspectorTest.ContextGroup();
+    let session = contextGroup.connect();
+    let Protocol = session.Protocol;
+
+    Protocol.Debugger.enable();
+    await Protocol.Debugger.setInstrumentationBreakpoint(
+        {instrumentation: 'beforeScriptExecution'});
+    const paused = Protocol.Debugger.oncePaused();
+    const result = Protocol.Runtime.evaluate({expression: '42'});
+    InspectorTest.log(`Paused: ${(await paused).params.reason}`);
+    contextGroup.stop();
+    InspectorTest.log(
+        `Evaluation returned: ${(await result).result.result.value}`);
+  },
+  async function testSessionStopDisablesDebugger() {
+    let contextGroup = new InspectorTest.ContextGroup();
+    let session = contextGroup.connect();
+    let Protocol = session.Protocol;
+
+    await Protocol.Debugger.enable();
+    contextGroup.stop();
+    const pauseResult = await Protocol.Debugger.pause();
+    InspectorTest.log(`Pause error(?): ${pauseResult?.error?.message}`);
+  },
+  async function testSessionStopDisallowsReenabling() {
+    let contextGroup = new InspectorTest.ContextGroup();
+    let session = contextGroup.connect();
+    let Protocol = session.Protocol;
+
+    await Protocol.Debugger.enable();
+    contextGroup.stop();
+    const pauseResultAfterStop = await Protocol.Debugger.pause();
+    InspectorTest.log(
+        `Pause error(?) after stop: ${pauseResultAfterStop?.error?.message}`);
+    await Protocol.Debugger.enable();
+    const pauseResult = await Protocol.Debugger.pause();
+    InspectorTest.log(
+        `Pause error(?) after re-enable: ${pauseResult?.error?.message}`);
+  }
+]);

test/inspector/inspector-test.cc

@@ -73,6 +73,8 @@ class UtilsExtension : public InspectorIsolateData::SetupGlobalTask {
     utils->Set(isolate, "cancelPauseOnNextStatement",
                v8::FunctionTemplate::New(
                    isolate, &UtilsExtension::CancelPauseOnNextStatement));
+    utils->Set(isolate, "stop",
+               v8::FunctionTemplate::New(isolate, &UtilsExtension::Stop));
     utils->Set(isolate, "setLogConsoleApiMessageCalls",
                v8::FunctionTemplate::New(
                    isolate, &UtilsExtension::SetLogConsoleApiMessageCalls));
@@ -275,6 +277,17 @@ class UtilsExtension : public InspectorIsolateData::SetupGlobalTask {
                 });
   }
 
+  static void Stop(const v8::FunctionCallbackInfo<v8::Value>& args) {
+    if (args.Length() != 1 || !args[0]->IsInt32()) {
+      FATAL("Internal error: stop(context_group_id).");
+    }
+    int context_group_id = args[0].As<v8::Int32>()->Value();
+    RunSyncTask(backend_runner_,
+                [&context_group_id](InspectorIsolateData* data) {
+                  data->Stop(context_group_id);
+                });
+  }
+
   static void SetLogConsoleApiMessageCalls(
       const v8::FunctionCallbackInfo<v8::Value>& args) {
     if (args.Length() != 1 || !args[0]->IsBoolean()) {

test/inspector/isolate-data.cc

@@ -201,6 +201,14 @@ void InspectorIsolateData::BreakProgram(
   }
 }
 
+void InspectorIsolateData::Stop(int context_group_id) {
+  v8::SealHandleScope seal_handle_scope(isolate());
+  for (int session_id : GetSessionIds(context_group_id)) {
+    auto it = sessions_.find(session_id);
+    if (it != sessions_.end()) it->second->stop();
+  }
+}
+
 void InspectorIsolateData::SchedulePauseOnNextStatement(
     int context_group_id, const v8_inspector::StringView& reason,
     const v8_inspector::StringView& details) {

test/inspector/isolate-data.h

@@ -78,6 +78,7 @@ class InspectorIsolateData : public v8_inspector::V8InspectorClient {
   void BreakProgram(int context_group_id,
                     const v8_inspector::StringView& reason,
                     const v8_inspector::StringView& details);
+  void Stop(int context_group_id);
   void SchedulePauseOnNextStatement(int context_group_id,
                                     const v8_inspector::StringView& reason,
                                     const v8_inspector::StringView& details);

test/inspector/protocol-test.js

@@ -160,6 +160,10 @@ InspectorTest.ContextGroup = class {
     utils.cancelPauseOnNextStatement(this.id);
   }
 
+  stop() {
+    utils.stop(this.id);
+  }
+
   addScript(string, lineOffset, columnOffset, url) {
     utils.compileAndRunWithOrigin(this.id, string, url || '', lineOffset || 0, columnOffset || 0, false);
   }