[debugger] Fix step out when instrumentation breaks are turned on

When triggering a step out action, we check whether we already
are at a return or suspend location. If not, we first flood all
return positions with breakpoints, set the fast_forward_to_return_
flag and continue.

With the new way of reporting instrumentation breakpoints, we now
may get into the situation where we stopped on an instrumentation,
but may still need to continue until we reach the return point for
the step out. This CL fixes a bug in which we ran into a DCHECK
that expected us to stop on a return location (since
fast_forward_to_return_ is set to true), but we didn't.

Drive-by: adapt other stepping tests to properly wait for all pauses

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

src/debug/debug.cc

@@ -503,7 +503,9 @@ void Debug::Break(JavaScriptFrame* frame, Handle<JSFunction> break_target) {
 
   // Find the break location where execution has stopped.
   BreakLocation location = BreakLocation::FromFrame(debug_info, frame);
-  if (IsBreakOnInstrumentation(debug_info, location)) {
+  const bool hitInstrumentationBreak =
+      IsBreakOnInstrumentation(debug_info, location);
+  if (hitInstrumentationBreak) {
     OnInstrumentationBreak();
   }
 
@@ -540,7 +542,9 @@ void Debug::Break(JavaScriptFrame* frame, Handle<JSFunction> break_target) {
   // StepOut at not return position was requested and return break locations
   // were flooded with one shots.
   if (thread_local_.fast_forward_to_return_) {
-    DCHECK(location.IsReturnOrSuspend());
+    // We might hit an instrumentation breakpoint before running into a
+    // return/suspend location.
+    DCHECK(location.IsReturnOrSuspend() || hitInstrumentationBreak);
     // We have to ignore recursive calls to function.
     if (current_frame_count > target_frame_count) return;
     ClearStepping();

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

@@ -1,28 +1,37 @@
 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: {"scriptId":"3","url":"foo.js"}.
+Paused with reason instrumentation, data {"scriptId":"3","url":"foo.js"} and scriptId: 3.
-Paused with reason: other and data: {}.
+Paused with reason other, data {} and scriptId: 3.
 
 Running test: testTriggeredPausePauseReason
-Paused with reason: instrumentation and data: {"scriptId":"4","url":"foo.js"}.
+Paused with reason instrumentation, data {"scriptId":"4","url":"foo.js"} and scriptId: 4.
-Paused with reason: other and data: {}.
+Paused with reason other, data {} and scriptId: 4.
 
 Running test: testSteppingPauseReason
-Paused with reason: instrumentation and data: {"scriptId":"5","url":"foo.js"}.
+Paused with reason instrumentation, data {"scriptId":"5","url":"foo.js"} and scriptId: 5.
-Paused with reason: other and data: {}.
+Paused with reason other, data {} and scriptId: 5.
-Paused with reason: other and data: {}.
+Paused with reason other, data {} and scriptId: 5.
-Paused with reason: instrumentation and data: {"scriptId":"6","url":"bar.js"}.
+Paused with reason instrumentation, data {"scriptId":"6","url":"bar.js"} and scriptId: 6.
+Paused with reason other, data {} and scriptId: 6.
 
 Running test: testOnlyReportOtherWithEmptyDataOnce
-Paused with reason: other and data: {}.
+Paused with reason other, data {} and scriptId: 7.
 
 Running test: testDebuggerStatementReason
-Paused with reason: instrumentation and data: {"scriptId":"8","url":"foo.js"}.
+Paused with reason instrumentation, data {"scriptId":"8","url":"foo.js"} and scriptId: 8.
-Paused with reason: other and data: {}.
+Paused with reason other, data {} and scriptId: 8.
 
 Running test: testAsyncSteppingPauseReason
-Paused with reason: instrumentation and data: {"scriptId":"9","url":"foo.js"}.
+Paused with reason instrumentation, data {"scriptId":"9","url":"foo.js"} and scriptId: 9.
-Paused with reason: other and data: {}.
+Paused with reason other, data {} and scriptId: 9.
-Paused with reason: other and data: {}.
+Paused with reason other, data {} and scriptId: 9.
-Paused with reason: instrumentation and data: {"scriptId":"10","url":"bar.js"}.
+Paused with reason instrumentation, data {"scriptId":"10","url":"bar.js"} and scriptId: 10.
+Paused with reason other, data {} and scriptId: 10.
+Paused with reason other, data {} and scriptId: 10.
+
+Running test: testSteppingOutPauseReason
+Paused with reason instrumentation, data {"scriptId":"11","url":"foo.js"} and scriptId: 11.
+Paused with reason other, data {} and scriptId: 11.
+Paused with reason instrumentation, data {"scriptId":"12","url":"bar.js"} and scriptId: 12.
+Paused with reason other, data {} and scriptId: 11.

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

@@ -5,12 +5,22 @@
 const { session, contextGroup, Protocol } = InspectorTest.start(
     `Test that all 'other' reasons are explicitly encoded on a pause event if they overlap with another reason`);
 
-function resumeOnPause({params: {reason, data}}) {
+function handlePause(
-  InspectorTest.log(`Paused with reason: ${reason} and data: ${
+    noInstrumentationStepAction, options,
-      data ? JSON.stringify(data) : '{}'}.`)
+    {params: {reason, data, callFrames}}) {
-  Protocol.Debugger.resume();
+  const scriptId = callFrames[0].functionLocation.scriptId;
+  InspectorTest.log(`Paused with reason ${reason}, data ${
+      data ? JSON.stringify(data) : '{}'} and scriptId: ${scriptId}.`);
+
+  if (reason === 'instrumentation') {
+    Protocol.Debugger.resume();
+  } else {
+    Protocol.Debugger[noInstrumentationStepAction](options);
+  }
 }
 
+const resumeOnPause = handlePause.bind(null, 'resume', null);
+
 async function setUpEnvironment() {
   await Protocol.Debugger.enable();
   await Protocol.Runtime.enable();
@@ -53,17 +63,6 @@ InspectorTest.runAsyncTestSuite([
     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) : '{}'}.`);
-      if (reason === 'instrumentation') {
-        Protocol.Debugger.resume();
-      } else {
-        Protocol.Debugger.stepInto();
-      }
-    });
-    Protocol.Debugger.onPaused(stepOnPause);
-
     const {result: {scriptId}} = await Protocol.Runtime.compileScript({
       expression: `setTimeout('console.log(3);//# sourceURL=bar.js', 0);`,
       sourceURL: 'foo.js',
@@ -74,7 +73,17 @@ InspectorTest.runAsyncTestSuite([
       url: 'foo.js',
     });
 
-    await Protocol.Runtime.runScript({scriptId});
+    const runPromise = Protocol.Runtime.runScript({scriptId});
+    // Pausing 5 times:
+    // 2x instrumentation breaks,
+    // 1x breakpoint,
+    // 2x step ins: end of setTimeout function, start of inner script.
+    for (var i = 0; i < 5; ++i) {
+      const msg = await Protocol.Debugger.oncePaused();
+      handlePause('stepInto', null, msg);
+    }
+
+    await runPromise;
     await tearDownEnvironment();
   },
   async function testOnlyReportOtherWithEmptyDataOnce() {
@@ -110,17 +119,41 @@ InspectorTest.runAsyncTestSuite([
     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});
+    const runPromise = Protocol.Runtime.runScript({scriptId});
+    // Pausing 6 times:
+    // 2x instrumentation breaks,
+    // 1x debugger statement,
+    // 3x steps in: start of setTimeout, start of inner script, end of inner script.
+    for (var i = 0; i < 6; ++i) {
+      const msg = await Protocol.Debugger.oncePaused();
+      handlePause('stepInto', {breakOnAsyncCall: true}, msg);
+    }
+    await runPromise;
+    await tearDownEnvironment();
+  },
+  async function testSteppingOutPauseReason() {
+    await setUpEnvironment();
+    await Protocol.Debugger.setInstrumentationBreakpoint(
+        {instrumentation: 'beforeScriptExecution'});
+    const expression = `
+    function test() {
+      debugger;
+      eval('console.log(3);//# sourceURL=bar.js');
+    }
+    test();
+    `
+    const {result: {scriptId}} = await Protocol.Runtime.compileScript(
+        {expression, sourceURL: 'foo.js', persistScript: true});
+
+    const runPromise = Protocol.Runtime.runScript({scriptId});
+    const stepOutOnPause = handlePause.bind(this, 'stepOut', null);
+    Protocol.Debugger.onPaused(stepOutOnPause);
+
+    await runPromise;
     await tearDownEnvironment();
   },
 ]);