Closed Bug 1166048 Opened 9 years ago Closed 9 years ago

error loading source w/ source maps emitted by old browserify versions

Categories

(DevTools :: Debugger, defect)

defect
Not set
normal

Tracking

(firefox39 unaffected, firefox40 fixed, firefox41 fixed, firefox42 fixed)

RESOLVED FIXED
Firefox 42
Tracking Status
firefox39 --- unaffected
firefox40 --- fixed
firefox41 --- fixed
firefox42 --- fixed

People

(Reporter: irakli, Assigned: fitzgen)

References

Details

(Keywords: regression)

Attachments

(5 files, 1 obsolete file)

I've being trying to use https://github.com/milankinen/livereactload to hot reload react components but with browserify toolchain and noticed that it has issues with only firefox. Not sure if it's invalid source map emitted or if it's bug in our tools but given it's not working only in Firefox it's worth looking into.

To reproduce try any of the provided example from the above repo with a small modification to generate source maps.

Only change required is with `dev` script https://github.com/milankinen/livereactload/blob/master/examples/01-basic-usage/scripts/dev#L4 to make marked line be this instead:

`node_modules/.bin/watchify site.js -v -d -t reactify -g livereactload -o static/bundle.js &`


Change is added `-d` option after `-v` that tells browserify to generate source maps.
I have seen this exact thing as well. Fixed it by modifying the relative paths. However, the fact that this didn't work out of the box was a bit surprising.
I have uploaded files to reproduce the case without having to build anything
I should have mentioned that issue only happens on some files, like `application.js` while it is not for other files.
looks like ton of ppl reported this issue in browserify issue tacker:

https://github.com/substack/node-browserify/issues/681
Running into this as well -- and looks like the browserify issues is closed, saying it's a bug in FF -- is this an issue with the spec at this point that we don't support the edge case that browserify generates? Nick, James, can either of you point me in the right direction for this? This is kinda bad.
Flags: needinfo?(nfitzgerald)
Flags: needinfo?(jlong)
I tried to fix it before but it wasn't a quick fix. The way we track URls and resolve them needs to be reworked (to handle invalid mappings, sigh). It's possible, just fell off the radar. I can re-prioritize it though.
I just tried yesterday morning with Nightly and latest browserify, and I could not reproduce any failures.
Flags: needinfo?(nfitzgerald)
Error I was seeing looks like it has to do with a babel transform with browserify, causing all non node_modules to display as "Error loading this URL: http://localhost/..."

Recreation steps: https://github.com/jsantell/source-map-test/tree/master/babel-to-browserify
And run index.html

Not sure if this is an error in our implementation. Chrome handles this via displaying source maps for node_module files, and the non-node-modules are just displayed as the generated source (which duplicates the node_modules source maps content, but atleast I can see the source!)
Flags: needinfo?(nfitzgerald)
Flags: needinfo?(nfitzgerald)
Summary: source maps emitted by browserify seem to not load only in firefox → error loading source w/ source maps emitted by old browserify versions
Woops didn't mean to clear that ni yet.
Flags: needinfo?(nfitzgerald)
jsantell, can you paste the browserify command you used to build? I ran with just `browserify -d index.js | less > build/index.js` and couldn't reproduce (which is strange, the raw sourcemap they made used to not work, I guess they fixed that).

You mentioned above using the babel loader doesn't work, but I don't know enough about browserify to know how to run that.
Flags: needinfo?(jlong)
Oh, ok that wasn't too hard:

browserify -d -t babelify index.js | less > build/index.js

I see the problem, I'll take a look.
I have a fix.
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED
Flags: needinfo?(nfitzgerald)
I figured out the problem. I don't know what the best fix is, but there are a few options.

In TabSources, after we fetch a sourcemap we set the sourceRoot of the map: https://github.com/mozilla/gecko-dev/blob/master/toolkit/devtools/server/actors/utils/TabSources.js#L436. We do this so that from now on we can expect to just get working absolute URLs for everything, so we can easily just fetch the source using the URL.

