If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Stepping out at a breakpoint pauses on the same line twice

RESOLVED FIXED in Firefox 57

Status

()

Firefox
Developer Tools: Debugger
P3
normal
RESOLVED FIXED
4 years ago
19 days ago

People

(Reporter: fitzgen, Assigned: tromey)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 57
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

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.
Priority: -- → P3

Updated

3 years ago
Summary: step out at breakpoint breaks on same breakpoint → Stepping out at a breakpoint pauses on the same line twice

Updated

3 years ago
Blocks: 1074182
(Assignee)

Comment 4

24 days ago
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.
Assignee: nobody → ttromey
(Assignee)

Comment 5

24 days ago
We had been experimenting with doing stuff in _makeOnEnterFrame, but really it has
to be in _makeOnPop.
Comment hidden (mozreview-request)
(Assignee)

Updated

24 days ago
Attachment #8903303 - Flags: review?(ystartsev)
Comment hidden (mozreview-request)

Comment 8

23 days ago
mozreview-review
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`
Attachment #8903303 - Flags: review+
(Assignee)

Comment 9

23 days ago
Comment on attachment 8903303 [details]
Bug 970469 - ignore breakpoints on the current line when stepping out;

Weirdness with the review line.
Attachment #8903303 - Flags: review?(yulia.startsev)
Comment hidden (mozreview-request)

Comment 11

20 days ago
mozreview-review
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!
Attachment #8903303 - Flags: review+
(Assignee)

Comment 12

19 days ago
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.
Attachment #8903303 - Flags: review?(yulia.startsev)

Comment 13

19 days ago
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
https://hg.mozilla.org/mozilla-central/rev/41a141a23b5f
Status: NEW → RESOLVED
Last Resolved: 19 days ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
You need to log in before you can comment on or make changes to this bug.