Bug 970469 - ignore breakpoints on the current line when stepping out; r?yulia draft
authorTom Tromey <tom@tromey.com>
Thu, 31 Aug 2017 14:21:57 -0600
changeset 657686 5cdb0c72758564822dbf46178e145ddcfbf87519
parent 657552 bafae81b6c529db3fa3ac3dada0407d880f237f9
child 729493 21d2d12c2ecc2e5eb4675cadbd5414a2bf29a8d0
push id77594
push userbmo:ttromey@mozilla.com
push dateFri, 01 Sep 2017 20:27:35 +0000
reviewersyulia
bugs970469
milestone57.0a1
Bug 970469 - ignore breakpoints on the current line when stepping out; r?yulia 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]