Closed Bug 1141507 Opened 5 years ago Closed 5 years ago

Refactor breakpoint sliding for source mapped sources

Categories

(DevTools :: Debugger, defect)

x86
macOS
defect
Not set

Tracking

(firefox39 fixed)

RESOLVED FIXED
Firefox 39
Tracking Status
firefox39 --- fixed

People

(Reporter: ejpbruel, Assigned: ejpbruel)

Details

Attachments

(2 files)

Breakpoint sliding for source-mapped sources should be performed on original locations, not generated locations.
Assignee: nobody → ejpbruel
This patch contains some preliminary refactors that we need. Among other things:

- The length of the breakpoint method names was starting to get out of hand, so I
  refactored those to something more concise.
- I factored out the code that updates the actor and actor map when the actual
  location differs from the original one into a separate then handler, because this
  code will be the same for both source mapped and non-source mapped actors.
- Even when we're not doing breakpoint sliding, we can still end up with
  an actual location that is different from the original location due to fuzzy
  search. Consequently, _setBreakpointAtOriginalLocation needs to return the
  original location corresponding to the generated location.
- I've added a stub implementation of getAllGeneratedLocations for. Right now this
  will simply return the result of getGeneratedLocation wrapped in an array, but
  eventually this will use allGeneratedPositionsFor to return all mappings that
  correspond to a given line.
Attachment #8575275 - Flags: review?(jlong)
Hopefully this patch is fairly self-explanatory. Let me know if you have any questions

Couple of notes:
- We now use getAllGeneratedLocations to get all generated locations for a given
  location in an original source (as opposed to just getting the first generated
  location we can find), and try to set a breakpoint on all of them.

  getAllGeneratedLocations is implemented using allGeneratedPositionsFor, which
  landed as part of the merge of the source-map library with fx-team.

- GeneratedLocation now contains both the first and last column for each mapping,
  so we can set breakpoints on column spans instead of single columns.

  The last column is computed lazily using computeColumnSpans, which landed as part
  of the merge of the source-map library with fx-team.

- We now slide on the original location, not the generated location, when sliding
  in source-mapped sources.
Attachment #8577295 - Flags: review?(jlong)
Comment on attachment 8575275 [details] [diff] [review]
Some preliminary refactors

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

Looks like this patch basically moves around some functions and that's all, and there's nothing in here concerning to me.

::: toolkit/devtools/server/actors/script.js
@@ +3027,5 @@
>  
>      if (entryPoints.length === 0) {
>        return false;
>      }
> +    setBreakpointAtEntryPoints(actor, entryPoints);

+1 for simplifying these names and dropping the "forActor"
Attachment #8575275 - Flags: review?(jlong) → review+
I'll finish looking at the other patch first thing tomorrow; I want to try to understand the changes as best as possible so I can give a good review.
Comment on attachment 8577295 [details] [diff] [review]
Refactor breakpoint sliding for source mapped actors.

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

Looks fine to me, a little hard to follow the big picture across several patches but that's fine. You remove a lot of code here but I think a lot of that showed up in previous patches in a different form.

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

How come you don't need to check the column? Seems like you would need that for column sliding, no?

::: toolkit/devtools/server/actors/script.js
@@ +2897,5 @@
>            return originalLocation;
>          }
>        } else {
> +        if (originalColumn === undefined) {
> +          let loop = (actualLocation) => {

nit: you don't need the parens around a single argument

@@ +2922,5 @@
> +                return this.threadActor.sources.getOriginalLocation(generatedLocations[0]);
> +              }
> +
> +              // Try the next line in the original source.
> +              return loop(new OriginalLocation(this, actualLine + 1));

Clever use of `loop` to get around the problems with promise control flow (eventually we should use generators more, but we can do that later)

@@ +5665,5 @@
>        return OriginalLocation.fromGeneratedLocation(generatedLocation);
>      });
>    },
>  
>    getAllGeneratedLocations: function (originalLocation) {

Docs for this function? I know you are going to continue refactoring but this would be a good function to have docs for (and should stay around). Explain when to use this and when to just use `getGeneratedLocation`.
Attachment #8577295 - Flags: review?(jlong) → review+
Try run looks good, pushing to fx-team (after rebase):
https://hg.mozilla.org/integration/fx-team/rev/a10fbddc674c
Try push for refactoring breakpoint sliding for source mapped actors:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb072bfda1f0
https://hg.mozilla.org/mozilla-central/rev/a10fbddc674c
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Try push for the second patch looks good, pushing to fx-team:
https://hg.mozilla.org/integration/fx-team/rev/a752e16a5251
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/a752e16a5251
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.