1096739 - Clean up ThreadActor.prototype._setBreakpoint

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

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

Couldn't take it anymore.

Comment 1

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

Attachment: breakpoints-refactor.patch

Doesn't fix the insanity of how we deal with columns + source maps, or really change behavior at all. It just makes it clear what offsets we actually are attempting to break on and generally just makes things more clear about what we are trying to do.

Additional benefit is that this also fixes part of bug 980526, but I think we can do even better than just this and make it so we never have to call findScripts except for the first time (and then just keep our cache up to date via onNewScript).

Flagging both of you for review because I think we are the ones who "understand" (I use that term very loosely) what _setBreakpoint is doing, and I want to make sure that we are all on the same page here.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=df2be1d075e7

Comment 2

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

Comment on attachment 8520351 [details] [diff] [review]
breakpoints-refactor.patch

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

::: browser/devtools/debugger/test/code_script-switching-02.js
@@ +4,5 @@
>  function secondCall() {
>    // This comment is useful: ☺
>    eval("debugger;");
>    function foo() {}
> +  if (x) {

This was necessary because of bug 1096736, and couldn't have worked correctly before. I have no idea what it did, but it was most assuredly doing something incorrect.

Comment 3

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

Comment on attachment 8520351 [details] [diff] [review]
breakpoints-refactor.patch

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

::: browser/devtools/debugger/test/code_script-switching-02.js
@@ +4,5 @@
>  function secondCall() {
>    // This comment is useful: ☺
>    eval("debugger;");
>    function foo() {}
> +  if (x) {

I should have said "I have no idea what setting a breakpoint on this line actually did ..."

Comment 4

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8520351 [details] [diff] [review]
breakpoints-refactor.patch

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

Ok, this patch looks good overall! I like how you've distributed functionality more sanely over separate functions.

I do still think we need some architectural changes as well. For instances, when you set a breakpoint on an empty line in a source mapped source, the breakpoint should slide to the smallest line that is greater than the given line in the source mapped source, not in the original source (which is what we're doing now). That means we're solving the problem of sliding breakpoints at the wrong level (it should be solved in some layer on top of _setBreakpoint, not in _setBreakpoint itself).

Anyway, I have a couple of ideas here. Are you still ok with me taking over the refactor after you land this?

::: toolkit/devtools/server/actors/script.js
@@ +1468,5 @@
> +      actor.condition = location.condition;
> +      return actor;
> +    }
> +
> +    storedBp.actor = actor = new BreakpointActor(this, {

You don't use storedBp after this, so you don't need to update it.

@@ +1498,5 @@
> +    for (let script of scripts) {
> +      this._findClosestOffsetMappings(location, script, scriptsAndOffsetMappings);
> +    }
> +
> +    for (let [script, mappings] of scriptsAndOffsetMappings) {

I still don't understand how this can contain more than one entry. Care to give an example?

Comment 5

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

(In reply to Eddy Bruel [:ejpbruel] from comment #4)
> Ok, this patch looks good overall! I like how you've distributed
> functionality more sanely over separate functions.
> 
> I do still think we need some architectural changes as well. For instances,
> when you set a breakpoint on an empty line in a source mapped source, the
> breakpoint should slide to the smallest line that is greater than the given
> line in the source mapped source, not in the original source (which is what
> we're doing now). That means we're solving the problem of sliding
> breakpoints at the wrong level (it should be solved in some layer on top of
> _setBreakpoint, not in _setBreakpoint itself).

Yes, this is only trying to clean up and better explain what we are doing now, not change behavior to be more correct (and that should happen in subsequent patches).

> Anyway, I have a couple of ideas here. Are you still ok with me taking over
> the refactor after you land this?

Please!

> ::: toolkit/devtools/server/actors/script.js
> @@ +1468,5 @@
> > +      actor.condition = location.condition;
> > +      return actor;
> > +    }
> > +
> > +    storedBp.actor = actor = new BreakpointActor(this, {
> 
> You don't use storedBp after this, so you don't need to update it.

Unfortunately we do :( This is the result of sharing mutable state all over the place :( bug 1097141.

> @@ +1498,5 @@
> > +    for (let script of scripts) {
> > +      this._findClosestOffsetMappings(location, script, scriptsAndOffsetMappings);
> > +    }
> > +
> > +    for (let [script, mappings] of scriptsAndOffsetMappings) {
> 
> I still don't understand how this can contain more than one entry. Care to
> give an example?

For the following snippet:

  //        1
  //2345678901234567
  for (var xx of foo)

We get offsets at column 9 and column 15. If a source map pointed us to column 12, both would be 3 columns away, and so we would set out BP on both.

Future source maps + breakpoints refactoring should just get rid of the whole findClosestOffsets stuff and the setting BPs at columns, but that's out of the scope of this patch, where I didn't want to change behavior, just better explain it.

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Nick Fitzgerald [:fitzgen] from comment #5)
> (In reply to Eddy Bruel [:ejpbruel] from comment #4)
> > I still don't understand how this can contain more than one entry. Care to
> > give an example?
> 
> For the following snippet:
> 
>   //        1
>   //2345678901234567
>   for (var xx of foo)
> 
> We get offsets at column 9 and column 15. If a source map pointed us to
> column 12, both would be 3 columns away, and so we would set out BP on both.

This seems like it would be best added as a comment in the code.

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8520351 [details] [diff] [review]
breakpoints-refactor.patch

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

I have a few questions and this is complicated enough that I'd like to take another look afterwards.

::: browser/devtools/debugger/test/code_script-switching-02.js
@@ +4,5 @@
>  function secondCall() {
>    // This comment is useful: ☺
>    eval("debugger;");
>    function foo() {}
> +  if (x) {

For the curious:

https://bugzilla.mozilla.org/show_bug.cgi?id=737803#c13

::: toolkit/devtools/server/actors/script.js
@@ +1451,5 @@
> +  /**
> +   * Get or create the BreakpointActor for the breakpoint at the given location.
> +   *
> +   * NB: This will override a pre-existing BreakpointActor's condition with
> +   * given the location's condition.

Typo.

@@ +1553,5 @@
> +   *               ...
> +   *             ]
> +   *           }
> +   */
> +  _findFirstLineWithOffsets: function (scripts, startLine) {

Perhaps findNextLineWithOffsets would be more accurate? Just a thought.

@@ +1556,5 @@
> +   */
> +  _findFirstLineWithOffsets: function (scripts, startLine) {
> +    const maxLine = Math.max(...scripts.map(s => s.startLine + s.lineCount));
> +
> +    for (let line = startLine; line <= maxLine; line++) {

Isn't maxLine guaranteed to not be a part of any provided script? For a script with { startLine: 5, lineCount: 2 } the mapping above gives a maxLine of 7, but the script only contains lines 5 and 6.

What am I missing here, and can you add it as a comment in the code?

@@ +1590,5 @@
>      };
> +    const actor = location.actor = this._getOrCreateBreakpointActor(location);
> +    const scripts = this.dbg.findScripts({
> +      url: location.url
> +    });

Why are we no longer limiting the search to scripts that contain the line and column?

@@ +1626,5 @@
>  
> +    if (actualLocation) {
> +      // Check whether we already have a breakpoint actor for the actual
> +      // location. If we do have an existing actor, then the actor we created
> +      // earlier is redundant and must be destroyed. If we do not have an

Wouldn't renaming "earlier" to "above" here minimize the confusion about which breakpoint we'll remove? Both were created earlier in time :)

@@ -1617,5 @@
>    _findClosestOffsetMappings: function (aTargetLocation,
>                                          aScript,
>                                          aScriptsAndOffsetMappings) {
> -    // If we are given a column, we will try and break only at that location,
> -    // otherwise we will break anytime we get on that line.

Why remove this part? Was it moved somewhere else and I can't see it?

Comment 8

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

(In reply to Panos Astithas [:past] (overloaded, please needinfo) from comment #7)
> @@ +1556,5 @@
> > +   */
> > +  _findFirstLineWithOffsets: function (scripts, startLine) {
> > +    const maxLine = Math.max(...scripts.map(s => s.startLine + s.lineCount));
> > +
> > +    for (let line = startLine; line <= maxLine; line++) {
> 
> Isn't maxLine guaranteed to not be a part of any provided script? For a
> script with { startLine: 5, lineCount: 2 } the mapping above gives a maxLine
> of 7, but the script only contains lines 5 and 6.
> 
> What am I missing here, and can you add it as a comment in the code?

Nope, you are right, this was a (mostly harmless) off by one error.

> @@ +1590,5 @@
> >      };
> > +    const actor = location.actor = this._getOrCreateBreakpointActor(location);
> > +    const scripts = this.dbg.findScripts({
> > +      url: location.url
> > +    });
> 
> Why are we no longer limiting the search to scripts that contain the line
> and column?

If we add the line/column filter, we miss scripts that we could otherwise slide down to.

> @@ +1626,5 @@
> >  
> > +    if (actualLocation) {
> > +      // Check whether we already have a breakpoint actor for the actual
> > +      // location. If we do have an existing actor, then the actor we created
> > +      // earlier is redundant and must be destroyed. If we do not have an
> 
> Wouldn't renaming "earlier" to "above" here minimize the confusion about
> which breakpoint we'll remove? Both were created earlier in time :)

Great idea :)

> @@ -1617,5 @@
> >    _findClosestOffsetMappings: function (aTargetLocation,
> >                                          aScript,
> >                                          aScriptsAndOffsetMappings) {
> > -    // If we are given a column, we will try and break only at that location,
> > -    // otherwise we will break anytime we get on that line.
> 
> Why remove this part? Was it moved somewhere else and I can't see it?

The non-column logic branch is replaced by using the simpler findNextLineWithOffsets + Debugger.Script.prototype.getLineOffsets to get the entry points to a line, which unifies it with the sliding behavior.

This method is only used for columns now, and the comment describing our logic branching for when there is or isn't a column was no longer valid.

Comment 9

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

Attachment: breakpoints-refactor.patch

Fixed up based on Panos's comments.

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8521612 [details] [diff] [review]
breakpoints-refactor.patch

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

> > Why are we no longer limiting the search to scripts that contain the line
> > and column?
> 
> If we add the line/column filter, we miss scripts that we could otherwise
> slide down to.

Is this worthy of a test?

Comment 11

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

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8bbf5db85ada