1370641 - Debugger pauses twice at single function call breakpoint

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Daniel Lehmann]

Attachment: example.html

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20170524174609

Steps to reproduce:

1. Open attached example.html in Firefox 53.0 or Firefox Developer Edition 54.0 beta12 (both affected, new and old debugger!).
2. Open Developer Tools, go to Debugger.
3. Set breakpoints in line 8 and line 11.
4. Reload to hit breakpoint in line 8.
5. Click "resume".
6. Next breakpoint in line 11 is hit, click "resume".
7. Click "resume" again...




Actual results:

- Breakpoint at line 8 is hit once (as expected :) )
- Breakpoint at line 11 is hit twice! That is, after hitting it once, one needs to click on "resume" again (step 7 above) to get past line 11.


Expected results:

- Breakpoint at line 11 should be hit only once. That is, when hitting the breakpoint, a single "resume" should get us past line 11.

Comment 1

[Daniel Lehmann]

Attachment: screencapture.ogv

Comment 2

[Tom Tromey :tromey]

I agree this is weird but I'm not sure what to do about it.

What is going on is that this code:

    someFun(     // set breakpoint here, resume when paused
      1
    );

generates:

00021:  getgname "someFun"              # someFun
00026:  gimplicitthis "someFun"         # someFun THIS
00031:  one                             # someFun THIS 1
00032:  call-ignores-rv 1               # someFun(...)

the relevant line table entries being:

  2:    1    21 [  21] xdelta  
  3:    1    21 [   0] setline  lineno 3
  5:    3    21 [   0] colspan 4
  7:    3    31 [  10] xdelta  
  8:    3    31 [   0] newline 
  9:    4    32 [   1] setline  lineno 3
 11:    3    32 [   0] colspan 4
 13:    3    36 [   4] setline  lineno 6


I always find the line table a bit hard to read but what I think this means is that
PC=21 is at line 3, PC=31 is at line 4 (just for that one instruction), and then
PC=32 is again at line 3.

So, if you step through the code, you will step to line 3, then line 4, and then back
to line 3.  This corresponds basically to "compute 'someFun'", "compute 1", and then
finally "call 'someFun(1)'".

If you set a breakpoint on line 3, then the breakpoint ends up with two locations: one
at PC=21 and one at PC=32.  So, you have to press continue twice.

This can't be solved by the heuristic of "if a stop doesn't change the line number, ignore it",
because that fails when putting a breakpoint on a loop head.

Maybe a more complicated heuristic would work, like: if the stop doesn't change the line number,
and this line has multiple entry points, and the last offset is different from this offset, then
don't stop.  But I'm having trouble convincing myself that this is correct in all cases.

Comment 3

[Tom Tromey :tromey]

See also bug 1145747, which proposes another approach, namely (in essence) not emitting line
notes for the argument lines.

Comment 4

[Daniel Lehmann]

Thanks for the great explaination and linking the other issue. Unfortunately, I am not really familiar with the internals of the execution, so some clarifying questions:

- As I understand it, the additional pause in the bug originally comes from the debugger first stepping at line 3 (line numbers from your comment, the "function name" PC=21), then line 4 (the argument), and then at line 3 again (the actual call PC=32).

- One solution, in Bug 1145747, would be to remove the step at line 4 by no longer emitting line notes at function call arguments, right?

- (see Bug 1145747:) However, you say that not stopping at the arguments is problematic because the arguments can themselves be complex expressions, i.e. function calls or getters (which are innocuous looking, but could execute a lot of code). Is that right?

- But even when we stop at arguments like function calls and getters, one cannot inspect their value anyway, do we? The argument values are only visible when entering the function. So what would be the purpose of pausing at the arguments, other than making the user aware there are some operations going on to get the argument?

- Also, why is there the first pause at line 3, i.e. why does the line table contain an entry for PC=21? Would the bug go away if this would not be emitted?

It seems your other idea to solve the problem, a more complicated heuristic, is very specific to this problem; I am not really sure it solves the underlying problems and its complexity is justified for this bug.

Comment 5

[Tom Tromey :tromey]

(In reply to Daniel Lehmann from comment #4)

> - (see Bug 1145747:) However, you say that not stopping at the arguments is
> problematic because the arguments can themselves be complex expressions,
> i.e. function calls or getters (which are innocuous looking, but could
> execute a lot of code). Is that right?

Yep.

> - But even when we stop at arguments like function calls and getters, one
> cannot inspect their value anyway, do we? The argument values are only
> visible when entering the function. So what would be the purpose of pausing
> at the arguments, other than making the user aware there are some operations
> going on to get the argument?

That's a good question, but I don't know the answer.
I know from reading the tests that it's intentional; but indeed it doesn't seem
actually useful. 

> - Also, why is there the first pause at line 3, i.e. why does the line table
> contain an entry for PC=21? Would the bug go away if this would not be
> emitted?

Yes, it would, but I don't know offhand if it would cause other problems.
Maybe it would cause profiling information to be allocated to the wrong line sometimes.
The note is being emitted there because this opcode is evaluating "someFun".

> It seems your other idea to solve the problem, a more complicated heuristic,
> is very specific to this problem; I am not really sure it solves the
> underlying problems and its complexity is justified for this bug.

Yeah.  Maybe this will be fixed a different way in the context of implementing
column breakpoints; the idea being to emit fewer line notes in some spots, seeing
that they aren't useful to this end (and that other additions will be needed as well).

Comment 6

[Jason Laster [:jlast]]

Hmm, my guess is this is fixed. Daniel, would you mind re-testing? By the way, i really like your strs.

Comment 7

[Logan Smyth [:loganfsmyth]]

Yeah was should be fixed by https://bugzilla.mozilla.org/show_bug.cgi?id=breakpoint-specificity since we no longer try to add breakpoints to every location on a line.

Comment 8

[Daniel Lehmann]

Hi Jason, Hi Logan,

Yep, can confirm: the second breakpont is only paused at once. So this is fixed in nightly.