[debug] Capture more cases for instrumentation breakpoints

The previous implementation would not explicitly send
`Debugger.paused` events for instrumentation breakpoints
if they were to overlap with breaks due to:
* regular breakpoints
* OOM
* exceptions
* asserts

This CL is a step towards making sure that a separate
`Debugger.paused` event is always sent for an instrumentation
breakpoint. In some cases where we have overlapping reasons
but only know of one, the 'instrumentation' reason,
we still just send out one paused event with the reason
being `instrumentation`.

Drive-by: send instrumentation notification to all sessions,
remember which breakpoints are instrumentation breakpoints

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

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

@@ -428,6 +428,7 @@ Response V8DebuggerAgentImpl::disable() {
   for (const auto& it : m_debuggerBreakpointIdToBreakpointId) {
     v8::debug::RemoveBreakpoint(m_isolate, it.first);
   }
+  m_breakpointsOnScriptRun.clear();
   m_breakpointIdToDebuggerBreakpointIds.clear();
   m_debuggerBreakpointIdToBreakpointId.clear();
   m_debugger->setAsyncCallStackDepth(this, 0);
@@ -733,6 +734,7 @@ void V8DebuggerAgentImpl::removeBreakpointImpl(
 #endif  // V8_ENABLE_WEBASSEMBLY
     v8::debug::RemoveBreakpoint(m_isolate, id);
     m_debuggerBreakpointIdToBreakpointId.erase(id);
+    m_breakpointsOnScriptRun.erase(id);
   }
   m_breakpointIdToDebuggerBreakpointIds.erase(breakpointId);
 }
@@ -1779,14 +1781,23 @@ void V8DebuggerAgentImpl::didPause(
   }
 
   auto hitBreakpointIds = std::make_unique<Array<String16>>();
+  bool hitInstrumentationBreakpoint = false;
 
   for (const auto& id : hitBreakpoints) {
     auto it = m_breakpointsOnScriptRun.find(id);
     if (it != m_breakpointsOnScriptRun.end()) {
-      hitReasons.push_back(std::make_pair(
-          protocol::Debugger::Paused::ReasonEnum::Instrumentation,
-          std::move(it->second)));
-      m_breakpointsOnScriptRun.erase(it);
+      if (!hitInstrumentationBreakpoint) {
+        // We may hit several instrumentation breakpoints: 1. they are
+        // kept around, and 2. each session may set their own.
+        // Only report one.
+        // TODO(kimanh): This will not be needed anymore if we
+        // make sure that we can only hit an instrumentation
+        // breakpoint once. This workaround is currently for wasm.
+        hitInstrumentationBreakpoint = true;
+        hitReasons.push_back(std::make_pair(
+            protocol::Debugger::Paused::ReasonEnum::Instrumentation,
+            std::move(it->second)));
+      }
       continue;
     }
     auto breakpointIterator = m_debuggerBreakpointIdToBreakpointId.find(id);

src/inspector/v8-debugger.cc

@@ -406,7 +406,6 @@ void V8Debugger::handleProgramBreak(
     v8::debug::PrepareStep(m_isolate, v8::debug::StepOut);
     return;
   }
-  const bool forScheduledBreak = hasScheduledBreakOnNextFunctionCall();
   m_targetContextGroupId = 0;
   m_pauseOnNextCallRequested = false;
   m_pauseOnAsyncCall = false;
@@ -435,77 +434,83 @@ void V8Debugger::handleProgramBreak(
   DCHECK(contextGroupId);
   m_pausedContextGroupId = contextGroupId;
 
-  // First pass is for any instrumentation breakpoints. This effectively does
-  // nothing if none of the breakpoints are instrumentation breakpoints.
-  // `sessionToInstrumentationBreakpoints` is only created if there is at least
-  // one session with an instrumentation breakpoint.
-  std::unique_ptr<
-      std::map<V8InspectorSessionImpl*, std::vector<v8::debug::BreakpointId>>>
-      sessionToInstrumentationBreakpoints;
-  if (forScheduledBreak) {
+  // 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, &breakpointIds, &sessionToInstrumentationBreakpoints](
-            V8InspectorSessionImpl* session) {
+        contextGroupId, [&pausedContext, &instrumentationBreakpointIds](
+                            V8InspectorSessionImpl* session) {
           if (!session->debuggerAgent()->acceptsPause(false)) return;
-
-          const std::vector<v8::debug::BreakpointId>
-              instrumentationBreakpointIds =
-                  session->debuggerAgent()
-                      ->instrumentationBreakpointIdsMatching(breakpointIds);
-          if (instrumentationBreakpointIds.empty()) return;
-
-          if (!sessionToInstrumentationBreakpoints) {
-            sessionToInstrumentationBreakpoints = std::make_unique<
-                std::map<V8InspectorSessionImpl*,
-                         std::vector<v8::debug::BreakpointId>>>();
-          }
-          (*sessionToInstrumentationBreakpoints)[session] =
-              instrumentationBreakpointIds;
           session->debuggerAgent()->didPause(
               InspectedContext::contextId(pausedContext), {},
               instrumentationBreakpointIds,
               v8::debug::ExceptionType::kException, false, false, false);
         });
-    if (sessionToInstrumentationBreakpoints) {
+    {
       v8::Context::Scope scope(pausedContext);
       m_inspector->client()->runMessageLoopOnPause(contextGroupId);
-
-      m_inspector->forEachSession(
-          contextGroupId, [&sessionToInstrumentationBreakpoints](
-                              V8InspectorSessionImpl* session) {
-            if (session->debuggerAgent()->enabled() &&
-                sessionToInstrumentationBreakpoints->count(session)) {
-              session->debuggerAgent()->didContinue();
-            }
-          });
+    }
+    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);
+      }
     }
   }
 
