Last Comment Bug 674283 - Add source map urls to JSScript
: Add source map urls to JSScript
Status: RESOLVED FIXED
[inbound]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla8
Assigned To: Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
:
Mentors:
Depends on: 686283
Blocks: 670002 674980
  Show dependency treegraph
 
Reported: 2011-07-26 10:54 PDT by Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
Modified: 2011-10-11 06:03 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
First attempt at a patch (9.56 KB, patch)
2011-07-26 18:56 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
no flags Details | Diff | Review
Patch v. 2 (10.16 KB, patch)
2011-07-27 14:17 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
no flags Details | Diff | Review
Patch v. 3 (10.26 KB, patch)
2011-07-27 15:22 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
jorendorff: review+
Details | Diff | Review

Description Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2011-07-26 10:54:44 PDT
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] [⏰PDT; UTC-7] 2011-07-26 18:56:49 PDT
Created attachment 548655 [details] [diff] [review]
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] 2011-07-27 10:23:44 PDT
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] [⏰PDT; UTC-7] 2011-07-27 14:17:01 PDT
Created attachment 548911 [details] [diff] [review]
Patch v. 2

Updated patch as per your feedback.
Comment 4 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2011-07-27 15:22:20 PDT
Created attachment 548937 [details] [diff] [review]
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] 2011-07-29 08:43:51 PDT
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] 2011-07-29 09:01:48 PDT
Fixed the nits and pushed.

http://hg.mozilla.org/integration/mozilla-inbound/rev/32b8eb4569b7
Comment 7 Jason Orendorff [:jorendorff] 2011-07-29 09:27:04 PDT
Comment on attachment 548937 [details] [diff] [review]
Patch v. 3

(er, r=me of course, with the bugs fixed)
Comment 8 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2011-07-29 11:09:09 PDT
(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] 2011-08-01 07:47:00 PDT
http://hg.mozilla.org/mozilla-central/rev/32b8eb4569b7

Note You need to log in before you can comment on or make changes to this bug.