[debugger] Remove separate `didPause` for instrumentation breakpoints

This removes the additional call to `didPause` solely for
instrumentation breakpoints. They will be reported along with any
other pause reasons, and if several apply, 'ambiguous' will be
reported as a reason.

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

src/debug/debug-interface.h

@@ -122,12 +122,15 @@ enum StepAction {
 
 // Record the reason for why the debugger breaks.
 enum class BreakReason : uint8_t {
+  kAlreadyPaused,
   kStep,
+  kAsyncStep,
   kException,
   kAssert,
   kDebuggerStatement,
   kOOM,
-  kScheduled
+  kScheduled,
+  kAgent
 };
 typedef base::EnumSet<BreakReason> BreakReasons;
 

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

@@ -351,6 +351,17 @@ Response isValidRangeOfPositions(std::vector<std::pair<int, int>>& positions) {
   }
   return Response::Success();
 }
+
+bool hitBreakReasonEncodedAsOther(v8::debug::BreakReasons breakReasons) {
+  // The listed break reasons are not explicitly encoded in CDP when
+  // reporting the break. They are summarized as 'other'.
+  v8::debug::BreakReasons otherBreakReasons(
+      {v8::debug::BreakReason::kStep,
+       v8::debug::BreakReason::kDebuggerStatement,
+       v8::debug::BreakReason::kScheduled, v8::debug::BreakReason::kAsyncStep,
+       v8::debug::BreakReason::kAlreadyPaused});
+  return breakReasons.contains_any(otherBreakReasons);
+}
 }  // namespace
 
 V8DebuggerAgentImpl::V8DebuggerAgentImpl(
@@ -382,7 +393,8 @@ void V8DebuggerAgentImpl::enableImpl() {
 
   if (isPaused()) {
     didPause(0, v8::Local<v8::Value>(), std::vector<v8::debug::BreakpointId>(),
-             v8::debug::kException, false, {});
+             v8::debug::kException, false,
+             v8::debug::BreakReasons({v8::debug::BreakReason::kAlreadyPaused}));
   }
 }
 
