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

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

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.

Comment 1

[Victor Porof [:vporof][:vp]]

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.

Comment 2

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: bundle.js

Comment 3

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: index.html

Comment 4

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

I have uploaded files to reproduce the case without having to build anything

Comment 5

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

I should have mentioned that issue only happens on some files, like `application.js` while it is not for other files.

Comment 6

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

looks like ton of ppl reported this issue in browserify issue tacker:

https://github.com/substack/node-browserify/issues/681

Comment 7

[Jordan Santell [:jsantell] [@jsantell]]

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.

Comment 8

[James Long (:jlongster)]

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.

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I just tried yesterday morning with Nightly and latest browserify, and I could not reproduce any failures.

Comment 10

[Jordan Santell [:jsantell] [@jsantell]]

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!)

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Woops didn't mean to clear that ni yet.

Comment 12

[James Long (:jlongster)]

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.

Comment 13

[James Long (:jlongster)]

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.

Comment 14

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I have a fix.

Comment 15

[James Long (:jlongster)]

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.

Comment 16

[James Long (:jlongster)]

(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 17

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 0: Upgrade the tree's copy of the mozilla/source-map library

Comment 18

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 1: Do not set the sourceRoot of a source map that has all source contents embedded

Comment 19

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 2: Add a test that we can load the contents of every source mapped source produced by babel and browserify

Comment 20

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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.

Comment 21

[James Long (:jlongster)]

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?

Comment 22

[James Long (:jlongster)]

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!

Comment 23

[James Long (:jlongster)]

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.

Comment 24

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(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!

Comment 25

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(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

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5047dd91ab56
https://hg.mozilla.org/integration/fx-team/rev/70d1222d4c9c
https://hg.mozilla.org/integration/fx-team/rev/171d7785a4b7

Comment 27

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

ni? me to request uplift in a couple days

Comment 28

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/5722a26a034e for https://treeherder.mozilla.org/logviewer.html#?job_id=3674048&repo=fx-team etc.

Comment 29

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push with fix for test_sourcemaps-06.js: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ed929b14d6e

Comment 30

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/b04ae81a481b
https://hg.mozilla.org/integration/fx-team/rev/fb504f7f8a46
https://hg.mozilla.org/integration/fx-team/rev/dc4eb26de24e

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b04ae81a481b
https://hg.mozilla.org/mozilla-central/rev/fb504f7f8a46
https://hg.mozilla.org/mozilla-central/rev/dc4eb26de24e

Comment 32

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

[Tracking Requested - why for this release]:

The debugger does not work with JS built with very popular build tools.

Comment 33

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 2: Add a test that we can load the contents of every source mapped source produced by babel and browserify

Comment 34

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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)

Comment 35

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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.

Comment 36

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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

Comment 37

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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

Comment 38

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

No need for checkin-needed on uplifts, we have different bug queries that'll find them :)

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/49900e53b9d4
https://hg.mozilla.org/releases/mozilla-aurora/rev/fefb16dee16d
https://hg.mozilla.org/releases/mozilla-aurora/rev/e6506f309195

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/357649efb208
https://hg.mozilla.org/releases/mozilla-beta/rev/7c965ad0f7e7
https://hg.mozilla.org/releases/mozilla-beta/rev/c423b47c384b

Comment 42

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

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.

Comment 43

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I think its fine.

Comment 44

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

(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.