772113 - Expose source map URLs in Debugger.Script

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Currently this information can be accessed through JSScript::getSourceMap(), but it is not exposed to javascript code via Debugger.Script.

Comment 1

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

Attachment: V1

Took a stab at this and feel like my patch should be working, however the test I added is failing. I'm not sure if it is failing because the code is actually incorrect, or if the test infrastructure is incorrect, which leads to my next topic:

Whenever I try to run the test with the "-g" flag to run it in gdb, I get an error saying that I can only run one test with gdb. The thing is though, I am specifying only my test to run by passing it as an argument on the command line!

Help/tips appreciated :P Thanks

Comment 2

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

Attachment: V2

Woops, forgot to add the test! Updated patch.

Comment 3

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

Attachment: V3

Fixed some stupid stuff in the test, so that it is actually asserting the right things >_<

Thanks to a tip from Terrence, I was able to get gdb running. I've found that the JSScript that I am calling `getSourceMap` on is not the same instance of JSScript that `setSourceMap` was called on and so `JS_ASSERT(hasSourceMap` is failing. Interestingly enough, it looks like weven though `hasSourceMap` is false, there is still an entry in `sourceMapMap` for the JSScript.

Comment 4

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

Attachment: v4

Rebased on top of patch for bug 774471. Test is passing now.

Comment 5

[Jim Blandy :jimb]

Comment on attachment 643147 [details] [diff] [review]
v4

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

Looks generally good; just a few things need to be taken care of.

