849069 - relative source map URLs should be resolved according to the spec's rules

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

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

The rules for relative source map URL resolution are defined here: https://docs.google.com/document/d/1U1RGAehQwRypUTovF1KRlpiOFze0b-_2gc6fAH0KY0k/edit?pli=1#heading=h.lmz475t4mvbx

Comment 1

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

Attachment: v1

https://tbpl.mozilla.org/?tree=Try&rev=fc0d64ce4468

Updating the source map library in bug 772119 fixed the issues I was having with this patch! Woo!

Verified that using coffeescript's defaults results in debuggable coffescript files without needing to play with urls or source roots or anything!

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 730378 [details] [diff] [review]
v1

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

This doesn't seem to work as expected in this test case:

http://astithas.com/test/coffee/binary_search.html

With the patch from bug 849071 I get a 404 for the coffee script file, when I toggle between original and generated sources.

::: toolkit/devtools/debugger/server/dbg-script-actors.js
@@ +506,5 @@
>      return locationPromise.then(function (aLocation) {
>        let line = aLocation.line;
> +      if (this.dbg.findScripts({ url: aLocation.url }).length == 0 ||
> +          line < 0 ||
> +          line === null) {

Why not line == null to catch undefined, too?

@@ +2431,5 @@
>            return {
>              url: source,
>              line: line
>            };
> +        }.bind(this));

This is not necessary, there is no reference to |this| in this function.

Comment 3

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

(In reply to Panos Astithas [:past] from comment #2)
> Comment on attachment 730378 [details] [diff] [review]
> v1
> 
> Review of attachment 730378 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This doesn't seem to work as expected in this test case:
> 
> http://astithas.com/test/coffee/binary_search.html
> 
> With the patch from bug 849071 I get a 404 for the coffee script file, when
> I toggle between original and generated sources.

What is happening is that we are hitting this issue: https://github.com/mozilla/source-map/issues/43

Will fix.

> 
> ::: toolkit/devtools/debugger/server/dbg-script-actors.js
> @@ +506,5 @@
> >      return locationPromise.then(function (aLocation) {
> >        let line = aLocation.line;
> > +      if (this.dbg.findScripts({ url: aLocation.url }).length == 0 ||
> > +          line < 0 ||
> > +          line === null) {
> 
> Why not line == null to catch undefined, too?

The source map lib will always return null, so I thought we could do strict equality, but I now realize that if there isn't a source map, we return what we were given so it could indeed be undefined. Will change.

> 
> @@ +2431,5 @@
> >            return {
> >              url: source,
> >              line: line
> >            };
> > +        }.bind(this));
> 
> This is not necessary, there is no reference to |this| in this function.

Good catch.

Will have a new patch up soon.

Comment 4

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

Attachment: v2

https://tbpl.mozilla.org/?tree=Try&rev=8a5a2640e991

* Updated to the latest mozilla/source-map

  * This fixes the issues with joining relative paths to their root that you were experiencing.

* Added two more xpcshell tests

  * One for making sure absolute source map urls work

  * One for making sure that relative source map urls work

* Added changes suggested in last review.

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 732194 [details] [diff] [review]
v2

Clearing review until the oranges are dealt with.

Comment 6

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

Attachment: V3

https://tbpl.mozilla.org/?tree=Try&rev=d0b7589b17e0

Rebased on the new patch for bug 772119

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 734104 [details] [diff] [review]
V3

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

::: toolkit/devtools/debugger/server/dbg-script-actors.js
@@ +2364,5 @@
>      }
>  
>      return this.sourceMap(aScript)
> +      .then((aSourceMap) => {
> +        return [this.source(s) for (s of aSourceMap.sources)];

You could probably get rid of the braces and the |return| here if you care.

Comment 8

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

Attachment: v3.1

https://tbpl.mozilla.org/?tree=Try&rev=3ddd5d28881d

* Removed the brackets+return that Panos mentioned

* Disabled source map tests that rely on file:// urls in b2g

Comment 9

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

> * Disabled source map tests that rely on file:// urls in b2g

Just want to note that this is what Panos and I decided upon in an irc conversation, for posterity.

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/integration/fx-team/rev/50b1fc3ef560

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

Backed out on suspicion of mochitest-chrome leaks:
https://hg.mozilla.org/integration/fx-team/rev/4ca11b942d5a

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

Relanded:
https://hg.mozilla.org/integration/fx-team/rev/4572a726100e

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/4572a726100e