897777 - fix source mapped source resolution when there is no source root

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

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

When there is no source root, and the sources are relative, they should be resolved from the generated script's url.

Comment 1

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

Attachment: v1

Haha, you shouldn't have told me that your review queue was empty >:)

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

Comment 2

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

(In reply to Nick Fitzgerald [:fitzgen] from comment #0)
> When there is no source root, and the sources are relative, they should be
> resolved from the generated script's url.

One more thing to add to this conditional: the source map has to be data URI embedded (or else we resolve them relative to the source map url).

Comment 3

[Jim Blandy :jimb]

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

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

Looks fine.

I see that the test suite does cover sourcemaps supplied via data: URLs; why didn't they catch this bug? Do they catch it now? If not, let's add tests that can tell whether this patch is landed or not.

::: toolkit/devtools/server/actors/script.js
@@ +3202,5 @@
>    },
>  
> +  _dirname: function TS__dirname(aPath) {
> +    return aPath.replace(/\/[^\/]+$/, "/");
> +  },

I see that this is just abstracting out code that already existed, but surely there is some appropriate chrome utility function for this. There's nothing on Services.io? We shouldn't be duplicating library functions without a good reason.

@@ +3208,4 @@
>    /**
>     * Return a promise of a SourceMapConsumer for the source map located at
>     * |aAbsSourceMapURL|, which must be absolute. If there is already such a
>     * promise extant, return it.

The comment should describe aScriptURL; this might be the place for the explanation you wrote in the bug's description.

@@ +3228,5 @@
> +   * Sets the source map's sourceRoot to be relative to the source map url.
> +   */
> +  _setSourceMapRoot: function TS__setSourceMapRoot(aSourceMap, aAbsSourceMapURL, aScriptURL) {
> +    let base = this._dirname(aAbsSourceMapURL);
> +    if (base.indexOf("data:") === 0) {

_dirname presumably won't change the URI scheme; could we avoid calling _dirname on aAbsSourceMapURL at all when it's 'data:'?

Comment 4

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

(In reply to Jim Blandy :jimb from comment #3)
> Comment on attachment 780734 [details] [diff] [review]
> v1
> 
> Review of attachment 780734 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks fine.
> 
> I see that the test suite does cover sourcemaps supplied via data: URLs; why
> didn't they catch this bug? Do they catch it now? If not, let's add tests
> that can tell whether this patch is landed or not.
> 

They were hard coded to work around this bug (>_<), hence the couple tests that went from foo.js to http://example.com/foo.js in their assertions. That is why they weren't throwing. They will catch any regressions after this patch lands, though.

> ::: toolkit/devtools/server/actors/script.js
> @@ +3202,5 @@
> >    },
> >  
> > +  _dirname: function TS__dirname(aPath) {
> > +    return aPath.replace(/\/[^\/]+$/, "/");
> > +  },
> 
> I see that this is just abstracting out code that already existed, but
> surely there is some appropriate chrome utility function for this. There's
> nothing on Services.io? We shouldn't be duplicating library functions
> without a good reason.

After investigating a bit, I think we can do

  return Services.io.newURI(".", null, aPath).spec;

> 
> @@ +3208,4 @@
> >    /**
> >     * Return a promise of a SourceMapConsumer for the source map located at
> >     * |aAbsSourceMapURL|, which must be absolute. If there is already such a
> >     * promise extant, return it.
> 
> The comment should describe aScriptURL; this might be the place for the
> explanation you wrote in the bug's description.

Will do!

> 
> @@ +3228,5 @@
> > +   * Sets the source map's sourceRoot to be relative to the source map url.
> > +   */
> > +  _setSourceMapRoot: function TS__setSourceMapRoot(aSourceMap, aAbsSourceMapURL, aScriptURL) {
> > +    let base = this._dirname(aAbsSourceMapURL);
> > +    if (base.indexOf("data:") === 0) {
> 
> _dirname presumably won't change the URI scheme; could we avoid calling
> _dirname on aAbsSourceMapURL at all when it's 'data:'?

Good catch!

Do you want me to flag you again, or just land with the updated patch and post it back here for posterity?

Comment 5

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

Attachment: v2

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

Comment 6

[Jim Blandy :jimb]

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

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

Excellent.

Comment 7

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

https://hg.mozilla.org/integration/fx-team/rev/55ac60d61c03

Ok, so apparently I did a merge with fx-team too, even though I hg pull -u'd. I kept trying to strip back to my previous revision and then just pull from fx-team but it doesn't seem to work. If this ends up causing problems I'd really like a tip on how to properly rebase like I can in git. Sorry.

Comment 8

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

(In reply to Nick Fitzgerald [:fitzgen] from comment #7)
> https://hg.mozilla.org/integration/fx-team/rev/55ac60d61c03
> 

Assuming you didn't hg qfinish, you just need to pop all of your queue before pulling and updating. I have a set of useful scripts [0] that help me with this (see hgqpl on line 217), but you probably want this:

1. hg phase --force --draft qbase:tip
2. hg qpop --all
2. hg pull --update

Then restore your queue as ordinary.
Rebase+refresh each patch in the queue if needed.

[0] https://gist.github.com/victorporof/3415836#file-config-fish-sh-L161

Comment 9

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

I did hg qfinish, tried to strip it off, but it didn't seem to work :-/

Comment 10

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

(In reply to Nick Fitzgerald [:fitzgen] from comment #9)
> I did hg qfinish, tried to strip it off, but it didn't seem to work :-/

Well, qfinish is specifically there to prevent you from messing around with mq anymore. When you qfinish, you basically turn your applied patches in mq into regular changesets.

To reimport those changesets as patches in mq, use hg qimport.

Comment 11

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

(tl;dr: always pop all patches before you pull, always qfinish after you pull)

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/aed16c078793
https://hg.mozilla.org/mozilla-central/rev/55ac60d61c03