::: js/src/shell/js.cpp
@@ +847,5 @@
>  
> +        if (!JS_GetProperty(cx, options, "sourceMapURL", &v))
> +            return false;
> +        if (JSVAL_IS_NULL(v)) {
> +            sourceMapURL = NULL;

Since sourceMapURL is initialized, I think we could just drop this 'if' clause.

@@ +852,5 @@
> +        } else if (!JSVAL_IS_VOID(v)) {
> +            JSString *s = JS_ValueToString(cx, v);
> +            if (!s)
> +                return false;
> +            const jschar* smurl = s->getCharsZ(cx);

You know, I think a lot of this kind of hair would go away if we just used a JSFlatString for the source map. You wouldn't need to copy it here, and you wouldn't need to worry about "releasing" it from the TokenStream. You'd need to give ScriptSource a less trivial 'mark' member function that marks the string, but it wouldn't be too bad.

If you agree, could you file a follow-up bug? I don't think we need to make the change immediately.

@@ +918,4 @@
>          JS_SetOptions(cx, options);
>  
>          JSScript *script = JS_CompileUCScript(cx, global, codeChars, codeLength, fileName, lineNumber);
> +        if (sourceMapURL && script->source && !script->source->setSourceMap(cx, sourceMapURL))

Isn't script->source guaranteed to be non-zero after bug 774471?

::: js/src/vm/Debugger.cpp
@@ +2544,5 @@
> +{
> +    THIS_DEBUGSCRIPT_SCRIPT(cx, argc, vp, "(get sourceMapURL)", args, obj, script);
> +
> +    if (!script->source)
> +        return false;

Returning false without registering an exception is equivalent to an OOM or a "slow script" kill; I assume this should throw an error, right?

And, after bug 774471, can the source pointer actually be null? Shouldn't this just be an assertion?

@@ +2552,5 @@
> +    if (sourceMapURL) {
> +        JSString *str = JS_NewUCStringCopyZ(cx, sourceMapURL);
> +        if (str == NULL) {
> +            return false;
> +        }

No need for braces.

Comment 6

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

Attachment: v5

Comment 7

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

Attachment: v6

Woops, found a bug in the last patch where I wasn't checking for OOM soon enough and was assuming that script was a valid pointer.

Comment 8

[Jim Blandy :jimb]

Comment on attachment 647727 [details] [diff] [review]
v5

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

The code that's in the patch now looks great! However, I would like to have better test coverage. We should have tests for at least the following:

- If a given 'evaluate' call produces many scripts (say, a top-level script, functions, and nested functions), is the source map visible on all of them?

- If two evaluate calls are passed different source maps, do the URLs get propagated to the right scripts? That is, do our tests prove that there isn't just one big global source map URL that .sourceMapURL always returns?

- Can we show that //@ comments get picked up? Can we observe what happens when there is both a header and a //@ comment? In other words, all four possible combinations of header/no header and comment/no comment should be covered.

::: js/src/jit-test/tests/debug/Script-sourceMapURL.js
@@ +4,5 @@
> +var dbg = new Debugger;
> +var gw = dbg.addDebuggee(g);
> +for (let sourceMapURL of ['file:///var/foo.js.map', null]) {
> +    g.evaluate("function f(x) { return 2*x; }", {sourceMapURL: sourceMapURL});
> +    let fw = gw.getOwnPropertyDescriptor('f').value;

This is very clever! If you prefer, you could also say:

let fw = gw.makeDebuggeeValue(g.f)

that being the "official" way to get Debugger.Object instances.

::: js/src/shell/js.cpp
@@ +846,5 @@
>          }
>  
> +        if (!JS_GetProperty(cx, options, "sourceMapURL", &v))
> +            return false;
> +        if (!JSVAL_IS_VOID(v) && !JSVAL_IS_NULL(v)) {

Nit: I think the truly Javascripty thing to do, unfortunately, is to let 'null' through here, and have it turn into the string "null". When in Rome... The jit-test test will need to change to use 'undefined'.

@@ +850,5 @@
> +        if (!JSVAL_IS_VOID(v) && !JSVAL_IS_NULL(v)) {
> +            JSString *s = JS_ValueToString(cx, v);
> +            if (!s)
> +                return false;
> +            const jschar* smurl = s->getCharsZ(cx);

Nit: It's SM style to put the * with the declared thing, not the type (although we're not entirely consistent).

@@ +857,5 @@
> +            int len = JS_GetStringLength(s);
> +            sourceMapURL = (jschar *) cx->malloc_((len + 1) * sizeof(jschar));
> +            if (!sourceMapURL)
> +                return false;
> +            js_strncpy(sourceMapURL, smurl, len);

Nit: I suggested defining js_strdup to you(?) in another bug, right? Would this be a second use of it?

Comment 9

[Jim Blandy :jimb]

Comment on attachment 647727 [details] [diff] [review]
v5

Argh, didn't mean to un-obsolete this patch. I think Splinter did me wrong...

Comment 10

[Jim Blandy :jimb]

Comment on attachment 647788 [details] [diff] [review]
v6

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

::: js/src/shell/js.cpp
@@ +916,5 @@
>          JS_SetOptions(cx, options);
>  
>          JSScript *script = JS_CompileUCScript(cx, global, codeChars, codeLength, fileName, lineNumber);
> +        if (!script)
> +            return false;

Oh, good catch. Sorry I missed this.

Comment 11

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

Attachment: v7

Updated based on feedback and expanded tests.

Also realized that comments of the form `//@ sourceMappingURL=...` weren't being saved (//@line never has that space, but sourceMappingURL can have it, or at least there is a space in the spec where I don't remember there being one), so I fixed the tokenizer and added a test for that as well.

Comment 12

[Jim Blandy :jimb]

Comment on attachment 648589 [details] [diff] [review]
v7

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

Thanks for the revisions! This is looking good. I did find one more potential problem that I'd like checked out, and a few more tests.

Some of these I should have caught on previous reviews; I'm sorry to send you around again. Thanks for being patient!

::: js/src/frontend/TokenStream.cpp
@@ +1242,5 @@
> +
> +    while ((c = getChar()) != '\n' && c != EOF && IsSpaceOrBOM2(c))
> +        continue;
> +
> +    if (c == 's' && peekChars(16, peeked) && CharsMatch(peeked, "ourceMappingURL=")) {

I don't think this is detectable at the moment, but if the match fails, you don't push back the '@', the spaces, or the 's'. This is unlike getAtLine, which leaves the stream state unchanged if the directive name doesn't match. If someone tries to add a third //@ directive in the future, they'll need to refactor things a bit.

For the time being, it doesn't seem like it matters much.

@@ +1248,3 @@
>          tokenbuf.clear();
>  
> +        while (!IsSpaceOrBOM2((c = getChar())) && c && c != EOF)

It looks like this will fail to push back a terminating space. If that terminating space was the comment-terminating newline, doesn't the subsequent loop in the caller consume the following line as a comment? Even if I'm misreading the code (quite possible), this case needs to be tested.

(I should have caught this in the previous review!)

@@ +1251,3 @@
>              tokenbuf.append(c);
>  
>          if (tokenbuf.empty())

Later in this function, couldn't the copy loop be rewritten as a memcpy or a PodCopy? I don't think the performance matters, but when I see a copy loop written out, I wonder, "Why didn't they use memcpy?" and I waste time reading it carefully.

::: js/src/jit-test/tests/debug/Script-sourceMapURL.js
@@ +5,5 @@
> +let gw = dbg.addDebuggee(g);
> +
> +function getSourceMapURL() {
> +    let fw = gw.makeDebuggeeValue(g.f);
> +    if (!fw) throw new Error('Could not make debuggee value.');

Does makeDebuggeeValue ever return a false value, if g.f is not a false value itself? We don't need to check for conditions that would cause the test script to throw an error anyway; the harness will report uncaught errors as failures automatically (unless there's a |jit-test| cookie that tells the harness to expect a particular error).

@@ +19,5 @@
> +assertEq(getSourceMapURL(), 'file:///var/foo.js.map');
> +
> +// Nested functions
> +dbg.onDebuggerStatement = function (frame) {
> +    assertEq(frame.script.sourceMapURL, 'file:///var/bar.js.map');

You need to set a flag or counter to check that the onDebuggerStatement handler fires at all, to avoid false passes. This is what the 'log' variable in many of the other jit-test/tests/debug tests are for.

For example, it would be enough to simply have the handler store the URL in a global variable, and have an assertion at the top level that the global variable got set to the right value.

@@ +30,5 @@
> +           '//@sourceMappingURL=file:///var/baz.js.map');
> +assertEq(getSourceMapURL(), 'file:///var/baz.js.map');
> +g.evaluate('function f() {}\n' +
> +           '//@ sourceMappingURL=file:///var/quux.js.map');
> +assertEq(getSourceMapURL(), 'file:///var/quux.js.map');

Since the spec says(?) that the URL is terminated by any space character, there should be a test to verify that it does.

As mentioned elsewhere, we need a test that the next line of tokens after the comment directive isn't skipped.

I think we still need a test for what happens when both header and comment directive are present.

Comment 13

[Jim Blandy :jimb]

(In reply to Jim Blandy :jimb from comment #12)
> Later in this function, couldn't the copy loop be rewritten as a memcpy or a
> PodCopy? I don't think the performance matters, but when I see a copy loop
> written out, I wonder, "Why didn't they use memcpy?" and I waste time
> reading it carefully.

Oh --- I see this is fixed in bug 774471.

Comment 14

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

> I think we still need a test for what happens when both header and comment
> directive are present.

The header patch depends on this patch to even make it possible to test the header patch. I can add a test case for this to that patch.

Will update and resubmit this patch soon.

Comment 15

[Jim Blandy :jimb]

(In reply to Nick Fitzgerald :fitzgen from comment #14)
> > I think we still need a test for what happens when both header and comment
> > directive are present.
> 
> The header patch depends on this patch to even make it possible to test the
> header patch. I can add a test case for this to that patch.

"Header" isn't quite the right word for me to have used. What I meant was, we can test here the behavior when both of the following are true:

- The code contains a //@sourceMappingURL directive, and

- The second argument to g.evaluate has a sourceMapURL property.

Comment 16

[Jim Blandy :jimb]

I would also like a test where the source code contains more than one //@sourceMappingURL directive, that verifies that we get the last one that appears.

Comment 17

[Ryan]

In WebKit nightly and Chrome both require that there be a space betten //@ and sourceMappingURL otherwise it won't work. Are you following this convention or like above are you requiring no space?

I could see this causing headaches.

Comment 18

[Jim Blandy :jimb]

Mozilla's code optionally accepts the space. The spec does not say the space is optional, though.

Comment 19

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

(In reply to Jim Blandy :jimb from comment #18)
> Mozilla's code optionally accepts the space. The spec does not say the space
> is optional, though.

Yeah looking back at the spec you are right. I will fix this before I resubmit this patch.

Comment 20

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

Attachment: v8

Removed the assert in ScriptSource::setSourceMap in favor of a warning.

New try push: https://tbpl.mozilla.org/?tree=Try&rev=051b700a4575

Comment 21

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

Also, requires the space after the '@' in the comment pragma.

Comment 22

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

Attachment: v9

Updating testScriptInfo to use a space after the '@' sign like we require now as per the spec. This was causing the jsapi-tests to fail in the try push. Should have caught this before uploading that last patch.

Comment 23

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

New try push: https://tbpl.mozilla.org/?tree=Try&rev=b9f080b0907a

Comment 24

[Jim Blandy :jimb]

Comment on attachment 651574 [details] [diff] [review]
v9

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

Looks great! Just one thing to fix, and this can land.

::: js/src/frontend/TokenStream.cpp
@@ +1239,3 @@
>          tokenbuf.clear();
>  
> +        while (!IsSpaceOrBOM2((c = peekChar())) && c && c != EOF) {

You need to check for EOF before passing c to IsSpaceOrBOM2: if peekChar returns EOF, that's -1; On Windows, jschar is wchar_t, which is signed, so IsSpaceOrBOM2 will use the -1 to index the js_isspace array; that's a bogus memory reference. If you look at the other uses of IsSpaceOrBOM2 in the file, you'll see they all take care of this.

This is a bug in the existing code; apologies for not catching that on review.

Comment 25

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

Attachment: v9.1

Change between v9 and v9.1:

-        while (!IsSpaceOrBOM2((c = peekChar())) && c && c != EOF) {
+        while ((c = peekChar()) && c != EOF && !IsSpaceOrBOM2(c)) {

Comment 26

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/96131c46e845

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/96131c46e845