If I load the JS from "file:///Users/james/tmp/source-map-test/babel-to-browserify/build/index.js", the sourceRoot gets set to "file:///Users/james/tmp/source-map-test/babel-to-browserify/build".

Now, in the sourcemap library, when we get a list of sources, it resolves each URL to an absolute one based off of sourceRoot: https://github.com/mozilla/source-map/blob/master/lib/source-map/source-map-consumer.js#L842-L852. So an absolute URL like "/Users/james/lib/add.js" turns into "file:///Users/james/lib/add.js", because it's an absolute URL it overrides all of the sourceRoot, but gets file:// prepended.

We use this absolutized sources array throughout TabSources, and that's what we call `sourceContentFor` with: https://github.com/mozilla/gecko-dev/blob/master/toolkit/devtools/server/actors/script.js#L2284

However, `sourceContentFor` only checks `this._sources`, which is the *unresolved* relative paths: https://github.com/mozilla/source-map/blob/master/lib/source-map/source-map-consumer.js#L634

Note that even if we didn't absolutize all the paths in the `sources` getter, `sourceContentFor` would still absolutize it based on the sourceRoot: https://github.com/mozilla/source-map/blob/master/lib/source-map/source-map-consumer.js#L630

So it depends on how you deal with URLs. Are they the raw URL in the sourcemap, or fully resolved absolute ones? The latter makes it easier to interact with a sourcemap, so we should do that. However, if that's what comes out of a sourcemap, it seems like that's what we should expect to *receive* in the sourcemap, i.e. is there any reason we keep `this._sources` around at all? Why not resolve all the source URLs and use that everywhere?

Seems like at least `sourceContentFor` should consult `this.sources` instead of `this._sources` at least. Not sure about other methods.