@@ -1825,18 +1837,18 @@ void V8DebuggerAgentImpl::didPause(
   const BreakReason otherHitReason =
       std::make_pair(protocol::Debugger::Paused::ReasonEnum::Other, nullptr);
   const bool otherBreakReasons =
-      hitRegularBreakpoint ||
-      breakReasons.contains(v8::debug::BreakReason::kStep) ||
-      breakReasons.contains(v8::debug::BreakReason::kDebuggerStatement) ||
-      breakReasons.contains(v8::debug::BreakReason::kScheduled);
+      hitRegularBreakpoint || hitBreakReasonEncodedAsOther(breakReasons);
   if (otherBreakReasons && std::find(hitReasons.begin(), hitReasons.end(),
                                      otherHitReason) == hitReasons.end()) {
     hitReasons.push_back(
         std::make_pair(protocol::Debugger::Paused::ReasonEnum::Other, nullptr));
   }
 
-  // TODO(kimanh): We should always know why we pause. Print a
-  // warning if we don't and fall back to Other.
+  // We should always know why we pause: either the pause relates to this agent
+  // (`hitReason` is non empty), or it relates to another agent (hit a
+  // breakpoint there, or a triggered pause was scheduled by other agent).
+  DCHECK(hitReasons.size() > 0 || !hitBreakpoints.empty() ||
+         breakReasons.contains(v8::debug::BreakReason::kAgent));
   String16 breakReason = protocol::Debugger::Paused::ReasonEnum::Other;
   std::unique_ptr<protocol::DictionaryValue> breakAuxData;
   if (hitReasons.size() == 1) {

src/inspector/v8-debugger.cc

@@ -238,7 +238,6 @@ void V8Debugger::breakProgramOnAssert(int targetContextGroupId) {
   if (!canBreakProgram()) return;
   DCHECK(targetContextGroupId);
   m_targetContextGroupId = targetContextGroupId;
-  m_scheduledAssertBreak = true;
   v8::debug::BreakRightNow(
       m_isolate, v8::debug::BreakReasons({v8::debug::BreakReason::kAssert}));
 }
@@ -408,6 +407,16 @@ void V8Debugger::handleProgramBreak(
     v8::debug::PrepareStep(m_isolate, v8::debug::StepOut);
     return;
   }
+
+  DCHECK(hasScheduledBreakOnNextFunctionCall() ==
+         (m_taskWithScheduledBreakPauseRequested ||
+          m_externalAsyncTaskPauseRequested || m_pauseOnNextCallRequested));
+  if (m_taskWithScheduledBreakPauseRequested ||
+      m_externalAsyncTaskPauseRequested)
+    breakReasons.Add(v8::debug::BreakReason::kAsyncStep);
+  if (m_pauseOnNextCallRequested)
+    breakReasons.Add(v8::debug::BreakReason::kAgent);
+
   m_targetContextGroupId = 0;
   m_pauseOnNextCallRequested = false;
   m_pauseOnAsyncCall = false;
@@ -418,7 +427,6 @@ void V8Debugger::handleProgramBreak(
   bool scheduledOOMBreak = m_scheduledOOMBreak;
   DCHECK(scheduledOOMBreak ==
          breakReasons.contains(v8::debug::BreakReason::kOOM));
-  bool scheduledAssertBreak = m_scheduledAssertBreak;
   bool hasAgents = false;
 
   m_inspector->forEachSession(
@@ -439,83 +447,14 @@ void V8Debugger::handleProgramBreak(
   DCHECK(contextGroupId);
   m_pausedContextGroupId = contextGroupId;
 
-  // Collect all instrumentation breakpoints.
-  std::set<v8::debug::BreakpointId> instrumentationBreakpointIdSet;
   m_inspector->forEachSession(
-      contextGroupId, [&breakpointIds, &instrumentationBreakpointIdSet](
-                          V8InspectorSessionImpl* session) {
-        if (!session->debuggerAgent()->acceptsPause(false)) return;
-
-        const std::vector<v8::debug::BreakpointId>
-            sessionInstrumentationBreakpoints =
-                session->debuggerAgent()->instrumentationBreakpointIdsMatching(
-                    breakpointIds);
-
-        instrumentationBreakpointIdSet.insert(
-            sessionInstrumentationBreakpoints.begin(),
-            sessionInstrumentationBreakpoints.end());
-      });
-  std::vector<v8::debug::BreakpointId> instrumentationBreakpointIds(
-      instrumentationBreakpointIdSet.begin(),
-      instrumentationBreakpointIdSet.end());
-
-  const bool regularBreakpointHit =
-      instrumentationBreakpointIds.size() < breakpointIds.size();
-  const bool hasNonInstrumentationBreakReason =
-      regularBreakpointHit || hasScheduledBreakOnNextFunctionCall() ||
-      scheduledAssertBreak || scheduledOOMBreak || !exception.IsEmpty();
-
-  std::vector<v8::debug::BreakpointId> regularBreakpointIds = breakpointIds;
-  if (hasNonInstrumentationBreakReason &&
-      !instrumentationBreakpointIds.empty()) {
-    // Send out pause events for instrumentation breakpoints.
-    m_inspector->forEachSession(
-        contextGroupId, [&pausedContext, &instrumentationBreakpointIds](
-                            V8InspectorSessionImpl* session) {
-          if (!session->debuggerAgent()->acceptsPause(false)) return;
-          session->debuggerAgent()->didPause(
-              InspectedContext::contextId(pausedContext), {},
-              instrumentationBreakpointIds,
-              v8::debug::ExceptionType::kException, false, {});
-        });
-    {
-      v8::Context::Scope scope(pausedContext);
-      m_inspector->client()->runMessageLoopOnPause(contextGroupId);
-    }
-    m_inspector->forEachSession(contextGroupId,
-                                [](V8InspectorSessionImpl* session) {
-                                  if (session->debuggerAgent()->enabled()) {
-                                    session->debuggerAgent()->didContinue();
-                                  }
-                                });
-
-    // Remove instrumentation breakpoints from regular breakpoints, as they
-    // have already been reported.
-    for (const v8::debug::BreakpointId& breakpointId :
-         instrumentationBreakpointIds) {
-      auto iter = std::find(regularBreakpointIds.begin(),
-                            regularBreakpointIds.end(), breakpointId);
-      if (iter != regularBreakpointIds.end()) {
-        regularBreakpointIds.erase(iter);
-      }
-    }
-  }
-
-  // If instrumentation breakpoints did coincide with other known reasons, then
-  // the remaining reasons are summarized in the following pause event.
-  // If we, however, do NOT know whether instrumentation breakpoints coincided
-  // with other reasons (hasNonInstrumentationBreakReason == false), then send
-  // instrumentation breakpoints here. The reason for this is that we do not
-  // want to trigger two pause events if we only break because of an
-  // instrumentation.
-  m_inspector->forEachSession(
-      contextGroupId, [&pausedContext, &exception, &regularBreakpointIds,
-                       &exceptionType, &isUncaught, &scheduledOOMBreak,
-                       &breakReasons](V8InspectorSessionImpl* session) {
+      contextGroupId,
+      [&pausedContext, &exception, &breakpointIds, &exceptionType, &isUncaught,
+       &scheduledOOMBreak, &breakReasons](V8InspectorSessionImpl* session) {
         if (session->debuggerAgent()->acceptsPause(scheduledOOMBreak)) {
           session->debuggerAgent()->didPause(
               InspectedContext::contextId(pausedContext), exception,
-              regularBreakpointIds, exceptionType, isUncaught, breakReasons);
+              breakpointIds, exceptionType, isUncaught, breakReasons);
         }
       });
   {
@@ -531,7 +470,6 @@ void V8Debugger::handleProgramBreak(
 
   if (m_scheduledOOMBreak) m_isolate->RestoreOriginalHeapLimit();
   m_scheduledOOMBreak = false;
-  m_scheduledAssertBreak = false;
 }
 
 namespace {

src/inspector/v8-debugger.h

@@ -205,7 +205,6 @@ class V8Debugger : public v8::debug::DebugDelegate,
   int m_ignoreScriptParsedEventsCounter;
   size_t m_originalHeapLimit = 0;
   bool m_scheduledOOMBreak = false;
-  bool m_scheduledAssertBreak = false;
   int m_targetContextGroupId = 0;
   int m_pausedContextGroupId = 0;
   int m_continueToLocationBreakpointId;

test/inspector/debugger/other-pause-reasons-expected.txt

 Test that all 'other' reasons are explicitly encoded on a pause event if they overlap with another reason
 
 Running test: testBreakpointPauseReason
-Paused with reason: instrumentation and data: {"url":"foo.js","scriptId":"3"}.
-Paused with reason: other and data: {}.
+Paused with reason: ambiguous and data: {"reasons":[{"reason":"instrumentation","auxData":{"url":"foo.js","scriptId":"3"}},{"reason":"other"}]}.
 
 Running test: testTriggeredPausePauseReason
 Paused with reason: ambiguous and data: {"reasons":[{"reason":"instrumentation","auxData":{"url":"foo.js","scriptId":"4"}},{"reason":"other"}]}.
 
 Running test: testSteppingPauseReason
-Paused with reason: instrumentation and data: {"url":"foo.js","scriptId":"5"}.
-Paused with reason: other and data: {}.
+Paused with reason: ambiguous and data: {"reasons":[{"reason":"instrumentation","auxData":{"url":"foo.js","scriptId":"5"}},{"reason":"other"}]}.
 Paused with reason: other and data: {}.
 Paused with reason: ambiguous and data: {"reasons":[{"reason":"instrumentation","auxData":{"url":"bar.js","scriptId":"6"}},{"reason":"other"}]}.
 
@@ -18,3 +16,8 @@ Paused with reason: other and data: {}.
 
 Running test: testDebuggerStatementReason
 Paused with reason: ambiguous and data: {"reasons":[{"reason":"instrumentation","auxData":{"url":"foo.js","scriptId":"8"}},{"reason":"other"}]}.
+
+Running test: testAsyncSteppingPauseReason
+Paused with reason: ambiguous and data: {"reasons":[{"reason":"instrumentation","auxData":{"url":"foo.js","scriptId":"9"}},{"reason":"other"}]}.
+Paused with reason: other and data: {}.
+Paused with reason: ambiguous and data: {"reasons":[{"reason":"instrumentation","auxData":{"url":"bar.js","scriptId":"10"}},{"reason":"other"}]}.

test/inspector/debugger/other-pause-reasons.js

@@ -105,5 +105,22 @@ InspectorTest.runAsyncTestSuite([
 
     await Protocol.Runtime.runScript({scriptId});
     await tearDownEnvironment();
-  }
+  },
+  async function testAsyncSteppingPauseReason() {
+    await setUpEnvironment();
+    await Protocol.Debugger.setInstrumentationBreakpoint(
+        {instrumentation: 'beforeScriptExecution'});
+    const stepOnPause = (({params: {reason, data}}) => {
+      InspectorTest.log(`Paused with reason: ${reason} and data: ${
+          data ? JSON.stringify(data) : '{}'}.`);
+      Protocol.Debugger.stepInto({breakOnAsyncCall: true});
+    });
+    Protocol.Debugger.onPaused(stepOnPause);
+    const expression =
+        `debugger; setTimeout('console.log(3);//# sourceURL=bar.js', 0);`;
+    const {result: {scriptId}} = await Protocol.Runtime.compileScript(
+        {expression, sourceURL: 'foo.js', persistScript: true});
+    await Protocol.Runtime.runScript({scriptId});
+    await tearDownEnvironment();
+  },
 ]);

test/inspector/debugger/set-instrumentation-breakpoint-expected.txt

@@ -87,10 +87,3 @@ paused with reason: instrumentation
     sourceMapURL : boo.js
     url : foo.js
 }
-
-Running test: testCoincideWithRegularBreakpoint
-regular breakpoint and instrumentation breakpoint are reported
-Set breakpoint: 8:beforeScriptExecution
-Set breakpoint: 1:0:0:test.js
-paused with reason: instrumentation and breakpoints: 
-paused with reason: other and breakpoints: 1:0:0:test.js

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

@@ -127,41 +127,5 @@ InspectorTest.runAsyncTestSuite([
     }
     await Protocol.Debugger.disable();
     await Protocol.Runtime.disable();
-  },
-
-  async function testCoincideWithRegularBreakpoint() {
-    await Protocol.Runtime.enable();
-    await Protocol.Debugger.enable();
-    InspectorTest.log('regular breakpoint and instrumentation breakpoint are reported');
-    const instrumentationResult = await Protocol.Debugger.setInstrumentationBreakpoint({
-      instrumentation: 'beforeScriptExecution'
-    });
-    InspectorTest.log(`Set breakpoint: ${instrumentationResult.result.breakpointId}`);
-
-    const { result: { scriptId } } = await Protocol.Runtime.compileScript({
-      expression: 'console.log(3);', sourceURL: 'test.js', persistScript: true });
-    const breakpoinResult = await Protocol.Debugger.setBreakpointByUrl({
-        lineNumber: 0,
-        url: 'test.js',
-        columnNumber: 0
-      });
-      InspectorTest.log(`Set breakpoint: ${breakpoinResult.result.breakpointId}`);
-
-      const runPromise = Protocol.Runtime.runScript({scriptId});
-      {
-        const {params: {reason, hitBreakpoints}} = await Protocol.Debugger.oncePaused();
-        InspectorTest.log(`paused with reason: ${reason} and breakpoints: ${hitBreakpoints}`);
-        await Protocol.Debugger.resume();
-      }
-
-      {
-        const {params: {reason, hitBreakpoints}} = await Protocol.Debugger.oncePaused();
-        InspectorTest.log(`paused with reason: ${reason} and breakpoints: ${hitBreakpoints}`);
-        await Protocol.Debugger.resume();
-      }
-
-      await runPromise;
-      await Protocol.Debugger.disable();
-      await Protocol.Runtime.disable();
-    }]
-    );
+  }
+]);