Closed
Bug 785689
Opened 12 years ago
Closed 12 years ago
Debugger breaking when it shouldn't
Categories
(DevTools :: Debugger, defect, P2)
DevTools
Debugger
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 18
People
(Reporter: vporof, Assigned: past)
Details
(Whiteboard: [fixed-in-fx-team])
Attachments
(1 file)
6.20 KB,
patch
|
vporof
:
review+
msucan
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•12 years ago
|
||
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.
Reporter | ||
Comment 2•12 years ago
|
||
Ah, I get it. So this happens only after stepping in on the last executed line. Doesn't happen in Chrome or Opera though.
Assignee | ||
Comment 3•12 years ago
|
||
I wonder whether it's an artifact of the way they implement stepping or a conscious product decision.
Comment 4•12 years ago
|
||
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?
Assignee | ||
Comment 5•12 years ago
|
||
(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•12 years ago
|
||
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).
Assignee | ||
Comment 7•12 years ago
|
||
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.
Assignee: nobody → past
Status: NEW → ASSIGNED
Priority: -- → P2
Assignee | ||
Comment 8•12 years ago
|
||
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!
Attachment #659245 -
Flags: review?(vporof)
Attachment #659245 -
Flags: review?(mihai.sucan)
Assignee | ||
Comment 9•12 years ago
|
||
Try: https://tbpl.mozilla.org/?tree=Try&rev=22d0840b7109
Comment 10•12 years ago
|
||
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.
Attachment #659245 -
Flags: review?(mihai.sucan) → review+
Reporter | ||
Comment 11•12 years ago
|
||
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+
Attachment #659245 -
Flags: review?(vporof) → review+
Assignee | ||
Updated•12 years ago
|
Whiteboard: [land-in-fx-team]
Assignee | ||
Comment 12•12 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/765fa82170e7
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
Reporter | ||
Comment 13•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/765fa82170e7
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Reporter | ||
Updated•12 years ago
|
Target Milestone: --- → Firefox 18
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•