904144 - [jsdbg2] parse "//# sourceURL=..." directives and expose them on Debugger.Source

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Same way we parse and expose "//# sourceMappingURL=..." directives, we should do the same with "//# sourceURL=...".

http://www.softwareishard.com/blog/firebug/firebug-tip-label-dynamic-scripts-with-sourceurl-directive/

Comment 1

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

16:13 < jimb> fitzgen: Debugger.Source.prototype.displayURL is probably a decent place and name for it.
16:13 < fitzgen> jimb: kk
16:13 < jimb> That will make it clear that it's not an official origin for the text.
16:13 < fitzgen> right

Comment 2

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

Attachment: wip

Adds Debugger.Source.prototype.displayURL, and all tests pass, but the code is rather ugly.

Comment 3

[Jim Blandy :jimb]

It would sure be nice to common up the sourceMappingURL and sourceURL parsing code.

Comment 4

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

Yup, that was my plan, hence the "ugly" statement ;)

Comment 5

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

Attachment: wip 2

Makes the code for parsing pragma/directives more generic and shared. Broke the tests. Can't get my printf's to dump to stdout when running the tests. It seems that the first sourceURL is being attached, but then I crash (exit code -11) and I'm not getting any info out of the test as to why.

What is the preferred way to debug here? I remember using gdb way back when I wrote the sourceMappingURL directive parsing code, but we use clang now, does that mean I should use lldb? (I'm so bad with both gdb and lldb; so frustrating not being able to just print/inspect objects like you can in JS!)

Help!

Comment 6

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

Attachment: bug-904144-source-url.patch

Turns out I was doing

  *destination[length] = ...

when I needed to

  (*destination)[length] = ...

It's been too long.

All SpiderMonkey debugging tips still appreciated :)

No try push because the tree is closed.

Comment 7

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

Do I need to add JS_GetScriptSourceURL(cx, script) to oldDebugApi.h and test it in js/src/jsapi-tests/testScriptInfo.cpp?

Don't I need to increment an XDR version number somewhere?

Comment 8

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

Attachment: bug-904144-source-url.patch

Increments the XDR version number subtrahend.

https://tbpl.mozilla.org/?tree=Try&rev=5692fe171ef3

Comment 9

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

Jim, it looks like this is failing on Windows XP, because apparently inside `getDirective` we are trying to make `peeked` and array of 0 length. However, `peeked`'s length is set to `directiveLength` which is only ever passed into `getDirective` as a literal (inside `getSourceURL` and `getSourceMappingURL`).

Any idea what is going on here?

Comment 10

[Jim Blandy :jimb]

Comment on attachment 803423 [details] [diff] [review]
bug-904144-source-url.patch

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

::: js/src/frontend/TokenStream.cpp
@@ +830,5 @@
> +bool
> +TokenStream::getDirective(bool isMultiline, bool shouldWarnDeprecated,
> +                                const char *directive, int directiveLength,
> +                                unsigned errorNumber, jschar **destination) {
> +    jschar peeked[directiveLength];

I think the reason MSVC is complaining about this is that variable-length arrays are actually a GNU extension. The C++ standard says that the length in an array declarator must be a constant expression.

So I think you need to declare peeked to be "big enough", and then assert that directiveLength fits within whatever size you chose.

Comment 11

[Jim Blandy :jimb]

Comment on attachment 803423 [details] [diff] [review]
bug-904144-source-url.patch

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

Looks good; r=me with the js.msg comment addressed.

::: js/src/frontend/BytecodeCompiler.cpp
@@ +40,5 @@
>      return true;
>  }
>  
>  static bool
> +SetSourceURL(ExclusiveContext *cx, TokenStream &tokenStream, ScriptSource *ss)

These setters and their callers are starting to look kind of dumb. Would it be worth a follow-up bug to just create the ScriptSource earlier, and let the TokenStream store stuff on it directly?

::: js/src/js.msg
@@ +412,5 @@
>  MSG_DEF(JSMSG_GENERATOR_FINISHED,     359, 0, JSEXN_TYPEERR, "generator has already finished")
>  MSG_DEF(JSMSG_TYPEDOBJECT_TOO_BIG, 360, 0, JSEXN_ERR, "Type is too large to allocate")
>  MSG_DEF(JSMSG_TYPEDOBJECT_NOT_TYPE_OBJECT, 361, 0, JSEXN_ERR, "Expected a type object")
> +MSG_DEF(JSMSG_DEPRECATED_SOURCE_URL,  362, 0, JSEXN_SYNTAXERR, "Using //@ to indicate sourceURL pragmas is deprecated. Use //# instead")
> +MSG_DEF(JSMSG_ALREADY_HAS_SOURCEURL,  363, 1, JSEXN_ERR,      "{0} is being assigned a source URL, yet already has one")

Rather than introducing two new error codes, you should just drop {1} into the existing messages, and pass the string to use to the error reporting functions.

::: js/src/jsscript.cpp
@@ +1324,5 @@
> +            }
> +            return false;
> +        }
> +        sourceURL_[sourceURLLen] = '\0';
> +    }

(I wonder if we could have an XDRState<mode>::codeLengthAndChars, and just use that twice.)

Comment 12

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

https://hg.mozilla.org/integration/fx-team/rev/66f4df0b1cb8

Comment 13

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

Backed out and relanded due to mangled commit message. This was kind of a mess :(

https://hg.mozilla.org/integration/fx-team/rev/951090191c43
https://hg.mozilla.org/integration/fx-team/rev/389e16afcbde
https://hg.mozilla.org/integration/fx-team/rev/1f1d6e481cec

Comment 14

[Ryan VanderMeulen [:RyanVM]]

FYI, DONTBUILD can save a lot of redundant builds and tests in such situations. Also, your saga is why we always recommend |hg out| prior to pushing :)

Comment 15

[Ryan VanderMeulen [:RyanVM]]

You can also consolidate the backout and re-land in one push to save resources.

Comment 16

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

Apologies! The reason I didn't do one re-land was because I was frantic/panicking and didn't want to break anyone else's stuff. Will be more careful next time!

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/66f4df0b1cb8
https://hg.mozilla.org/mozilla-central/rev/1f1d6e481cec