1434763 - Emit location source notes only at the beginning of each step

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Jason Laster [:jlast]]

Currently, the debugger will step one line at a time, even if there are multiple statements on the line.

For example these two lines will only be hit once

> debugger; debugger; debugger
> console.log("hi"); console.log("wait"); console.log("dont leave");

The reason for this is because the server onStep function waits until execution has advanced to a new line to pause because it cannot identify new valid pause locations on that line.

We can fix this by including column information in the source notes so that each new pause location can be identified by a new column offset. This will let the server know when it is appropriate to stop later on the same line.

Comment 1

[Jason Laster [:jlast]]

STR:
1. go to https://silly-stepping.glitch.me/
2. open the debugger
3. click "stuff"

Comment 2

[Jason Orendorff [:jorendorff]]

Jason, what do we need to change in the JS engine to support this?

Currently, the source notes contain the location information shown below. It's pretty good! If the Debugger object isn't exposing the right data, Jim or I can work on that; if you need more info than the starting column of the step, same deal.


function f() {  // 1
    debugger; debugger; debugger  // 2
    console.log("hi"); console.log("wait"); console.log("dont leave");  // 3
}


flags: LAMBDA NEEDS_CALLOBJECT CONSTRUCTOR
offset  line:col    op
------  --------    --
00000:  1:0         arguments                       # arguments
00001:  1:0         setaliasedvar "arguments" (hops = 0, slot = 2) # arguments
00006:  1:0         pop                             # 
00007:  1:0         functionthis                    # THIS
00008:  1:0         setaliasedvar ".this" (hops = 0, slot = 3) # THIS
00013:  1:0         pop                             # 
main:               
00014:  2:4         debugger                        # 
00015:  2:14        debugger                        # 
00016:  2:24        debugger                        # 
00017:  3:4         getgname "console"              # console
00022:  3:4         dup                             # console console
00023:  3:4         callprop "log"                  # console console.log
00028:  3:4         swap                            # console.log console
00029:  3:4         string "hi"                     # console.log console "hi"
00034:  3:4         call-ignores-rv 1               # console.log(...)
00037:  3:4         pop                             # 
00038:  3:23        getgname "console"              # console
00043:  3:23        dup                             # console console
00044:  3:23        callprop "log"                  # console console.log
00049:  3:23        swap                            # console.log console
00050:  3:23        string "wait"                   # console.log console "wait"
00055:  3:23        call-ignores-rv 1               # console.log(...)
00058:  3:23        pop                             # 
00059:  3:44        getgname "console"              # console
00064:  3:44        dup                             # console console
00065:  3:44        callprop "log"                  # console console.log
00070:  3:44        swap                            # console.log console
00071:  3:44        string "dont leave"             # console.log console "dont leave"
00076:  3:44        call-ignores-rv 1               # console.log(...)
00079:  3:44        pop                             # 
00080:  4:0         retrval                         #

Comment 3

[Jason Laster [:jlast]]

that does look good. So at the moment we use three parameters to determine if we should pause (frame, url, line). Column is likely to fine grained, but something like an operation type might help. for instance knowing it was a debugger op or call would be great. 

I can look into it more tomorrow, but that's what i'm thinking. It'd be good to think through the parameters and how the stepping behavior would change. 

https://searchfox.org/mozilla-central/source/devtools/server/actors/thread.js#514-517

Comment 4

[Jason Orendorff [:jorendorff]]

OK, so the problem here is that the engine's column information is usually too fine-grained. So even though in this case it's perfect, the debugger can't really stop every time we change columns because in many other cases it would be maddening.

jlast has many likely fixes up his sleeves, involving things like re-parsing the source outside of the JS engine and using that AST as extra information to consider every time an onStep event fires, to decide if it wants to pause.

The JS engine can help by emitting saner location information. Morphing bug.

Comment 5

[Jason Laster [:jlast]]

I believe this is now fixed.

Comment 6

[Logan Smyth [:loganfsmyth]]

Definitely addressed by https://bugzilla.mozilla.org/show_bug.cgi?id=breakpoint-specificity