Bug 1608078 - Stepping out of async function doesnt work properly. r=loganfsmyth
authorJason Laster <jlaster@mozilla.com>
Wed, 20 May 2020 22:22:31 +0000
changeset 531336 983b9b9624c0cdb86394a7aaeebd0035175b83c4
parent 531335 84c51f3b6e8601e96def8f3cdb229ffe55013110
child 531337 5ed4c347cc66266d13ce2289ed55df9101affa70
push id37438
push userabutkovits@mozilla.com
push dateThu, 21 May 2020 09:36:57 +0000
treeherdermozilla-central@2d00a1a6495c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersloganfsmyth
bugs1608078
milestone78.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1608078 - Stepping out of async function doesnt work properly. r=loganfsmyth Differential Revision: https://phabricator.services.mozilla.com/D75235
devtools/server/actors/thread.js
devtools/server/tests/xpcshell/stepping-async.js
devtools/server/tests/xpcshell/test_stepping-12.js
devtools/server/tests/xpcshell/test_stepping-19.js
devtools/server/tests/xpcshell/xpcshell.ini
--- a/devtools/server/actors/thread.js
+++ b/devtools/server/actors/thread.js
@@ -79,16 +79,74 @@ loader.lazyRequireGetter(
 );
 loader.lazyRequireGetter(
   this,
   "PausedDebuggerOverlay",
   "devtools/server/actors/highlighters/paused-debugger",
   true
 );
 
