Closed Bug 1556903 Opened 1 year ago Closed 3 months ago

Support index mapped sourcemaps (sourcemap sections)

Categories

(DevTools :: Debugger, enhancement, P2)

69 Branch
x86_64
macOS
enhancement

Tracking

(firefox77 fixed)

RESOLVED FIXED
Firefox 77
Tracking Status
firefox77 --- fixed

People

(Reporter: andrew, Assigned: andrew)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/74.0.3729.169 Safari/537.36

Steps to reproduce:

Use a JS source map where the map is an indexed map as described in the draft spec at https://docs.google.com/document/d/1U1RGAehQwRypUTovF1KRlpiOFze0b-_2gc6fAH0KY0k/edit#heading=h.535es3xeprgt

This functionality does work in Chrome.

I've created a simple test case to demonstrate this: https://github.com/andrewnicols/devtools-examples/tree/post_processed_example

This functionality is required where built files are concatenated into a single file, and a secondary map is generated from it.

Actual results:

The following error is seen in the console:

Error while fetching an original source: Error: "http://boysenberry.local/mdn/devtools-examples/sourcemaps-in-console/js/main.coffee" is not in the SourceMap.
Source URL: http://boysenberry.local/mdn/devtools-examples/sourcemaps-in-console/js/main.coffee

Expected results:

The source map should have been displayed and worked as expected.

Component: Untriaged → Debugger
OS: Unspecified → macOS
Product: Firefox → DevTools
Hardware: Unspecified → x86_64
Version: Trunk → 69 Branch

Some more information:

If the indexed sourcemap contains a sourcesContent, then it does work, but it does not allow for the content to be loaded from elsewhere via the sources array, though that array is required and it does complain if it's not present.

I think I've tracked this down to a change in source-map when indexed source maps were added.

Following the introduction of the IndexedSourceMapConsumer there are now two slightly different implementations of sourceContentFor().

The standard one in the BasicSourceMapConsumer.
And the one for indexed sources in the IndexedSourceMapConsumer.

The implementation in BasicSourceMapConsumer() returns null if this.sourcesContent is empty which means that getOriginalSourceText fetches the source from originalSource.url. It does this regardless of the nullOnMissing setting. If there is a sourcesContent, but it does not contain the index, then either null is returned or an Error is thrown depending upon the value of nullOnMissing.

The implementation in IndexedSourceMapConsumer checks each section for the file and then, if it does not contain the index, then either null is returned or an Error is thrown depending upon the value of nullOnMissing.

Essentially there are two bugs here:

  1. source-map returns null instead of throwing an Error in BasicSourceMapConsumer.getSourcesFor() regardless of the setting of nullOnMissing; and
  2. we should pass true as the second argument to getSourcesFor() so that we consistently get a null value, and fetch the source file.

The following patch seems to do the trick:

diff --git a/devtools/client/shared/source-map/worker.js b/devtools/client/shared/source-map/worker.js
index e679e05468c0..c6e11f4fe8b8 100644
--- a/devtools/client/shared/source-map/worker.js
+++ b/devtools/client/shared/source-map/worker.js
@@ -2264,7 +2264,7 @@ async function getOriginalSourceText(originalSource) {
     return null;
   }

-  let text = map.sourceContentFor(originalSource.url);
+  let text = map.sourceContentFor(originalSource.url, true);
   if (!text) {
     text = (await networkRequest(originalSource.url, { loadFromCache: false })).content;
   }

Thanks, it is nice to see the fix is in the source map worker and not in the underlying source-map library.

It would be great to have a small test.

Here is a good one to copy. And some docs

If you're interested in contributing the fix or discussing further, I would be happy to discuss tomorrow https://devtools-html-slack.herokuapp.com

Thanks Jason,

I've written some tests - basically identical to sourcemaps2.html and it's JS, but with the map modified to use an indexed sourcemap.

Currently waiting for git to reclone the official mercurial repo so that I can push to phabricator.

The implementation of sourceContentFor differs between the BasicSourceMapConsumer, and IndexedSourceMapConsumers with
indexed consumers throwing an Error when the worker was expecting a null.
We now pass true to the nullOnEmpty parameter to the sourceContentFor function to ensure a consistent result with all
Consumers.

Priority: -- → P2
Type: defect → enhancement
Summary: index mapped sourcemaps do not work → Support index mapped sourcemaps (sourcemap sections)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → andrew
Status: NEW → ASSIGNED
Pushed by loganfsmyth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7d4ac54cd44a
Return null when source not found. r=loganfsmyth
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 77
You need to log in before you can comment on or make changes to this bug.