I'm sure Nick has better thoughts on a solution.
(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #14)
> I have a fix.

Awh man, I just spent time explaining the entire thing, then I collided with your simple comment :p ok cool!
Comment on attachment 8630661 [details] [diff] [review]
Part 0: Upgrade the tree's copy of the mozilla/source-map library

This just needs a rubber stamp. All the changes in here have been r+'d already when merged into the mozilla/source-map library and this just updates our tree's copy.
Attachment #8630661 - Flags: review?(jlong)
Attachment #8630662 - Flags: review?(jlong)
Attachment #8630663 - Flags: review?(jlong)
Comment on attachment 8630661 [details] [diff] [review]
Part 0: Upgrade the tree's copy of the mozilla/source-map library

Review of attachment 8630661 [details] [diff] [review]:
-----------------------------------------------------------------

I don't have a context for some of these changes, but I'm going to assume upgrading the lib is safe because these have already gone through a PR.

::: toolkit/devtools/sourcemap/tests/unit/Utils.jsm
@@ +581,5 @@
>        // If the only part of the root that is left is the scheme (i.e. http://,
>        // file:///, etc.), one or more slashes (/), or simply nothing at all, we
>        // have exhausted all components, so the path is not relative to the root.
>        aRoot = aRoot.slice(0, index);
> +      if (aRoot.match(/^([^\/]+:\/)?\/*$/)) {

I'm assuming this was from a previous change in the sourcemap library? This wasn't in your PR. Fine with me, just making sure.

@@ +622,5 @@
>     * stubbed out mapping.
>     */
>    function compareByOriginalPositions(mappingA, mappingB, onlyCompareOriginal) {
> +    var cmp = mappingA.source - mappingB.source;
> +    if (cmp !== 0) {

Are these changes your jit optimizations, with the 50-75% speed increase?
Attachment #8630661 - Flags: review?(jlong) → review+
Comment on attachment 8630662 [details] [diff] [review]
Part 1: Do not set the sourceRoot of a source map that has all source contents embedded

Review of attachment 8630662 [details] [diff] [review]:
-----------------------------------------------------------------

yay!
Attachment #8630662 - Flags: review?(jlong) → review+
Comment on attachment 8630663 [details] [diff] [review]
Part 2: Add a test that we can load the contents of every source mapped source produced by babel and browserify

Review of attachment 8630663 [details] [diff] [review]:
-----------------------------------------------------------------

Sweet! just a few nits

::: toolkit/devtools/server/tests/unit/babel_and_browserify_script_with_source_map.js
@@ +75,5 @@
> +  return str.toUpperCase()
> +}
> +
> +},{}]},{},[1])
> +//# sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW9uIjozLCJzb3VyY2VzIjpbIi4uLy4uLy4uLy4uL3Vzci9sb2NhbC9saWIvbm9kZV9tb2R1bGVzL2Jyb3dzZXJpZnkvbm9kZV9tb2R1bGVzL2Jyb3dzZXItcGFjay9fcHJlbHVkZS5qcyIsIi9Vc2Vycy9qc2FudGVsbC9EZXYvc291cmNlLW1hcC10ZXN0L2luZGV4LmpzIiwiL1VzZXJzL2pzYW50ZWxsL0Rldi9zb3VyY2UtbWFwLXRlc3QvbGliL2FkZC5qcyIsIi9Vc2Vycy9qc2FudGVsbC9EZXYvc291cmNlLW1hcC10ZXN0L2xpYi9zdWJ0cmFjdC5qcyIsIm5vZGVfbW9kdWxlcy91cHBlci1jYXNlL3VwcGVyLWNhc2UuanMiXSwibmFtZXMiOltdLCJtYXBwaW5ncyI6IkFBQUE7OztBQ0FBLElBQUksR0FBRyxHQUFHLE9BQU8sQ0FBQyxXQUFXLENBQUMsQ0FBQztBQUMvQixJQUFJLFFBQVEsR0FBRyxPQUFPLENBQUMsZ0JBQWdCLENBQUMsQ0FBQztBQUN6QyxJQUFJLFNBQVMsR0FBRyxPQUFPLENBQUMsWUFBWSxDQUFDLENBQUM7O0FBRXRDLENBQUM7U0FBTSxHQUFHLENBQUMsQ0FBQyxFQUFDLENBQUMsQ0FBQztFQUFBLENBQUU7Ozs7O0FDSmpCLE1BQU0sQ0FBQyxPQUFPLEdBQUcsVUFBVSxDQUFDLEVBQUUsQ0FBQyxFQUFFO0FBQy9CLFNBQU8sQ0FBQyxHQUFHLENBQUMsQ0FBQztDQUNkLENBQUM7Ozs7O0FDRkYsTUFBTSxDQUFDLE9BQU8sR0FBRyxVQUFVLENBQUMsRUFBRSxDQUFDLEVBQUU7QUFDL0IsU0FBTyxDQUFDLEdBQUcsQ0FBQyxDQUFDO0NBQ2QsQ0FBQzs7O0FDRkY7QUFDQTtBQUNBO0FBQ0E7QUFDQTtBQUNBO0FBQ0E7QUFDQTtBQUNBO0FBQ0E7QUFDQTtBQUNBO0FBQ0E7QUFDQTtBQUNBO0FBQ0E7QUFDQTtBQUNBO0FBQ0E7QUFDQTtBQUNBO0FBQ0E7QUFDQTtBQUNBO0FBQ0E7QUFDQTtBQUNBO0FBQ0E7QUFDQTtBQUNBO0FBQ0E7QUFDQTtBQUNBO0FBQ0E7QUFDQTtBQUNBO0FBQ0E7QUFDQTtBQUNBO0FBQ0E7QUFDQTtBQUNBO0FBQ0E7QUFDQTtBQUNBO0FBQ0E7QUFDQTtBQUNBO0FBQ0E7QUFDQTtBQUNBIiwiZmlsZSI6ImdlbmVyYXRlZC5qcyIsInNvdXJjZVJvb3QiOiIiLCJzb3VyY2VzQ29udGVudCI6WyIoZnVuY3Rpb24gZSh0LG4scil7ZnVuY3Rpb24gcyhvLHUpe2lmKCFuW29dKXtpZighdFtvXSl7dmFyIGE9dHlwZW9mIHJlcXVpcmU9PVwiZnVuY3Rpb25cIiYmcmVxdWlyZTtpZighdSYmYSlyZXR1cm4gYShvLCEwKTtpZihpKXJldHVybiBpKG8sITApO3ZhciBmPW5ldyBFcnJvcihcIkNhbm5vdCBmaW5kIG1vZHVsZSAnXCIrbytcIidcIik7dGhyb3cgZi5jb2RlPVwiTU9EVUxFX05PVF9GT1VORFwiLGZ9dmFyIGw9bltvXT17ZXhwb3J0czp7fX07dFtvXVswXS5jYWxsKGwuZXhwb3J0cyxmdW5jdGlvbihlKXt2YXIgbj10W29dWzFdW2VdO3JldHVybiBzKG4/bjplKX0sbCxsLmV4cG9ydHMsZSx0LG4scil9cmV0dXJuIG5bb10uZXhwb3J0c312YXIgaT10eXBlb2YgcmVxdWlyZT09XCJmdW5jdGlvblwiJiZyZXF1aXJlO2Zvcih2YXIgbz0wO288ci5sZW5ndGg7bysrKXMocltvXSk7cmV0dXJuIHN9KSIsInZhciBhZGQgPSByZXF1aXJlKFwiLi9saWIvYWRkXCIpO1xudmFyIHN1YnRyYWN0ID0gcmVxdWlyZShcIi4vbGliL3N1YnRyYWN0XCIpO1xudmFyIHVwcGVyQ2FzZSA9IHJlcXVpcmUoXCJ1cHBlci1jYXNlXCIpO1xuXG4oKCkgPT4gYWRkKDEsMikpO1xuIiwibW9kdWxlLmV4cG9ydHMgPSBmdW5jdGlvbiAoYSwgYikge1xuICByZXR1cm4gYSArIGI7XG59O1xuIiwibW9kdWxlLmV4cG9ydHMgPSBmdW5jdGlvbiAoYSwgYikge1xuICByZXR1cm4gYSAtIGI7XG59O1xuIiwiLyoqXG4gKiBTcGVjaWFsIGxhbmd1YWdlLXNwZWNpZmljIG92ZXJyaWRlcy5cbiAqXG4gKiBTb3VyY2U6IGZ0cDovL2Z0cC51bmljb2RlLm9yZy9QdWJsaWMvVUNEL2xhdGVzdC91Y2QvU3BlY2lhbENhc2luZy50eHRcbiAqXG4gKiBAdHlwZSB7T2JqZWN0fVxuICovXG52YXIgTEFOR1VBR0VTID0ge1xuICB0cjoge1xuICAgIHJlZ2V4cDogL1tcXHUwMDY5XS9nLFxuICAgIG1hcDoge1xuICAgICAgJ1xcdTAwNjknOiAnXFx1MDEzMCdcbiAgICB9XG4gIH0sXG4gIGF6OiB7XG4gICAgcmVnZXhwOiAvW1xcdTAwNjldL2csXG4gICAgbWFwOiB7XG4gICAgICAnXFx1MDA2OSc6ICdcXHUwMTMwJ1xuICAgIH1cbiAgfSxcbiAgbHQ6IHtcbiAgICByZWdleHA6IC9bXFx1MDA2OVxcdTAwNkFcXHUwMTJGXVxcdTAzMDd8XFx1MDA2OVxcdTAzMDdbXFx1MDMwMFxcdTAzMDFcXHUwMzAzXS9nLFxuICAgIG1hcDoge1xuICAgICAgJ1xcdTAwNjlcXHUwMzA3JzogJ1xcdTAwNDknLFxuICAgICAgJ1xcdTAwNkFcXHUwMzA3JzogJ1xcdTAwNEEnLFxuICAgICAgJ1xcdTAxMkZcXHUwMzA3JzogJ1xcdTAxMkUnLFxuICAgICAgJ1xcdTAwNjlcXHUwMzA3XFx1MDMwMCc6ICdcXHUwMENDJyxcbiAgICAgICdcXHUwMDY5XFx1MDMwN1xcdTAzMDEnOiAnXFx1MDBDRCcsXG4gICAgICAnXFx1MDA2OVxcdTAzMDdcXHUwMzAzJzogJ1xcdTAxMjgnXG4gICAgfVxuICB9XG59XG5cbi8qKlxuICogVXBwZXIgY2FzZSBhIHN0cmluZy5cbiAqXG4gKiBAcGFyYW0gIHtTdHJpbmd9IHN0clxuICogQHJldHVybiB7U3RyaW5nfVxuICovXG5tb2R1bGUuZXhwb3J0cyA9IGZ1bmN0aW9uIChzdHIsIGxvY2FsZSkge1xuICB2YXIgbGFuZyA9IExBTkdVQUdFU1tsb2NhbGVdXG5cbiAgc3RyID0gc3RyID09IG51bGwgPyAnJyA6IFN0cmluZyhzdHIpXG5cbiAgaWYgKGxhbmcpIHtcbiAgICBzdHIgPSBzdHIucmVwbGFjZShsYW5nLnJlZ2V4cCwgZnVuY3Rpb24gKG0pIHsgcmV0dXJuIGxhbmcubWFwW21dIH0pXG4gIH1cblxuICByZXR1cm4gc3RyLnRvVXBwZXJDYXNlKClcbn1cbiJdfQ==

