Closed Bug 849069 Opened 7 years ago Closed 7 years ago

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

Categories

(DevTools :: Debugger, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 23

People

(Reporter: fitzgen, Assigned: fitzgen)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

Attached patch v1 (obsolete) — Splinter Review
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!
Attachment #730378 - Flags: review?(past)
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.
Attachment #730378 - Flags: review?(past)
(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.
Attached patch v2 (obsolete) — Splinter Review
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.
Attachment #730378 - Attachment is obsolete: true
Attachment #732194 - Flags: review?(past)
Comment on attachment 732194 [details] [diff] [review]
v2

Clearing review until the oranges are dealt with.
Attachment #732194 - Flags: review?(past)
Attached patch V3 (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=d0b7589b17e0

Rebased on the new patch for bug 772119
Attachment #732194 - Attachment is obsolete: true
Attachment #734104 - Flags: review?(past)
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.
Attachment #734104 - Flags: review?(past) → review+
Attached patch v3.1Splinter Review
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
Attachment #734104 - Attachment is obsolete: true
Whiteboard: [land-in-fx-team]
> * 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.
Backed out on suspicion of mochitest-chrome leaks:
https://hg.mozilla.org/integration/fx-team/rev/4ca11b942d5a
Whiteboard: [fixed-in-fx-team]
Relanded:
https://hg.mozilla.org/integration/fx-team/rev/4572a726100e
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/4572a726100e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 23
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.