-  // Second pass is for other breakpoints (which may include instrumentation
-  // breakpoints in certain scenarios).
+  // 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, &breakpointIds, &exceptionType, &isUncaught,
-       &scheduledOOMBreak, &scheduledAssertBreak,
-       &sessionToInstrumentationBreakpoints](V8InspectorSessionImpl* session) {
+      contextGroupId, [&pausedContext, &exception, &regularBreakpointIds,
+                       &exceptionType, &isUncaught, &scheduledOOMBreak,
+                       &scheduledAssertBreak](V8InspectorSessionImpl* session) {
         if (session->debuggerAgent()->acceptsPause(scheduledOOMBreak)) {
-          std::vector<v8::debug::BreakpointId> sessionBreakpointIds =
-              breakpointIds;
-          if (sessionToInstrumentationBreakpoints) {
-            const std::vector<v8::debug::BreakpointId>
-                instrumentationBreakpointIds =
-                    (*sessionToInstrumentationBreakpoints)[session];
-            for (const v8::debug::BreakpointId& breakpointId :
-                 instrumentationBreakpointIds) {
-              auto iter = std::find(sessionBreakpointIds.begin(),
-                                    sessionBreakpointIds.end(), breakpointId);
-              sessionBreakpointIds.erase(iter);
-            }
-          }
           session->debuggerAgent()->didPause(
               InspectedContext::contextId(pausedContext), exception,
-              sessionBreakpointIds, exceptionType, isUncaught,
+              regularBreakpointIds, exceptionType, isUncaught,
               scheduledOOMBreak, scheduledAssertBreak);
         }
       });

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

@@ -87,3 +87,10 @@ 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,5 +127,41 @@ 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();
+    }]
+    );

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

@@ -10,12 +10,16 @@ Setting instrumentation breakpoint
 }
 Compiling wasm module.
 Paused at v8://test/compile_module with reason "instrumentation".
+Hit breakpoints: []
 Instantiating module.
 Paused at v8://test/instantiate with reason "instrumentation".
+Hit breakpoints: []
 Paused at wasm://wasm/20da547a with reason "instrumentation".
 Script wasm://wasm/20da547a byte offset 26: Wasm opcode 0x01 (kExprNop)
+Hit breakpoints: []
 Instantiating a second time (should trigger no breakpoint).
 Paused at v8://test/instantiate2 with reason "instrumentation".
+Hit breakpoints: []
 Done.
 
 Running test: testBreakInStartFunctionCompileTwice
@@ -28,12 +32,16 @@ Setting instrumentation breakpoint
 }
 Instantiating module.
 Paused at v8://test/instantiate with reason "instrumentation".
+Hit breakpoints: []
 Paused at wasm://wasm/20da547a with reason "instrumentation".
 Script wasm://wasm/20da547a byte offset 26: Wasm opcode 0x01 (kExprNop)
+Hit breakpoints: []
 Instantiating a second time (should trigger another breakpoint).
 Paused at v8://test/instantiate with reason "instrumentation".
+Hit breakpoints: []
 Paused at wasm://wasm/20da547a with reason "instrumentation".
 Script wasm://wasm/20da547a byte offset 26: Wasm opcode 0x01 (kExprNop)
+Hit breakpoints: []
 Done.
 
 Running test: testBreakInExportedFunction
@@ -46,12 +54,16 @@ Setting instrumentation breakpoint
 }
 Instantiating wasm module.
 Paused at v8://test/instantiate with reason "instrumentation".
+Hit breakpoints: []
 Calling exported function 'func' (should trigger a breakpoint).
 Paused at v8://test/call_func with reason "instrumentation".
+Hit breakpoints: []
 Paused at wasm://wasm/8c388106 with reason "instrumentation".
 Script wasm://wasm/8c388106 byte offset 33: Wasm opcode 0x01 (kExprNop)
+Hit breakpoints: []
 Calling exported function 'func' a second time (should trigger no breakpoint).
 Paused at v8://test/call_func with reason "instrumentation".
+Hit breakpoints: []
 Done.
 
 Running test: testBreakOnlyWithSourceMap
@@ -68,4 +80,5 @@ Instantiating wasm module with source map.
 Calling exported function 'func' (should trigger a breakpoint).
 Paused at wasm://wasm/c8e3a856 with reason "instrumentation".
 Script wasm://wasm/c8e3a856 byte offset 33: Wasm opcode 0x01 (kExprNop)
+Hit breakpoints: []
 Done.

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

@@ -11,10 +11,15 @@ session.setupScriptMap();
 Protocol.Debugger.onPaused(async msg => {
   let top_frame = msg.params.callFrames[0];
   let reason = msg.params.reason;
+  let hitBreakpoints = msg.params.hitBreakpoints;
   InspectorTest.log(`Paused at ${top_frame.url} with reason "${reason}".`);
   if (!top_frame.url.startsWith('v8://test/')) {
     await session.logSourceLocation(top_frame.location);
   }
+  // Report the hit breakpoints to make sure that it is empty, as
+  // instrumentation breakpoints should not be reported as normal
+  // breakpoints.
+  InspectorTest.log(`Hit breakpoints: ${JSON.stringify(hitBreakpoints)}`)
   Protocol.Debugger.resume();
 });