Let the eval "//# sourceURL=..." trick work for stack traces

RESOLVED FIXED in mozilla36

Status

()

Core
JavaScript Engine
--
enhancement
RESOLVED FIXED
7 years ago
a year ago

People

(Reporter: Michael Bolin, Assigned: fitzgen)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {dev-doc-complete})

Trunk
mozilla36
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(relnote-firefox 36+)

Details

(Whiteboard: [DocArea=JS])

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

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

7 years ago
jimb:

Thought I would cc you on this bug. looks like some debugger / console interplay

Updated

7 years ago
Severity: normal → enhancement
OS: Linux → All
Hardware: x86_64 → All
Version: unspecified → Trunk

Comment 2

7 years ago
see also bug 332176

Comment 3

7 years ago
Created attachment 484839 [details]
try console.error

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

7 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

7 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 6

6 years ago
For Greasemonkey's use case, fixing this would also fix bug 307984.
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

6 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.
Duplicate of this bug: 668440

Comment 10

6 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

6 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

6 years ago
As long as Source Maps work for eval (and evalinsandbox) then that would definitely handle Greasemonkey's use case.
Blocks: 833744

Comment 13

4 years ago
sourceURL seems to work for me on FF 19.0

Comment 14

4 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?
Flags: needinfo?(general)
Also, if it's a short enough task, I'm willing to work on it.
Whiteboard: [mentoree waiting for mentor]
Blocks: 880663
Blocks: 880664
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
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)

Comment 20

4 years ago
Do any other browsers do the same thing?
Flags: needinfo?(luke)
See Also: → bug 904144
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?
I was told you were the man to talk to about this.  See comment 21.
Flags: needinfo?(nfitzgerald)
# 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)
No longer blocks: 833744
Assignee: general → nobody

Comment 24

3 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

3 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

3 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

3 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)
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

3 years ago
See Also: → bug 771597, bug 833744

Comment 29

3 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

3 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

3 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: nobody → nfitzgerald
Status: NEW → ASSIGNED
Created attachment 8504737 [details] [diff] [review]
source-url-stack-traces-pt-1.patch
Attachment #8504737 - Flags: review?(shu)
Created attachment 8504738 [details] [diff] [review]
source-url-stack-traces-pt-2.patch
Attachment #8504738 - Flags: review?(shu)
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e80f91bd6677
Blocks: 1082581
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 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+
Created attachment 8505881 [details] [diff] [review]
source-url-stack-traces-pt-1.patch
Attachment #8504737 - Attachment is obsolete: true
Attachment #8505881 - Flags: review+
Created attachment 8505882 [details] [diff] [review]
source-url-stack-traces-pt-2.patch
Attachment #8504738 - Attachment is obsolete: true
Attachment #8505882 - Flags: review+
Carrying over r+s.

New try push to be sure: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=52a0bb1e7fab
Keywords: checkin-needed
Whiteboard: [mentoree waiting for mentor]
https://hg.mozilla.org/integration/mozilla-inbound/rev/3dbe5e1f6a7d
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4d577ba087a
Keywords: checkin-needed
Will this also work for `Function()`?
Keywords: dev-doc-needed
(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

3 years ago
Just wanted to say thanks for pushing on this. Much needed and appreciated.

Comment 44

3 years ago
https://hg.mozilla.org/mozilla-central/rev/3dbe5e1f6a7d
https://hg.mozilla.org/mozilla-central/rev/c4d577ba087a
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36

Comment 45

3 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: --- → ?
(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.
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).
relnote-firefox: ? → 36+
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]
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/
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

2 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"
(In reply to Sean Hogan from comment #51)
Filed bug 1192882 for this issue.

Updated

2 years ago
Depends on: 1150886

Comment 53

a year ago
Is it possible to get the original unadulterated stack (say after a buggy sourcemap is loaded)?
(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.