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

RESOLVED FIXED in mozilla27

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: fitzgen, Assigned: fitzgen)

Tracking

25 Branch
mozilla27
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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/
Blocks: 833744
See Also: → bug 583083
Assignee: general → nfitzgerald
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
Summary: [jsdbg2] parse "//# sourceURL=..." directives and expose them on Debugger.Script → [jsdbg2] parse "//# sourceURL=..." directives and expose them on Debugger.Source
Created attachment 801967 [details] [diff] [review]
wip

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

Comment 3

4 years ago
It would sure be nice to common up the sourceMappingURL and sourceURL parsing code.
Yup, that was my plan, hence the "ugly" statement ;)
Created attachment 802639 [details] [diff] [review]
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!
Attachment #801967 - Attachment is obsolete: true
Created attachment 802672 [details] [diff] [review]
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.
Attachment #802639 - Attachment is obsolete: true
Attachment #802672 - Flags: review?(jimb)
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?
Created attachment 803423 [details] [diff] [review]
bug-904144-source-url.patch

Increments the XDR version number subtrahend.

https://tbpl.mozilla.org/?tree=Try&rev=5692fe171ef3
Attachment #802672 - Attachment is obsolete: true
Attachment #802672 - Flags: review?(jimb)
Attachment #803423 - Flags: review?(jimb)
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

4 years ago
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

4 years ago
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.)
Attachment #803423 - Flags: review?(jimb) → review+
https://hg.mozilla.org/integration/fx-team/rev/66f4df0b1cb8
Whiteboard: [fixed-in-fx-team]
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
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 :)
You can also consolidate the backout and re-land in one push to save resources.
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!
https://hg.mozilla.org/mozilla-central/rev/66f4df0b1cb8
https://hg.mozilla.org/mozilla-central/rev/1f1d6e481cec
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.