+const PROMISE_REACTIONS = new WeakMap();
+function cacheReactionsForFrame(frame) {
+  if (frame.asyncPromise) {
+    const reactions = frame.asyncPromise.getPromiseReactions();
+    const existingReactions = PROMISE_REACTIONS.get(frame.asyncPromise);
+    if (
+      reactions.length > 0 &&
+      (!existingReactions || reactions.length > existingReactions.length)
+    ) {
+      PROMISE_REACTIONS.set(frame.asyncPromise, reactions);
+    }
+  }
+}
+
+function createStepForReactionTracking(onStep) {
+  return function() {
+    cacheReactionsForFrame(this);
+    return onStep ? onStep.apply(this, arguments) : undefined;
+  };
+}
+
+const getAsyncParentFrame = frame => {
+  if (!frame.asyncPromise) {
+    return null;
+  }
+
+  // We support returning Frame actors for frames that are suspended
+  // at an 'await', and here we want to walk upward to look for the first
+  // frame that will be resumed when the current frame's promise resolves.
+  let reactions =
+    PROMISE_REACTIONS.get(frame.asyncPromise) ||
+    frame.asyncPromise.getPromiseReactions();
+
+  while (true) {
+    // We loop here because we may have code like:
+    //
+    //   async function inner(){ debugger; }
+    //
+    //   async function outer() {
+    //     await Promise.resolve().then(() => inner());
+    //   }
+    //
+    // where we can see that when `inner` resolves, we will resume from
+    // `outer`, even though there is a layer of promises between, and
+    // that layer could be any number of promises deep.
+    if (!(reactions[0] instanceof Debugger.Object)) {
+      break;
+    }
+
+    reactions = reactions[0].getPromiseReactions();
+  }
+
+  if (reactions[0] instanceof Debugger.Frame) {
+    return reactions[0];
+  }
+  return null;
+};
+
 /**
  * JSD2 actors.
  */
 
 /**
  * Creates a ThreadActor.
  *
  * ThreadActors manage a JSInspector object and manage execution/inspection
@@ -802,22 +860,16 @@ const ThreadActor = ActorClassWithSpec(t
       }
 
       const { sourceActor, line, column } = this.sources.getFrameLocation(
         frame
       );
 
       packet.why = reason;
 
-      if (this.suspendedFrame) {
-        this.suspendedFrame.onStep = undefined;
-        this.suspendedFrame.onPop = undefined;
-        this.suspendedFrame = undefined;
-      }
-
       if (!sourceActor) {
         // If the frame location is in a source that not pass the 'allowSource'
         // check and thus has no actor, we do not bother pausing.
         return undefined;
       }
 
       packet.frame.where = {
         actor: sourceActor.actorID,
@@ -866,26 +918,30 @@ const ThreadActor = ActorClassWithSpec(t
       return undefined;
     };
   },
 
   _makeOnPop: function({ pauseAndRespond, steppingType }) {
     const thread = this;
     return function(completion) {
       // onPop is called when we temporarily leave an async/generator
-      if (completion.await || completion.yield) {
+      if (steppingType != "finish" && (completion.await || completion.yield)) {
         thread.suspendedFrame = this;
         thread.dbg.onEnterFrame = undefined;
         return undefined;
       }
 
       // Note that we're popping this frame; we need to watch for
       // subsequent step events on its caller.
       this.reportedPop = true;
 
+      // Cache the frame so that the onPop and onStep hooks are cleared
+      // on the next pause.
+      thread.suspendedFrame = this;
+
       if (steppingType != "finish" && !thread.sources.isFrameBlackBoxed(this)) {
         return pauseAndRespond(this, packet =>
           thread.createCompletionGrip(packet, completion)
         );
       }
 
       thread._attachSteppingHooks(this, "next", completion);
       return undefined;
@@ -1061,28 +1117,35 @@ const ThreadActor = ActorClassWithSpec(t
         case "next":
           if (stepFrame.script) {
             if (!this.sources.isFrameBlackBoxed(stepFrame)) {
               stepFrame.onStep = onStep;
             }
           }
         // Fall through.
         case "finish":
+          stepFrame.onStep = createStepForReactionTracking(stepFrame.onStep);
           stepFrame.onPop = onPop;
           break;
       }
     }
 
     return true;
   },
 
   /**
    * Clear the onStep and onPop hooks for all frames on the stack.
    */
   _clearSteppingHooks: function() {
+    if (this.suspendedFrame) {
+      this.suspendedFrame.onStep = undefined;
+      this.suspendedFrame.onPop = undefined;
+      this.suspendedFrame = undefined;
+    }
+
     let frame = this.youngestFrame;
     if (frame?.onStack) {
       while (frame) {
         frame.onStep = undefined;
         frame.onPop = undefined;
         frame = frame.older;
       }
     }
@@ -1209,17 +1272,19 @@ const ThreadActor = ActorClassWithSpec(t
     }
   },
 
   /**
    * Helper method that returns the next frame when stepping.
    */
   _getNextStepFrame: function(frame) {
     const endOfFrame = frame.reportedPop;
-    const stepFrame = endOfFrame ? frame.older : frame;
+    const stepFrame = endOfFrame
+      ? frame.older || getAsyncParentFrame(frame)
+      : frame;
     if (!stepFrame || !stepFrame.script) {
       return null;
     }
     return stepFrame;
   },
 
   frames: function(start, count) {
     if (this.state !== "paused") {
@@ -1252,41 +1317,19 @@ const ThreadActor = ActorClassWithSpec(t
         frame = currentFrame.olderSavedFrame;
         if (frame && !isValidSavedFrame(this, frame)) {
           frame = null;
         }
       } else if (
         this._options.shouldIncludeAsyncLiveFrames &&
         currentFrame.asyncPromise
       ) {
-        // We support returning Frame actors for frames that are suspended
-        // at an 'await', and here we want to walk upward to look for the first
-        // frame that will be resumed when the current frame's promise resolves.
-        let reactions = currentFrame.asyncPromise.getPromiseReactions();
-        while (true) {
-          // We loop here because we may have code like:
-          //
-          //   async function inner(){ debugger; }
-          //
-          //   async function outer() {
-          //     await Promise.resolve().then(() => inner());
-          //   }
-          //
-          // where we can see that when `inner` resolves, we will resume from
-          // `outer`, even though there is a layer of promises between, and
-          // that layer could be any number of promises deep.
-          if (!(reactions[0] instanceof Debugger.Object)) {
-            break;
-          }
-
-          reactions = reactions[0].getPromiseReactions();
-        }
-
-        if (reactions[0] instanceof Debugger.Frame) {
-          frame = reactions[0];
+        const asyncFrame = getAsyncParentFrame(currentFrame);
+        if (asyncFrame) {
+          frame = asyncFrame;
         }
       }
     };
 
     let i = 0;
     while (frame && i < start) {
       walkToParentFrame();
       i++;
new file mode 100644
--- /dev/null
+++ b/devtools/server/tests/xpcshell/stepping-async.js
@@ -0,0 +1,31 @@
+"use strict";
+/* exported stuff */
+
+async function timer() {
+  return Promise.resolve();
+}
+
+function oops() {
+  return `oops`;
+}
+
+async function inner() {
+  oops();
+  await timer();
+  Promise.resolve().then(async () => {
+    Promise.resolve().then(() => {
+      oops();
+    });
+    oops();
+  });
+  oops();
+}
+
+async function stuff() {
+  debugger;
+  const task = inner();
+  oops();
+  await task;
+  oops();
+  debugger;
+}
--- a/devtools/server/tests/xpcshell/test_stepping-12.js
+++ b/devtools/server/tests/xpcshell/test_stepping-12.js
@@ -81,16 +81,17 @@ async function testInterleaving({ thread
 
   const step2 = await stepOver(threadFront);
   equal(step2.frame.where.line, 5);
   equal(step2.frame.where.column, 43);
 
   const step3 = await resumeAndWaitForPause(threadFront);
   equal(step3.frame.where.line, 9);
   equal(debuggee.result, "yay");
+
   resume(threadFront);
 }
 
 async function testMultipleSteps({ threadFront, debuggee }) {
   function evaluateTestCode() {
     Cu.evalInSandbox(
       `
         (async function simpleRace() {
new file mode 100644
--- /dev/null
+++ b/devtools/server/tests/xpcshell/test_stepping-19.js
@@ -0,0 +1,94 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+/* eslint-disable no-shadow, max-nested-callbacks */
+
+"use strict";
+
+/**
+ * Check that step out stops at the async parent's frame.
+ */
+
+async function testFinish({ threadFront, devToolsClient }) {
+  await close(devToolsClient);
+
+  do_test_finished();
+}
+
+async function invokeAndPause({ global, threadFront }, expression) {
+  return executeOnNextTickAndWaitForPause(
+    () => Cu.evalInSandbox(expression, global),
+    threadFront
+  );
+}
+
+async function steps(threadFront, sequence) {
+  const locations = [];
+  for (const cmd of sequence) {
+    const packet = await step(threadFront, cmd);
+    locations.push(getPauseLocation(packet));
+  }
+  return locations;
+}
+
+async function step(threadFront, cmd) {
+  return cmd(threadFront);
+}
+
+function getPauseLocation(packet) {
+  const { line, column } = packet.frame.where;
+  return { line, column };
+}
+
+async function stepOutBeforeTimer(dbg, func, frameIndex, expectedLocation) {
+  const { threadFront } = dbg;
+
+  await invokeAndPause(dbg, `${func}()`);
+  await steps(threadFront, [stepOver, stepIn]);
+
+  const { frames } = await threadFront.frames(0, 5);
+  const frameActorID = frames[frameIndex].actorID;
+  const packet = await stepOut(threadFront, frameActorID);
+
+  deepEqual(
+    getPauseLocation(packet),
+    expectedLocation,
+    `step out location in ${func}`
+  );
+
+  await resumeAndWaitForPause(threadFront);
+  await resume(threadFront);
+}
+
+async function stepOutAfterTimer(dbg, func, frameIndex, expectedLocation) {
+  const { threadFront } = dbg;
+
+  await invokeAndPause(dbg, `${func}()`);
+  await steps(threadFront, [stepOver, stepIn, stepOver, stepOver]);
+
+  const { frames } = await threadFront.frames(0, 5);
+  const frameActorID = frames[frameIndex].actorID;
+  const packet = await stepOut(threadFront, frameActorID);
+
+  deepEqual(
+    getPauseLocation(packet),
+    expectedLocation,
+    `step out location in ${func}`
+  );
+
+  await resumeAndWaitForPause(threadFront);
+  await dbg.threadFront.resume();
+}
+
+function run_test() {
+  return (async function() {
+    const dbg = await setupTestFromUrl("stepping-async.js");
+
+    info(`Test stepping out before timer;`);
+    await stepOutBeforeTimer(dbg, "stuff", 0, { line: 27, column: 2 });
+
+    info(`Test stepping out after timer;`);
+    await stepOutAfterTimer(dbg, "stuff", 0, { line: 29, column: 2 });
+
+    await testFinish(dbg);
+  })();
+}
--- a/devtools/server/tests/xpcshell/xpcshell.ini
+++ b/devtools/server/tests/xpcshell/xpcshell.ini
@@ -12,16 +12,17 @@ support-files =
   post_init_target_scoped_actors.js
   pre_init_global_actors.js
   pre_init_target_scoped_actors.js
   registertestactors-lazy.js
   sourcemapped.js
   testactors.js
   hello-actor.js
   stepping.js
+  stepping-async.js
   source-03.js
   setBreakpoint-on-column.js
   setBreakpoint-on-column-minified.js
   setBreakpoint-on-column-in-gcd-script.js
   setBreakpoint-on-column-with-no-offsets.js
   setBreakpoint-on-column-with-no-offsets-in-gcd-script.js
   setBreakpoint-on-line.js
   setBreakpoint-on-line-in-gcd-script.js
@@ -178,16 +179,17 @@ skip-if = true # breakpoint sliding is n
 [test_stepping-11.js]
 [test_stepping-12.js]
 [test_stepping-13.js]
 [test_stepping-14.js]
 [test_stepping-15.js]
 [test_stepping-16.js]
 [test_stepping-17.js]
 [test_stepping-18.js]
+[test_stepping-19.js]
 [test_stepping-with-skip-breakpoints.js]
 [test_framebindings-01.js]
 [test_framebindings-02.js]
 [test_framebindings-03.js]
 [test_framebindings-04.js]
 [test_framebindings-05.js]
 [test_framebindings-06.js]
 [test_framebindings-07.js]