Bug 1608078 - Stepping out of async function doesnt work properly. r=loganfsmyth
☠☠ backed out by c3ac91c63eeb ☠ ☠
authorJason Laster <jlaster@mozilla.com>
Wed, 20 May 2020 17:53:37 +0000
changeset 531310 7050d098119c2dc261011291ffdf7a90318de3f0
parent 531309 6c337b4f49aae7b7d1344e36b6058a75728414c2
child 531311 25f50b4d346e7cc094440600dc6290237876a2f9
push id37437
push usernerli@mozilla.com
push dateThu, 21 May 2020 02:34:41 +0000
treeherdermozilla-central@3d91ba9e1d25 [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,32 +918,35 @@ 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;
 
       if (steppingType != "finish" && !thread.sources.isFrameBlackBoxed(this)) {
         return pauseAndRespond(this, packet =>
           thread.createCompletionGrip(packet, completion)
         );
       }
 
+      // Cache the frame so that the onPop and onStep hooks are cleared
+      // on the next pause.
+      thread.suspendedFrame = this;
       thread._attachSteppingHooks(this, "next", completion);
       return undefined;
     };
   },
 
   hasMoved: function(frame, newType) {
     const newLocation = this.sources.getFrameLocation(frame);
 
@@ -1061,28 +1116,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 +1271,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 +1316,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
@@ -79,18 +79,21 @@ async function testInterleaving({ thread
   const step1 = await stepOver(threadFront);
   equal(step1.frame.where.line, 4);
 
   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(step3.frame.where.line, 8);
   equal(debuggee.result, "yay");
+  const step4 = await resumeAndWaitForPause(threadFront);
+  equal(step4.frame.where.line, 9);
+
   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]