Closed
Bug 583083
Opened 14 years ago
Closed 10 years ago
Let the eval "//# sourceURL=..." trick work for stack traces
Categories
(Core :: JavaScript Engine, enhancement)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla36
Tracking | Status | |
---|---|---|
relnote-firefox | --- | 36+ |
People
(Reporter: mbolin, Assigned: fitzgen)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete, Whiteboard: [DocArea=JS])
Attachments
(3 files, 2 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
fitzgen
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
fitzgen
:
review+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.9pre) Gecko/20100727 Ubuntu/9.10 (karmic) Namoroka/3.6.9pre Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.9pre) Gecko/20100727 Ubuntu/9.10 (karmic) Namoroka/3.6.9pre Both Firebug and the Webkit Inspector make it possible to name a source code buffer created via eval() by appending a special comment to the end of the code being evaluated using "//@ sourceURL=NAME". This is important when parsing JS stack traces in code that is loaded dynamically via an XHR and eval'd, as explained in detail on this Closure Library bug: http://code.google.com/p/closure-library/issues/detail?id=196 This has already been filed as a bug against V8 (although this feature is supported in the Webkit inspector, apparently it is not available to JS in general): http://code.google.com/p/v8/issues/detail?id=672 Also, while we're on the subject, including character offsets (as Chrome does) in JS stacktraces is critical to debugging obfuscated JS properly. As explained in the Closure bug above, both the line and character numbers are used in Closure source maps to map between the obfuscated code and the original code. Without this, deciphering bugs reported from client-side stack traces is nearly impossible. It would be incredibly helpful if Firefox could support this feature. Reproducible: Always Steps to Reproduce: // This is a simple test to demonstrate the error. var s = [ 'var t = "hello world";', 'throw new Error(t);\n', '//@ sourceURL=foo.js'].join('\n') + '\n'; try { eval(s); } catch (e) { if (e.stack.indexOf('foo.js') < 0) { alert('stack trace was missing source: ' + e.stack); } } Actual Results: currently, the above code alerts a message with the stack trace that includes: Error("hello world")@:0 Expected Results: ideally, it would include: @foo.js:2:0 It is possible to work around the @sourceURL thing by loading the files via <script> tags, but it is harder to monitor the loading of <script> tags as compared to XHRs. (Also, other browsers do not make it as easy to load a set of <script> tags in parallel and then evaluate them in order, as explained here: http://groups.google.com/group/closure-library-discuss/browse_thread/thread/d1973d0ddc477aa2) By comparison, there is no reasonable workaround for the lack of character offset information in a stack trace. This information already appears to be available to Firebug (without it, it would not be possible to use the Closure Inspector: http://code.google.com/closure/compiler/docs/inspector.html), so hopefully it is not difficult to expose. For this reason, it is currently more compelling to encourage our users to use Chrome so that we can determine the source of any client-side errors.
Comment 1•14 years ago
|
||
jimb: Thought I would cc you on this bug. looks like some debugger / console interplay
Updated•14 years ago
|
Severity: normal → enhancement
OS: Linux → All
Hardware: x86_64 → All
Version: unspecified → Trunk
Comment 3•14 years ago
|
||
If you replace the line: 'throw new Error(t);\n', with the line 'console.error(t);\n', you get a nice call stack as illustrated here.
Reporter | ||
Comment 4•14 years ago
|
||
OK, but that does not help the use case that I am talking about where the stack trace needs to be sent to the server so it can be deobfuscated. The client JS code cannot read what is written to the console.
Comment 5•14 years ago
|
||
I hit the same problem. I use the @ sourceURL trick. The filenames are ok in Firebug but they are wrong in the stack traces. This is very annoying because stack traces are very difficult to understand with lots of anonymous functions in lots of files loaded through XHR.
Comment 7•13 years ago
|
||
The "character offset" (column number) bug is bug 618650. This bug seems to be about making the JS engine parse //@sourceurl at the end of eval'd programs and treat the eval'ed program as originating from sourceurl (is there any way to add a line number or line and column numbers?). Moving to Core / JavaScript Engine. /be
Assignee: nobody → general
Status: UNCONFIRMED → NEW
Component: Developer Tools → JavaScript Engine
Ever confirmed: true
Product: Firefox → Core
QA Contact: developer.tools → general
Comment 8•13 years ago
|
||
Actually, for Greasemonkey, we'd like something more like "here starts file X" -- because of @require, we actually have multiple files concatenated (inside the same anonymous function scope) and evaluated at once. We could insert fake keys as source URLs, detect them and reverse them back into truth (we mostly do this now) but ideally, whatever is implemented would handle this for us.
Comment 10•13 years ago
|
||
(In reply to comment #9) > *** Bug 668440 has been marked as a duplicate of this bug. *** Not sure I understand the scope of this particular bug, which my bug was marked a duplicate of. Is this bug saying that `// @sourceURL=...` would be an annotation that could be added to a script no matter how it's executed? In other words, will this naming annotation comment be able to work in all of these: eval(), script.text injection, new Function(), setTimeout("string"), <script src=...>, <script>/*inline*/</script>, etc? Will it make this arbitrary name label (usually a filename) available to all the devtools, debugger, etc?
Comment 11•13 years ago
|
||
(In reply to comment #8) > Actually, for Greasemonkey, we'd like something more like "here starts file > X" -- because of @require, we actually have multiple files concatenated > (inside the same anonymous function scope) and evaluated at once. We could > insert fake keys as source URLs, detect them and reverse them back into > truth (we mostly do this now) but ideally, whatever is implemented would > handle this for us. It seems to me that sourceURL has been around and in use by tools like Firebug and Web Inspector for some time, and I'm not sure that we want to monkey with that meaning. Having support for sourceURL in spidermonkey directly would likely allow Firebug to rip out some code... apparently, they're listening to the scripts as they come in via the network stacking and ripping the sourceURLs off then(!) Dealing with multiple concatenated files is actually one of the use cases for Source Maps: https://wiki.mozilla.org/DevTools/Features/SourceMap Source Maps are new and under development (in both Firefox tools and Web Inspector), so now is a good time to speak up if there's something about the implementation that's going on there that wouldn't handle the Greasemonkey case.
Comment 12•13 years ago
|
||
As long as Source Maps work for eval (and evalinsandbox) then that would definitely handle Greasemonkey's use case.
Comment 14•12 years ago
|
||
my bad, I think I misread my unit test. I'm fixing https://code.google.com/p/closure-library/source/browse/closure/goog/module/moduleloader_test.html for latest FF
This would be extremely useful for bug 872421, which is itself on the blocking path i.e. for making (chrome) workers useful.
Blocks: 872421
So, what would it take to get this bug moving (or an equivalent one with SourceMaps + eval())? Is this something that can become a mentored bug?
Updated•12 years ago
|
Flags: needinfo?(general)
Also, if it's a short enough task, I'm willing to work on it.
Updated•12 years ago
|
Whiteboard: [mentoree waiting for mentor]
Assignee | ||
Comment 18•11 years ago
|
||
s/@/#/ as per discussion in dev-js-sourcemap. See also, http://www.softwareishard.com/blog/firebug/firebug-tip-label-dynamic-scripts-with-sourceurl-directive/ which has a nice little regexp for us.
Summary: Let the eval //@sourceURL trick work for stack traces → Let the eval "//# sourceURL=..." trick work for stack traces
Comment 19•11 years ago
|
||
needinfo on a fake account isn't gonna do anything more than the comment alone does. :-) I think I'm tentatively opposed to modifying new Error().stack's behavior. But if you're only affecting some non-web-exposed developer API, that's all cool.
Flags: needinfo?(general) → needinfo?(luke)
Assignee | ||
Updated•11 years ago
|
Comment 21•11 years ago
|
||
I've started looking into resolving this issue. Can someone point me to the right places to look to figure out how source maps are implemented and associated with scripts?
Comment 22•11 years ago
|
||
I was told you were the man to talk to about this. See comment 21.
Flags: needinfo?(nfitzgerald)
Assignee | ||
Comment 23•11 years ago
|
||
# Regarding source maps: I don't think that it is feasible to source map Error#stack itself, but it would be better to do it in tools. We have to fetch source maps over the network, if someone accesses Error#stack before we have downloaded and parsed the source map, are we going to block until we have it? Are we going to give non-source mapped stacks until we have a source map and then switch all of a sudden? Do we want to expose whether or not we are source mapping to js? To content js? Seems sketchy to introduce this kind of non-deterministic behavior to JS directly. With tools (which don't suffer from the above issues), once you have a source map, you can import SourceMap.jsm and use SourceMapConsumer#originalPositionFor to query original source locations given a generated source location. Docs on SourceMap.jsm here: https://github.com/mozilla/source-map Bug to use source maps in the console: bug 670002 But to use source maps in the profiler: bug 923858 # Regarding //# sourceURL=<url>: I landed bug 904144 which adds a "sourceURL" getter to ScriptSource instances. Not sure how to wire the plumbing from that to error stacks, but I assume there is some code somewhere that grabs the "filename" property from a ScriptSource and uses it in the stack frame. Should be pretty easy to just check if scriptSource.hasSourceUrl() and use the source url in that case.
Flags: needinfo?(nfitzgerald)
Updated•10 years ago
|
Assignee: general → nobody
Comment 24•10 years ago
|
||
TL;DR ~ IDEs and apps that support scripting should be able to evaluate strings and build tracebacks. Please include sourceURLs from evaluated code in the error.stack passed to onerror. --- I've been using sourceURLs in a Web based shell, where the user is entering CoffeeScript that gets compiled and evaluated in the browser. Each of the user's inputs has an incrementing sourceURL, like input1.js. To do CoffeeScript tracebacks on runtime errors, the user's input and its source map get preemptively stored in a hash, then the window.onerror hook's used to catch the line and column numbers, and the sourceURL for the JavaScript error. From there, we can build a CoffeeScript traceback, using Moz Source Maps and the stored data. This works nicely on Chromium, but not FireFox, where everything else just works. It's impossible to build a traceback if you can't know which 'file' the line and column number map to. This issue is not specific to languages that use source maps either. If the shell used native JavaScript, it'd still need to know which input line x, col y is actually in. Cheers
Comment 25•10 years ago
|
||
I posted a comment at the end of 2010. Why hasn't this been solved? The consequence for us is that all our front-end developers are switching to Chrome because of this issue. Really sad as they were great fans of FF + Firebug.
Comment 26•10 years ago
|
||
+1 Bruno - I actually went back to FF for a month or two, but reverted to Chrome when it was the only browser my app would work in. I really miss FireFox and Mozilla, and feel like a sellout :(
Comment 27•10 years ago
|
||
(In reply to carl.input from comment #26) > +1 Bruno - I actually went back to FF for a month or two, but reverted to > Chrome when it was the only browser my app would work in. I really miss > FireFox and Mozilla, and feel like a sellout :( This bug is not about any missing functionality for Firefox users so your app should just work fine. It's just a developer-facing issue. Sadly, this issue is more prevalent than it looks like. Source Maps are supported by Firefox since ages, it's just "dynamic" scripts that lack Source Map support. However it seems that quite a lot of compiler tools incl. module loaders for the web produce such code that Firefox does not support Source Maps with. Nick, do you have any comment on when this can be tackled?
Flags: needinfo?(nfitzgerald)
Assignee | ||
Comment 28•10 years ago
|
||
Strictly regarding Error.prototype.stack: There seemed to be consensus (or at least no one was saying otherwise) on es-discuss that it was safe to expose this information to the web via Error.prototype.stack since Chrome already exposes it, so I guess it can be tackled now? Don't know if this requires an intent-to-implement mailing list post, since this would expose new stuff to web content.
Flags: needinfo?(nfitzgerald)
Updated•10 years ago
|
Comment 29•10 years ago
|
||
(In reply to Florian Bender from comment #27) > (In reply to carl.input from comment #26) > > +1 Bruno - I actually went back to FF for a month or two, but reverted to > > Chrome when it was the only browser my app would work in. I really miss > > FireFox and Mozilla, and feel like a sellout :( > > This bug is not about any missing functionality for Firefox users so your > app should just work fine. It's just a developer-facing issue. Not in this case. The app is a shell that renders tracebacks to the user, so this is a user-facing issue for us. It's not just compiler tools; there's a ton of apps that would add scripting if they could. This stuff doesn't actually work in Chrome yet [V8 doesn't provide the line and column number of *syntax* errors], but I'm told that's fixed, just pending. My app uses CoffeeScript, so our syntax/compilation errors are dealt with by the Coffee Compiler. Other programming apps use Skulpt and Brython etc., so they don't depend on the browser for tracebacks at all. There's a lot of user-facing apps that could use tracebacks if they'd ever worked.
Comment 30•10 years ago
|
||
Our app works fine in Firefox and Chrome. All our client scripts are loaded through a special module loader that "evals" them. This loader adds the //@sourceUrl=... annotation at the end of each script. In Chrome this works great: all our source modules are nicely organized in a tree view and we get the filenames and line numbers in stack traces. In FF we don't get the filenames in stack traces. Instead we get the URL of the loader module itself, which is not helpful.
Comment 31•10 years ago
|
||
Exactly. In FF, there's no way of rendering a traceback that includes eval'ed code. Our scripts don't exist when the app's launched, they're created by the user at runtime. In Chrome, currently, you just can't get the line and column numbers on syntax errors.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED
Assignee | ||
Comment 34•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e80f91bd6677
Comment 35•10 years ago
|
||
Comment on attachment 8504737 [details] [diff] [review] source-url-stack-traces-pt-1.patch Review of attachment 8504737 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the InflateString comment addressed. ::: js/src/jsexn.cpp @@ +300,5 @@ > + const char *s = i.scriptFilename(); > + if (s) { > + length = strlen(s); > + foundLength = true; > + filename = InflateString(cx, s, &length); StringBuffer takes care of inflating its own internal buffer, it seems like. It also seems like scriptDisplayURL is the only char16_t thing here, so no need to inflate if we aren't using scriptDisplayURL. ::: js/src/vm/Stack.h @@ +1632,5 @@ > > /* > * Get an abstract frame pointer dispatching to either an interpreter, > * baseline, or rematerialized optimized frame. > */ Driveby: remove this comment block please.
Attachment #8504737 -
Flags: review?(shu) → review+
Comment 36•10 years ago
|
||
Comment on attachment 8504738 [details] [diff] [review] source-url-stack-traces-pt-2.patch Review of attachment 8504738 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/SavedStacks.cpp @@ +679,5 @@ > + locationp->source = AtomizeChars(cx, displayURL, js_strlen(displayURL)); > + if (!locationp->source) > + return false; > + } else { > + const char *filename = iter.scriptFilename(); Style nit: below you use ?: to give filename a default value. Would be nice to make the two consistent. @@ +684,5 @@ > + if (!filename) > + filename = ""; > + locationp->source = Atomize(cx, filename, strlen(filename)); > + if (!locationp->source) > + return false; Style nit: could factor out the |if (!locationp->source) return false| @@ +707,5 @@ > + } else { > + const char *filename = script->filename() ? script->filename() : ""; > + source = Atomize(cx, filename, strlen(filename)); > + if (!source) > + return false; Style nit: ditto on factoring out |if (!source)|
Attachment #8504738 -
Flags: review?(shu) → review+
Assignee | ||
Comment 37•10 years ago
|
||
Attachment #8504737 -
Attachment is obsolete: true
Attachment #8505881 -
Flags: review+
Assignee | ||
Comment 38•10 years ago
|
||
Attachment #8504738 -
Attachment is obsolete: true
Attachment #8505882 -
Flags: review+
Assignee | ||
Comment 39•10 years ago
|
||
Carrying over r+s. New try push to be sure: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=52a0bb1e7fab
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Whiteboard: [mentoree waiting for mentor]
Comment 40•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3dbe5e1f6a7d https://hg.mozilla.org/integration/mozilla-inbound/rev/c4d577ba087a
Keywords: checkin-needed
Assignee | ||
Comment 42•10 years ago
|
||
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #41) > Will this also work for `Function()`? Yup: js> f = new Function("return Error().stack; //# sourceURL=new-function.js") (function anonymous() { return Error().stack; //# sourceURL=new-function.js }) js> print(f()) anonymous@new-function.js:1:8 @typein:2:7
Comment 43•10 years ago
|
||
Just wanted to say thanks for pushing on this. Much needed and appreciated.
Comment 44•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3dbe5e1f6a7d https://hg.mozilla.org/mozilla-central/rev/c4d577ba087a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment 45•10 years ago
|
||
Release Note Request (optional, but appreciated) [Why is this notable]: Dev++ [Suggested wording]: Extend Source Map support to Stack Traces [Links (documentation, blog post, etc)]: (MDN dev-doc-needed)
relnote-firefox:
--- → ?
Assignee | ||
Comment 46•10 years ago
|
||
(In reply to Florian Bender from comment #45) > [Suggested wording]: Extend Source Map support to Stack Traces This doesn't actually have anything to do with source maps, really. Here's a better wording: Use the filename supplied by the `//# sourceURL=<filename>` directive for stack frames in the string returned by the `Error.prototype.stack` getter.
Comment 47•10 years ago
|
||
Added in the release notes with Nick's wording. We exchanged on IRC and he is going to write a blog post to explain a bit more this change (the current wording is quite technical).
Comment 48•10 years ago
|
||
See http://fitzgeraldnick.com/weblog/59/ Could probably need a note on https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/Stack and in some debugging docs.
Whiteboard: [DocArea=JS]
Comment 49•10 years ago
|
||
Thanks Florian, we exchanged on this topic with Nick and I used his blog post. https://www.mozilla.org/en-US/firefox/36.0a2/auroranotes/
Comment 50•10 years ago
|
||
https://developer.mozilla.org/en-US/docs/Tools/Debugger#Debug_eval_sources https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/Stack#Stack_of_eval%27ed_code https://developer.mozilla.org/en-US/Firefox/Releases/36#Developer_Tools
Keywords: dev-doc-needed → dev-doc-complete
Comment 51•9 years ago
|
||
I don't think this is completely fixed. While the stack-trace contains the faked sourceURL - which is ok if you actually catch the error - the error that is logged to the console (when not caught) does not contain the faked sourceURL. For example, var s = [ 'x.y.z === a.b.c;', '//@ sourceURL=foo.js' ].join('\n') + '\n'; eval(s); On Chrome this will log an error indicating the appropriate line in "foo.js"
Assignee | ||
Comment 52•9 years ago
|
||
(In reply to Sean Hogan from comment #51) Filed bug 1192882 for this issue.
Comment 53•8 years ago
|
||
Is it possible to get the original unadulterated stack (say after a buggy sourcemap is loaded)?
Assignee | ||
Comment 54•8 years ago
|
||
(In reply to Jasvir Nagra from comment #53) > Is it possible to get the original unadulterated stack (say after a buggy > sourcemap is loaded)? This bug is unrelated to sourcemaps, it is only about renaming sources via the `//# sourceURL` pragma. In general, `Error().stack` will never be source mapped. Within the debugger, you can always disable source maps as well. https://developer.mozilla.org/en-US/docs/Tools/Debugger/How_to/Use_a_source_map is geared towards how to enable and use source maps, but should give you the info you need to disable them as well.
You need to log in
before you can comment on or make changes to this bug.
Description
•