1557196 - Column breakpoint cannot be set on pretty printed source

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

[Mike Hansford]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:68.0) Gecko/20100101 Firefox/68.0

Steps to reproduce:

Using Firefox Developer 68.0b7

1. Browse to https://davidwalsh.name/
2. In the Debugger, open wp-content/themes/punky/js/libs/main.js
3. Pretty print the source in the debugger
4. Attempt to set a column breakpoint (eg. line 3 or 9)
5. Note that you won't be shown any potential locations for column breakpoints.

Actual results:

You are not shown any potential locations for column breakpoints.

I also observed that in the un-pretty printed source, that I was shown 6 potential locations for column breakpoints, all near the start of the file.

Expected results:

As at 6 June:
Line 3: var t = n().hash.replace('#', m),
e = $$('#' + t + ', a[name=' + t + ']');

I would expect to be able to insert column breakpoints at .hash and .replace (at least) and probably somewhere along the assignment of e.

Comment 1

[David Walsh :davidwalsh]

I love that site!

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:jlast, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks for the report!

I am able to reproduce the bug using STRs from comment #0
There are no available locations for columns breakpoints -> BUG
Honza

Comment 4

[Thomas Wisniewski [:twisniewski]]

This would be a very nice quality-of-life improvement for those of us diagnosing webcompat issues.

Comment 5

[Ksenia Berezina [:ksenia]]

I agree with Thomas, this affects me regularly when diagnosing webcompat issues. It would save us a lot of time to have this fixed.

Comment 6

[Jason Laster [:jlast]]

Thanks. That's good to know. I'll see if we can prioritize this soon

Comment 7

[Jason Laster [:jlast]]

Thanks for filing Mike.

You're right that there is a difference between the column breakpoints before and after pretty printing. Unfortunately, one of the limitations of our pretty-printer is that it only offers line based source maps, which means that we cannot offer column breakpoints for pretty printed files.

I'm going to keep this bug open, but de-prioritize it as it is not something we can realistically change without a significant performance cost.

Comment 8

[Mike Hansford]

That's a shame. I'm the only Firefox-using web developer in my team and all my teammates using the browser-beginning-with-C can do this. My hopes of converting them to a better browser are taking a hit. However, I can understand the decision. The performance cost of the change would probably hurt my chances even more.

Comment 9

[Jason Laster [:jlast]]

I'm sorry to hear that. What is your use case for wanting column breakpoints + pretty printing?

Comment 10

[:Harald Kirschner :digitarald]

Mike, thanks for the extra detail, I see pretty print coming up as source of issues in other channels as well. Not having full breakpoint support is a no-go and is high on the list for our plans for improving pretty print.

Comment 11

[Jason Laster [:jlast]]

Feel free to join our slack discussion. I'd love to follow up and learn more about the scenarios where more granular breakpoints would be helpful. Alternatively, we can also improve how we are pretty printing in certain cases to add/remove linebreaks.

Comment 12

[Mike Hansford]

Hi Jason
The site I used in the example is not the site I work on. The site I work on is a 700k+ line big ball of jQuery mud and no one should be forced to look under the bonnet of this site. Method chaining is heavily used and abused. Column breakpoints are helpful for seeing what is coming out of different stages of a method chain. They improve precision with debugging.
My prior comment should be read with your tongue firmly planted in your cheek. While my fellow developers can do this with their browser and I can't do it with mine, I'd rather have a performant, stable browser to work with. I'm going to jump on the slack channel you suggested.

Comment 13

[David Walsh :davidwalsh]

This is still an issue.

Comment 14

[Jason Laster [:jlast]]

I think the reason this does not work is that column breakpoints requires source maps with column data. Pretty fast only attempts to map lines because it is expensive to include columns in the map.

Comment 15

[Karl Dubost💡 :karlcow]

And I had this case today on a diagnostic that was very confusing.
https://github.com/webcompat/web-bugs/issues/83590#issuecomment-1009611727
where the logged value was not the one I thought it was.

