935366 - Unify SourceMap.jsm and source-map.js

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

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

We have two copies of the source map lib based on the environment it is needed in. We should eliminate one copy by using the trick that DevToolsUtils.js(m) use.

Comment 1

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

Attachment: bug-935366-source-map-like-devtoolsutils.patch

All debugger server tests pass, but the source map tests themselves depend on requiring sub-modules of the source map lib which is broken with this approach.

Comment 2

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

I'm unassigning myself because I won't be working on this anytime soon.

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: patch v1

This patch removes SourceMap.jsm completely.
Instead, we use the devtools module loader to load source-map.js.

Here is the related PR for upstream changes:
  https://github.com/mozilla/source-map/pull/198

In Utils.jsm, I'm not using Require.jsm anymore, nor am I using devtools module loader,
that, to allow source-map *and* tests to run in the same scope and share the same define/require
definition!

Also, I don't understand why I get various random changes in source-map.js
as I used the 0.4.4 tag and it seems to be the last used update from bug 1182278.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6648ca6c0af5

Comment 4

[Alexandre Poirot [:ochameau]]

Note that, it should ease implementing bug 888524, but my priority here is to just drop the JSM to ease hacking on devtools codebase.

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: Remove unused Require.jsm

SourceMap seems to be the last one to use Require.jsm, let's remove it!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba6c5b598475

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: patch v2

Fixed obvious failure.
But I still have test timeout without any error on browser/devtools/debugger/test/browser_dbg_auto-pretty-print-01.js.

Comment 7

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d544506c167f

Comment 8

[Alexandre Poirot [:ochameau]]

Attachment: patch v3

https://treeherder.mozilla.org/#/jobs?repo=try&revision=56770ce87784

Comment 9

[Alexandre Poirot [:ochameau]]

Attachment: patch v4

After various iterations, here is a patch that should have a green try:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf0f1866c8b2
  (I'm expecting this one to fail on xpcshell, it was with a previous iteration)
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=1bdffa5a19eb
  (But I just pushed this patch just for xpcshell here)

Comment 10

[Alexandre Poirot [:ochameau]]

Attachment: patch v5

Finally got a green try.
I had to change source-map require path in order to support bug 1134628
initiative to have require path matching filesystem layout.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=81c89c3c53c6

Comment 11

[Alexandre Poirot [:ochameau]]

I think this patch is going to fix bug 965856 as well.

Comment 12

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

Comment on attachment 8642397 [details] [diff] [review]
patch v5

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

Looks good, but we need to figure out how to avoid breaking pretty-fast and its tests on node. I think we should make sure that the loader still supports `require("source-map")`.

The alternative would be adding

  var sourceMap = this.sourceMap
    || (function () { try { return require("source-map"); } catch (e) { } }())
    || require("toolkit/devtools/sourcemap/source-map");

Which is a lot more gross IMO.

::: toolkit/devtools/pretty-fast/pretty-fast.js
@@ +17,5 @@
>  }(this, function () {
>    "use strict";
>  
>    var acorn = this.acorn || require("acorn/acorn");
> +  var sourceMap = this.sourceMap || require("devtools/toolkit/sourcemap/source-map");

This will break the node version of pretty-fast and the pretty-fast tests. This line needs to remain `require("source-map")` for those to work.

::: toolkit/devtools/pretty-fast/tests/unit/test.js
@@ +524,5 @@
>    }
>  
>  ];
>  
> +var sourceMap = this.sourceMap || require("devtools/toolkit/sourcemap/source-map");

Ditto

::: toolkit/devtools/sourcemap/UPGRADING.md
@@ +12,1 @@
>      $ cp dist/test/* /path/to/mozilla-central/toolkit/devtools/sourcemap/tests/unit/

While you're here, can you turn this comment into a shell script so we can do

  $ ./upgrade-source-map.sh

Comment 13

[Alexandre Poirot [:ochameau]]

Attachment: patch v6

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c88aab4eb9d2

Ok, so I kept require("source-map"), for now.
We will most likely change that in bug 1134628
in order to make require path more explicit, by mapping filesystem layout
(and make it easier to reload tools from source directory, without build system).

Comment 14

[Alexandre Poirot [:ochameau]]

Comment on attachment 8643952 [details] [diff] [review]
patch v6

(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #12)
> Comment on attachment 8642397 [details] [diff] [review]
> ::: toolkit/devtools/sourcemap/UPGRADING.md
> @@ +12,1 @@
> >      $ cp dist/test/* /path/to/mozilla-central/toolkit/devtools/sourcemap/tests/unit/
> 
> While you're here, can you turn this comment into a shell script so we can do
> 
>   $ ./upgrade-source-map.sh

Writting such shell script would require many parameters:
git revision, path to gecko, figure out if it is better to use npm run-script or nodejs. I think it is already handy to have these instructions and let you debug each line which can easily fail for various reasons.
It would be more reasonable to offer such script if we get rid of the build/npm/node step.

Comment 15

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

(In reply to Alexandre Poirot [:ochameau] from comment #13)
> Ok, so I kept require("source-map"), for now.
> We will most likely change that in bug 1134628
> in order to make require path more explicit, by mapping filesystem layout
> (and make it easier to reload tools from source directory, without build
> system).

I don't think we want this to change because third party libs and our own libs that also live outside of m-c expect to be able to `require("source-map")`. Maintaining upstream patches is not fun.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/a926fd3044b7
https://hg.mozilla.org/integration/fx-team/rev/a0d4ce93ef07

Comment 17

[Alexandre Poirot [:ochameau]]

Attachment: Fix b2g scope issue

Comment 18

[Alexandre Poirot [:ochameau]]

Comment on attachment 8644435 [details] [diff] [review]
Fix b2g scope issue

Nick, there is a failure on b2g, typical issue with scope on b2g where we have to bind exported symbols on `this`.

Here is the failure:
  https://treeherder.mozilla.org/logviewer.html#?job_id=4108119&repo=fx-team

As I wasn't sure if that was also failure for loadSubScript, I also added some fixes in source-map.js.

Comment 19

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

Comment on attachment 8644435 [details] [diff] [review]
Fix b2g scope issue

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

r=me with corresponding PR to upstream mozilla/source-map

Comment 20

[Alexandre Poirot [:ochameau]]

Related github PR: https://github.com/mozilla/source-map/pull/201

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/13cfa6b095a0

Comment 22

[Alexandre Poirot [:ochameau]]

I have to headout, if this hotfix wasn't enough, please backout all patches of this bug as well as bug 1188401 that will revert-conflict on top of this bug.

Here is the try run for the hotfix:
https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=13cfa6b095a0

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a926fd3044b7
https://hg.mozilla.org/mozilla-central/rev/a0d4ce93ef07
https://hg.mozilla.org/mozilla-central/rev/13cfa6b095a0