674283 - Add source map urls to JSScript

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

The source map standard[0] specifies that scripts should advertise the existence of an associated source map via comments of the form "//@sourceMappingURL=<URL>". The specified URL should be attached to the JSScript so that developer tools can use this information for debugging, logging, etc.

Aside: the reason the developer tools need to get the information from JSScript, rather than from inspecting the script's source from the network cache, is becauase the network cache would not contain eval'd scripts or scripts created via certain types of script injection.



[0]: https://docs.google.com/document/d/1U1RGAehQwRypUTovF1KRlpiOFze0b-_2gc6fAH0KY0k/edit?pli=1

Comment 1

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

Attachment: First attempt at a patch

Questions:

* Am I allocating the sourcemap string correctly in the TokenStream?
* Am I freeing the sourcemap correctly?
* What needs to be better?

(I apologize for my lack of C++ skillz).

Comment 2

[Jason Orendorff [:jorendorff]]

Comment on attachment 548655 [details] [diff] [review]
First attempt at a patch

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

I know this is a lot of comments, but it basically looks good. Nice work.

Fix it up and request a new review. We can land this today.

::: js/src/jsdbgapi.cpp
@@ +1634,5 @@
> +JS_PUBLIC_API(const char *)
> +JS_GetScriptSourcemap(JSContext *cx, JSScript *script)
> +{
> +    return script->sourcemap;
> +}

Throughout the patch, please capitalize the M in sourceMap/SourceMap.

Also throughout, treat source map URLs as jschar strings, not char. (I know filenames are char, but that's a mistake.)

::: js/src/jsscan.cpp
@@ +1207,5 @@
> +        peeked[4] == 'r' &&
> +        peeked[5] == 'c' &&
> +        peeked[6] == 'e' &&
> +        peeked[7] == 'M' &&
> +        peeked[8] == 'a' &&

This generates perfectly good code, but the 18 lines of "does this character match?" really breaks up the code. Instead, let's add a helper function, like:

bool CharsMatch(const jschar *p, const char *q) {
    while (*q) {
        if (*p++ != *q++)
            return false;
    }
    return true;
}

So that you can say:

    if (peekChars(18, peeked) && CharsMatch(peeked, "@sourceMappingURL=")) {
        skipChars(18);
        ...

Change the @line implementation to use this too, please.

@@ +1228,5 @@
> +
> +        if (tokenbuf.empty()) {
> +            /* The source map's URL was missing. */
> +            return false;
> +        }

If this was going to be a syntax error, you would have to report it. But I think in this case you just want to return true.

@@ +1234,5 @@
> +        len = tokenbuf.length();
> +        sm = (char *) cx->malloc_(sizeof(char) * tokenbuf.length() + 1);
> +        if (sm == NULL) {
> +            return false;
> +        }

This `return false` is correct.

Style nit:  There are no curly braces around single statements in the JS engine (unless it's the body of an if/for/while statement whose head is multiple lines).

@@ +1240,5 @@
> +            sm[i] = tokenbuf[i];
> +        }
> +        sm[i] = '\0';
> +
> +        sourcemap = (const char *) sm;

Remove the cast here.

(Truncating the bytes there is definitely the wrong thing. That's why I'm suggesting jschars everywhere. The browser passes us jschars representing the source code; this way we don't throw anything away.)

::: js/src/jsscan.h
@@ +342,4 @@
>      }
>      const CharBuffer &getTokenbuf() const { return tokenbuf; }
>      const char *getFilename() const { return filename; }
> +    const char *getSourcemap() const { return sourcemap; }

Use `const jschar *` as the return type here, but...

@@ +629,4 @@
>      const jschar        *prevLinebase;  /* start of previous line;  NULL if on the first line */
>      TokenBuf            userbuf;        /* user input buffer */
>      const char          *filename;      /* input filename or null */
> +    const char          *sourcemap;     /* sourcemap or null */

... `jschar *` here instead of `const char *`, since we're going to call free later.

::: js/src/jsscript.cpp
@@ +1336,5 @@
>      script->pcCounters.destroy(cx);
>  
> +    if (script->sourcemap != NULL) {
> +        cx->free_((void *) script->sourcemap);
> +    }

Remove the cast. Since sourceMap will be non-const now, it's unnecessary.

Style nits: Again remove the curly braces; also in js/src we don't say "!= NULL" unless we absolutely have to, so remove that.

::: js/src/jsscript.h
@@ +498,4 @@
>      js::Bindings    bindings;   /* names of top-level variables in this script
>                                     (and arguments if this is a function script) */
>      JSPrincipals    *principals;/* principals for this script */
> +    const char      *sourcemap; /* source map file or null */

Again, `jschar *` here, no `const`.

Comment 3

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

Attachment: Patch v. 2

Updated patch as per your feedback.

Comment 4

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

Attachment: Patch v. 3

Replaced TokenStream::getSourceMap with TokenStream::releaseSourceMap and made TokenStream's destructor free the source map string if it hasn't been released.

Comment 5

[Jason Orendorff [:jorendorff]]

I'll finish this up and push it today. It had to be unbitrotted, as js/src has changed even in the past few days. Also, js::TokenStream::getAtSourceMap needs to free any previously allocated sourceMap buffer. Also, valgrind found this:

> sourceMap = (jschar *) cx->malloc_(sizeof(jschar) * tokenbuf.length() + 1);

See it? :)

Comment 6

[Jason Orendorff [:jorendorff]]

Fixed the nits and pushed.

http://hg.mozilla.org/integration/mozilla-inbound/rev/32b8eb4569b7

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 548937 [details] [diff] [review]
Patch v. 3

(er, r=me of course, with the bugs fixed)

Comment 8

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

(In reply to comment #5)
> I'll finish this up and push it today. It had to be unbitrotted, as js/src
> has changed even in the past few days. Also, js::TokenStream::getAtSourceMap
> needs to free any previously allocated sourceMap buffer. Also, valgrind
> found this:
> 
> > sourceMap = (jschar *) cx->malloc_(sizeof(jschar) * tokenbuf.length() + 1);
> 
> See it? :)

D'oh! Good catch :)

Thanks!

Comment 9

[Marco Bonardo [:mak] (Away 10-21 August)]

http://hg.mozilla.org/mozilla-central/rev/32b8eb4569b7