[jsdbg2] Debugger hits while loop twice

VERIFIED FIXED in Firefox 45

Status

()

--
minor
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: sebo, Assigned: tromey)

Tracking

unspecified
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

4 years ago
Created attachment 8559648 [details]
Test case

The debugger stops twice at a breakpoint on a while loop when the script execution is continued.

STR:
1. Open the DevTools Debugger (Ctrl+Shift+S) on attached test case
2. Set a breakpoint at the while loop (line 3)
3. Reload the page
   => The debugger stops at line 3.
4. Continue the script execution (F8)

=> The debugger stops again at line 3, which should not happen.

Stepping after the first hit works as expected, though.

Sebastian

Updated

4 years ago
Flags: needinfo?(jimb)

Comment 1

4 years ago
I think this is probably a dupe of bug 1003554, where we're tackling a bunch of these issues.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: needinfo?(jimb)
Resolution: --- → DUPLICATE
Duplicate of bug: 1003554
(Reporter)

Comment 2

4 years ago
Hmm, I'm not sure this is really the same as it just happens when you continue the script execution. It does not occur when single-stepping through the code.
Though I'll check again as soon as bug 1003554 is fixed.

Sebastian

Comment 3

4 years ago
Here's why I think it's a dup:

When we set breakpoints on a line, we ask the JavaScript engine, among all bytecode operations attributed to that source line, which offsets are entry points to that line? Lines like the the 'for' head in 

   for (expr1; expr2; expr3) 
      body;

have entry points before both expr1 and expr3, so setting a source-level breakpoint there actually requires two breakpoints at the bytecode level.

Unfortunately, the bytecode compiler doesn't just tell us where these entry points are; we have to infer them from the bytecode-to-source mapping. And not only do we do that job indifferently, but also we're finding that the source locations themselves are not especially accurate.

All this discussion is happening on bug 1003554.
(Assignee)

Comment 4

4 years ago
I'm un-duping this since it isn't exactly like the problems being addressed in bug 1003554.

The deal here is that the entry point computation seems to make perfect sense;
it is just weird for users :(


Here's the disassembly:

loc     op
-----   --
main:
00000:  goto 6 (+6)
00005:  loophead
00006:  loopentry 129
00008:  false
00009:  ifne 5 (-4)
00014:  getgname "console"
00019:  dup
00020:  callprop "log"
00025:  swap
00026:  string "hi"
00031:  call 1
00034:  pop
00035:  retrval

Source notes:
 ofs line    pc  delta desc     args
---- ---- ----- ------ -------- ------
  0:    1     0 [   0] while    offset 9
  2:    1     5 [   5] newline 
  3:    2     5 [   0] colspan 4
  5:    2     6 [   1] setline  lineno 1
  7:    1     6 [   0] colspan 6
  9:    1    14 [   8] xdelta  
 10:    1    14 [   0] newline 
 11:    2    14 [   0] colspan 5
 13:    2    14 [   0] newline 
 14:    3    35 [  21] xdelta  
 15:    3    35 [   0] colspan 18


What happens here is that line 1 gets two entry points: PC=0, the loopentry
instruction; and PC=6, the start of the condition computation.

Maybe one fix would be to change EmitWhile to *not* give a line number to
that initial "goto".  Then only the condition computation would show up
as an entry point.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
(Assignee)

Comment 5

4 years ago
> Maybe one fix would be to change EmitWhile to *not* give a line number to
> that initial "goto".  Then only the condition computation would show up
> as an entry point.

There doesn't seem to be a clean way to do this.
The line note is emitted by EmitTree, not EmitWhile,
so this plan would involve a special case there.

I think bug also happens for:

  for (; false; )
     ;
(Assignee)

Comment 6

4 years ago
One more thing I realized today is that a fix to somehow ignore this
leading "goto" is going to run into:

https://bugzilla.mozilla.org/show_bug.cgi?id=1003554#c38
(Assignee)

Comment 7

4 years ago
Created attachment 8563650 [details] [diff] [review]
special-case "while" and "for" line notes

Here's what the resulting patch looks like.  This works for all the
cases I've tried, and passes the "debug" jit-tests.

I'm not as sure as I was yesterday that this approach is so very ugly.
Well, it is ugly, I just think alternatives will be equivalent.  My
reasoning is this:

The most principled way to deal with this bug and the associated bugs
would be to have the bytecode emitter generate a special table of
entry points.  However, I think this would effectively mean putting a
call to construct this table at all the spots that currently update
the line table.  And then, to deal with the "while" and "for" issue,
it would need a hack just like the hack in this patch.
(Assignee)

Updated

4 years ago
Depends on: 1134198
(Assignee)

Updated

4 years ago
Assignee: nobody → ttromey
(Assignee)

Updated

4 years ago
Depends on: 1003554
(Assignee)

Comment 9

4 years ago
Created attachment 8588046 [details] [diff] [review]
special-case "while" and "for" line notes

Rebased and updated for parser changes.
Attachment #8563650 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8588046 - Flags: review?(jimb)
(Assignee)

Comment 10

3 years ago
Created attachment 8668027 [details] [diff] [review]
special-case "while" and "for" line notes

Rebased.
(Assignee)

Updated

3 years ago
Attachment #8588046 - Attachment is obsolete: true
Attachment #8588046 - Flags: review?(jimb)
(Assignee)

Updated

3 years ago
Attachment #8668027 - Flags: review?(jimb)

Updated

3 years ago
Attachment #8668027 - Flags: review?(jimb) → review+
(Assignee)

Comment 11

3 years ago
Created attachment 8669047 [details] [diff] [review]
special-case "while" and "for" line notes

Move operator to end of line.
Attachment #8668027 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8669047 - Flags: review+
(Assignee)

Comment 12

3 years ago
Created attachment 8677554 [details] [diff] [review]
special-case "while" and "for" line notes

Rebased & updated.
Attachment #8669047 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8677554 - Flags: review+
(Assignee)

Comment 15

3 years ago
Created attachment 8684942 [details] [diff] [review]
special-case "while" and "for" line notes

Rebased.
(Assignee)

Updated

3 years ago
Attachment #8684942 - Flags: review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Attachment #8677554 - Attachment is obsolete: true

Comment 18

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/76ba596cf150
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
(Reporter)

Comment 19

3 years ago
Works for me, thank you!

Sebastian
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.