Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Reporter
	Description • 12 years ago

Right now we only record the source map url if it is found in a comment pragma ("//@sourceMapURL=foo/bar/baz.map") by the parser, however the spec also specifies that you can associate a script with a source map if the script is transferred with the "X-SourceMap" HTTP header.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Reporter
	Comment 1 • 12 years ago

(Henri, I'm requesting feedback from you for the script loader side of things. Jonas Sicking told me you were the one to ask for feedback from, I hope that is alright.)

Here is a patch that works based on my informal testing with GDB and breakpoints for scripts loaded with normal <script> tags, dynamically injected <script> tags, and web workers.

Concerns and questions I have about the patch and my approach:

* How should I write tests for this? Where should I add the tests?

* Should I add a sourceMapURL parameter to all of the JS_Evaluate... functions in jsapi even if they aren't ever called from script loaders (as far as I know from my (admittedly incomplete) gdb testing)?

* There is a whole lot of supplying NULL when there is no source map url available, should the sourceMapURL parameter have a default value of NULL in jsapi?

* As far as order of parameters goes, where should the sourceMapURL be? I just added it to the end of the parameters list whenever I needed to change a function's prototype.

* Right now, you can only call JSScript::setSourceMap once per JSSCript due to JS_ASSERT(!hasSourceMap) in setSourceMap. However, this is going to fail if a script has both the //@sourceMapURL comment pragma and the X-SourceMap header. The spec has no answer for which should be preferred if they do not match (connecting scripts and source maps is outside the domain of the spec, which only defines what a source map is).

* Is there a strlen for jschar*? That would save me a few lines of code (and potential for bugs) in the patch.

* If we fail to successfully call setSourceMap in JSScript::Create, should we fail and return NULL even though the source map isn't really required for the script to be created or run?

* Lastly, when I run the jsapi tests, everything passes; when I run js/src/tests/jstests.py, I get failures due to timeouts. I don't know if this is significant or not:

  [1453|   0| 187]  48% ======================>                         |  441.0s
  TIMEOUT - ecma_5/Object/15.2.3.6-dictionary-redefinition-1-of-8.js
  [1453|   1| 187]  48% ======================>                         |  451.6s
  TIMEOUT - ecma_5/Object/15.2.3.6-dictionary-redefinition-2-of-8.js
  [1453|   2| 187]  48% ======================>                         |  591.0s
  TIMEOUT - ecma_5/Object/15.2.3.6-dictionary-redefinition-3-of-8.js
  [1453|   3| 187]  48% ======================>                         |  601.7s
  TIMEOUT - ecma_5/Object/15.2.3.6-dictionary-redefinition-4-of-8.js
  [1453|   4| 187]  48% ======================>                         |  741.1s
  TIMEOUT - ecma_5/Object/15.2.3.6-dictionary-redefinition-5-of-8.js
  [3166|   5| 191] 100% ===============================================>| 1463.0s
  TIMEOUTS
      ecma_5/Object/15.2.3.6-dictionary-redefinition-1-of-8.js
      ecma_5/Object/15.2.3.6-dictionary-redefinition-2-of-8.js
      ecma_5/Object/15.2.3.6-dictionary-redefinition-3-of-8.js
      ecma_5/Object/15.2.3.6-dictionary-redefinition-4-of-8.js
      ecma_5/Object/15.2.3.6-dictionary-redefinition-5-of-8.js
  FAIL

Thank you both for your feedback,

Nick

	Jim Blandy :jimb
	Comment 2 • 12 years ago

Comment on attachment 639531 [details] [diff] [review]
V1

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

::: js/src/frontend/BytecodeCompiler.h
@@ +19,5 @@
>                bool compileAndGo, bool noScriptRval, bool needScriptGlobal,
>                const jschar *chars, size_t length,
>                const char *filename, unsigned lineno, JSVersion version,
> +              const jschar *sourceMapURL, JSString *source_ = NULL,
> +              unsigned staticLevel = 0);

I think it would be better to give sourceMapURL a default value. Then there'd be no need for the /* sourceMapURL = */ comments on most of the calls. Adding sourceMapURL as the last argument would allow the call in EvalKernel to avoid mentioning it, too.

::: js/src/jsapi.h
@@ +4714,4 @@
>                                   JSPrincipals *principals,
>                                   const jschar *chars, unsigned length,
>                                   const char *filename, unsigned lineno,
> +                                 jsval *rval, const jschar *sourceMapURL);

I kind of think the compilation options are getting out of hand with this family of functions --- and I had more stuff I wanted to add to it yesterday. But that can be a separate bug. Here, I actually do think we should add [deep breath] 

