1362416 - cannot set breakpoint in for loop without var declaration

Status: VERIFIED FIXED

(DevTools :: Debugger, defect, P2)

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: 20170418123106

Steps to reproduce:

1. Open attached example.html in Firefox 53.0 or Firefox Developer Edition 54.0a2 (both affected, new and old debugger!).
2. Open Developer Tools, go to Debugger.
3. Reload once to enable breakpoint sliding.
3. Click on gutter to set breakpoints in line 13 and 21.


Actual results:

- Breakpoint in line 13 is removed and one in line 16 added instead (due to sliding).
- Breakpoint in line 21 is removed and one in line 24 added instead (due to sliding).


Expected results:

- Breakpoint in line 13 should stay and not slide somewhere else.
- Breakpoint in line 21 should stay and not slide somewhere else.
- Note: Chrome 58.0 allows setting breakpoints in lines 13 and 21.
- Note: In both cases (line 13 in a for...in loop, line 21 in a for...of loop) the loop header does not contain a declaration with "var" keyword. In the loops with a "var" declaration, setting breakpoints works (lines 9 and 17).

Comment 1

[Daniel Lehmann]

Attachment: Video capture of bug in Firefox Developer Edition 54.0a2

Comment 2

[Jason Laster [:jlast]]

Thanks for filing. This is likely a server related issue so i'll leave it open here as opposed to moving it to our github repo for the new ui github.com/devtools-html/debugger.html/

Comment 3

[Jim Blandy :jimb]

That certainly looks like a problem at a low level, perhaps even in the Debugger API itself.

Comment 4

[Tom Tromey :tromey]

The patch in bug #1362403 fixes case 4, but not case 2.

Comment 5

[Tom Tromey :tromey]

In this bug, the line table seems ok, but stepping through FlowGraphSummary gives something weird:

(gdb) p lineno
$21 = 18446744073709551615

... while it is examining the loophead op.

I think the issue is that FlowGraphSummary::populate assumes that a branch target is always visited
before the branch that leads there -- but this is not true.

I'm sure this bug arose once before but I can't find it now.

Comment 6

[Tom Tromey :tromey]

I have a mildly ugly patch for this, based on top of my patch for bug 1362403.

Comment 7

[Tom Tromey :tromey]

Attachment: Bug 1362416 - fix FlowGraphSummary to handle branch target before the branch;

FlowGraphSummary walks the bytecode linearly, assuming that a branch
instruction will always be visited before the branch's target.  However,
this is not the case for JSOP_LOOPHEAD, leading to an incorrect line
number (-1).  This patch changes it to instead reuse the location of the
previous opcode, which is correct in the case of a loop head.

Review commit: https://reviewboard.mozilla.org/r/136648/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/136648/

Comment 8

[Jim Blandy :jimb]

Comment on attachment 8864993 [details]
Bug 1362416 - fix FlowGraphSummary to handle branch target before the branch;

https://reviewboard.mozilla.org/r/136648/#review139762

Looks good to me.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b2bec2797b6d
fix FlowGraphSummary to handle branch target before the branch; r=jimb

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b2bec2797b6d

Comment 11

[Md.Tarikul Islam Oashi]

I have reproduced this bug with nightly 55.0a1 (2017-05-05) (64-bit) on "Linux Mint 18.1 Serena"(64 Bit).

The bug's fix is now verified on Latest nightly 55.0a1.

Build ID 	20170530100155
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:55.0) Gecko/20100101 Firefox/55.0

[Bugday-20170531]

Comment 12

[Md. Rahimul Islam]

I have successfully reproduced the bug in firefox Nightly 55.0a1 (2017-05-05) (32-bit) with windows 10 (32 bit) 

Verified as fixed with latest nightly 55.0a1 (2017-05-31) (32-bit)

Build ID:   20170531030204
Mozilla/5.0 (Windows NT 10.0; rv:55.0) Gecko/20100101 Firefox/55.0

Comment 13

[Md. Rahimul Islam]

 As per Comment 11 & Comment 12, I am marking this bug as verified fixed.