Bug 1172572 - teach debugger to step into calls at the beginning of statements (part 2). r=loganfsmyth

Differential Revision: https://phabricator.services.mozilla.com/D40733

--HG--
extra : moz-landing-system : lando

devtools/server/actors/breakpoint.js

@@ -208,7 +208,7 @@ BreakpointActor.prototype = {
       return undefined;
     }
 
-    if (!this.threadActor.hasMoved(location, "breakpoint")) {
+    if (!this.threadActor.hasMoved(frame, "breakpoint")) {
       return undefined;
     }
 

devtools/server/actors/thread.js

@@ -807,12 +807,10 @@ const ThreadActor = ActorClassWithSpec(threadSpec, {
       }
 
       // Continue forward until we get to a valid step target.
-      const { onStep, onPop } = this._makeSteppingHooks(
+      const { onStep, onPop } = this._makeSteppingHooks({
-        null,
+        steppingType: "next",
-        "next",
+        rewinding: false,
-        false,
+      });
-        null
-      );
 
       if (this.dbg.replaying) {
         const offsets = this._findReplayingStepOffsets(
@@ -830,7 +828,7 @@ const ThreadActor = ActorClassWithSpec(threadSpec, {
     };
   },
 
-  _makeOnPop: function({ pauseAndRespond, startLocation, steppingType }) {
+  _makeOnPop: function({ pauseAndRespond, steppingType }) {
     const thread = this;
     const result = function(completion) {
       // onPop is called with 'this' set to the current frame.
@@ -857,15 +855,12 @@ const ThreadActor = ActorClassWithSpec(threadSpec, {
       if (steppingType == "finish") {
         const parentFrame = thread._getNextStepFrame(this);
         if (parentFrame && parentFrame.script) {
-          // We can't use the completion value in stepping hooks if we're
+          const { onStep, onPop } = thread._makeSteppingHooks({
-          // replaying, as we can't use its contents after resuming.
+            steppingType: "next",
-          const ncompletion = thread.dbg.replaying ? null : completion;
+            rewinding: false,
-          const { onStep, onPop } = thread._makeSteppingHooks(
+            completion,
-            location,
+          });
-            "next",
+
-            false,
-            ncompletion
-          );
           if (thread.dbg.replaying) {
             const parentLocation = thread.sources.getFrameLocation(parentFrame);
             const offsets = thread._findReplayingStepOffsets(
@@ -885,31 +880,17 @@ const ThreadActor = ActorClassWithSpec(threadSpec, {
         }
       }
 
-      return pauseAndRespond(this, packet => {
+      return pauseAndRespond(this, packet =>
-        if (completion) {
+        thread.createCompletionGrip(packet, completion)
-          thread.createCompletionGrip(packet, completion);
+      );
-        } else {
-          packet.why.frameFinished = {
-            terminated: true,
-          };
-        }
-        return packet;
-      });
     };
 
-    // When stepping out, we don't want to stop at a breakpoint that
-    // happened to be set exactly at the spot where we stepped out.
-    // See bug 970469.  We record the location here and check
-    // it when a breakpoint is hit.  Furthermore we store this on the
-    // function because, while we could store it directly on the
-    // frame, if we did we'd also have to find the appropriate spot to
-    // clear it.
-    result.location = startLocation;
-
     return result;
   },
 
-  hasMoved: function(newLocation, newType) {
+  hasMoved: function(frame, newType) {
+    const newLocation = this.sources.getFrameLocation(frame);
+
     if (!this._priorPause) {
       return true;
     }
@@ -928,44 +909,9 @@ const ThreadActor = ActorClassWithSpec(threadSpec, {
     return line !== newLocation.line || column !== newLocation.column;
   },
 
-  // Return whether reaching a script offset should be considered a distinct
-  // "step" from another location.
-  _intraFrameLocationIsStepTarget: function(startLocation, script, offset) {
-    // Only allow stepping stops at entry points for the line.
-    if (!script.getOffsetMetadata(offset).isBreakpoint) {
-      return false;
-    }
-
-    const location = this.sources.getScriptOffsetLocation(script, offset);
-
-    if (!startLocation || startLocation.url !== location.url) {
-      return true;
-    }
-
-    // TODO(logan): When we remove points points, this can be removed too as
-    // we assert that we're at a different frame offset from the last time
-    // we paused.
-    if (!this.hasMoved(location)) {
-      return false;
-    }
-
-    // When pause points are specified for the source,
-    // we should pause when we are at a stepOver pause point
-    const pausePoints = location.sourceActor.pausePoints;
-    const pausePoint =
-      pausePoints && findPausePointForLocation(pausePoints, location);
-
-    if (pausePoint) {
-      return pausePoint.step;
-    }
-
-    return script.getOffsetMetadata(offset).isStepStart;
-  },
-
   _makeOnStep: function({
     pauseAndRespond,
     startFrame,
-    startLocation,
     steppingType,
     completion,
     rewinding,
@@ -981,37 +927,45 @@ const ThreadActor = ActorClassWithSpec(threadSpec, {
         this.onPop = undefined;
         return undefined;
       }
-      const location = thread.sources.getFrameLocation(this);
 
-      // Continue if the source is black boxed.
+      if (thread._validFrameStepOffset(this, startFrame)) {
-      if (thread.sources.isBlackBoxed(location.url)) {
-        return undefined;
-      }
-
-      // A step has occurred if we are rewinding and have changed frames.
-      if (rewinding && this !== startFrame) {
-        return pauseAndRespond(this);
-      }
-
-      // A step has occurred if we reached a step target.
-      if (
-        thread._intraFrameLocationIsStepTarget(
-          startLocation,
-          this.script,
-          this.offset
-        )
-      ) {
         return pauseAndRespond(this, packet =>
           thread.createCompletionGrip(packet, completion)
         );
       }
 
-      // Otherwise, let execution continue (we haven't executed enough code to
-      // consider this a "step" yet).
       return undefined;
     };
   },
 
+  _validFrameStepOffset: function(frame, startFrame) {
+    const location = this.sources.getFrameLocation(frame);
+    const offsetMetadata = frame.script.getOffsetMetadata(frame.offset);
+
+    // Continue if the source is blackboxed or
+    // the current location is not a possible breakpoint position.
+    if (
+      !offsetMetadata.isBreakpoint ||
+      this.sources.isBlackBoxed(location.url)
+    ) {
+      return false;
+    }
+
+    // Pause if the frame has changed.
+    if (frame !== startFrame) {
+      return true;
+    }
+
+    // Continue if the location has not changed, which can
+    // occur via loops and recursion.
+    if (!this.hasMoved(frame)) {
+      return false;
+    }
+
+    // Pause if the current location is a step position.
+    return offsetMetadata.isStepStart;
+  },
+
   createCompletionGrip: function(packet, completion) {
     if (!completion) {
       return packet;
@@ -1038,7 +992,7 @@ const ThreadActor = ActorClassWithSpec(threadSpec, {
    * backwards (rewinding == true), return an array of all the step targets
    * that could be reached next from startLocation.
    */
-  _findReplayingStepOffsets: function(startLocation, frame, rewinding) {
+  _findReplayingStepOffsets: function(frame, rewinding) {
     const worklist = [frame.offset],
       seen = [],
       result = [];
@@ -1048,13 +1002,7 @@ const ThreadActor = ActorClassWithSpec(threadSpec, {
         continue;
       }
       seen.push(offset);
-      if (
+      if (this._validFrameStepOffset(frame)) {
-        this._intraFrameLocationIsStepTarget(
-          startLocation,
-          frame.script,
-          offset
-        )
-      ) {
         if (!result.includes(offset)) {
           result.push(offset);
         }
@@ -1073,12 +1021,13 @@ const ThreadActor = ActorClassWithSpec(threadSpec, {
   /**
    * Define the JS hook functions for stepping.
    */
-  _makeSteppingHooks: function(
+  _makeSteppingHooks: function({ steppingType, rewinding, completion }) {
-    startLocation,
+    // We can't use the completion value in stepping hooks if we're
-    steppingType,
+    // replaying, as we can't use its contents after resuming.
-    rewinding,
+    if (this.dbg.replaying) {
-    completion
+      completion = null;
-  ) {
+    }
+
     // Bind these methods and state because some of the hooks are called
     // with 'this' set to the current frame. Rather than repeating the
     // binding in each _makeOnX method, just do it once here and pass it
@@ -1087,9 +1036,8 @@ const ThreadActor = ActorClassWithSpec(threadSpec, {
       pauseAndRespond: (frame, onPacket = k => k) =>
         this._pauseAndRespond(frame, { type: "resumeLimit" }, onPacket),
       startFrame: this.youngestFrame,
-      startLocation: startLocation,
+      steppingType,
-      steppingType: steppingType,
+      rewinding,
-      rewinding: rewinding,
       completion,
     };
 
@@ -1130,12 +1078,10 @@ const ThreadActor = ActorClassWithSpec(threadSpec, {
       steppingType = "next";
     }
 
-    const location = this.sources.getFrameLocation(this.youngestFrame);
+    const { onEnterFrame, onPop, onStep } = this._makeSteppingHooks({
-    const { onEnterFrame, onPop, onStep } = this._makeSteppingHooks(
-      location,
       steppingType,
-      rewinding
+      rewinding,
-    );
+    });
 
     // Make sure there is still a frame on the stack if we are to continue
     // stepping.
@@ -1154,7 +1100,6 @@ const ThreadActor = ActorClassWithSpec(threadSpec, {
           if (stepFrame.script) {
             if (this.dbg.replaying) {
               const offsets = this._findReplayingStepOffsets(
-                location,
                 stepFrame,
                 rewinding
               );
@@ -1176,11 +1121,7 @@ const ThreadActor = ActorClassWithSpec(threadSpec, {
               // Set an onStep handler in the older frame to stop at the call site.
               // Make sure the offsets we use are valid breakpoint locations, as we
               // cannot stop at other offsets when replaying.
-              const offsets = this._findReplayingStepOffsets(
+              const offsets = this._findReplayingStepOffsets(olderFrame, true);
-                {},
-                olderFrame,
-                true
-              );
               olderFrame.setReplayingOnStep(onStep, offsets);
             }
           } else {
@@ -1663,7 +1604,7 @@ const ThreadActor = ActorClassWithSpec(threadSpec, {
   createProtocolCompletionValue: function(completion) {
     const protoValue = {};
     if (completion == null) {
-      protoValue.terminated = true;
+      return protoValue;
     } else if ("return" in completion) {
       protoValue.return = createValueGrip(
         completion.return,
@@ -1883,7 +1824,7 @@ const ThreadActor = ActorClassWithSpec(threadSpec, {
     // 2. breakpoints are disabled
     // 3. the source is blackboxed
     if (
-      !this.hasMoved(location, "debuggerStatement") ||
+      !this.hasMoved(frame, "debuggerStatement") ||
       this.skipBreakpoints ||
       this.sources.isBlackBoxed(url)
     ) {
@@ -2194,11 +2135,6 @@ this.reportError = function(error, prefix = "") {
   dumpn(msg);
 };
 
-function findPausePointForLocation(pausePoints, location) {
-  const { line: line, column: column } = location;
-  return pausePoints[line] && pausePoints[line][column];
-}
-
 /**
  * Unwrap a global that is wrapped in a |Debugger.Object|, or if the global has
  * become a dead object, return |undefined|.

devtools/server/tests/unit/test_stepping-01.js

@@ -32,7 +32,6 @@ function getPauseLocation(packet) {
 }
 
 function getPauseReturn(packet) {
-  dump(`>> getPauseReturn yo ${JSON.stringify(packet.why)}\n`);
   return packet.why.frameFinished.return;
 }
 
@@ -50,9 +49,7 @@ async function stepOutOfA(dbg, func, expectedLocation) {
   const { threadFront } = dbg;
   await steps(threadFront, [stepOver, stepIn]);
 
-  dump(`>>> oof\n`);
   const packet = await stepOut(threadFront);
-  dump(`>>> foo\n`);
 
   deepEqual(
     getPauseLocation(packet),
@@ -69,7 +66,6 @@ async function stepOverInA(dbg, func, expectedLocation) {
   await steps(threadFront, [stepOver, stepIn]);
 
   let packet = await stepOver(threadFront);
-  dump(`>> stepOverInA hi\n`);
   equal(getPauseReturn(packet).ownPropertyLength, 1, "a() is returning obj");
 
   packet = await stepOver(threadFront);
@@ -81,18 +77,18 @@ async function stepOverInA(dbg, func, expectedLocation) {
   await dbg.threadFront.resume();
 }
 
-async function testStep(dbg, func, expectedLocation) {
+async function testStep(dbg, func, expectedValue) {
-  await stepOverInA(dbg, func, expectedLocation);
+  await stepOverInA(dbg, func, expectedValue);
-  await stepOutOfA(dbg, func, expectedLocation);
+  await stepOutOfA(dbg, func, expectedValue);
 }
 
 function run_test() {
   return (async function() {
     const dbg = await setupTestFromUrl("stepping.js");
 
-    await testStep(dbg, "arithmetic", { line: 17, column: 0 });
+    await testStep(dbg, "arithmetic", { line: 16, column: 8 });
-    await testStep(dbg, "composition", { line: 22, column: 0 });
+    await testStep(dbg, "composition", { line: 21, column: 3 });
-    await testStep(dbg, "chaining", { line: 27, column: 0 });
+    await testStep(dbg, "chaining", { line: 26, column: 6 });
 
     await testFinish(dbg);
   })();

devtools/server/tests/unit/test_stepping-09.js

@@ -25,6 +25,7 @@ add_task(
     const step2 = await stepOut(threadFront);
     // The bug was that we'd step right past the end of the function and never pause.
     equal(step2.frame.where.line, 2);
+    equal(step2.frame.where.column, 31);
     deepEqual(step2.why.frameFinished.return, { type: "undefined" });
   })
 );

devtools/server/tests/unit/test_stepping-11.js

@@ -22,8 +22,7 @@ add_task(
     );
 
     await waitForEvent(threadFront, "paused");
-    await threadFront.stepOver();
+    const packet = await stepOver(threadFront);
-    const packet = await waitForEvent(threadFront, "paused");
     Assert.equal(packet.frame.where.line, 3, "step to line 3");
     await threadFront.resume();
   })

devtools/server/tests/unit/test_stepping-13.js

@@ -0,0 +1,40 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+/*
+ * Check that is possible to step into both the inner and outer function
+ * calls.
+ */
+
+add_task(
+  threadFrontTest(async ({ threadFront, targetFront, debuggee }) => {
+    dumpn("Evaluating test code and waiting for first debugger statement");
+
+    const consoleFront = await targetFront.getFront("console");
+    consoleFront.evaluateJSAsync(
+      `(function () {
+        const a = () => { return 2 };
+        debugger;
+        a(a())
+      })()`
+    );
+
+    await waitForEvent(threadFront, "paused");
+    const step1 = await stepOver(threadFront);
+    Assert.equal(step1.frame.where.line, 4, "step to line 4");
+
+    const step2 = await stepIn(threadFront);
+    Assert.equal(step2.frame.where.line, 2, "step in to line 2");
+
+    const step3 = await stepOut(threadFront);
+    Assert.equal(step3.frame.where.line, 4, "step back to line 4");
+    Assert.equal(step3.frame.where.column, 9, "step out to column 9");
+
+    const step4 = await stepIn(threadFront);
+    Assert.equal(step4.frame.where.line, 2, "step in to line 2");
+
+    await threadFront.resume();
+  })
+);

devtools/server/tests/unit/xpcshell.ini

@@ -193,6 +193,7 @@ skip-if = true # breakpoint sliding is not supported bug 1525685
 [test_stepping-10.js]
 [test_stepping-11.js]
 [test_stepping-12.js]
+[test_stepping-13.js]
 [test_stepping-with-skip-breakpoints.js]
 [test_framebindings-01.js]
 [test_framebindings-02.js]