It might be nice to extract this out to a normal sourcemap when we want to look at it in the future. I did this with the `exorcist` transform. Files are here if you want to just grab them: http://jlongster.com/s/1166048/

::: toolkit/devtools/server/tests/unit/test_sourcemaps-16.js
@@ +19,5 @@
> +  });
> +  do_test_pending();
> +}
> +
> +const testSourcemap = Task.async(function* (threadResponse, tabClient, threadClient, tabResponse) {

nit: what's our coding style for this? most places I thought didn't put a space after `function*`. I don't really care for a test though.
Attachment #8630663 - Flags: review?(jlong) → review+
(In reply to James Long (:jlongster) from comment #21)
> Comment on attachment 8630661 [details] [diff] [review]
> Part 0: Upgrade the tree's copy of the mozilla/source-map library
> 
> Review of attachment 8630661 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't have a context for some of these changes, but I'm going to assume
> upgrading the lib is safe because these have already gone through a PR.

Yeah Eddy has already reviewed all this other stuff, I just didn't quite feel like doing r=me on something we will likely uplift, so I asked you for the rubber stamp :) Thanks!
(In reply to James Long (:jlongster) from comment #23)
> It might be nice to extract this out to a normal sourcemap when we want to
> look at it in the future. I did this with the `exorcist` transform. Files
> are here if you want to just grab them: http://jlongster.com/s/1166048/

I'd prefer to keep the test case as similar as possible to how browserify and babel do it in the wild.

> ::: toolkit/devtools/server/tests/unit/test_sourcemaps-16.js
> @@ +19,5 @@
> > +  });
> > +  do_test_pending();
> > +}
> > +
> > +const testSourcemap = Task.async(function* (threadResponse, tabClient, threadClient, tabResponse) {
> 
> nit: what's our coding style for this? most places I thought didn't put a
> space after `function*`. I don't really care for a test though.

Seems my preferred style is in the minority:

  fitzgen@nfitzgerald-22150 :: (bug-1166048-babel-browserify-source-maps) :: ~/src/mozilla-central-opt
      $ git grep "function\\* (" toolkit/devtools/ browser/devtools/ | wc -l
      324

  fitzgen@nfitzgerald-22150 :: (bug-1166048-babel-browserify-source-maps) :: ~/src/mozilla-central-opt
      $ git grep "function\\*(" toolkit/devtools/ browser/devtools/ | wc -l
      1175
ni? me to request uplift in a couple days
Flags: needinfo?(nfitzgerald)
Try push with fix for test_sourcemaps-06.js: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ed929b14d6e
Flags: needinfo?(nfitzgerald)
Flags: needinfo?(nfitzgerald)
Depends on: 1182278
[Tracking Requested - why for this release]:

The debugger does not work with JS built with very popular build tools.
Flags: needinfo?(nfitzgerald)
Blocks: 1182278
No longer depends on: 1182278
Comment on attachment 8633733 [details] [diff] [review]
Part 2: Add a test that we can load the contents of every source mapped source produced by babel and browserify

(This is the version of the patch that landed with the fix for the try failure from earlier versions)
Attachment #8633733 - Flags: review+
Comment on attachment 8630661 [details] [diff] [review]
Part 0: Upgrade the tree's copy of the mozilla/source-map library

(Just going to do one approval request comment for all three parts.)

---------------------

Approval Request Comment
[Feature/regressing bug #]:

Unknown

[User impact if declined]: 

Debugger does not work with JS built with very popular build tools / "transpilers".

[Describe test coverage new/current, TreeHerder]:

New tests to cover this bug. Its been working on m-c for a few days. There are many npm modules that also use this library, and they did find two minor regressions that the debugger doesn't actually trigger in bug 1182278. However, I take that as a sign of assurance because there are *many* downstream npm modules dependent on this library and they found the bugs *very* quickly and haven't found any others since then.

[Risks and why]:

Risks are that we are introducing new bugs in source map handling, but all our existing tests are green, things seem fine on nightly for the last few days, and npm dependent modules quickly shook out minor regressions, which we have fixes for in bug 1182278.

[String/UUID change made/needed]:

None.
Attachment #8630661 - Flags: approval-mozilla-beta?
Attachment #8630661 - Flags: approval-mozilla-aurora?
Comment on attachment 8630662 [details] [diff] [review]
Part 1: Do not set the sourceRoot of a source map that has all source contents embedded

See comment 35
Attachment #8630662 - Flags: approval-mozilla-beta?
Attachment #8630662 - Flags: approval-mozilla-aurora?
Comment on attachment 8633733 [details] [diff] [review]
Part 2: Add a test that we can load the contents of every source mapped source produced by babel and browserify

See comment 35
Attachment #8633733 - Flags: approval-mozilla-beta?
Attachment #8633733 - Flags: approval-mozilla-aurora?
Comment on attachment 8630661 [details] [diff] [review]
Part 0: Upgrade the tree's copy of the mozilla/source-map library

Recent regression, we still have time to fix potential fallout, taking it to aurora & beta.
Attachment #8630661 - Flags: approval-mozilla-beta?
Attachment #8630661 - Flags: approval-mozilla-beta+
Attachment #8630661 - Flags: approval-mozilla-aurora?
Attachment #8630661 - Flags: approval-mozilla-aurora+
Attachment #8630662 - Flags: approval-mozilla-beta?
Attachment #8630662 - Flags: approval-mozilla-beta+
Attachment #8630662 - Flags: approval-mozilla-aurora?
Attachment #8630662 - Flags: approval-mozilla-aurora+
Attachment #8633733 - Flags: approval-mozilla-beta?
Attachment #8633733 - Flags: approval-mozilla-beta+
Attachment #8633733 - Flags: approval-mozilla-aurora?
Attachment #8633733 - Flags: approval-mozilla-aurora+
Flags: qe-verify+
No need for checkin-needed on uplifts, we have different bug queries that'll find them :)
Keywords: checkin-needed
I see Sylvestre marked this as needing manual verification (qe-verify+), but I see comment 35 states: "New tests to cover this bug. Its been working on m-c for a few days. There are many npm modules that also use this library...".

Nick, do you think this still needs manual verification? I'd ask Sylvestre to clarify, but he's on vacation.
Flags: needinfo?(nfitzgerald)
I think its fine.
Flags: needinfo?(nfitzgerald)
(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #43)
> I think its fine.

Thanks Nick, I'm removing the qe-verify flag. Sylvestre, feel free to add it back if you indeed think manual verification is needed here.
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.