Comment 16

[Nicolas Chevobbe [:nchevobbe]]

As far as I can tell, prettyFast is using SourceNode#toStringWithSourceMap source-node.js, which should produce column mapping, so the issue must be somewhere else

I created a smaller STR:

1. On https://ffx-devtools-pretty-print.glitch.me/ , open the debugger
2. Open 1557196.js
3. Add a breakpoint on line 3

-> you can then set column breakpoints on multiple locations

  const text = ➤this.todos.➤map(todo => ➤"TODO:" + todo);

4. Pretty print the file. line 3 was moved to line 6, but remains the same
5. Add a breakpoint on line 6

-> there's are no indication of possible column breakpoints

---

Alex, would you know why this is happening and if it can be fixed?

Comment 17

[Alexandre Poirot [:ochameau]]

Here is a debugging script which can help:

diff --git a/devtools/client/debugger/dist/pretty-print-worker.js b/devtools/client/debugger/dist/pretty-print-worker.js
index 2e1a63dff3eca..57cb6cedcae80 100644
--- a/devtools/client/debugger/dist/pretty-print-worker.js
+++ b/devtools/client/debugger/dist/pretty-print-worker.js
@@ -1040,6 +1040,7 @@ var __WEBPACK_AMD_DEFINE_FACTORY__, __WEBPACK_AMD_DEFINE_RESULT__;/* This Source
             lineStr += buffer[i];
           }
 
+    dump(" BufferLine["+bufferLine+"] bufferColumn:"+bufferColumn+" lineStr:"+lineStr+"\n");
           result.add(new SourceNode(bufferLine, bufferColumn, options.url, lineStr));
           buffer.splice(0, buffer.length);
           bufferLine = -1;

This logs the following output against comment 16 STR:

 BufferLine[1] bufferColumn:0 lineStr:document.addEventListener('click', function onClick() {

 BufferLine[2] bufferColumn:2 lineStr:  const todos = [

 BufferLine[2] bufferColumn:17 lineStr:    1,

 BufferLine[2] bufferColumn:19 lineStr:    2

 BufferLine[2] bufferColumn:20 lineStr:  ];

 BufferLine[3] bufferColumn:2 lineStr:  const text = this.todos.map(todo=>'TODO:' + todo);  

 BufferLine[4] bufferColumn:0 lineStr:})

This highlights that pretty-fast works per line.
I'm wondering if we would need more refined SourceNode's...?

For example here:
BufferLine[3] bufferColumn:2 lineStr: const text = this.todos.map(todo=>'TODO:' + todo);
We map the whole line to generated line 3 column 2.

But I'm not sure it is the root cause. It isn't clear how the source-map would map to the right column.

Down the line we have the right information over here:
https://searchfox.org/mozilla-central/rev/d39a17381a14606032f7b8e82789bf281beb1241/devtools/client/debugger/src/actions/breakpoints/breakpointPositions.js#179
Before calling mapLocations the locations are only for the generated file and are 100% correct.

positions = await mapLocations(positions, thunkArgs);

After mapLocations the orignal locations are added, but are all bound to column 0:

  {
    "location": {
      "sourceId": "source-url-https://ffx-devtools-pretty-print.glitch.me/cases/1557196.js/originalSource-f7b92a045351c6b44954e5ffb7de82d4",
      "sourceUrl": "https://ffx-devtools-pretty-print.glitch.me/cases/1557196.js:formatted",
      "line": 6,
      "column": 0
    },
    "generatedLocation": {
      "line": 3,
      "column": 15,
      "sourceId": "source-url-https://ffx-devtools-pretty-print.glitch.me/cases/1557196.js",
      "sourceUrl": "https://ffx-devtools-pretty-print.glitch.me/cases/1557196.js"
    }
  },
  {
    "location": {
      "sourceId": "source-url-https://ffx-devtools-pretty-print.glitch.me/cases/1557196.js/originalSource-f7b92a045351c6b44954e5ffb7de82d4",
      "sourceUrl": "https://ffx-devtools-pretty-print.glitch.me/cases/1557196.js:formatted",
      "line": 6,
      "column": 0
    },
    "generatedLocation": {
      "line": 3,
      "column": 26,
      "sourceId": "source-url-https://ffx-devtools-pretty-print.glitch.me/cases/1557196.js",
      "sourceUrl": "https://ffx-devtools-pretty-print.glitch.me/cases/1557196.js"
    }
  },
  {
    "location": {
      "sourceId": "source-url-https://ffx-devtools-pretty-print.glitch.me/cases/1557196.js/originalSource-f7b92a045351c6b44954e5ffb7de82d4",
      "sourceUrl": "https://ffx-devtools-pretty-print.glitch.me/cases/1557196.js:formatted",
      "line": 6,
      "column": 0
    },
    "generatedLocation": {
      "line": 3,
      "column": 38,
      "sourceId": "source-url-https://ffx-devtools-pretty-print.glitch.me/cases/1557196.js",
      "sourceUrl": "https://ffx-devtools-pretty-print.glitch.me/cases/1557196.js"
    }
  }

Again, it is not clear how the sourcemap could possibly match the right column with the very few information we currently pass to SourceNode...
But it isn't clearer how refined it should become as the line we are talking about isn't changed by pretty printing (but that, the source-map can't know if the source is similar or not).
Should we emit many SourceNodes per lines, based on the token, token that would typically be a breakpoint positition, in order to avoid emitting one mapping per original column?

Otherwise I did some search within chromium and here is a few interesting pointers:
https://source.chromium.org/chromium/chromium/src/+/main:out/fuchsia-Debug/gen/third_party/devtools-frontend/src/front_end/entrypoints/formatter_worker/JavaScriptFormatter.js;l=20
https://source.chromium.org/chromium/chromium/src/+/main:third_party/devtools-frontend/src/front_end/entrypoints/formatter_worker/FormattedContentBuilder.ts;l=120
It isn't clear by reading the code if they are trying to do a more refined per column mapping.

Comment 18

[Nicolas Chevobbe [:nchevobbe]]

Thanks Alex.
So it does look like this is an issue from prettyFast
We can see in the test snapshots that the columns are always 0 (https://searchfox.org/mozilla-central/source/devtools/client/debugger/src/workers/pretty-print/tests/__snapshots__/prettyFast.spec.js.snap)
And that does come from the "optimization" that is done to buffer lines in https://searchfox.org/mozilla-central/rev/3a13c5ad9c1dbd065e484a399af75eb98c235def/devtools/client/debugger/src/workers/pretty-print/pretty-fast.js#756-801

/**
 * Write a pretty printed string to the result SourceNode.
 *
 * We buffer our writes so that we only create one mapping for each line in
 * the source map. This enhances performance by avoiding extraneous mapping
 * serialization, and flattening the tree that
 * `SourceNode#toStringWithSourceMap` will have to recursively walk. When
 * timing how long it takes to pretty print jQuery, this optimization
 * brought the time down from ~390 ms to ~190ms!
 */
