In the "Breaking twice at same line" dev-developer-tools thread, Honza said: > I am experiencing a case where JS execution is breaking twice at the same line. > Here are my STRs: > > 1) Load: https://getfirebug.com/tests/head/script/stepping/1179/issue1179.html > 2) Open debugger and create breakpoints on line 14 and 21 > 3) Click the "Break" button on the page > 4) Execution stops at line 14 > 5) Step into, execution breaks at line 21 > 6) Step out, execution breaks at line 21 again -> BUG

Copying my reply from the list: When you click "step out" that translates into a resume packet with the "finish" resume limit in the RDP. When handling that resume limit, we only add an onPop hook, not an onStep or onEnterFrame hook[0]. We avoid breakpoints when stepping via checking if the onStep hook is present on the frame[1], but this obviously doesn't work for our case because we didn't set the onStep hook. Moreover, we *would* want to hit the breakpoint in most cases, just not when we are already on that line. We could try and remember what line we are stepping out from and add that check alongside the existing ones. Another option would be to look at which bytecodes we are setting breakpoints on in this case and see if there is a way to avoid breaking on unnecessary bytecodes so that we wouldn't even have the breakpoint's hit hook called for whatever bytecode we are hitting on step out. [0] http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/script.js#970 [1] http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/script.js#4132

The first option sounds reasonable to me, but I don't think I understand the second one. What would be the criteria to identify bytecodes that we should break at versus those where we should just continue? If I understand correctly, this would require changing the Debugger API and behavior, but that's a separate issue.

Yeah, I think the second idea was pretty half baked.

Yulia and I debugged this a bit. At first we thought that the problem could be solved by single-stepping one instruction in the onEnterFrame function -- the idea being to already be stopped at the first instruction. However, breakpoints are apparently notified after steps, so this didn't work. So, in the end, I'm going to do what Nick suggested in comment #1.

We had been experimenting with doing stuff in _makeOnEnterFrame, but really it has to be in _makeOnPop.

Comment on attachment 8903303 [details] Bug 970469 - ignore breakpoints on the current line when stepping out; https://reviewboard.mozilla.org/r/175094/#review180532 Looks great overall! a few minor nits regarding variable declaration and equality operators. ::: devtools/server/actors/breakpoint.js:155 (Diff revision 2) > > if (this.threadActor.sources.isBlackBoxed(url) > - || frame.onStep) { > + || frame.onStep > + // 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. > + || (frame.onPop && frame.onPop.originalLocation && maybe we can save this into a variable ie `isPoppedFrameOnBreakpoint` or similar ::: devtools/server/actors/breakpoint.js:156 (Diff revision 2) > if (this.threadActor.sources.isBlackBoxed(url) > - || frame.onStep) { > + || frame.onStep > + // 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. > + || (frame.onPop && frame.onPop.originalLocation && > + frame.onPop.originalLocation.originalLine == originalLine && here we want to do `===` rather than `==` because when `frame.onPop.originalLocation.originalColumn = 0` `frame.onPop.originalLocation.originalColumn == false // true` and 0 is a valid number -- It might be that this never happens but we don't have type checking here so this might be safer ::: devtools/server/actors/script.js:780 (Diff revision 2) > }; > }, > > - _makeOnPop: function ( > - { thread, pauseAndRespond, createValueGrip: createValueGripHook }) { > - return function (completion) { > + _makeOnPop: function ({ thread, pauseAndRespond, createValueGrip: createValueGripHook, > + startLocation }) { > + let result = function (completion) { this can be const ::: devtools/server/tests/unit/test_stepping-08.js:10 (Diff revision 2) > + > +/** > + * Check that step out doesn't double stop on a breakpoint. Bug 970469. > + */ > + > +var gDebuggee; nit: these should be `let` rather than `var` ::: devtools/server/tests/unit/test_stepping-08.js:39 (Diff revision 2) > + 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"); > + let source = threadClient.source(dbgStmt.frame.where.source); nit: the `let` for line 39, 43, and 47 should be `const`

Comment on attachment 8903303 [details] Bug 970469 - ignore breakpoints on the current line when stepping out; Weirdness with the review line.

Comment on attachment 8903303 [details] Bug 970469 - ignore breakpoints on the current line when stepping out; https://reviewboard.mozilla.org/r/175094/#review181072 :+1: looks good!

Comment on attachment 8903303 [details] Bug 970469 - ignore breakpoints on the current line when stepping out; Not sure why there were two review requests, clearing one.

Pushed by ttromey@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/41a141a23b5f ignore breakpoints on the current line when stepping out; r=ystartsev+600802