Bug 970469 - ignore breakpoints on the current line when stepping out; r=ystartsev+600802
authorTom Tromey <tom@tromey.com>
Thu, 31 Aug 2017 14:21:57 -0600
changeset 428395 41a141a23b5fca85d418de493640c50a34f65e0e
parent 428394 250d2a032e5bc77af8403f1a0b80bf2ec26e0ae1
child 428396 742f0335e8583e7c277218a5dfb7f41d9d9bd9f7
push id7761
push userjlund@mozilla.com
push dateFri, 15 Sep 2017 00:19:52 +0000
treeherdermozilla-beta@c38455951db4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersystartsev
bugs970469, 600802
milestone57.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 970469 - ignore breakpoints on the current line when stepping out; r=ystartsev+600802 MozReview-Commit-ID: KTzygRac2D7
devtools/server/actors/breakpoint.js
devtools/server/actors/script.js
devtools/server/tests/unit/head_dbg.js
devtools/server/tests/unit/test_stepping-08.js
devtools/server/tests/unit/xpcshell.ini
--- a/devtools/server/actors/breakpoint.js
+++ b/devtools/server/actors/breakpoint.js
@@ -135,25 +135,38 @@ let BreakpointActor = ActorClassWithSpec
    *
    * @param frame Debugger.Frame
    *        The stack frame that contained the breakpoint.
    */
   hit: function (frame) {
     // Don't pause if we are currently stepping (in or over) or the frame is
     // black-boxed.
     let generatedLocation = this.threadActor.sources.getFrameLocation(frame);
-    let { originalSourceActor } = this.threadActor.unsafeSynchronize(
+    let {
+      originalSourceActor,
+      originalLine,
+      originalColumn
+    } = this.threadActor.unsafeSynchronize(
       this.threadActor.sources.getOriginalLocation(generatedLocation));
     let url = originalSourceActor.url;
 
     if (this.threadActor.sources.isBlackBoxed(url)
         || frame.onStep) {
       return undefined;
     }
 
+    // If we're trying to pop this frame, and we see a breakpoint at
+    // the spot at which popping started, ignore it.  See bug 970469.
+    const locationAtFinish = frame.onPop && frame.onPop.originalLocation;
+    if (locationAtFinish &&
+        locationAtFinish.originalLine === originalLine &&
+        locationAtFinish.originalColumn === originalColumn) {
+      return undefined;
+    }
+
     let reason = {};
 
     if (this.threadActor._hiddenBreakpoints.has(this.actorID)) {
       reason.type = "pauseOnDOMEvents";
     } else if (!this.condition) {
       reason.type = "breakpoint";
       // TODO: add the rest of the breakpoints on that line (bug 676602).
       reason.actors = [ this.actorID ];
--- a/devtools/server/actors/script.js
+++ b/devtools/server/actors/script.js
@@ -770,19 +770,19 @@ const ThreadActor = ActorClassWithSpec(t
       let url = originalSourceActor.url;
 
       return this.sources.isBlackBoxed(url)
         ? undefined
         : pauseAndRespond(frame);
     };
   },
 
-  _makeOnPop: function (
-    { thread, pauseAndRespond, createValueGrip: createValueGripHook }) {
-    return function (completion) {
+  _makeOnPop: function ({ thread, pauseAndRespond, createValueGrip: createValueGripHook,
+                          startLocation }) {
+    const result = function (completion) {
       // onPop is called with 'this' set to the current frame.
 
       const generatedLocation = thread.sources.getFrameLocation(this);
       const { originalSourceActor } = thread.unsafeSynchronize(
         thread.sources.getOriginalLocation(generatedLocation));
       const url = originalSourceActor.url;
 
       if (thread.sources.isBlackBoxed(url)) {
@@ -802,16 +802,27 @@ const ThreadActor = ActorClassWithSpec(t
         } else if (completion.hasOwnProperty("yield")) {
           packet.why.frameFinished.return = createValueGripHook(completion.yield);
         } else {
           packet.why.frameFinished.throw = createValueGripHook(completion.throw);
         }
         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 original 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.originalLocation = startLocation;
+
+    return result;
   },
 
   _makeOnStep: function ({ thread, pauseAndRespond, startFrame,
                            startLocation, steppingType }) {
     // Breaking in place: we should always pause.
     if (steppingType === "break") {
       return function () {
         return pauseAndRespond(this);
--- a/devtools/server/tests/unit/head_dbg.js
+++ b/devtools/server/tests/unit/head_dbg.js
@@ -762,16 +762,30 @@ function stepIn(client, threadClient) {
  */
 function stepOver(client, threadClient) {
   dumpn("Stepping over.");
   return threadClient.stepOver()
     .then(() => waitForPause(client));
 }
 
 /**
+ * Resume JS execution for a step out and wait for the pause after the step
+ * has been taken.
+ *
+ * @param DebuggerClient client
+ * @param ThreadClient threadClient
+ * @returns Promise
+ */
+function stepOut(client, threadClient) {
+  dumpn("Stepping out.");
+  return threadClient.stepOut()
+    .then(() => waitForPause(client));
+}
+
+/**
  * Get the list of `count` frames currently on stack, starting at the index
  * `first` for the specified thread.
  *
  * @param ThreadClient threadClient
  * @param Number first
  * @param Number count
  * @returns Promise
  */
new file mode 100644
--- /dev/null
+++ b/devtools/server/tests/unit/test_stepping-08.js
@@ -0,0 +1,75 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+/**
+ * Check that step out doesn't double stop on a breakpoint.  Bug 970469.
+ */
+
+var gDebuggee;
+var gClient;
+var gCallback;
+
+function run_test() {
+  do_test_pending();
+  run_test_with_server(DebuggerServer, function () {
+    run_test_with_server(WorkerDebuggerServer, do_test_finished);
+  });
+}
+
+function run_test_with_server(server, callback) {
+  gCallback = callback;
+  initTestDebuggerServer(server);
+  gDebuggee = addTestGlobal("test-stepping", server);
+  gClient = new DebuggerClient(server.connectPipe());
+  gClient.connect(testStepOutWithBreakpoint);
+}
+
+async function testStepOutWithBreakpoint() {
+  const [attachResponse,, threadClient] = await attachTestTabAndResume(gClient,
+                                                                       "test-stepping");
+  ok(!attachResponse.error, "Should not get an error attaching");
+
+  dumpn("Evaluating test code and waiting for first debugger statement");
+  const dbgStmt = await executeOnNextTickAndWaitForPause(evaluateTestCode, gClient);
+  equal(dbgStmt.frame.where.line, 3, "Should be at debugger statement on line 3");
+
+  dumpn("Setting breakpoint in innerFunction");
+  const source = threadClient.source(dbgStmt.frame.where.source);
+  await source.setBreakpoint({ line: 7 });
+
+  dumpn("Step in to innerFunction");
+  const step1 = await stepIn(gClient, threadClient);
+  equal(step1.frame.where.line, 7);
+
+  dumpn("Step out of innerFunction");
+  const step2 = await stepOut(gClient, threadClient);
+  // The bug was that we'd stop again at the breakpoint on line 7.
+  equal(step2.frame.where.line, 10);
+
+  finishClient(gClient, gCallback);
+}
+
+function evaluateTestCode() {
+  /* eslint-disable */
+  Cu.evalInSandbox(
+    `                                   //  1
+    function outerFunction() {          //  2
+      debugger; innerFunction();        //  3
+    }                                   //  4
+                                        //  5
+    function innerFunction() {          //  6
+      let x = 0;                        //  7
+      let y = 72;                       //  8
+      return x+y;                       //  9
+    }                                   // 10
+    outerFunction();                    // 11
+    `,                                  // 12
+    gDebuggee,
+    "1.8",
+    "test_stepping-08-test-code.js",
+    1
+  );
+  /* eslint-enable */
+}
--- a/devtools/server/tests/unit/xpcshell.ini
+++ b/devtools/server/tests/unit/xpcshell.ini
@@ -180,16 +180,17 @@ reason = only ran on B2G
 [test_interrupt.js]
 [test_stepping-01.js]
 [test_stepping-02.js]
 [test_stepping-03.js]
 [test_stepping-04.js]
 [test_stepping-05.js]
 [test_stepping-06.js]
 [test_stepping-07.js]
+[test_stepping-08.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]
 [test_pause_exceptions-01.js]