[debugger] Explicitly encode 'other' as reason for breaks and pauses

This CL explicitly encodes the 'other' reason for breaking for:
* regular breakpoints
* triggered pause events.

The reason for explicitly encoding the reason is that we may otherwise
not know why we pause when we handle it. This knowledge is needed
in order to fully support instrumentation breakpoints, e.g. if we do
not know that we paused on a triggered pause, and this happens to
overlap with an instrumentation, we would previously only report
'instrumentation' as a reason which would be wrong.

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

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

@@ -1128,14 +1128,14 @@ void V8DebuggerAgentImpl::cancelPauseOnNextStatement() {
 Response V8DebuggerAgentImpl::pause() {
   if (!enabled()) return Response::ServerError(kDebuggerNotEnabled);
   if (isPaused()) return Response::Success();
+
+  pushBreakDetails(protocol::Debugger::Paused::ReasonEnum::Other, nullptr);
   if (m_debugger->canBreakProgram()) {
     m_debugger->interruptAndBreak(m_session->contextGroupId());
   } else {
-    if (m_breakReason.empty()) {
-      m_debugger->setPauseOnNextCall(true, m_session->contextGroupId());
-    }
-    pushBreakDetails(protocol::Debugger::Paused::ReasonEnum::Other, nullptr);
+    m_debugger->setPauseOnNextCall(true, m_session->contextGroupId());
   }
+
   return Response::Success();
 }
 
@@ -1782,7 +1782,7 @@ void V8DebuggerAgentImpl::didPause(
 
   auto hitBreakpointIds = std::make_unique<Array<String16>>();
   bool hitInstrumentationBreakpoint = false;
-
+  bool hitRegularBreakpoint = false;
   for (const auto& id : hitBreakpoints) {
     auto it = m_breakpointsOnScriptRun.find(id);
     if (it != m_breakpointsOnScriptRun.end()) {
@@ -1808,16 +1808,25 @@ void V8DebuggerAgentImpl::didPause(
     hitBreakpointIds->emplace_back(breakpointId);
     BreakpointType type;
     parseBreakpointId(breakpointId, &type);
-    if (type != BreakpointType::kDebugCommand) continue;
-    hitReasons.push_back(std::make_pair(
-        protocol::Debugger::Paused::ReasonEnum::DebugCommand, nullptr));
+    if (type == BreakpointType::kDebugCommand) {
+      hitReasons.push_back(std::make_pair(
+          protocol::Debugger::Paused::ReasonEnum::DebugCommand, nullptr));
+    } else {
+      hitRegularBreakpoint = true;
+    }
   }
 
+  if (hitRegularBreakpoint) {
+    hitReasons.push_back(
+        std::make_pair(protocol::Debugger::Paused::ReasonEnum::Other, nullptr));
+  }
   for (size_t i = 0; i < m_breakReason.size(); ++i) {
     hitReasons.push_back(std::move(m_breakReason[i]));
   }
   clearBreakDetails();
 
+  // TODO(kimanh): We should always know why we pause. Print a
+  // warning if we don't and fall back to Other.
   String16 breakReason = protocol::Debugger::Paused::ReasonEnum::Other;
   std::unique_ptr<protocol::DictionaryValue> breakAuxData;
   if (hitReasons.size() == 1) {

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

@@ -0,0 +1,8 @@
+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: {}.
+
+Running test: testTriggeredPausePauseReason
+Paused with reason: ambiguous and data: {"reasons":[{"reason":"instrumentation","auxData":{"url":"foo.js","scriptId":"4"}},{"reason":"other"}]}.

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

@@ -0,0 +1,49 @@
+// Copyright 2021 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 that all 'other' reasons are explicitly encoded on a pause event if they overlap with another reason`);
+
+Protocol.Debugger.onPaused(({params: {reason, data}}) => {
+  InspectorTest.log(`Paused with reason: ${reason} and data: ${data ? JSON.stringify(data) : '{}'}.`)
+  Protocol.Debugger.resume();
+});
+
+async function setUpEnvironment() {
+  await Protocol.Debugger.enable();
+  await Protocol.Runtime.enable();
+}
+
+async function tearDownEnvironment() {
+  await Protocol.Debugger.disable();
+  await Protocol.Runtime.disable();
+}
+
+InspectorTest.runAsyncTestSuite([
+  async function testBreakpointPauseReason() {
+    await setUpEnvironment()
+    await Protocol.Debugger .setInstrumentationBreakpoint({
+      instrumentation: 'beforeScriptExecution'
+    });
+    const {result: { scriptId }} = await Protocol.Runtime.compileScript({
+    expression: `console.log('foo');`, sourceURL: 'foo.js', persistScript: true });
+    await Protocol.Debugger.setBreakpointByUrl({
+        lineNumber: 0,
+        columnNumber: 0,
+        url: 'foo.js',
+        });
+    await Protocol.Runtime.runScript({ scriptId });
+    tearDownEnvironment();
+  },
+  async function testTriggeredPausePauseReason() {
+    await setUpEnvironment();
+    await Protocol.Debugger.setInstrumentationBreakpoint({
+      instrumentation: 'beforeScriptExecution'
+    });
+    Protocol.Debugger.pause();
+    await Protocol.Runtime.evaluate({
+      expression: `console.log('foo');//# sourceURL=foo.js`});
+    tearDownEnvironment();
+  }]
+);