1556903 - Support index mapped sourcemaps (sourcemap sections)

Status: RESOLVED FIXED

(DevTools :: Debugger, enhancement, P2)

Description

[Andrew Nicols]

Attachment: Basic example to demonstrate the issue

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.

Comment 1

[Andrew Nicols]

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.

Comment 2

[Andrew Nicols]

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;
   }

Comment 3

[Jason Laster [:jlast]]

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

Comment 4

[Jason Laster [:jlast]]

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

Comment 5

[Andrew Nicols]

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.

Comment 6

[Andrew Nicols]

Attachment: Bug 1556903 - Return null when source not found.

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.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7d4ac54cd44a
Return null when source not found. r=loganfsmyth

Comment 8

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/7d4ac54cd44a