878307 - When stepping through source mapped code, we should continue stepping until we are at a new location in the original source

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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.

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Looking into this, it totally needs the synchronize function from bug 901712.

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: bug-878307-source-map-stepping-2.patch

https://tbpl.mozilla.org/?tree=Try&rev=04dd85de0679

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Filed bug 906249 because onResume is getting out of control.

Comment 4

[Jim Blandy :jimb]

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.

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(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

[Jim Blandy :jimb]

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.

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(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

[Jim Blandy :jimb]

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

[Jim Blandy :jimb]

As explained in bug 908452 comment 5, I think there's no need for that bug to block this. Sorry for the impedance.

Comment 10

[Jim Blandy :jimb]

I'll look at the review afresh.

Comment 11

[Jim Blandy :jimb]

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.

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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

Comment 13

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: bug-878307-source-map-stepping-2.patch

patch that landed (includes jimb's minor requests)

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d40157f3fef6