Closed
Bug 878307
Opened 11 years ago
Closed 11 years ago
When stepping through source mapped code, we should continue stepping until we are at a new location in the original source
Categories
(DevTools :: Debugger, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 26
People
(Reporter: fitzgen, Assigned: fitzgen)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
Details | Diff | Splinter Review |
When treating JS as a target for compilation, often we will have multiple operations in JS mapping back to something that is considered a single operation in the original source. For example, imagine a language like Python which will automatically upgrade ints to big ints as needed. If you were to compile an addition in Python to asm.js, you wouldn't just generate an add, you would generate overflow checks, and branches for upgrading to your big ints if needed, etc. This would result in the debugger user having to step multiple times for the same addition operation in the original source before the cursor moved past the addition. To improve this user experience, when we are stepping in the debugger server, we should check source map the current frame and if it is still the same source mapped location as before, we should step again. This process should be repeated until the current frame's source mapped location is different than what it originally was before stepping (which would indicate the completion of the operation in the original source language). Side note: I think I can lay the ground work for this while implementing blackboxing. The generic idea is the same for both features: the debugger server should keep stepping until a given predicate is fulfilled.
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → nfitzgerald
Priority: -- → P2
Assignee | ||
Comment 1•11 years ago
|
||
Looking into this, it totally needs the synchronize function from bug 901712.
Depends on: 901712
Assignee | ||
Comment 2•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=04dd85de0679
Attachment #791595 -
Flags: review?(jimb)
Comment 4•11 years ago
|
||
Still reviewing, but this looks like the right thing. Two requests: 1) (In a follow-up) It might be possible to handle the !script cases more simply, just by never establishing an onStep handler for such frames at all. Right now, Debugger never generates D.Frame instances that don't have scripts, but when we have visible non-debuggee frames and frames for native calls, those will lack scripts. In either case, running until the frame changes is the best 'step' could do anyway. 2) On IRC you suggested a 'getGeneratedLocation' function that takes care of doing the bytecode-offset-to-generated-source mapping, and handles scriptless frames (for the time being) by returning { null, null, null } objects. That sounds like a nice clean-up.
Assignee | ||
Comment 5•11 years ago
|
||
(In reply to Jim Blandy :jimb from comment #4) > Still reviewing, but this looks like the right thing. Two requests: > > 1) (In a follow-up) It might be possible to handle the !script cases more > simply, just by never establishing an onStep handler for such frames at all. > Right now, Debugger never generates D.Frame instances that don't have > scripts, but when we have visible non-debuggee frames and frames for native > calls, those will lack scripts. In either case, running until the frame > changes is the best 'step' could do anyway. Filed bug 908283 > 2) On IRC you suggested a 'getGeneratedLocation' function that takes care of > doing the bytecode-offset-to-generated-source mapping, and handles > scriptless frames (for the time being) by returning { null, null, null } > objects. That sounds like a nice clean-up. Will do.
Comment 6•11 years ago
|
||
Comment on attachment 791595 [details] [diff] [review] bug-878307-source-map-stepping-2.patch Review of attachment 791595 [details] [diff] [review]: ----------------------------------------------------------------- This looks like what's necessary, but I'm concerned about doing the actual resumption of the debuggee in a different event tick than the one where we begin processing the onResume request. We don't have anything that prevents the event loop from stuffing other events down our throat while we wait for that getOriginalLocation promise to resolve.
Attachment #791595 -
Flags: review?(jimb)
Assignee | ||
Comment 7•11 years ago
|
||
(In reply to Nick Fitzgerald [:fitzgen] from comment #5) > (In reply to Jim Blandy :jimb from comment #4) > > 2) On IRC you suggested a 'getGeneratedLocation' function that takes care of > > doing the bytecode-offset-to-generated-source mapping, and handles > > scriptless frames (for the time being) by returning { null, null, null } > > objects. That sounds like a nice clean-up. > > Will do. Filed bug 908337 because this cleaned up a lot of code in places that were unrelated to this bug.
Comment 8•11 years ago
|
||
To address the concerns I raised in comment 6, I've filed bug 908452, with a patch. Not wedded to the solution, but we do need to address the issue.
Comment 9•11 years ago
|
||
As explained in bug 908452 comment 5, I think there's no need for that bug to block this. Sorry for the impedance.
No longer depends on: 908452
Comment 11•11 years ago
|
||
Comment on attachment 791595 [details] [diff] [review] bug-878307-source-map-stepping-2.patch Review of attachment 791595 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, just some minor comments. ::: toolkit/devtools/server/actors/script.js @@ +767,2 @@ > > + let script = this.youngestFrame.script; It'd be prettier to say "startFrame.script", since we just grabbed it in the line above. @@ +776,5 @@ > + let onEnterFrame = aFrame => { > + let { url } = this.synchronize(this.sources.getOriginalLocation( > + aFrame.script.url, > + aFrame.script.getOffsetLine(aFrame.offset), > + getOffsetColumn(aFrame.offset, aFrame.script))); If we're going to try to cope with scriptless frames, then we should do so here, as well. @@ +791,5 @@ > + > + let { url } = thread.synchronize(thread.sources.getOriginalLocation( > + this.script.url, > + this.script.getOffsetLine(this.offset), > + getOffsetColumn(this.offset, this.script))); Similarly re scriptless frames.
Attachment #791595 -
Flags: review+
Assignee | ||
Comment 12•11 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/d40157f3fef6
Whiteboard: [fixed-in-fx-team]
Assignee | ||
Comment 13•11 years ago
|
||
patch that landed (includes jimb's minor requests)
Attachment #791595 -
Attachment is obsolete: true
Comment 14•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d40157f3fef6
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 26
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•