When applications have source maps, it's possible for an original line to map to a generated column. We ran into this case in this bug: https://github.com/devtools-html/debugger.html/issues/5280 Here, line 18, column undefined mapped to line 21205 column 4. As a result the client tried to set a breakpoint at (21205, 4). The problem is that 4 comes before the first bytecode offset so the breakpoint is never set. > class ComponentsSidebar { > onComponentChange() { > var searchBug = this.props.searchBug > } > } We can fix this by defaulting to adding a breakpoint at the first offset if no entrypoints are found.

Comment on attachment 8948792 [details] Bug 1436151 - Breakpoints at the beginning of a line are missed. https://reviewboard.mozilla.org/r/218156/#review223992 Code analysis found 2 defects in this patch: - 2 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: devtools/server/actors/source.js:919 (Diff revision 2) > + return true; > + } > + > + return false; > + } > + Error: More than 1 blank line not allowed. [eslint: no-multiple-empty-lines] ::: devtools/server/actors/source.js:934 (Diff revision 2) > - entryPoints.push({ script, offsets: [offset] }); > + entryPoints.push({ script, offsets: [offset] }); > - } > + } > - } > + } > - } > + } > - } > Error: More than 1 blank line not allowed. [eslint: no-multiple-empty-lines]

Comment on attachment 8948792 [details] Bug 1436151 - Breakpoints at the beginning of a line are missed. https://reviewboard.mozilla.org/r/218156/#review223994 Code analysis found 4 defects in this patch: - 4 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: devtools/server/actors/source.js:914 (Diff revision 1) > } > } > - } else { > + > + if (entryPoints.length > 0) { > + setBreakpointAtEntryPoints(actor, entryPoints); > + return true; Error: Trailing spaces not allowed. [eslint: no-trailing-spaces] ::: devtools/server/actors/source.js:919 (Diff revision 1) > + return true; > + } > + > + return false; > + } > + Error: More than 1 blank line not allowed. [eslint: no-multiple-empty-lines] ::: devtools/server/actors/source.js:934 (Diff revision 1) > - entryPoints.push({ script, offsets: [offset] }); > + entryPoints.push({ script, offsets: [offset] }); > - } > + } > - } > + } > - } > + } > - } > Error: More than 1 blank line not allowed. [eslint: no-multiple-empty-lines] ::: devtools/server/actors/source.js:949 (Diff revision 1) > + }); > + > + if (columnToOffsetMap.length > 0) { > + let { columnNumber: column, offset } = columnToOffsetMap[0]; > + if (generatedColumn < column) { > + entryPoints.push({ script, offsets: [offset] }); Error: Trailing spaces not allowed. [eslint: no-trailing-spaces]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=19c766e3c97e11f987d4a489b3fdc22dd9c234d0&selectedJob=160700245 ^ the certViewer is very mad at me :)

Comment on attachment 8948792 [details] Bug 1436151 - Breakpoints at the beginning of a line are missed. https://reviewboard.mozilla.org/r/218154/#review224990 Looks good, but there are some changes I'd like to see. ::: devtools/server/actors/source.js:918 (Diff revision 3) > + setBreakpointAtEntryPoints(actor, entryPoints); > + return true; > + } > + > + return false; > + } Could we write it this way? if (generatedColumn === undefined) { ... handle whole-line breakpoints ... } else { ... look for columns within range ... if (entryPoints.length === 0) { ... look for first column on line ... } } if (entryPoints.length === 0) { return false; } setBreakpointAtEntryPoints(actor, entryPoints); return true; I think it's clearer that these are three different strategies for producing `entryPoints`, but we always return `false` if there was nothing there (the patched version misses this in the column case, I think), and call `setBreakpointAtEntryPoints` if we did find something. ::: devtools/server/actors/source.js:942 (Diff revision 3) > - return false; > + for (let script of scripts) { > + let columnToOffsetMap = script > + .getAllColumnOffsets() > + .filter(({ lineNumber }) => { > + return lineNumber === generatedLine; > + }); `getAllColumnOffsets` returns a potentially very large data set. Let's not recompute the scripts' `columnToOffsetMap`s again; maybe we can save them in an array after we compute them the first time? (I'm not happy with `Debugger.Script` not offering a better API for just looking up the offsets we need; I have a bad feeling about just how much allocation `getAllColumnOffsets` is doing...)

Comment on attachment 8948792 [details] Bug 1436151 - Breakpoints at the beginning of a line are missed. https://reviewboard.mozilla.org/r/218156/#review225694 Looks great - thanks!

The linter had some comments, too. Were those addressed?

carrying over the r+

Pushed by apavel@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a8aa7e2af130 Breakpoints at the beginning of a line are missed. r=jimb

Backed out for xpcshell failures on /test_setBreakpoint-at-the-beginning-of-a-line.js Backout push: https://hg.mozilla.org/mozilla-central/rev/941681a0ddc64f46dda9c2e69ea36a78bc307dee Log: https://treeherder.mozilla.org/logviewer.html#?job_id=162613565&repo=mozilla-central&lineNumber=2461

There were also test-verify failures on /test_setBreakpoint-at-the-beginning-of-a-line.js Log: https://treeherder.mozilla.org/logviewer.html#?job_id=162612059&repo=mozilla-central&lineNumber=9986

These XPCshell failures showed up after the merges. Conflict with bug 1437828.

carrying over the r+

carry over

Pushed by jlaster@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/29223aed98f3 Breakpoints at the beginning of a line are missed. r=jimb