Bug 1856658 - [devtools] Better integrate SourceMapLoader into breakable positions computation. r=devtools-reviewers,nchevobbe

We can avoid the creation of an handful of object creations by passing
the "breakpoint positions" objects directly to the Source Map Loader.
(avoid convertToList)

We can also tune `SourceMapLoader.getOriginalLocations` output to better
fit the requirements of the main thread.

Differential Revision: https://phabricator.services.mozilla.com/D189950
This commit is contained in:
Alexandre Poirot 2023-10-05 15:03:32 +00:00
parent 4a83fa6e97
commit 0e5417ff35
8 changed files with 153 additions and 200 deletions

View File

@ -15,27 +15,67 @@ import { makeBreakpointId } from "../../utils/breakpoint";
import { memoizeableAction } from "../../utils/memoizableAction";
import { fulfilled } from "../../utils/async-value";
import {
debuggerToSourceMapLocation,
sourceMapToDebuggerLocation,
createLocation,
} from "../../utils/location";
import { validateSource } from "../../utils/context";
async function mapLocations(generatedLocations, { getState, sourceMapLoader }) {
if (!generatedLocations.length) {
return [];
async function mapLocations(
breakpointPositions,
generatedSource,
isOriginal,
originalSourceId,
{ getState, sourceMapLoader }
) {
// Map breakable positions from generated to original locations.
let mappedBreakpointPositions = await sourceMapLoader.getOriginalLocations(
breakpointPositions,
generatedSource.id
);
// The Source Map Loader will return null when there is no source map for that generated source.
// Consider the map as unrelated to source map and process the source actor positions as-is.
if (!mappedBreakpointPositions) {
mappedBreakpointPositions = breakpointPositions;
}
const originalLocations = await sourceMapLoader.getOriginalLocations(
generatedLocations.map(debuggerToSourceMapLocation)
);
return originalLocations.map((location, index) => ({
// If location is null, this particular location doesn't map to any original source.
location: location
? sourceMapToDebuggerLocation(getState(), location)
: generatedLocations[index],
generatedLocation: generatedLocations[index],
}));
const locations = [];
for (let line in mappedBreakpointPositions) {
// createLocation expects a number and not a string.
line = parseInt(line, 10);
for (const columnOrSourceMapLocation of mappedBreakpointPositions[line]) {
// When processing a source unrelated to source map, `mappedBreakpointPositions` will be equal to `breakpointPositions`.
// and columnOrSourceMapLocation will always be a number.
// But it will also be a number if we process a source mapped file and SourceMapLoader didn't find any valid mapping
// for the current position (line and column).
// When this happen to be a number it means it isn't mapped and columnOrSourceMapLocation refers to the column index.
if (typeof columnOrSourceMapLocation == "number") {
const generatedLocation = createLocation({
line,
column: columnOrSourceMapLocation,
source: generatedSource,
});
locations.push({ location: generatedLocation, generatedLocation });
} else {
// Otherwise, for sources which are mapped. `columnOrSourceMapLocation` will be a SourceMapLoader location object.
// This location object will refer to the location where the current column (columnOrSourceMapLocation.generatedColumn)
// mapped in the original file.
const generatedLocation = createLocation({
line,
column: columnOrSourceMapLocation.generatedColumn,
source: generatedSource,
});
locations.push({
location: sourceMapToDebuggerLocation(
getState(),
columnOrSourceMapLocation
),
generatedLocation,
});
}
}
}
return locations;
}
// Filter out positions, that are not in the original source Id
@ -66,24 +106,6 @@ function filterByUniqLocation(positions) {
});
}
function convertToList(results, source) {
const positions = [];
for (const line in results) {
for (const column of results[line]) {
positions.push(
createLocation({
line: Number(line),
column,
source,
})
);
}
}
return positions;
}
function groupByLine(results, source, line) {
const isOriginal = source.isOriginal;
const positions = {};
@ -208,8 +230,13 @@ async function _setBreakpointPositions(location, thunkArgs) {
}
}
let positions = convertToList(results, generatedSource);
positions = await mapLocations(positions, thunkArgs);
let positions = await mapLocations(
results,
generatedSource,
location.source.isOriginal,
location.source.id,
thunkArgs
);
// `mapLocations` may compute for a little while asynchronously,
// ensure that the location is still valid before continuing.
validateSource(getState(), location.source);

View File