JS_EvaluateUCScriptForPrincipalsVersionOriginSourceMap

instead of adding a parameter to this function.

::: js/src/jsscript.cpp
@@ +845,5 @@
>  JSScript::setSourceMap(JSContext *cx, jschar *sourceMap)
>  {
> +    // TODO: This assertion will fail if we have both a |//@sourceMapURL|
> +    // comment pragma and an |X-SourceMap| HTTP header. Which should take
> +    // precedence?

Either way is probably fine. It might be good to warn if you've got conflicting URLs (i.e. call JS_ReportErrorFlagsAndNumber, with flags including JSREPORT_WARNING); I think that will find its way into the web console, in Firefox.

@@ +1122,5 @@
>      script->staticLevel = uint16_t(staticLevel);
>  
> +    // If there is a source map URL, copy it and then save it via
> +    // |setSourceMap|.
> +    if (sourceMapURL != NULL) {

I'd just say 'if (sourceMapURL)'.

@@ +1123,5 @@
>  
> +    // If there is a source map URL, copy it and then save it via
> +    // |setSourceMap|.
> +    if (sourceMapURL != NULL) {
> +        // TODO: Is there a strlen for jschar *?

Yes, it's called js_strlen. It, and other jschar utilities, are in jsstr.cpp. You could add a js_strdup there instead of writing it out here.

@@ +1129,5 @@
> +        while (sourceMapURL[smlen] != '\0') {
> +            smlen++;
> +        }
> +        if (smlen > 0) {
> +            jschar *smURL = (jschar *) cx->malloc_((smlen + 1) * sizeof(jschar));

You need to check for NULL here; cx->malloc_ returns NULL if it can't get the memory it needs.

@@ +1138,5 @@
> +            smURL[i] = '\0';
> +
> +            if (!script->setSourceMap(cx, smURL)) {
> +                cx->free_(smURL);
> +                // TODO: Should I return NULL here?

Yes, because you were unable to do what the caller requested, and there's no protocol for reporting partial failures.

	Henri Sivonen (:hsivonen)
	Comment 3 • 12 years ago

Comment on attachment 639531 [details] [diff] [review]
V1

(In reply to Nick Fitzgerald :fitzgen from comment #1)
> (Henri, I'm requesting feedback from you for the script loader side of
> things. Jonas Sicking told me you were the one to ask for feedback from, I
> hope that is alright.)

OK.

> * How should I write tests for this?

I gather that we have a chrome-exposed JS API for debugging JS these days. The tests should probably be chrome mochitests that use that API to verify that the correct source mapping has been performed.

> Where should I add the tests?

http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/chrome/ seems like a reasonable place assuming that jseng doesn't already have a place for chrome mochitests testing the debugger API.

> * Should I add a sourceMapURL parameter to all of the JS_Evaluate...
> functions in jsapi even if they aren't ever called from script loaders (as
> far as I know from my (admittedly incomplete) gdb testing)?
> 
> * There is a whole lot of supplying NULL when there is no source map url
> available, should the sourceMapURL parameter have a default value of NULL in
> jsapi?
> 
> * As far as order of parameters goes, where should the sourceMapURL be? I
> just added it to the end of the parameters list whenever I needed to change
> a function's prototype.

I defer to jimb on these questions.

> * Right now, you can only call JSScript::setSourceMap once per JSSCript due
> to JS_ASSERT(!hasSourceMap) in setSourceMap. However, this is going to fail
> if a script has both the //@sourceMapURL comment pragma and the X-SourceMap
> header. The spec has no answer for which should be preferred if they do not
> match (connecting scripts and source maps is outside the domain of the spec,
> which only defines what a source map is).

Please get the spec fixed to have a definite answer.

As for what the answer should be, if Chrome already supports both the comment pragma and the HTTP header, the best thing to do is probably to specify whatever Chrome chose to do.

If Chrome hasn't implemented both yet, giving the HTTP header precedence would be consistent with how character encoding declarations work even though source maps don't appear to have the same implementation constraints that make it desirable to give precedence to the value the implementation has the opportunity to see first. On the other hand, giving the comment pragma precedence is more likely to be correct in the light of Ruby's Postulate (see http://intertwingly.net/slides/2004/devcon/68.html ).

Even though it doesn't really belong in this bug, now that I'm commenting on the spec, here are some more comments on the spec:

It's unfortunate that the formant has a version identifier. Experience suggests that versioning is an anti-pattern for Web formats and it is instead a better to give all formats in place in a manner that degrades gracefully instead of making processors behave differently based on a version identifier. In the case of source maps, it would make sense to assume that a future version of source maps adds new name-value pairs to the top-level JSON object and processors built for older versions simply ignore those. After all, there's not really anything better that a processor program today can do if it sees a source map with the version number greater than 3, so the version identifier is pointless.

The spec explains what the valid format is, but it doesn't say how to process input in such a way that all conforming processors process any input (including invalid input) in a consistent way. (The spec says things like "always present if there is a source field". Well, what if it isn't present?)

Kudos for defining UTF-8 as the only encoding.

The section about compression should be rewritten as an informative suggestion to use HTTP-level gzip compression instead of suggesting that "allowing" HTTP payload to be gzip compressed belongs in the jurisdiction of a spec that describes a format that can potentially appear as a HTTP payload.

It's sad that there's such a thing as known extensions with x_google_* naming instead of a process for the stakeholders to claim on prefixed names right away. To avoid the trouble of migrating from x_google_linecount to plain linecount in the future, it would have been better to provide a low-bureaucracy way for Google to claim the identifier 'linecount' for their purpose. This way, the identifier could be promoted into an official part of the spec without renaming in the future. On the other hand, if Google's notion of 'linecount' turned out to be bad, semantically different standard feature dealing with line counts could use a different identifier (e.g. 'lines').

It's unfortunate that the HTTP header uses the X- anti-pattern for naming instead of being called just 'SourceMap'. If Chrome has already been deployed with support for X-SourceMap, it's probably not worthwhile to change this anymore. However, in the future, please mint names without the X- (and let the IANA know but if the IANA refuses to register, use the name with X- anyway). See http://tools.ietf.org/html/draft-ietf-appsawg-xdash-05

The )]} exception to JSON should be hoisted into the JSON spec so that formats building on top of JSON such as source maps don't need to specify ad hoc deltas to the underlying syntax.

> * Is there a strlen for jschar*? That would save me a few lines of code (and
> potential for bugs) in the patch.
> 
> * If we fail to successfully call setSourceMap in JSScript::Create, should
> we fail and return NULL even though the source map isn't really required for
> the script to be created or run?

I defer to jimb on these questions.

> * Lastly, when I run the jsapi tests, everything passes; when I run
> js/src/tests/jstests.py, I get failures due to timeouts. I don't know if
> this is significant or not:

I don't know if that's known-random.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Reporter
	Comment 4 • 12 years ago

I have updated the patch based on feedback given here, and also have added a test.

The test fails because you obviously can't do |new window.Debugger(window)|. However, I have no idea how to rewrite this test such that the intent is preserved, and that the debugger won't blow up.

I'd really appreciate some help turning this test into a reality.

	Jim Blandy :jimb
	Comment 5 • 12 years ago

I've landed bug 771705 (also known as "DAMN"). SourceMaps should be passed from the JSAPI boundary through to the compiler via a field in JS::CompileOptions; that will make this patch much simpler.

	Jim Blandy :jimb
	Comment 6 • 12 years ago

(In reply to Jim Blandy :jimb from comment #5)
> that will make this patch much simpler.

Or, certain parts of it, at least.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Reporter
	Comment 7 • 12 years ago

We just got s/X-SourceMap/SourceMap/ in the spec! Woo!

	Jim Blandy :jimb
	Comment 8 • 12 years ago

Comment on attachment 644013 [details] [diff] [review]
V2

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

I understand you've got a revised patch; looking forward to it. Here are a few comments on the old one.

::: js/src/js.msg
@@ +352,4 @@
>  MSG_DEF(JSMSG_REST_WITH_DEFAULT,      299, 0, JSEXN_SYNTAXERR, "rest parameter may not have a default")
>  MSG_DEF(JSMSG_NONDEFAULT_FORMAL_AFTER_DEFAULT, 300, 0, JSEXN_SYNTAXERR, "parameter(s) with default followed by parameter without default")
>  MSG_DEF(JSMSG_YIELD_IN_DEFAULT,       301, 0, JSEXN_SYNTAXERR, "yield in default expression")
> +MSG_DEF(JSMSG_ALREADY_HAS_SOURCEMAP,  302, 1, JSEXN_NONE,      "{0} is being assigned a source map, yet already has one")

I think you need to actually give this an exception type, probably JSEXN_ERR, in case someone is running with JSOPTION_WERROR.

::: js/src/jsscript.cpp
@@ +1308,5 @@
>  
>  bool
> +ScriptSource::setSourceMap(JSContext *cx, jschar *sourceMapURL, const char *filename) {
> +    if (hasSourceMap) {
> +        JS_ReportErrorFlagsAndNumber(cx, JSREPORT_WARNING,

The way to report a warning is to pass JSREPORT_WARNING (as you're doing), but then return false if JS_ReportErrorFlagsAndNumber returns false. That function will return true if it did turn out to be just a warning, but false if the user had set JSOPTION_WERROR and turned all warnings into errors.

@@ +1318,1 @@
>          return false;

I understand this has become an assert in the newest version of the patch --- perfect.

@@ +1569,5 @@
>  
> +    // If there is a source map URL, copy it and then save it via
> +    // |setSourceMap|.
> +    if (sourceMapURL) {
> +        int smlen = js_strlen(sourceMapURL);

You really should define js_strdup in jsstr.cpp, and then just call it from here.

@@ +1759,4 @@
>  
>      jschar *sourceMap = (jschar *) bce->parser->tokenStream.copySourceMap(cx);
>      if (sourceMap) {
> +        if (!script->source || !script->source->setSourceMap(cx, sourceMap, script->filename)) {

There'll always be a script->source after bug 779017, right? So the script->source test could either go away, or become an assertion.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Reporter
	Comment 9 • 12 years ago

I'm splitting the SourceMap header support in the workers' script loader out to a new bug: bug 780269

This should make it easier to land this bug, since we can't even debug workers yet.

Will be updating the patch here, soon.

	Eddy Bruel [:ejpbruel] Assignee
	Comment 10 • 11 years ago

This patch does a couple of things, mainly cleanup and preparation work for the actual patch: 

- Try to introduce some naming consistency by preferring the use of
  "sourceMapURL" over "sourceMap".
- Create a copy of the sourceMapURL at the caller, rather than the expect the
  callees to do this for us. This is much less error prone.
- Add a field to the CompileOptions struct that allow JSAPI consumers to pass a
  source map URL. This will be our public interface for nsScriptLoader.
- If a source map URL is specified as a compile option, any source map URLs
  specified as comment pragmas are ignored. This implies that source map URLs in
  HTTP headers will override those in comment pragmas (I'm making a call here.
  If this is not the desired behavior this is the time to let me know)

Flagging jimb for review since he's probably seen most of this code and I can't think of anyone better from the back of my head (jim, if you can, feel free to pass review on to someone else).

	Eddy Bruel [:ejpbruel] Assignee
	Comment 11 • 11 years ago

This patch looks for any appearances of the X-SourceMap header, and, if found, passes the corresponding source map URL as a compile option to JSAPI.

Note that the previous patch made it so that source map URLs specified as compile options override any source map URLS specified as comment pragmas. If this is not the desired behavior, it can easily be changed, so just let me know in that case.

Flagging Henri Sivonen for review since he previously provided feedback on this bug and is familiar with the nsScriptLoader code as far as I know.

	Henri Sivonen (:hsivonen)
	Comment 12 • 11 years ago

Comment on attachment 745848 [details] [diff] [review]
Fetch source map URLs from HTTP headers + test

I'm still unhappy that the name of the header starts with "X-", but I guess that ship has sailed in Chrome and it's better to be compatible. :-(

	Jim Blandy :jimb
	Comment 13 • 11 years ago

Comment on attachment 745844 [details] [diff] [review]
Clean up source map API

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

::: js/src/frontend/BytecodeCompiler.cpp
@@ +213,5 @@
>  
> +    if (options.sourceMapURL) {
> +        if (!ss->setSourceMapURL(cx, options.sourceMapURL))
> +            return false;
> +    } else if (!SetSourceMap(cx, parser.tokenStream, ss, script))

Doesn't this mean we'll never warn when a source map URL comes from both comments in the source and from headers? It's setSourceMap{,URL} that checks; that's only called from SetSourceMap; and now that'll only be called for one or the other origin.

::: js/src/frontend/TokenStream.cpp
@@ +842,4 @@
>              return false;
>  
> +        PodCopy(sourceMapURL_, tokenbuf.begin(), sourceMapURLLength);
> +        sourceMapURL_[sourceMapURLLength] = '\0';

This is a pre-existing problem, but: it's a pity there's no js_strndup in jsstr.cpp (see later comment about js_strdup).

::: js/src/frontend/TokenStream.h
@@ +677,1 @@
>      }

So glad to see this weird hand-off stuff going away.

::: js/src/jsscript.cpp
@@ -1416,5 @@
>      return true;
>  }
>  
>  bool
> -ScriptSource::setSourceMap(JSContext *cx, jschar *sourceMapURL, const char *filename)

Ah, when the filename moved from the JSScript to the ScriptSource, nobody realized that passing it here had become silly. Nice work.

@@ +1432,5 @@
> +        return true;
> +    sourceMapURL_ = cx->pod_malloc<jschar>(len);
> +    if (!sourceMapURL_)
> +        return false;
> +    js_strncpy(sourceMapURL_, sourceMapURL, len); 

Could you call js_strdup (jsstr.cpp) here?

	Jim Blandy :jimb
	Comment 14 • 11 years ago

For future reviews, I have a favor to ask:

I find makes it a lot easier to review patches if purely mechanical changes (like the "sourceMap" to "sourceMapURL" changes) are in their own patch. When I'm scanning for changes to semantics (like the ownership of the URL string, say), it's nice to have fewer "noise" changes (like the rename) to recognize and ignore.

Obviously, I'm happy to review patches in whatever form you'd like, and there's no need to do that for this review; it's just something that would help me out.

	Eddy Bruel [:ejpbruel] Assignee
	Comment 15 • 11 years ago

Comment on attachment 745844 [details] [diff] [review]
Clean up source map API

Jim, you didn't actually r+ this (I assume you intended to, since you only had minor comments). Could you r+ this patch so it can land?

	Eddy Bruel [:ejpbruel] Assignee
	Comment 16 • 11 years ago

New patch with comments by jimb addressed.

jim, if its not too much trouble, could you get this patch into production in my absence too?

	Jim Blandy :jimb
	Comment 17 • 11 years ago

Comment on attachment 757052 [details] [diff] [review]
Clean up source map API (v2)

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

Looks great, with minor shell leak fixed.

::: js/src/shell/js.cpp
@@ +1080,4 @@
>              const jschar *smurl = JS_GetStringCharsZ(cx, sourceMapURL);
>              if (!smurl)
>                  return false;
> +            if (!script->scriptSource()->setSourceMapURL(cx, smurl))

Since setSourceMapURL doesn't take ownership of the string it's passed, this is now a leak; you need to JS_free smurl.

	Eddy Bruel [:ejpbruel] Assignee
	Comment 18 • 11 years ago

Apparently, this didn't land yet?

	Rob Campbell [:rc] (:robcee)
	Comment 19 • 11 years ago

bump.

	Eddy Bruel [:ejpbruel] Assignee
	Comment 20 • 11 years ago

(In reply to Jim Blandy :jimb from comment #17)
> Comment on attachment 757052 [details] [diff] [review]
> Clean up source map API (v2)
> 
> Review of attachment 757052 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks great, with minor shell leak fixed.
> 
> ::: js/src/shell/js.cpp
> @@ +1080,4 @@
> >              const jschar *smurl = JS_GetStringCharsZ(cx, sourceMapURL);
> >              if (!smurl)
> >                  return false;
> > +            if (!script->scriptSource()->setSourceMapURL(cx, smurl))
> 
> Since setSourceMapURL doesn't take ownership of the string it's passed, this
> is now a leak; you need to JS_free smurl.

No it's not. JS_GetStringCharsZ returns a pointer to sourceMapURL.d.u1.chars, which is owned by sourceMapURL. setSourceMapURL takes ownership of a *copy* of smurl, so there should be no leaks going on here.

	Eddy Bruel [:ejpbruel] Assignee
	Comment 21 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6b1258576be

	Eddy Bruel [:ejpbruel] Assignee
	Comment 22 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a0161f8a11e

	Hannes Verschore [:h4writer]
	Comment 23 • 11 years ago

(In reply to Eddy Bruel [:ejpbruel] from comment #21)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/c6b1258576be

Fyi the commit log is referring to the wrong bug number.

	Eddy Bruel [:ejpbruel] Assignee
	Comment 24 • 11 years ago

(In reply to Hannes Verschore [:h4writer] from comment #23)
> (In reply to Eddy Bruel [:ejpbruel] from comment #21)
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/c6b1258576be
> 
> Fyi the commit log is referring to the wrong bug number.

*sigh*. Oh well, at least the second commit got it right. Releng should be able to figure it out.

	Jim Blandy :jimb
	Comment 25 • 11 years ago

(In reply to Eddy Bruel [:ejpbruel] from comment #20)
> No it's not. JS_GetStringCharsZ returns a pointer to
> sourceMapURL.d.u1.chars, which is owned by sourceMapURL. setSourceMapURL
> takes ownership of a *copy* of smurl, so there should be no leaks going on
> here.

Indeed it does. Sorry about that.

	Ryan VanderMeulen [:RyanVM]
	Comment 26 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/5a0161f8a11e

	Florian Scholz (Open Web Docs)
	Comment 27 • 7 years ago

https://developer.mozilla.org/en-US/Firefox/Releases/27#Developer_Tools
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/SourceMap