785689 - Debugger breaking when it shouldn't

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Victor Porof [:vporof][:vp]]

STR
1. open http://htmlpad.org/debugger/
2. open debugger
3. click me!
4. keep stepping in until the script execution finishes
5. click me! again

Debugger breaks on line 26 even though there's no breakpoint or debugger statement there.

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

Actually this is as it should be. When you step in after the last statement in load(), you request the debugger to pause again before the next line of javascript executes. That is exactly what happens when you click the button, causing the load() handler to enter at line 26. The fact that after stepping out of load() execution returns to the event loop, which is native code, perhaps makes it seem like a plain resume, but it isn't.

Comment 2

[Victor Porof [:vporof][:vp]]

Ah, I get it. So this happens only after stepping in on the last executed line.
Doesn't happen in Chrome or Opera though.

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

I wonder whether it's an artifact of the way they implement stepping or a conscious product decision.

Comment 4

[Rob Campbell [:rc] (:robcee)]

I agree it's odd. Took me a second to figure out what you meant in Comment #1.

While it could be a conscious decision it occurs to me that we could use this behavior to implement "Pause on Next Execution" similar to what Firebug has.

Should we close this bug? Morph it to implement Break on Next?

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Rob Campbell [:rc] (:robcee) from comment #4)
> I agree it's odd. Took me a second to figure out what you meant in Comment
> #1.
> 
> While it could be a conscious decision it occurs to me that we could use
> this behavior to implement "Pause on Next Execution" similar to what Firebug
> has.
> 
> Should we close this bug? Morph it to implement Break on Next?

So Firebug's pause button actually steps, huh? I didn't know that. Sounds useful.

Comment 6

[Rob Campbell [:rc] (:robcee)]

it sets "Break on Next" mode. Which means everything's working, but will pause on the next JS execution. Totally useful, though somewhat hard to use in practice if you can't precisely engineer the state you want to break in. Typically, you'll end up pausing in some unrelated event which is why I think we need to be able to set specific types of events to pause on. (separate bug).

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

As I was looking into something else, it occurred to me that my explanation in comment 1 should hold for all kinds of stepping. When I tried to verify it though, it appears that only stepping in causes this behavior. After digging into it, I've come to realize that it's a bug, and I'll post a patch shortly.

I've filed bug 789430 for making the pause button behave like Firebug's "Break on Next", as discussed above.

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v1

The problem was that the onEnterFrame hook (the only one that is not set on a frame, but on the debugger instance) was set even in the case when no more stack frames were left. Step-in is the only consumer of onEnterFrame hooks, thus the behavior that Victor described.

Since Rob is away, I will have to summon my dream team for this review!

Comment 9

[Panos Astithas (he/him) [:past] (please ni?)]

Try: https://tbpl.mozilla.org/?tree=Try&rev=22d0840b7109

Comment 10

[Mihai Sucan [:msucan]]

Comment on attachment 659245 [details] [diff] [review]
Patch v1

Not an expert in this stuff, but it seems to fit well to what you guys discussed in the bug comments. Patch makes sense.

Comment 11

[Victor Porof [:vporof][:vp]]

Comment on attachment 659245 [details] [diff] [review]
Patch v1

Review of attachment 659245 [details] [diff] [review]:
-----------------------------------------------------------------

Yup, the code is obvious, can't give anything else but r+

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/integration/fx-team/rev/765fa82170e7

Comment 13

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/mozilla-central/rev/765fa82170e7