The current way we do breakpoint sliding is broken in two ways: 1. We assume that setting a breakpoint on a line in an original source corresponds to setting a breakpoint to a single line in a generated source. There is nothing in the source map spec that guarantees that a single original line can only map to a single generated line. 2. We perform sliding on generated lines, not original lines. Apparently, we assume that sliding to the next line in the generated source corresponds to sliding to the next line in the original source. Again, there is nothing in the source map spec that guarantees this. What we should be doing is this: given an original source + line, find all generated locations that correspond to that line. Next, find all entry points that correspond to one or more of these generated locations. If no entry points are found, slide to the next line in the *original* source, compute the generated locations again, and proceed as before. Continue until either we find at least on entry point or until we go past the last line in the original source. There are quite some changes we need to make to get there from where we currently are. The plan is to make setBreakpoint the central place to perform breakpoint sliding. That means setBreakpoint will have to behave as follows: 1. It should take an original location as argument. 2. It should return a promise (because we will have to compute generated locations from the original location) 3. This promise should resolve to the actual (original) location. This bug is about step 2, i.e. refactor setBreakpoint so that it always return a promise.

Hi James. See the bug description for an explanation of this patch. If there's anything unclear about that, let's arrange for a vidyo meet so we can discuss in more detail.

Comment on attachment 8551805 [details] [diff] [review] patch Review of attachment 8551805 [details] [diff] [review]: ----------------------------------------------------------------- This is a small enough change, sounds good. Don't we define just `resolve` at the top of the file when we import promises, or do you like to be more explicit with `Promise.resolve`?

(In reply to Eddy Bruel [:ejpbruel] from comment #0) > 1. We assume that setting a breakpoint on a line in an original source > corresponds > to setting a breakpoint to a single line in a generated source. There is > nothing in the source map spec that guarantees that a single original > line can > only map to a single generated line. You've mentioned this a few times but I still don't quite understand. If you could provide some simple code that demonstrates it, that would help. When we set a bp on a line, we find the bytecode position that is the beginning on the line, right? The beginning of a line in an original source will correspond to a single "beggining of the line" position in the generated source. I do understand that a source may compile to multiple lines, but are you saying that the code may compile to code where the "first" line in the generated code may not be executed? as in, the multiple lines that the single original line compiled to may be entered differently. (again, some code examples would really help) If we set breakpoints on all the generated lines, don't we have to be careful when stepping so that a "step over" actually steps to the next original line, and doesn't break on any of the breakpoints we set?

(In reply to James Long (:jlongster) from comment #3) > (In reply to Eddy Bruel [:ejpbruel] from comment #0) > > 1. We assume that setting a breakpoint on a line in an original source > > corresponds > > to setting a breakpoint to a single line in a generated source. There is > > nothing in the source map spec that guarantees that a single original > > line can > > only map to a single generated line. > > You've mentioned this a few times but I still don't quite understand. If you > could provide some simple code that demonstrates it, that would help. When > we set a bp on a line, we find the bytecode position that is the beginning > on the line, right? The beginning of a line in an original source will > correspond to a single "beggining of the line" position in the generated > source. I do understand that a source may compile to multiple lines, but are > you saying that the code may compile to code where the "first" line in the > generated code may not be executed? as in, the multiple lines that the > single original line compiled to may be entered differently. (again, some > code examples would really help) > > If we set breakpoints on all the generated lines, don't we have to be > careful when stepping so that a "step over" actually steps to the next > original line, and doesn't break on any of the breakpoints we set? That's not completely accurate. For breakpoints in non-sourcemapped sources, we can simply set breakpoints on all offsets that are entry points for the given line. For sourcemapped sources, the beginning of the given line is always at column 0 in the original source, but may be at any column in the generated source. In a minified script, for example, setting a breakpoint on line 20 in an original source could map to line 1, column 240 in the generated source. Even if we find the corresponding line in the generated source, we cannot simply set breakpoints on all offsets that map that are entry points for the given line, like we could with non-sourcemapped sources, since multiple original lines can be mapped to a single generated line. This is why we introduced the notion of column breakpoints, so we can set breakpoints on offsets that are entry points for a specific column. For that to work, however, we need to know exactly what columns an original line spans in the generated source. Finally, there is nothing in the source map spec that guarantees that a single original line maps to a contiguous column spans in a given generated line. It's perfectly possible to map the first half of the original line to one column span generated line, and the second half to another, in such a way that those column spans are not contiguous. Admittedly, this is a very unlikely scenario, and I doubt any source maps actually do this, but since the spec allows for it, we should at least support it.

(In reply to James Long (:jlongster) from comment #2) > Comment on attachment 8551805 [details] [diff] [review] > patch > > Review of attachment 8551805 [details] [diff] [review]: > ----------------------------------------------------------------- > > This is a small enough change, sounds good. Don't we define just `resolve` > at the top of the file when we import promises, or do you like to be more > explicit with `Promise.resolve`? If it's ok with you, I would prefer to get rid of those shorthands completely, and be explicit about the fact that we're using promises everywhere. What's your opinion?

Try run for this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=977df51b8ed0

Try push looks green, landed on fx-team: https://hg.mozilla.org/integration/fx-team/rev/fcd8ddf5cd31