Closed Bug 904144 Opened 11 years ago Closed 11 years ago

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


(Core :: JavaScript Engine, defect)

25 Branch
Not set





(Reporter: fitzgen, Assigned: fitzgen)




(1 file, 3 obsolete files)

Same way we parse and expose "//# sourceMappingURL=..." directives, we should do the same with "//# sourceURL=...".
See Also: → 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
Attached patch wip (obsolete) — Splinter Review
Adds Debugger.Source.prototype.displayURL, and all tests pass, but the code is rather ugly.
It would sure be nice to common up the sourceMappingURL and sourceURL parsing code.
Yup, that was my plan, hence the "ugly" statement ;)
Attached patch wip 2 (obsolete) — Splinter Review
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!)

Attachment #801967 - Attachment is obsolete: true
Attached patch bug-904144-source-url.patch (obsolete) — Splinter Review
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?
Increments the XDR version number subtrahend.
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 on attachment 803423 [details] [diff] [review]

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 on attachment 803423 [details] [diff] [review]

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+
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!
Closed: 11 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.