Use built-in URL

[tromey]

source-map implements its own URL parsing, but it seems to me that it would be better to just use the built-in URL.

[tromey]

For node we need require("url") but for the browser we don't want that.

[tromey]

But maybe we could work around that in devtools-source-map.

[tromey]

We'll also need to work around it for the bundle that's put in devtools/shared/sourcemap. Maybe we ought to make a new webpack config for that.

[tromey]

One problem the current approach causes is that library users can't pass a data: URL as the source map URL to SourceMapConsumer, because the current code can't parse a data: URL. (Though, maybe this is minor, because passing a data: URL there is useless anyway...)

[tromey]

This problem came up again here: https://bugzilla.mozilla.org/show_bug.cgi?id=1451274

I wrote a quick patch to change this but I am not completely sure it is correct. E.g., it regresses this test:

  assert.equal(libUtil.join('http://foo.org/a//', '//b'), 'http://b');

and this one:

  assert.equal(libUtil.join('http://www.example.com', '//foo.org/bar'), 'http://foo.org/bar');

I can't recall offhand if these are part of the spec or not, will have to check.

[edmorley]

Hi! Is there anything we can do to help move this forwards?

Several Mozilla web teams use webpack based workflows, so are potentially affected by sourcemap issues that come up (eg webpack-contrib/style-loader#303). It would be good to try and keep our own employees using Firefox for web development :-)

[tromey]

Hi! Is there anything we can do to help move this forwards?

Sure - take over the patch here: https://github.com/tromey/source-map/tree/use-real-url
The main thing is to investigate the issues pointed out above (#275 (comment)).

For the webpack-contrib issue you linked to, a further part would be to add tests for blob URLs.

If you really want to do all this I can also tell you how to get everything landed. I'm not working on devtools any more, which is why I'm not doing this... but another option would be to contact someone in devtools to see if anyone there can do it.

[jasonLaster]

This bug is a priority.

If we can land this branch master...tromey:use-real-url, here I'd be happy to cut a new release and land it in MC.

Here is a link to the devtools-source-maps package. @tromey we moved it into the debugger.html repo so it can be updated when the debugger updates.

[heyflo]

I tried for some hours to implement correct sourcemaps for my dev environment with webpack and I just could not solve it. Just to find out it's a known issue and it's working perfectly on Chrome. I'm using the last build on Firefox Dev Edition(62.0b3) and the style loader on webpack 4 (4.12.1)

Here is what I get :

[jigz]

I'm having issues too

[jasonLaster]

Hey @heyflo @JigzPalillo I'd be happy to take a look next week when I'm back. Ping me in our slack.

👋 https://devtools-html-slack.herokuapp.com/

[devinrm]

Hi, is there any movement on this yet? It's a bummer not to be able to use FF for development. Thanks for all your hard work!

[redeyes2015]

Hi there:

I was brought here from webpack-contrib/style-loader#303, hoping I can do something to push things forward, but things are, for sure, more complex than I thought.

TL;DR:

1. It seems that I solved the URL dependency issue, in my fork.
2. devtools-html/debugger.html still uses source-map 0.6.1, but 0.7 introduce the use of WebAssembly, so how to load the .wasm file is still hanging, and the progress seems halted (WIP - Update source-map to 0.7.2 (#5598) firefox-devtools/devtools-core#995 and [SourceMaps] update to source-map to 0.7.2 firefox-devtools/debugger#5598).
3. Even applying WIP - Update source-map to 0.7.2 (#5598) firefox-devtools/devtools-core#995, there are still issues (please check the last part).

Hoping some one could show me some way I could provide some help.

---

I managed to pick up @tromey 's work, in my fork, fixed the dependency problem on "URL", and confirmed that message "sourceMapUrl could not be parsed" did not appear. I also ran the test suits, and it passed, so I am not sure about whether the issues metioned in #275 (comment) are still there.

(The followings are more about devtools-html/debugger.html, but it was the main reason I got tangled, so please allow me to log things here)

However, for devtools-html/debugger.html, it still uses [email protected], fixing the url problem lead me to the issue of "where to get lib/mapping.wasm" due to the breaking change introduced in source-map 0.7.

The work of updating to 0.7.2 (firefox-devtools/debugger#5598) seems already halted. I copied firefox-devtools/devtools-core#995 to debugger.html, again in my fork, since "package/dev-tools-source-map" got moved to a different repository, it seemed worked, at least no message of "sourceMapUrl could not be parsed" and "lib/mapping.wasm", but the file names were not the paths of original CSS files, and I saw error messages on the terminal where I invoked Firefox:

console.error: "Error while calling actor 'cssUsage's method 'createEditorReportForSheet'" "stylesheetActor is null; can't access its \"rawSheet\" property"
console.error: "createEditorReportForSheet@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/csscoverage.js:339:11\nhandler@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1178:21\nonPacket@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:1711:15\nreceiveMessage@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/child-transport.js:66:5\nMessageListener.receiveMessage*_addListener@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/child-transport.js:40:5\nready@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/child-transport.js:57:5\n_onConnection@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:1146:5\nconnectToParent@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:567:12\nonConnect</<@resource://devtools/server/startup/frame.js:50:22\nonConnect<@resource://devtools/server/startup/frame.js:49:7\nexports.makeInfallible/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:14\nMessageListener.receiveMessage*@resource://devtools/server/startup/frame.js:72:5\n@resource://devtools/server/startup/frame.js:19:4\n"
console.error: "Protocol error (unknownError): stylesheetActor is null; can't access its \"rawSheet\" property"

My guess is firefox-devtools/devtools-core#995 has not taken care of all the breaking change between source-map 0.6.1 and 0.7.

I just want to share things I stumbled on, and that it is really saddening that we can not use Firefox as the main developing environment. I will be very delighted if any one could give me some suggestion or any review, despite my awkward English.

[jasonLaster]

Thanks for all the help. This is on our Sprint for the week. Would you like to pair with us to get it in?

…

On Tue, Sep 25, 2018, 6:51 AM rein ***@***.***> wrote: Hi there: I was brought here from webpack-contrib/style-loader#303 <webpack-contrib/style-loader#303>, hoping I can do something to push things forward, but thing are, for sure, more complex than I thought. TL;DR: 1. It seems that I solved the URL dependency issue, in my fork <https://github.com/redeyes2015/source-map/tree/use-real-url>. 2. devtools-html/debugger.html <https://github.com/devtools-html/devtools-core> still uses source-map 0.6.1, but 0.7 introduce the use of WebAssembly, so how to load the .wasm file is still hanging, and the progress seems halted ( firefox-devtools/devtools-core#995 <firefox-devtools/devtools-core#995> and firefox-devtools/debugger#5598 <firefox-devtools/debugger#5598>). 3. Even experimenting with firefox-devtools/devtools-core#995 <firefox-devtools/devtools-core#995>, there are still some issue (please check the last part). ------------------------------ I managed to pick up @tromey <https://github.com/tromey> 's work, in my fork <https://github.com/redeyes2015/source-map/tree/use-real-url>, fixed the dependency problem on "URL", and confirmed that message "sourceMapUrl could not be parsed" did not appear. I also ran the test suits, and it passed, so I am not sure about whether the issues metioned in #275 (comment) <#275 (comment)> are still there. (The followings are more about devtools-html/debugger.html, but it was the main reason I got tangled, so please allow me to log things here) However, for devtools-html/debugger.html <https://github.com/devtools-html/devtools-core>, it still uses ***@***.***, fixing the url problem lead me to the issue of "where to get lib/mapping.wasm" due to the breaking change introduced in source-map 0.7. The work of updating to 0.7.2 (firefox-devtools/debugger#5598 <firefox-devtools/debugger#5598>) seems already halted. I copied firefox-devtools/devtools-core#995 <firefox-devtools/devtools-core#995> to debugger.html, again in my fork <https://github.com/redeyes2015/debugger.html/tree/update-source-map>, since "package/dev-tools-source-map" got moved to a different repository, it seemed worked, at least no message of "sourceMapUrl could not be parsed" and "lib/mapping.wasm", but the file names were not the paths of original CSS files, and I saw error messages on the terminal where I invoked Firefox: console.error: "Error while calling actor 'cssUsage's method 'createEditorReportForSheet'" "stylesheetActor is null; can't access its \"rawSheet\" property" console.error: ***@***.***://devtools/shared/base-loader.js -> ***@***.***://devtools/shared/base-loader.js -> ***@***.***://devtools/shared/base-loader.js -> ***@***.***://devtools/shared/base-loader.js -> ***@***.***://devtools/shared/base-loader.js -> ***@***.***://devtools/shared/base-loader.js -> ***@***.***://devtools/shared/base-loader.js -> ***@***.***://devtools/shared/base-loader.js -> ***@***.******@***.******@***.***://devtools/shared/base-loader.js -> ***@***.******@***.***://devtools/server/startup/frame.js:19:4\n" console.error: "Protocol error (unknownError): stylesheetActor is null; can't access its \"rawSheet\" property" My guess is firefox-devtools/devtools-core#995 <firefox-devtools/devtools-core#995> has not taken care of all the breaking change between source-map 0.6.1 and 0.7. I just want to share things I stumbled on, and that it is really saddening that we can not use Firefox as the main developing environment. I will be very delighted if any one could give me some suggestion or any review, despite my awkward English. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#275 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAPiYlLAOH40SNDkfXor-LH_DzjfYLNmks5uegrJgaJpZM4Pg5pX> .

[redeyes2015]

I'd love to join, but I am not familiar with the process. Where should I start?

[jasonLaster]

ping me here https://devtools-html-slack.herokuapp.com/ and we can jump into appear.in

[jasonLaster]

@redeyes2015 i would try in the simplest form possible here.

We can figure out how to integrate it in the debugger :)

[redeyes2015]

After more experiments, I found that it is object url (blob:...) actually triggers the error. I made a seemingly minimum reproduce here: https://github.com/redeyes2015/firefox-source-map-error-min-reproduce/blob/master/dist/index.html

And for this particular case, this patch seems suffice:

diff --git a/lib/util.js b/lib/util.js
index 35bd93d..15a37dd 100644
--- a/lib/util.js
+++ b/lib/util.js
@@ -526,7 +526,7 @@ function computeSourceURL(sourceRoot, sourceURL, sourceMapURL) {
   //   If the sources are not absolute URLs after prepending of the
   //   “sourceRoot”, the sources are resolved relative to the
   //   SourceMap (like resolving script src in a html document).
-  if (sourceMapURL) {
+  if (sourceMapURL && !sourceMapURL.startsWith("blob:")) {
     const parsed = urlParse(sourceMapURL);
     if (!parsed) {
       throw new Error("sourceMapURL could not be parsed");

As for using built-in URL ... I starts to feel there are too many exceptions to handle like:

1. //foo/bar might be a protocol-relative URL, or a path to be normalized to /foo/bar
2. data URL simply not parsable
3. new URL("blob:http://localhost:9000/12345-12345-12345") get weird result (origin part should not be overlap with pathname, right?) :

href: "blob:http://localhost:9000/12345-12345-12345"
origin: "http://localhost:9000"
pathname: "http://localhost:9000/12345-12345-12345"
protocol: "blob:"

Maybe it needs a second thought.

[tromey]

I don't really recall the details of the patch, but I thought the relative stuff was handled correctly at least. blob does need a special case somewhere, as does data. This might have been handled in the wrapper stuff that devtools uses. Anyway the main thing is not to have a bad parser, but a conforming one; the current parser fails in a number of cases.

Ok, I see I'd commented on the //foo/bar case in #275 (comment). That one requires checking the spec. It may not be valid to do this at all.

[loganfsmyth]

Heya, I've been looking at your PR a bit too because I'm trying to get this landed for the DevTools.

//foo/bar might be a protocol-relative URL, or a path to be normalized to /foo/bar

I think at the end of the day, focusing on URL semantics is the way to go on this, so I'd favor protocol-relative there.

data URL simply not parsable

Could you clarify what you mean about this?

new URL("blob:http://localhost:9000/12345-12345-12345") get weird result (origin part should not be overlap with pathname, right?) :

Since origin is more security-related, it is really up to the spec to define this. Here, you can see that it specifically pulls from the path by re-parsing the path as its own URL, as defined in https://url.spec.whatwg.org/#origin

And for this particular case, this patch seems suffice:

If I'm understanding right, for this case, we can consider refactoring computeSourceURL so that instead of doing

sourceURL = join(parsed.toString(), sourceURL);

we'd do

try {
  sourceURL = new URL(sourceURL, parsed);
} catch (e) {}

because new URL("omg.js", "blob:http://localhost:9000/12345-12345-12345"); will throw since it knows that it can't produce a valid URL.

Can you spot any issues with this?

[loganfsmyth]

Looking at this again @redeyes2015, I think I'd prefer

  if (sourceMapURL) {
    const parsed = urlParse(sourceMapURL);
    if (!parsed) {
      throw new Error("sourceMapURL could not be parsed");
    }

    const absoluteURL = urlParse(sourceURL, sourceMapURL); // pass second param here as second to `URL`.
    if (!absoluteURL) {
      throw new Error(`${sourceURL} could not be made relative to ${sourceMapURL}`);
    }
    sourceURL = absoluteURL;
  }

so it would throw if sourceMapURL was data: or blob: because there is legitimately no URL that we can safely generate in that context, if the sourceURL wasn't already ab absolute URL. Does that seem reasonable?

[tromey]

Does that seem reasonable?

I think it's fine, assuming urlParse will work like the URL constructor when sourceURL is absolute -- that is, ignore sourceMapURL.

Also beware of this though: https://github.com/devtools-html/debugger.html/blob/master/packages/devtools-source-map/src/utils/fetchSourceMap.js#L30. This should probably be expanded to recognize blob: as well.

[redeyes2015]

I pushed another version, and I added a unit test to replicate what I believe
to be the minimum reproduce of https://github.com/redeyes2015/firefox-source-map-error-min-reproduce/blob/master/dist/index.html .

For computeSourceURL:

As lib/source-map-consumer.js relies on computeSourceURL to solve the URL, and does not do extra check against blob: , I think it might be better to make computeSourceURL catch and ignore the error might be better. The second version (#275 (comment)) , might be reasonable, but lib/source-map-consumer.js needs to be modified as well to pass the test I just added.

For join:

I am a bit confused about join. Since none of "http://", "//example.com", and "www.example.com" is not acceptable for URL, there are two if blocks dealing with these special cases can be removed. The case of join("http://", "www.example.com") works before the last section of join pass "http:/www.example.com" to normalize, and surprisingly for me, normalize does normalize that into "http://www.example.com" because URL accepts "http:/www.example.com" .

May be I should just remove those two if blocks?

Another thing I hesitate about is this patch translate spaces in path into "%20", but the previous version does not. May I hope someone to assure me this would be OK?

data URL simply not parsable

Could you clarify what you mean about this?

I am so sorry I provided mis-information again 😢 . I thought new URL("data:...") would throw, but it does not.

Thanks for point out the origin section of the URL spec! I should have read it before jumping in...

[digitarald]

@loganfsmyth I assume this can be closed?

[mbalaam]

Edit: In my particular case (using style-loader) I now get a different error message. Previously I would get: Source map error: Error: sourceMapURL could not be parsed but now I get Source map error: TypeError: Invalid URL: .

~~Will this fix be released in Firefox 67? ~~

Firefox 66 (Developer Edition) seems even more broken than before, but today’s Firefox nightly seem fine (marked as 67)

[loganfsmyth]

@mbalaam This change landed in 65. I would recommend filing a bug in Bugzilla with reproduction steps, and we can go from there.

[nearwood]

Just updated to 66.0b13 and this is fixed for me, thanks!

[alvaro-escalante]

Still getting the blob url on 67.0b6 Dev edition, it fails to point to the right scss file, working well on chrome

[loganfsmyth]

@alvaro-escalante It would be best to file an issue in Bugzilla so we can track the issue.

[Austaras]

blob url still not working on 67.0b9
https://bugzilla.mozilla.org/show_bug.cgi?id=1543896

[jkrems]

The bug in bugzilla was closed as fixed (https://bugzilla.mozilla.org/show_bug.cgi?id=1607639), closing this issue.