1129784 - Column breakpoints should slide to the right

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Eddy Bruel [:ejpbruel]]

When setting a column breakpoint on column 0 of an indented line, the source map library cannot find an exact mapping for column 0, so it tries to find the closest one that is less than the mapping we are looking for. As a result, the column breakpoint slides to the left, onto the previous line, which is not what you'd expect.

What should happen is that the source map library finds the closest mapping that is greater than the one we are looking for, so the column breakpoint slides to the right.

This will mostly involve changes in the source map library at:
https://github.com/mozilla/source-map/.

The purpose of this bug is to merge those changes back into mozilla-central.

Comment 1

[Eddy Bruel [:ejpbruel]]

Attachment: Implement column sliding for source mapped sources

Currently, when setting a column breakpoint on an empty column, we try to slide it to the next closest line. What we should do is try to slide it to the next closest column first, until we hit the end of the line, and only then switch to line sliding.

Note that I had to remove an optimization from invertSourceMap because generatedPositionFor and originalPositionFor are no longer the only methods we use. (we now also have allGeneratedPositionsFor, which doesn't have an inverse).

Comment 2

[Eddy Bruel [:ejpbruel]]

Attachment: Implement column sliding for non-source mapped sources

This patch implements the same algorithm for non-source mapped sources.

Comment 3

[James Long (:jlongster)]

Comment on attachment 8578808 [details] [diff] [review]
Implement column sliding for non-source mapped sources

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

r- but I might be reading this logic wrong. If you can answer my questions I will re-review it.

::: toolkit/devtools/server/actors/script.js
@@ +2809,1 @@
>          if (originalColumn === undefined) {

Shouldn't this be `originalColumn !== undefined`? I must be reading this wrong... but in this `if` block is where you do column sliding.

@@ +2809,3 @@
>          if (originalColumn === undefined) {
> +          // To perform breakpoint sliding for column breakpoints, we need to
> +          // build a map from line numbers to a list of entry points for each

I think you mean "build a map from column numbers..."

@@ +2839,4 @@
>              }
>            }
>  
>            // Now that we have a map from line numbers to a list of entry points

from column numbers

@@ +2863,1 @@
>            }

After this while loop, don't you need to do the check to see if it failed? See the line breakpoint sliding. You check `if (actualLine === lineToEntryPointsMap.length) ... ` and then return `originalLocation` indicating that sliding failed.

Comment 4

[Eddy Bruel [:ejpbruel]]

(In reply to James Long (:jlongster) from comment #3)
> Comment on attachment 8578808 [details] [diff] [review]
> Implement column sliding for non-source mapped sources
> 
> Review of attachment 8578808 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- but I might be reading this logic wrong. If you can answer my questions I
> will re-review it.
> 
> ::: toolkit/devtools/server/actors/script.js
> @@ +2809,1 @@
> >          if (originalColumn === undefined) {
> 
> Shouldn't this be `originalColumn !== undefined`? I must be reading this
> wrong... but in this `if` block is where you do column sliding.

Yeah, that's a bug. Good catch.

> 
> @@ +2809,3 @@
> >          if (originalColumn === undefined) {
> > +          // To perform breakpoint sliding for column breakpoints, we need to
> > +          // build a map from line numbers to a list of entry points for each
> 
> I think you mean "build a map from column numbers..."
> 

Jup.

> @@ +2839,4 @@
> >              }
> >            }
> >  
> >            // Now that we have a map from line numbers to a list of entry points
> 
> from column numbers
> 
> @@ +2863,1 @@
> >            }
> 
> After this while loop, don't you need to do the check to see if it failed?
> See the line breakpoint sliding. You check `if (actualLine ===
> lineToEntryPointsMap.length) ... ` and then return `originalLocation`
> indicating that sliding failed.

No. If column sliding fails, we fall back to line sliding. If line sliding fails, breakpoint sliding as a whole fails.

Comment 5

[Eddy Bruel [:ejpbruel]]

Attachment: Implement column sliding for non-source mapped sources

New patch with comments addressed.

Comment 6

[James Long (:jlongster)]

Comment on attachment 8580077 [details] [diff] [review]
Implement column sliding for non-source mapped sources

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

Looks good

Comment 7

[James Long (:jlongster)]

Comment on attachment 8578806 [details] [diff] [review]
Implement column sliding for source mapped sources

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

Looks fine to me

::: toolkit/devtools/server/actors/common.js
@@ +438,5 @@
>    },
>  
>    equals: function (other) {
>      return this.generatedSourceActor.url == other.generatedSourceActor.url &&
> +           this.generatedLine === other.generatedLine;

I feel like I already asked this somewhere else, but how come you don't compare columns as well?

::: toolkit/devtools/server/actors/script.js
@@ +2614,5 @@
>        }
>        generator.addMapping(mapping);
>      }).then(() => {
>        generator.setSourceContent(this.url, code);
> +      let consumer = SourceMapConsumer.fromSourceMap(generator);

Nice cleanup here.

@@ +2933,2 @@
>          } else {
> +          return slideByLine(originalLine + 1);

You don't have to do this now, but in the future we should start using Task. I think it would make code like this clearer. Although the recursive functions is pretty clever.

Comment 8

[Eddy Bruel [:ejpbruel]]

(In reply to James Long (:jlongster) from comment #7)
> Comment on attachment 8578806 [details] [diff] [review]
> Implement column sliding for source mapped sources
> 
> Review of attachment 8578806 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks fine to me
> 
> ::: toolkit/devtools/server/actors/common.js
> @@ +438,5 @@
> >    },
> >  
> >    equals: function (other) {
> >      return this.generatedSourceActor.url == other.generatedSourceActor.url &&
> > +           this.generatedLine === other.generatedLine;
> 
> I feel like I already asked this somewhere else, but how come you don't
> compare columns as well?

We *should* be doing that, but doing so breaks several tests, and I haven't yet figured out why. I intend to fix this in a future patch.
> 
> ::: toolkit/devtools/server/actors/script.js
> @@ +2614,5 @@
> >        }
> >        generator.addMapping(mapping);
> >      }).then(() => {
> >        generator.setSourceContent(this.url, code);
> > +      let consumer = SourceMapConsumer.fromSourceMap(generator);
> 
> Nice cleanup here.
> 
> @@ +2933,2 @@
> >          } else {
> > +          return slideByLine(originalLine + 1);
> 
> You don't have to do this now, but in the future we should start using Task.
> I think it would make code like this clearer. Although the recursive
> functions is pretty clever.

I would love to use Task, but it is currently not supported in workers, and script.js is needed for worker debugging.

Comment 9

[Eddy Bruel [:ejpbruel]]

Try push for column sliding in source mapped sources:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=00704c1bd775

Comment 10

[Eddy Bruel [:ejpbruel]]

Bug 1099209 landed, so we can try this again:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bca2395171c5

Comment 11

[Eddy Bruel [:ejpbruel]]

Try push looks good. Landing on fx-team:
https://hg.mozilla.org/integration/fx-team/rev/b0ed0d8f967a

Comment 12

[Eddy Bruel [:ejpbruel]]

Try push for column sliding in non-sourcemapped sources:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9bdeb3d76a76

Comment 13

[Eddy Bruel [:ejpbruel]]

Some failures on the try push, but afaict, those are all infra related. Pushing to fx-team:
https://hg.mozilla.org/integration/fx-team/rev/0a61d88c8eec

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b0ed0d8f967a
https://hg.mozilla.org/mozilla-central/rev/0a61d88c8eec