Emit location source notes only at the beginning of each step

RESOLVED FIXED

Status

()

defect
P3
normal
RESOLVED FIXED
2 years ago
2 months ago

People

(Reporter: jlast, Unassigned)

Tracking

(Blocks 1 bug)

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 affected)

Details

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.
STR:
1. go to https://silly-stepping.glitch.me/
2. open the debugger
3. click "stuff"
Blocks: js-devtools
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                         #
Flags: needinfo?(jlaster)
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
Flags: needinfo?(jlaster)
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.
Summary: Step Over should support multiple statements on the same line → Emit location source notes only at the beginning of each step
Priority: -- → P3
Blocks: dbg-stepping

I believe this is now fixed.

Flags: needinfo?(lsmyth)
Blocks: dbg-control
No longer blocks: dbg-stepping
Status: NEW → RESOLVED
Closed: 5 months ago
Flags: needinfo?(lsmyth)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.