[inspector] Handle instrumentation break with multiple sessions

Currently, any session can resume instrumentation breaks by sending
Debugger.resume command. That can lead to unreliable breakpoint
placement because sessions can resume too early.

The early resumption can happen in two ways:

- When we have two instrumented sessions, the first one to resume
  can prevent the other one from setting its breakpoints
  before executing the code.

- With one instrumented session and one without instrumentation
  breakpoints, the uninstrumented session's Debugger.resume
  command can resume the instrumentation pause before the
  instrumented session can set its breakpoints.

This patch fixes both of these issues by changing the instrumentation
pause resumption logic to take note of the sessions that were notified
about the instrumentation breakpoints. The debugger will only resume
once all those sessions resume (or disconnect).

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

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

@@ -452,6 +452,7 @@ Response V8DebuggerAgentImpl::disable() {
   m_state->setBoolean(DebuggerAgentState::skipAllPauses, false);
   m_state->remove(DebuggerAgentState::blackboxPattern);
   m_enableState = kDisabled;
+  m_instrumentationFinished = true;
   m_state->setBoolean(DebuggerAgentState::debuggerEnabled, false);
   m_debugger->disable();
   return Response::Success();
@@ -1352,6 +1353,8 @@ Response V8DebuggerAgentImpl::pause() {
 Response V8DebuggerAgentImpl::resume(Maybe<bool> terminateOnResume) {
   if (!isPaused()) return Response::ServerError(kDebuggerNotPaused);
   m_session->releaseObjectGroup(kBacktraceObjectGroup);
+
+  m_instrumentationFinished = true;
   m_debugger->continueProgram(m_session->contextGroupId(),
                               terminateOnResume.fromMaybe(false));
   return Response::Success();
@@ -1946,6 +1949,7 @@ void V8DebuggerAgentImpl::didPauseOnInstrumentation(
       m_debuggerBreakpointIdToBreakpointId.end()) {
     DCHECK_GT(protocolCallFrames->size(), 0);
     if (protocolCallFrames->size() > 0) {
+      m_instrumentationFinished = false;
       breakReason = protocol::Debugger::Paused::ReasonEnum::Instrumentation;
       const String16 scriptId =
           protocolCallFrames->at(0)->getLocation()->getScriptId();

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

@@ -161,6 +161,7 @@ class V8DebuggerAgentImpl : public protocol::Debugger::Backend {
 
   void reset();
 
+  bool instrumentationFinished() { return m_instrumentationFinished; }
   // Interface for V8InspectorImpl
   void didPauseOnInstrumentation(v8::debug::BreakpointId instrumentationId);
 
@@ -268,6 +269,7 @@ class V8DebuggerAgentImpl : public protocol::Debugger::Backend {
 
   bool m_skipAllPauses = false;
   bool m_breakpointsActive = false;
+  bool m_instrumentationFinished = true;
 
   std::unique_ptr<V8Regex> m_blackboxPattern;
   std::unordered_map<String16, std::vector<std::pair<int, int>>>

src/inspector/v8-debugger.cc

@@ -110,14 +110,20 @@ void V8Debugger::disable() {
   if (isPaused()) {
     bool scheduledOOMBreak = m_scheduledOOMBreak;
     bool hasAgentAcceptsPause = false;
-    m_inspector->forEachSession(
-        m_pausedContextGroupId, [&scheduledOOMBreak, &hasAgentAcceptsPause](
-                                    V8InspectorSessionImpl* session) {
-          if (session->debuggerAgent()->acceptsPause(scheduledOOMBreak)) {
-            hasAgentAcceptsPause = true;
-          }
-        });
-    if (!hasAgentAcceptsPause) m_inspector->client()->quitMessageLoopOnPause();
+
+    if (m_instrumentationPause) {
+      quitMessageLoopIfAgentsFinishedInstrumentation();
+    } else {
+      m_inspector->forEachSession(
+          m_pausedContextGroupId, [&scheduledOOMBreak, &hasAgentAcceptsPause](
+                                      V8InspectorSessionImpl* session) {
+            if (session->debuggerAgent()->acceptsPause(scheduledOOMBreak)) {
+              hasAgentAcceptsPause = true;
+            }
+          });
+      if (!hasAgentAcceptsPause)
+        m_inspector->client()->quitMessageLoopOnPause();
+    }
   }
   if (--m_enableCount) return;
   clearContinueToLocation();
@@ -239,14 +245,32 @@ void V8Debugger::requestPauseAfterInstrumentation() {
   m_requestedPauseAfterInstrumentation = true;
 }
 
+void V8Debugger::quitMessageLoopIfAgentsFinishedInstrumentation() {
+  bool allAgentsFinishedInstrumentation = true;
+  m_inspector->forEachSession(
+      m_pausedContextGroupId,
+      [&allAgentsFinishedInstrumentation](V8InspectorSessionImpl* session) {
+        if (!session->debuggerAgent()->instrumentationFinished()) {
+          allAgentsFinishedInstrumentation = false;
+        }
+      });
+  if (allAgentsFinishedInstrumentation) {
+    m_inspector->client()->quitMessageLoopOnPause();
+  }
+}
+
 void V8Debugger::continueProgram(int targetContextGroupId,
                                  bool terminateOnResume) {
   if (m_pausedContextGroupId != targetContextGroupId) return;
   if (isPaused()) {
-    if (terminateOnResume) {
+    if (m_instrumentationPause) {
+      quitMessageLoopIfAgentsFinishedInstrumentation();
+    } else if (terminateOnResume) {
       v8::debug::SetTerminateOnResume(m_isolate);
+      m_inspector->client()->quitMessageLoopOnPause();
+    } else {
+      m_inspector->client()->quitMessageLoopOnPause();
     }
-    m_inspector->client()->quitMessageLoopOnPause();
   }
 }
 

src/inspector/v8-debugger.h

@@ -211,6 +211,8 @@ class V8Debugger : public v8::debug::DebugDelegate,
 
   bool hasScheduledBreakOnNextFunctionCall() const;
 
+  void quitMessageLoopIfAgentsFinishedInstrumentation();
+
   v8::Isolate* m_isolate;
   V8InspectorImpl* m_inspector;
   int m_enableCount;

test/inspector/debugger/instrumentation-multiple-sessions-expected.txt

@@ -0,0 +1,64 @@
+Checks instrumentation pause with multiple sessions
+
+Running test: testTwoInstrumentationBreaksResume
+Created two sessions.
+Paused 1: instrumentation
+Paused 2: instrumentation
+Resumed session 1
+Resumed session 2
+Evaluation result: 42
+Evaluation finished
+
+Running test: testInstrumentedSessionNotification
+Created two sessions.
+Session 1 paused (instrumentation)
+Session 2 paused (other)
+Resumed session 1
+Resumed session 2
+Evaluation result: 42
+Evaluation finished
+
+Running test: testNonInstrumentedSessionCannotsResumeInstrumentationPause
+Created two sessions.
+Session 1 paused (instrumentation)
+Session 2 paused (other)
+Called "resume" on session 2
+Called "resume" on session 1
+Resumed session 1
+Resumed session 2
+Evaluation result: 42
+Evaluation finished
+
+Running test: testEvaluationFromNonInstrumentedSession
+Created two sessions.
+Session 1 paused (instrumentation)
+Session 2 paused (other)
+Called "resume" on session 1
+Resumed session 1
+Resumed session 2
+Evaluation result: 42
+Evaluation finished
+
+Running test: testTransparentEvaluationFromNonInstrumentedSessionDuringPause
+Created two sessions.
+Session 1 paused (instrumentation)
+Session 2 paused (other)
+Resumed session 1
+Session 2 evaluation result: 42
+
+Running test: testInstrumentationStopResumesWithOtherSessions
+Created two sessions.
+Session 1 paused (instrumentation)
+Stopped session 1
+Resumed session 2
+Session 2 evaluation result: 42
+
+Running test: testInstrumentationPauseAndNormalPause
+Created two sessions.
+Session 1 paused (instrumentation)
+Session 2 paused (other)
+Session 2 pause requested
+Session 1 instrumentation resume requested
+Session 2 paused (other)
+Session 2 resumed
+Session 1 evaluation result: 42

test/inspector/debugger/instrumentation-multiple-sessions.js

@@ -0,0 +1,292 @@
+// 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 instrumentation pause with multiple sessions');
+
+InspectorTest.runAsyncTestSuite([
+  async function testTwoInstrumentationBreaksResume() {
+    // Initialize two sessions with instrumentation breakpoints.
+    let contextGroup = new InspectorTest.ContextGroup();
+    let session1 = contextGroup.connect();
+    let Protocol1 = session1.Protocol;
+    Protocol1.Debugger.enable();
+    await Protocol1.Debugger.setInstrumentationBreakpoint(
+        {instrumentation: 'beforeScriptExecution'});
+    const session2 = contextGroup.connect();
+    const Protocol2 = session2.Protocol;
+    await Protocol2.Debugger.enable();
+    await Protocol2.Debugger.setInstrumentationBreakpoint(
+        {instrumentation: 'beforeScriptExecution'});
+    InspectorTest.log('Created two sessions.');
+
+    // Expect both sessions pausing on instrumentation breakpoint.
+    const paused1 = Protocol1.Debugger.oncePaused();
+    const paused2 = Protocol2.Debugger.oncePaused();
+    const evaluationFinished =
+        Protocol1.Runtime.evaluate({expression: '42'})
+            .then(
+                r => InspectorTest.log(
+                    `Evaluation result: ${r.result.result.value}`));
+
+    // Verify the instrumentation breakpoint puased the sessions.
+    InspectorTest.log(`Paused 1: ${(await paused1).params.reason}`);
+    InspectorTest.log(`Paused 2: ${(await paused2).params.reason}`);
+
+    // Let us call resume in the first session and make sure that this
+    // does not resume the instrumentation pause (the instrumentation
+    // pause should only resume once all sessions ask for resumption).
+    //
+    // Unfortunately, we cannot check for absence of resumptions, so
+    // let's just give the evaluation chance to finish early by calling
+    // 'resume' on the first session multiple times.
+    for (let i = 0; i < 20; i++) {
+      await Protocol1.Debugger.resume();
+    }
+    InspectorTest.log('Resumed session 1');
+
+    // Resuming the second session should allow the evaluation to
+    // finish.
+    await Protocol2.Debugger.resume();
+    InspectorTest.log('Resumed session 2');
+
+    await evaluationFinished;
+    InspectorTest.log('Evaluation finished');
+  },
+  async function testInstrumentedSessionNotification() {
+    // Initialize two debugger sessions - one with instrumentation
+    // breakpoints, one without.
+    let contextGroup = new InspectorTest.ContextGroup();
+    let session1 = contextGroup.connect();
+    let Protocol1 = session1.Protocol;
+    Protocol1.Debugger.enable();
+    await Protocol1.Debugger.setInstrumentationBreakpoint(
+        {instrumentation: 'beforeScriptExecution'});
+    const session2 = contextGroup.connect();
+    const Protocol2 = session2.Protocol;
+    await Protocol2.Debugger.enable();
+    InspectorTest.log('Created two sessions.');
+
+    // Verify that the instrumented session sees the instrumentation pause.
+    const paused1 = Protocol1.Debugger.oncePaused();
+    const paused2 = Protocol2.Debugger.oncePaused();
+    const evaluationFinished =
+        Protocol1.Runtime.evaluate({expression: '42'})
+            .then(
+                r => InspectorTest.log(
+                    `Evaluation result: ${r.result.result.value}`));
+    InspectorTest.log(`Session 1 paused (${(await paused1).params.reason})`);
+    InspectorTest.log(`Session 2 paused (${(await paused2).params.reason})`);
+
+    const onResume1 = Protocol1.Debugger.onceResumed();
+    const onResume2 = Protocol2.Debugger.onceResumed();
+    await Protocol1.Debugger.resume();
+    await onResume1;
+    InspectorTest.log('Resumed session 1');
+    await onResume2;
+    InspectorTest.log('Resumed session 2');
+
+    await evaluationFinished;
+    InspectorTest.log('Evaluation finished');
+  },
+  async function testNonInstrumentedSessionCannotsResumeInstrumentationPause() {
+    // Initialize two debugger sessions - one with instrumentation
+    // breakpoints, one without.
+    let contextGroup = new InspectorTest.ContextGroup();
+    let session1 = contextGroup.connect();
+    let Protocol1 = session1.Protocol;
+    Protocol1.Debugger.enable();
+    await Protocol1.Debugger.setInstrumentationBreakpoint(
+        {instrumentation: 'beforeScriptExecution'});
+    const session2 = contextGroup.connect();
+    const Protocol2 = session2.Protocol;
+    await Protocol2.Debugger.enable();
+    InspectorTest.log('Created two sessions.');
+
+    // Make sure the non-instrumentation session does not pause or resume on
+    // instrumentation.
+    Protocol2.Debugger.onResumed(
+        m => InspectorTest.log('[Unexpected] Session 2 resumed'));
+
+    // Induce instrumentation pause.
+    const paused1 = Protocol1.Debugger.oncePaused();
+    const paused2 = Protocol2.Debugger.oncePaused();
+    const evaluationFinished =
+        Protocol1.Runtime.evaluate({expression: '42'})
+            .then(
+                r => InspectorTest.log(
+                    `Evaluation result: ${r.result.result.value}`));
+    InspectorTest.log(`Session 1 paused (${(await paused1).params.reason})`);
+    InspectorTest.log(`Session 2 paused (${(await paused2).params.reason})`);
+
+    // Calling 'resume' on the non-instrumented session should not have any
+    // effect on the session in the instrumentation pause.
+    for (let i = 0; i < 10; i++) {
+      await Protocol2.Debugger.resume();
+    }
+    InspectorTest.log('Called "resume" on session 2');
+
+    const onResume1 = Protocol1.Debugger.onceResumed();
+    const onResume2 = Protocol2.Debugger.onceResumed();
+    await Protocol1.Debugger.resume();
+    InspectorTest.log('Called "resume" on session 1');
+    await onResume1;
+    InspectorTest.log('Resumed session 1');
+    await onResume2;
+    InspectorTest.log('Resumed session 2');
+
+    await evaluationFinished;
+    InspectorTest.log('Evaluation finished');
+  },
+  async function testEvaluationFromNonInstrumentedSession() {
+    // Initialize two debugger sessions - one with instrumentation
+    // breakpoints, one without.
+    let contextGroup = new InspectorTest.ContextGroup();
+    let session1 = contextGroup.connect();
+    let Protocol1 = session1.Protocol;
+    Protocol1.Debugger.enable();
+    await Protocol1.Debugger.setInstrumentationBreakpoint(
+        {instrumentation: 'beforeScriptExecution'});
+    const session2 = contextGroup.connect();
+    const Protocol2 = session2.Protocol;
+    await Protocol2.Debugger.enable();
+    InspectorTest.log('Created two sessions.');
+
+    // Start evaluation in the non-instrumentation session and expect that
+    // the instrumentation session is paused.
+    const paused1 = Protocol1.Debugger.oncePaused();
+    const paused2 = Protocol2.Debugger.oncePaused();
+    const evaluationFinished =
+        Protocol2.Runtime.evaluate({expression: '42'})
+            .then(
+                r => InspectorTest.log(
+                    `Evaluation result: ${r.result.result.value}`));
+    InspectorTest.log(`Session 1 paused (${(await paused1).params.reason})`);
+    InspectorTest.log(`Session 2 paused (${(await paused2).params.reason})`);
+
+    const onResume1 = Protocol1.Debugger.onceResumed();
+    const onResume2 = Protocol2.Debugger.onceResumed();
+    await Protocol1.Debugger.resume();
+    InspectorTest.log('Called "resume" on session 1');
+    await onResume1;
+    InspectorTest.log('Resumed session 1');
+    await onResume2;
+    InspectorTest.log('Resumed session 2');
+
+    await evaluationFinished;
+    InspectorTest.log('Evaluation finished');
+  },
+  async function
+      testTransparentEvaluationFromNonInstrumentedSessionDuringPause() {
+        // Initialize two debugger sessions - one with instrumentation
+        // breakpoints, one without.
+        let contextGroup = new InspectorTest.ContextGroup();
+        let session1 = contextGroup.connect();
+        let Protocol1 = session1.Protocol;
+        Protocol1.Debugger.enable();
+        await Protocol1.Debugger.setInstrumentationBreakpoint(
+            {instrumentation: 'beforeScriptExecution'});
+        const session2 = contextGroup.connect();
+        const Protocol2 = session2.Protocol;
+        await Protocol2.Debugger.enable();
+        InspectorTest.log('Created two sessions.');
+
+        // Enter instrumentation pause.
+        const paused1 = Protocol1.Debugger.oncePaused();
+        const paused2 = Protocol2.Debugger.oncePaused();
+        Protocol1.Runtime.evaluate({expression: 'null'})
+        InspectorTest.log(
+            `Session 1 paused (${(await paused1).params.reason})`);
+        InspectorTest.log(
+            `Session 2 paused (${(await paused2).params.reason})`);
+
+        // Start evaluation in session 2.
+        const evaluation = Protocol2.Runtime.evaluate({expression: '42'});
+
+        await Protocol1.Debugger.resume();
+        InspectorTest.log('Resumed session 1');
+
+        // Make sure the evaluation finished.
+        InspectorTest.log(`Session 2 evaluation result: ${
+            (await evaluation).result.result.value}`);
+      },
+  async function testInstrumentationStopResumesWithOtherSessions() {
+    // Initialize two debugger sessions - one with instrumentation
+    // breakpoints, one without.
+    let contextGroup = new InspectorTest.ContextGroup();
+
+    let session1 = contextGroup.connect();
+    let Protocol1 = session1.Protocol;
+    Protocol1.Debugger.enable();
+    await Protocol1.Debugger.setInstrumentationBreakpoint(
+        {instrumentation: 'beforeScriptExecution'});
+
+    const session2 = contextGroup.connect();
+    const Protocol2 = session2.Protocol;
+    await Protocol2.Debugger.enable();
+
+    InspectorTest.log('Created two sessions.');
+
+    // Enter instrumentation pause.
+    const paused1 = Protocol1.Debugger.oncePaused();
+    Protocol1.Runtime.evaluate({expression: 'null'})
+    InspectorTest.log(`Session 1 paused (${(await paused1).params.reason})`);
+
+    // Start evaluation in session 2.
+    const evaluation = Protocol2.Runtime.evaluate({expression: '42'});
+
+    // Stop the first session.
+    const onResume2 = Protocol2.Debugger.onceResumed();
+    session1.stop();
+    InspectorTest.log('Stopped session 1');
+
+    await onResume2;
+    InspectorTest.log('Resumed session 2');
+
+    // Make sure the second session gets the evaluation result.
+    InspectorTest.log(`Session 2 evaluation result: ${
+        (await evaluation).result.result.value}`);
+  },
+  async function testInstrumentationPauseAndNormalPause() {
+    // Initialize two debugger sessions - one with instrumentation
+    // breakpoints, one without.
+    let contextGroup = new InspectorTest.ContextGroup();
+    let session1 = contextGroup.connect();
+    let Protocol1 = session1.Protocol;
+    Protocol1.Debugger.enable();
+    await Protocol1.Debugger.setInstrumentationBreakpoint(
+        {instrumentation: 'beforeScriptExecution'});
+
+    const session2 = contextGroup.connect();
+    const Protocol2 = session2.Protocol;
+    await Protocol2.Debugger.enable();
+
+    InspectorTest.log('Created two sessions.');
+
+    // Enter instrumentation pause.
+    const paused1 = Protocol1.Debugger.oncePaused();
+    const instrumentationPaused2 = Protocol2.Debugger.oncePaused();
+    const evaluation = Protocol1.Runtime.evaluate({expression: '42'})
+    InspectorTest.log(`Session 1 paused (${(await paused1).params.reason})`);
+    InspectorTest.log(
+        `Session 2 paused (${(await instrumentationPaused2).params.reason})`);
+
+    await Protocol2.Debugger.pause();
+    InspectorTest.log('Session 2 pause requested');
+    await Protocol1.Debugger.resume();
+    InspectorTest.log('Session 1 instrumentation resume requested');
+
+    // Check that the second session pauses and resumes correctly.
+    const userPaused2 = Protocol1.Debugger.oncePaused();
+    InspectorTest.log(
+        `Session 2 paused (${(await userPaused2).params.reason})`);
+
+    const resumed2 = Protocol2.Debugger.onceResumed();
+    Protocol2.Debugger.resume();
+    await resumed2;
+    InspectorTest.log('Session 2 resumed');
+
+    InspectorTest.log(`Session 1 evaluation result: ${
+        (await evaluation).result.result.value}`);
+  }
+]);