@ -7,14 +7,9 @@ import {
selectors,
watchForState,
createStore,
makeOriginalSource,
makeSource,
} from "../../../utils/test-head";
import {
createSource,
mockCommandClient,
} from "../../tests/helpers/mockCommandClient";
import { getBreakpointsList } from "../../../selectors";
import { mockCommandClient } from "../../tests/helpers/mockCommandClient";
import { isFulfilled, isRejected } from "../../../utils/async-value";
import { createLocation } from "../../../utils/location";
@ -75,112 +70,6 @@ describe("loadGeneratedSourceText", () => {
).not.toBe(-1);
});
it("should update breakpoint text when a source loads", async () => {
const fooOrigContent = createSource("fooOrig", "var fooOrig = 42;");
const fooGenContent = createSource("fooGen", "var fooGen = 42;");
const store = createStore(
{
...mockCommandClient,
sourceContents: async () => fooGenContent,
getSourceActorBreakpointPositions: async () => ({ 1: [0] }),
getSourceActorBreakableLines: async () => [],
},
{},
{
getGeneratedRangesForOriginal: async () => [
{ start: { line: 1, column: 0 }, end: { line: 1, column: 1 } },
],
getOriginalLocations: async items =>
items.map(item => ({
...item,
sourceId:
item.sourceId === fooGenSource1.id
? fooOrigSources1[0].id
: fooOrigSources2[0].id,
})),
getOriginalSourceText: async s => ({
text: fooOrigContent.source,
contentType: fooOrigContent.contentType,
}),
}
);
const { dispatch, getState } = store;
const fooGenSource1 = await dispatch(
actions.newGeneratedSource(makeSource("fooGen1"))
);
const fooOrigSources1 = await dispatch(
actions.newOriginalSources([makeOriginalSource(fooGenSource1)])
);
const fooGenSource2 = await dispatch(
actions.newGeneratedSource(makeSource("fooGen2"))
);
const fooOrigSources2 = await dispatch(
actions.newOriginalSources([makeOriginalSource(fooGenSource2)])
);
await dispatch(actions.loadOriginalSourceText(fooOrigSources1[0]));
await dispatch(
actions.addBreakpoint(
createLocation({
source: fooOrigSources1[0],
line: 1,
column: 0,
}),
{}
)
);
const breakpoint1 = getBreakpointsList(getState())[0];
expect(breakpoint1.text).toBe("");
expect(breakpoint1.originalText).toBe("var fooOrig = 42;");
const fooGenSource1SourceActor =
selectors.getFirstSourceActorForGeneratedSource(
getState(),
fooGenSource1.id
);
await dispatch(actions.loadGeneratedSourceText(fooGenSource1SourceActor));
const breakpoint2 = getBreakpointsList(getState())[0];
expect(breakpoint2.text).toBe("var fooGen = 42;");
expect(breakpoint2.originalText).toBe("var fooOrig = 42;");
const fooGenSource2SourceActor =
selectors.getFirstSourceActorForGeneratedSource(
getState(),
fooGenSource2.id
);
await dispatch(actions.loadGeneratedSourceText(fooGenSource2SourceActor));
await dispatch(
actions.addBreakpoint(
createLocation({
source: fooGenSource2,
line: 1,
column: 0,
}),
{}
)
);
const breakpoint3 = getBreakpointsList(getState())[1];
expect(breakpoint3.text).toBe("var fooGen = 42;");
expect(breakpoint3.originalText).toBe("");
await dispatch(actions.loadOriginalSourceText(fooOrigSources2[0]));
const breakpoint4 = getBreakpointsList(getState())[1];
expect(breakpoint4.text).toBe("var fooGen = 42;");
expect(breakpoint4.originalText).toBe("var fooOrig = 42;");
});
it("loads two sources w/ one request", async () => {
let resolve;
let count = 0;

View File

@ -39,7 +39,6 @@ import {
selectors,
actions,
makeSource,
makeSourceURL,
waitForState,
} from "../../utils/test-head";
@ -221,44 +220,6 @@ describe("adding sources", () => {
expect(selectors.getBreakpointCount(getState())).toEqual(1);
});
it("corresponding breakpoints are added to the original source", async () => {
const sourceURL = makeSourceURL("bar.js");
const store = createStore(mockClient({ 5: [2] }), loadInitialState(), {
getOriginalURLs: async source => [
{
id: sourceMapLoader.generatedToOriginalId(source.id, sourceURL),
url: sourceURL,
},
],
getOriginalSourceText: async () => ({ text: "" }),
getGeneratedLocation: async location => location,
getOriginalLocation: async location => location,
getGeneratedRangesForOriginal: async () => [
{ start: { line: 0, column: 0 }, end: { line: 10, column: 10 } },
],
getOriginalLocations: async items =>
items.map(item => ({
...item,
sourceId: sourceMapLoader.generatedToOriginalId(
item.sourceId,
sourceURL
),
})),
});
const { getState, dispatch } = store;
expect(selectors.getBreakpointCount(getState())).toEqual(0);
await dispatch(
actions.newGeneratedSource(makeSource("bar.js", { sourceMapURL: "foo" }))
);
await waitForState(store, state => selectors.getBreakpointCount(state) > 0);
expect(selectors.getBreakpointCount(getState())).toEqual(1);
});
it("add corresponding breakpoints for multiple sources", async () => {
const store = createStore(
mockClient({ 5: [2] }),

View File

@ -59,6 +59,17 @@ addIntegrationTask(async function testReloadingStableOriginalSource(
info("Check that breakpoint is on the first line within the function `foo`");
assertTextContentOnLine(dbg, 8, expectedOriginalFileContentOnBreakpointLine);
info(
"Check that the source text snippet displayed in breakpoints panel is correct"
);
assertBreakpointSnippet(
dbg,
1,
isCompressed
? "nonSourceMappedFunction();"
: "await nonSourceMappedFunction();"
);
info(
"Check that the breakpoint is displayed in correct location in bundle.js (generated source)"
);
@ -73,6 +84,16 @@ addIntegrationTask(async function testReloadingStableOriginalSource(
expectedGeneratedFileContentOnBreakpointLine
);
}
info(
"The breakpoint snippet doesn't change when moving to generated content"
);
assertBreakpointSnippet(
dbg,
1,
isCompressed
? `nonSourceMappedFunction(),console.log("YO")}}]);`
: "await nonSourceMappedFunction();"
);
await closeTab(dbg, "bundle.js");
@ -115,6 +136,11 @@ addIntegrationTask(async function testReloadingStableOriginalSource(
} else {
is(breakpoint.generatedLocation.line, 89);
}
assertBreakpointSnippet(
dbg,
1,
isCompressed ? `log("HEY")` : `console.log("HEY")`
);
// This reload removes the content related to the lines in the original
// file where the breakpoints where set.

View File

@ -56,6 +56,10 @@ addIntegrationTask(async function testReloadingRemovedOriginalSources(
} else {
is(breakpoint.generatedLocation.line, 80);
}
info(
"Assert that the breakpoint snippet is originaly set to the to-be-removed original source content"
);
assertBreakpointSnippet(dbg, 1, `console.log("Removed original");`);
await resume(dbg);
@ -93,6 +97,10 @@ addIntegrationTask(async function testReloadingRemovedOriginalSources(
} else {
is(breakpoint.generatedLocation.line, 80);
}
info(
"Assert that the breakpoint snippet changed to the new original source content"
);
assertBreakpointSnippet(dbg, 1, `console.log("New original");`);
await resume(dbg);
info("Wait for reload to complete after resume");

View File

@ -65,6 +65,16 @@ addIntegrationTask(async function testReloadingChangedGeneratedSource(
expectedGeneratedFileContentOnBreakpointLine
);
}
info(
"Check that the source code snipper shown in the breakpoint panel is correct"
);
assertBreakpointSnippet(
dbg,
1,
isCompressed
? `baabar(),console.log("YO")},foobar()}]);`
: `await baabar();`
);
await closeTab(dbg, "bundle-with-another-original.js");
@ -170,6 +180,15 @@ addIntegrationTask(async function testReloadingChangedGeneratedSource(
is(breakpoint.generatedLocation.line, 103);
}
info("Check that the breakpoint snippet is still the same");
assertBreakpointSnippet(
dbg,
1,
isCompressed
? `baabar(),console.log("YO")},foobar()}]);`
: `await baabar();`
);
if (!isCompressed) {
await resume(dbg);
info("Wait for reload to complete after resume");

View File

@ -1666,9 +1666,9 @@ async function assertLogBreakpoint(dbg, line) {
ok(hasLogClass, `Log breakpoint on line ${line}`);
}
function assertBreakpointSnippet(dbg, index, snippet) {
function assertBreakpointSnippet(dbg, index, expectedSnippet) {
const actualSnippet = findElement(dbg, "breakpointLabel", 2).innerText;
is(snippet, actualSnippet, `Breakpoint ${index} snippet`);
is(actualSnippet, expectedSnippet, `Breakpoint ${index} snippet`);
}
const selectors = {

View File

@ -216,20 +216,43 @@ async function getGeneratedLocation(location) {
};
}
async function getOriginalLocations(locations) {
const maps = {};
const results = [];
for (const location of locations) {
let map = maps[location.sourceId];
if (map === undefined) {
map = await getSourceMap(location.sourceId);
maps[location.sourceId] = map || null;
}
results.push(map ? getOriginalLocationSync(map, location) : null);
/**
* Map the breakable positions (line and columns) from generated to original locations.
*
* @param {Object} breakpointPositions
* List of columns per line refering to the breakable columns per line
* for a given source:
* {
* 1: [2, 6], // On line 1, column 2 and 6 are breakable.
* ...
* }
* @param {string} sourceId
* The ID for the generated source.
*/
async function getOriginalLocations(breakpointPositions, sourceId) {
const map = await getSourceMap(sourceId);
if (!map) {
return null;
}
return results;
for (const line in breakpointPositions) {
const breakableColumnsPerLine = breakpointPositions[line];
for (let i = 0; i < breakableColumnsPerLine.length; i++) {
const column = breakableColumnsPerLine[i];
const mappedLocation = getOriginalLocationSync(map, {
sourceId,
line: parseInt(line, 10),
column,
});
if (mappedLocation) {
// As we replace the `column` with the mappedLocation,
// also transfer the generated column so that we can compute both original and generated locations
// in the main thread.
mappedLocation.generatedColumn = column;
breakableColumnsPerLine[i] = mappedLocation;
}
}
}
return breakpointPositions;
}
function getOriginalLocationSync(map, location) {