[inspector] Trigger requested pause after instrumentation pause

If a CDP client requests Debugger.pause during instrumentation pause,
the requests is currently ignored.

With this patch, the debugger will take note of a pause request during
instrumentation pause and enter the pause once the instrumentation pause
resumes.

Bug: chromium:1381967
Change-Id: I4d0337a92fa31d0666ab02b54f95aba4d89592b3
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4008379
Commit-Queue: Jaroslav Sevcik <jarin@chromium.org>
Reviewed-by: Kim-Anh Tran <kimanh@chromium.org>
Cr-Commit-Position: refs/heads/main@{#84138}

src/debug/debug-interface.h

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

src/debug/debug.cc

@@ -480,16 +480,19 @@ void Debug::Unload() {
   debug_delegate_ = nullptr;
 }
 
-void Debug::OnInstrumentationBreak() {
+debug::DebugDelegate::PauseAfterInstrumentation
+Debug::OnInstrumentationBreak() {
   RCS_SCOPE(isolate_, RuntimeCallCounterId::kDebugger);
-  if (!debug_delegate_) return;
+  if (!debug_delegate_) {
+    return debug::DebugDelegate::kNoPauseAfterInstrumentationRequested;
+  }
   DCHECK(in_debug_scope());
   HandleScope scope(isolate_);
   DisableBreak no_recursive_break(this);
 
   Handle<Context> native_context(isolate_->native_context());
-  debug_delegate_->BreakOnInstrumentation(v8::Utils::ToLocal(native_context),
-                                          kInstrumentationId);
+  return debug_delegate_->BreakOnInstrumentation(
+      v8::Utils::ToLocal(native_context), kInstrumentationId);
 }
 
 void Debug::Break(JavaScriptFrame* frame, Handle<JSFunction> break_target) {
@@ -512,21 +515,28 @@ void Debug::Break(JavaScriptFrame* frame, Handle<JSFunction> break_target) {
   BreakLocation location = BreakLocation::FromFrame(debug_info, frame);
   const bool hitInstrumentationBreak =
       IsBreakOnInstrumentation(debug_info, location);
+  bool shouldPauseAfterInstrumentation = false;
   if (hitInstrumentationBreak) {
-    OnInstrumentationBreak();
+    debug::DebugDelegate::PauseAfterInstrumentation pauseDuringInstrumentation =
+        OnInstrumentationBreak();
+    shouldPauseAfterInstrumentation =
+        pauseDuringInstrumentation ==
+        debug::DebugDelegate::kPauseAfterInstrumentationRequested;
   }
 
   // Find actual break points, if any, and trigger debug break event.
   bool has_break_points;
+  bool scheduled_break =
+      scheduled_break_on_function_call() || shouldPauseAfterInstrumentation;
   MaybeHandle<FixedArray> break_points_hit =
       CheckBreakPoints(debug_info, &location, &has_break_points);
   if (!break_points_hit.is_null() || break_on_next_function_call() ||
-      scheduled_break_on_function_call()) {
+      scheduled_break) {
     StepAction lastStepAction = last_step_action();
     DCHECK_IMPLIES(scheduled_break_on_function_call(),
                    lastStepAction == StepNone);
     debug::BreakReasons break_reasons;
-    if (scheduled_break_on_function_call()) {
+    if (scheduled_break) {
       break_reasons.Add(debug::BreakReason::kScheduled);
     }
     // Clear all current stepping setup.

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 = {});
-  void OnInstrumentationBreak();
+  debug::DebugDelegate::PauseAfterInstrumentation OnInstrumentationBreak();
 
   base::Optional<Object> OnThrow(Handle<Object> exception)
       V8_WARN_UNUSED_RESULT;

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

@@ -1328,9 +1328,16 @@ void V8DebuggerAgentImpl::cancelPauseOnNextStatement() {
 
 Response V8DebuggerAgentImpl::pause() {
   if (!enabled()) return Response::ServerError(kDebuggerNotEnabled);
-  if (isPaused()) return Response::Success();
 
-  if (m_debugger->canBreakProgram()) {
+  if (m_debugger->isInInstrumentationPause()) {
+    // If we are inside an instrumentation pause, remember the pause request
+    // so that we can enter the requested pause once we are done
+    // with the instrumentation.
+    m_debugger->requestPauseAfterInstrumentation();
+  } else if (isPaused()) {
+    // Ignore the pause request if we are already paused.
+    return Response::Success();
+  } else if (m_debugger->canBreakProgram()) {
     m_debugger->interruptAndBreak(m_session->contextGroupId());
   } else {
     pushBreakDetails(protocol::Debugger::Paused::ReasonEnum::Other, nullptr);

src/inspector/v8-debugger.cc

@@ -202,6 +202,10 @@ bool V8Debugger::canBreakProgram() {
   return v8::debug::CanBreakProgram(m_isolate);
 }
 
+bool V8Debugger::isInInstrumentationPause() const {
+  return m_instrumentationPause;
+}
+
 void V8Debugger::breakProgram(int targetContextGroupId) {
   DCHECK(canBreakProgram());
   // Don't allow nested breaks.
@@ -225,6 +229,10 @@ void V8Debugger::interruptAndBreak(int targetContextGroupId) {
       nullptr);
 }
 
+void V8Debugger::requestPauseAfterInstrumentation() {
+  m_requestedPauseAfterInstrumentation = true;
+}
+
 void V8Debugger::continueProgram(int targetContextGroupId,
                                  bool terminateOnResume) {
   if (m_pausedContextGroupId != targetContextGroupId) return;
@@ -510,11 +518,11 @@ void V8Debugger::ScriptCompiled(v8::Local<v8::debug::Script> script,
       });
 }
 
-void V8Debugger::BreakOnInstrumentation(
+V8Debugger::PauseAfterInstrumentation V8Debugger::BreakOnInstrumentation(
     v8::Local<v8::Context> pausedContext,
     v8::debug::BreakpointId instrumentationId) {
   // Don't allow nested breaks.
-  if (isPaused()) return;
+  if (isPaused()) return kNoPauseAfterInstrumentationRequested;
 
   int contextGroupId = m_inspector->contextGroupId(pausedContext);
   bool hasAgents = false;
@@ -523,9 +531,10 @@ void V8Debugger::BreakOnInstrumentation(
         if (session->debuggerAgent()->acceptsPause(false /* isOOMBreak */))
           hasAgents = true;
       });
-  if (!hasAgents) return;
+  if (!hasAgents) return kNoPauseAfterInstrumentationRequested;
 
   m_pausedContextGroupId = contextGroupId;
+  m_instrumentationPause = true;
   m_inspector->forEachSession(
       contextGroupId, [instrumentationId](V8InspectorSessionImpl* session) {
         if (session->debuggerAgent()->acceptsPause(false /* isOOMBreak */)) {
@@ -536,14 +545,22 @@ void V8Debugger::BreakOnInstrumentation(
   {
     v8::Context::Scope scope(pausedContext);
     m_inspector->client()->runMessageLoopOnInstrumentationPause(contextGroupId);
-    m_pausedContextGroupId = 0;
   }
+  bool requestedPauseAfterInstrumentation =
+      m_requestedPauseAfterInstrumentation;
+
+  m_requestedPauseAfterInstrumentation = false;
+  m_pausedContextGroupId = 0;
+  m_instrumentationPause = false;
 
   m_inspector->forEachSession(contextGroupId,
                               [](V8InspectorSessionImpl* session) {
                                 if (session->debuggerAgent()->enabled())
                                   session->debuggerAgent()->didContinue();
                               });
+  return requestedPauseAfterInstrumentation
+             ? kPauseAfterInstrumentationRequested
+             : kNoPauseAfterInstrumentationRequested;
 }
 
 void V8Debugger::BreakProgramRequested(

src/inspector/v8-debugger.h

@@ -58,8 +58,10 @@ class V8Debugger : public v8::debug::DebugDelegate,
   v8::debug::ExceptionBreakState getPauseOnExceptionsState();
   void setPauseOnExceptionsState(v8::debug::ExceptionBreakState);
   bool canBreakProgram();
+  bool isInInstrumentationPause() const;
   void breakProgram(int targetContextGroupId);
   void interruptAndBreak(int targetContextGroupId);
+  void requestPauseAfterInstrumentation();
   void continueProgram(int targetContextGroupId,
                        bool terminateOnResume = false);
   void breakProgramOnAssert(int targetContextGroupId);
@@ -191,8 +193,8 @@ 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;
-  void BreakOnInstrumentation(v8::Local<v8::Context> paused_context,
-                              v8::debug::BreakpointId) override;
+  PauseAfterInstrumentation BreakOnInstrumentation(
+      v8::Local<v8::Context> paused_context, v8::debug::BreakpointId) override;
   void ExceptionThrown(v8::Local<v8::Context> paused_context,
                        v8::Local<v8::Value> exception,
                        v8::Local<v8::Value> promise, bool is_uncaught,
@@ -218,6 +220,8 @@ class V8Debugger : public v8::debug::DebugDelegate,
   bool m_scheduledOOMBreak = false;
   int m_targetContextGroupId = 0;
   int m_pausedContextGroupId = 0;
+  bool m_instrumentationPause = false;
+  bool m_requestedPauseAfterInstrumentation = false;
   int m_continueToLocationBreakpointId;
   String16 m_continueToLocationTargetCallFrames;
   std::unique_ptr<V8StackTraceImpl> m_continueToLocationStack;

test/inspector/debugger/pause-on-instrumentation-expected.txt

@@ -0,0 +1,10 @@
+Test if pauses break execution when requested during instrumentation pause in js
+
+Running test: testPauseDuringInstrumentationPause
+Set instrumentation breakpoints and requested script evaluation.
+Paused at foo.js with reason "instrumentation".
+Requested debugger pause.
+Resumed.
+Paused at foo.js with reason "other".
+Resumed.
+Done.

test/inspector/debugger/pause-on-instrumentation.js

@@ -0,0 +1,56 @@
+// 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.
+
+const {session, contextGroup, Protocol} = InspectorTest.start(
+    'Test if pauses break execution when requested during instrumentation pause in js');
+
+session.setupScriptMap();
+
+function logPause(msg) {
+  const top_frame = msg.params.callFrames[0];
+  const reason = msg.params.reason;
+  const url = session.getCallFrameUrl(top_frame);
+  InspectorTest.log(`Paused at ${url} with reason "${reason}".`);
+};
+
+// Test that requests to pause during instrumentation pause cause the debugger
+// to pause after it is done with the instrumentation pause.
+async function testPauseDuringInstrumentationPause() {
+  await Protocol.Runtime.enable();
+  await Protocol.Debugger.enable();
+
+  await Protocol.Debugger.setInstrumentationBreakpoint(
+      {instrumentation: 'beforeScriptExecution'});
+  const runPromise = Protocol.Runtime.evaluate(
+      {expression: 'console.log("Hi");\n//# sourceURL=foo.js'});
+  InspectorTest.log(
+      'Set instrumentation breakpoints and requested script evaluation.');
+
+  // First pause: instrumentation
+  {
+    const pause = await Protocol.Debugger.oncePaused();
+    logPause(pause);
+    // Request debugger pause from within the instrumentation pause.
+    await Protocol.Debugger.pause({});
+    InspectorTest.log('Requested debugger pause.');
+    await Protocol.Debugger.resume();
+    InspectorTest.log('Resumed.');
+  }
+
+  // Second pause: explicit pause request
+  {
+    const pause = await Protocol.Debugger.oncePaused();
+    logPause(pause);
+    await Protocol.Debugger.resume();
+    InspectorTest.log('Resumed.');
+  }
+
+  await runPromise;
+
+  InspectorTest.log('Done.');
+  await Protocol.Runtime.disable();
+  await Protocol.Debugger.disable();
+}
+
+InspectorTest.runAsyncTestSuite([testPauseDuringInstrumentationPause]);

test/inspector/debugger/set-breakpoint-on-instrumentation.js

@@ -31,7 +31,7 @@ function handlePause(msg) {
   const url = session.getCallFrameUrl(top_frame);
   InspectorTest.log(`Paused at ${url} with reason "${reason}".`);
   InspectorTest.log(
-      `Hit breakpoints: ${JSON.stringify(msg.params.hitBreakpoints)}`)
+      `Hit breakpoints: ${JSON.stringify(msg.params.hitBreakpoints)}`);
   return Protocol.Debugger.resume();
 };