fix source mapped source resolution when there is no source root

RESOLVED FIXED in Firefox 25

Status

defect
RESOLVED FIXED
6 years ago
Last year

People

(Reporter: fitzgen, Assigned: fitzgen)

Tracking

25 Branch
Firefox 25
x86
macOS
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

When there is no source root, and the sources are relative, they should be resolved from the generated script's url.
Assignee: nobody → nfitzgerald
Posted patch v1 (obsolete) — Splinter Review
Haha, you shouldn't have told me that your review queue was empty >:)

https://tbpl.mozilla.org/?tree=Try&rev=aa8a3764b697
Attachment #780734 - Flags: review?(jimb)
(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

6 years ago
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:'?

Updated

6 years ago
Attachment #780734 - Flags: review?(jimb) → review+
(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 6

6 years ago
Comment on attachment 781952 [details] [diff] [review]
v2

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

Excellent.
Attachment #781952 - Flags: review?(jimb) → review+
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.
Whiteboard: [fixed-in-fx-team]
(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
I did hg qfinish, tried to strip it off, but it didn't seem to work :-/
(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.
(tl;dr: always pop all patches before you pull, always qfinish after you pull)
https://hg.mozilla.org/mozilla-central/rev/aed16c078793
https://hg.mozilla.org/mozilla-central/rev/55ac60d61c03
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 25

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.