const write = (function() {
  const buffer = [];
  let bufferLine = -1;
  let bufferColumn = -1;
  return function innerWrite(str, line, column, ignoreNewline) {
    if (line != null && bufferLine === -1) {
      bufferLine = line;
    }
    if (column != null && bufferColumn === -1) {
      bufferColumn = column;
    }
    buffer.push(str);

    if (str == "\n" && !ignoreNewline) {
      let lineStr = "";
      for (let i = 0, len = buffer.length; i < len; i++) {
        lineStr += buffer[i];
      }
      result.add(
        new SourceNode(bufferLine, bufferColumn, options.url, lineStr)
      );
      buffer.splice(0, buffer.length);
      bufferLine = -1;
      bufferColumn = -1;
    }
  };
})();

So we could try to remove the buffering to see if that would solve the column breakpoint issue.
If it does, then we can check the performance hit and see if it's worth it

Comment 19

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #18)

So we could try to remove the buffering to see if that would solve the column breakpoint issue.

The good news is that it does seem to work \o/ …

If it does, then we can check the performance hit and see if it's worth it

… the bad news is that pretty print performance is regressed by more than 200% (DAMP)

Comment 20

[Nicolas Chevobbe [:nchevobbe]]

okay, so I did some profiling and it looks like we were spending quite some time in postMessage, I guess because we now have a lot more items in the mapping.

The mapping that is sent back from the pretty-print worker has the following shape:

[{
  generated: {
    line: 1,
    column
  }, 
  original: {
    line: 1,
    column
  }, 
  name: null,
  source: "https://..."
}, {...}]

And then we post it to the sourcemap worker (possibly multiple times) https://searchfox.org/mozilla-central/rev/3a13c5ad9c1dbd065e484a399af75eb98c235def/devtools/client/debugger/src/actions/sources/prettyPrint.js#58-68

const { code, mappings } = await prettyPrintWorker.prettyPrint({
  text: contentValue.value,
  url,
});
await sourceMapLoader.applySourceMap(generatedSource.id, url, code, mappings);

// The source map URL service used by other devtools listens to changes to
// sources based on their actor IDs, so apply the mapping there too.
for (const { actor } of actors) {
  await sourceMapLoader.applySourceMap(actor, url, code, mappings);
}

Because the shape of the mapping array is complex, it's costly to post to/from workers.
But we only really need the original line and columns, and generated line and columns. We already have the source (it's the pretty-printed file), and as far as I can tell name is always null (not sure what it could refer to though).

So instead of passing this multiple-level array back from the pretty print worker, I tried only passing a 1-d array, with the following shape:

[
      // mapping #1
      originalLine,
      originalColumn,
      generatedLine,
      generatedColumn,
      // mapping #2
      originalLine,
      originalColumn,
      generatedLine,
      generatedColumn,
      …
      // mapping #n
]

and sending it as is to the sourcemap worker
And then in the sourcemap worker, turning this array back into something that SourceMapGenerator#addMapping can consume

And here's the TRY push comparing the base version vs the version with column mapping + simpler mapping structure:
https://treeherder.mozilla.org/perfherder/compare?originalProject=try&newProject=try&newRevision=e307ce24422940ae5c5d9521cc00ddddfcc4a070&framework=12&originalRevision=13cc2d125b563d392079e1ed2e178b3566eeb525&page=1

It still regresses (25% and 17%), but I think that could be acceptable as we are making column breakpoints working on pretty printed files

Comment 21

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1557196 - [devtools] Don't buffer lines in pretty-fast. r=ochameau.

This allows us to get accurate mappings for columns as well, which is
required for column breakpoints to work.

A test is added to ensure column breakpoints work as expected after pretty-printing

Comment 22

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1557196 - [devtools] Simplify mapping structure posted to/from workers. r=bomsy!,ochameau.

We used to get the mappings array from the pretty fast worker to the prettyPrint
action, and then pass this array to the sourceMap service (also a worker).
As the mapping can be quite large, and given it has a complex shape, passing it
to and from workers (via postMessage) was costly.

So here, instead of passing only the mapping, we directly generate the sourceMap
from the pretty fast worker, and pass it to the action which forwards it to
the sourceMap service.

This helps reduce the overhead we were seeing when not buffering the lines in
the pretty print worker, in the previous patch of this stack.

Depends on D168383

Comment 23

[Phabricator Automation]

Comment on attachment 9315101 [details]
Bug 1557196 - [devtools] Simplify mapping structure posted to/from workers. r=bomsy!,ochameau.

Revision D168384 was moved to bug 1815452. Setting attachment 9315101 [details] to obsolete.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c92cd49e0f7a
[devtools] Don't buffer lines in pretty-fast. r=ochameau,devtools-reviewers.

Comment 25

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/c92cd49e0f7a