50447 - JS Exceptions should have filename/linenumber properties

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Robert Ginda]

The ECMA spec only defines Error.prototype.[constructor, name, message, and
toString] properties.  Is anyone aware of the reason "fileName" and "lineNumber"
were left out?

I can think of three possibilities:
1. Security?  I could imagine that knowing the filename a particular script
lived in, or even a part of it's path, could be used for something evil.  Though
I'm not exactly sure what.

2. Embedding Specific?  JS might be embedded in an environment w/o filenames? 
If that's the case, I think we should be free to add these new properties.

3. Holdover from Java?  I imagine you don't get this info from Java, because the
source ins't available at runtime, but, hey, this is JS.

Either way, from my read, the spec doesn't mention anything about extending
Error.prototype being either a good or a bad thing.

In a situation where the only useful thing to do about an exception is report
the source and abort whatever it was that was going on, these two fields are the
most helpful.  For example, if a user were to load a script at runtime (via some
solution for bug 48974), getting "Syntax Error: syntax error" as an error
message would be nearly useless.

So, I've worked up a patch to change this (surprise.)  jsexn.diff adds these
properties to exception objects.  It does not (AFAIK) interfere with the
existing exception object behavior.  The new properties are NOT printed in the
toString() method.  They appear as optional 2nd and 3rd paramaters the the
constructor, and only appear in the return value of toSource() if they are
present[*] in the object.  They are, however, read, write, delete, and
enumerable.

file_and_line.js is a testcase for these new properties.  If you'd like to check
it for yourself, save it in js/tests/ecma_3/Exceptions/[**] and run `perl
jsDriver.pl -e smdebug -l tests/ecma_3/Exceptions/file_and_line.js`

flames, comments, suggestions?

* where "present" is defined as String(fileName) is not "", and
Int32(lineNumber) is not 0
** This won't go under the ecma_3 suite (rather, a new js1_5 suite) if it gets
checked in, but currently that's the only suite with a compatible shell.js
file.  Some of the tests currently in ecma_3 should be refactored into js1_5 as
well.

Comment 1

[Robert Ginda]

Attachment: jsexn.diff

Comment 2

[Robert Ginda]

Attachment: file_and_line.js

Comment 3

[Brendan Eich [:brendan]]

+    if (!JS_GetProperty(cx, obj, js_lineno_str, &v) ||
+        !js_ValueToInt32 (cx, v, &lineno)) {
+        /* XXX should toSource() really fail if lineno can't be converted to
+         * a number? */
+            return JS_FALSE;
+    }

The return is overindented.

+        if (lineno_as_str)
+            /* no filename, but have line number,
+             * need to append ``, "", {lineno_as_str}'' */
+            length += 6 + lineno_as_str->length;

Prevailing style is to use braces around multi-line then statements
and to use block comments with stars aligned vertically for multi-line
comments

+    if (reportp)
+    {
+        if (reportp->filename)
+        {
+            fnamestr = JS_NewStringCopyZ(cx, reportp->filename);
+            /* Store 'filename' as a javascript-visible value. */
+            if (!JS_DefineProperty(cx, errObject, js_filename_str,
+                                   STRING_TO_JSVAL(fnamestr), NULL, NULL,
+                                   JSPROP_ENUMERATE)) {
+                return JS_FALSE;
+            }
+            /* Store 'lineno' as a javascript-visible value. */
+            if (!JS_DefineProperty(cx, errObject, js_lineno_str,
+                                   INT_TO_JSVAL((int)reportp->lineno), NULL,
+                                   NULL, JSPROP_ENUMERATE)) {
+                return JS_FALSE;
+            }
+        }
+        
+    }
+    

Wahhh, non-conforming brace style alert!

     }
+    if (!JS_DefineProperty(cx, protos[0], js_filename_str,
+                           STRING_TO_JSVAL(cx->runtime->emptyString),
+                           NULL, NULL, JSPROP_ENUMERATE)) {
+        return NULL;
+    }
+    if (!JS_DefineProperty(cx, protos[0], js_lineno_str,
+                           INT_TO_JSVAL(0),
+                           NULL, NULL, JSPROP_ENUMERATE)) {
+        return NULL;
+    }

Why define these properties on the prototypes?

Truly anal code would worry about a line number > 2**30 - 1, which won't fit in 
an INT jsval, but I say WIB -- why worry?

Nits aside, looks good.  Pick those nits and a=brendan.

/be

Comment 4

[Robert Ginda]

The message property was already being defined there, and like a good little
lemming, I did the same.  Putting the properties there does at least guarantee
that they exist on all errors, and have known default values.  

Other nits picked, fix checked in.

Comment 5

[aaronb]

The summing effect for lineNumber on calls to Script(), Script.compile() and 
eval() is no good. If my script compiles another script using any of these 
methods, I want to know where in the passed-in script the error occurs. The sum 
of the line of the call and the line in the passed in script means nothing to 
anyone!
 

Comment 6

[Robert Ginda]

May change doen't actually do any summing, it just tacks whatever lineno is in
the error report onto the Error object.  The line number is exactly what you'd
have seen on the console if you'd let the exception go uncaught.  That being
said, I agree line numbers reported by eval(), etc. calls are somewhat useless.

An uncaught eval() error might fool someone into thinking some other part of the
script failed (regardless of wether we sum or not.)  How about we set a special
filename in eval() calls (it appears that eval() calls and Script[.compile]()
calls go through jsobj.c:897).  Maybe something like
"http://calling/script/filename.js(eval@callLineNo)" and set the starting line
number to 1?

I don't particularly like that filename scheme, maybe someone can come up with a
better idea.

Comment 7

[John Bandhauer]

I don't like encoding line numbers into the file name. This will just trip up 
debugger clients more sophisticated than the simple error reporter. The 
(data rich) stack is available via the jsapi. Why does eval use a line number 
other than 1?

Comment 8

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

eval() might well be operating on strings that are more than one line, in which
case the line number should match just like it does in any other script.

It seems a shame that you could have identical file/line pairs for two different
syntax errors, because eval inherits the filename, but without an altered
filename (you could cons up a data: url with the contents of the eval'd script,
tee hee!) I don't see how to disambiguate.

Comment 9

[Robert Ginda]

We could hide the line number after an embedded null in the filename to keep
debuggers happy! >:)  (sorry, I'm mostly kidding.)

but really...

js> void(eval ("o.f =\nfunction () { bla; }"));
js> foo;
typein:7: ReferenceError: foo is not defined
js> o.f();
typein:7: ReferenceError: bla is not defined
js> 

Would it be better to report a line number of two?  I don't think so.  I think
they're both equally useless and confusing.

Here are the choices I see, anybody got others?

a. Leave it as is.
If we sum the line numbers, a multi-line eval() wrapped in a try/catch needs to
know at what line it appears in in order to report a useful line number.  If
code introduced by the eval() is called after the eval(), you're SOL.

b. Change it not to sum the line numbers.
If we don't sum the line numbers, the newbie who eval()s a single line in the
middle of his script will be quite confused when some error occurs at line 1,
which happens to be a comment, or some unrelated piece of code.

c. Munge the filename somehow, don't sum any line numbers.
Something easy for debuggers "in the know" to parse, add a pref to turn it off
when working with an existing debugger.  (What good is the debugger going to be
if it takes you to the wrong line number of the right source, anyway?)

This one's got my vote.

d. Append optional params to eval() and Script() calls to allow passing in of
filename/linenumber.
Security problems with error spoofing?
MORE undocumented optional params?

d. some api extension to allow for "evalOffset"s in error reports.

Comment 10

[aaronb]

An alternative solution:

Nest SyntaxError objects. The top level object should always report the line 
number in the top-level script (which is usually what is desired). If the top-
level script knows an error may be due to code compiled by eval(), 
Script.compile(), etc., it can look at exception.next.lineNumber (or whatever) 
to get the line number within the dynamically compiled code.

The objects could be nested arbitrarily deeply. This way, a smart top-level 
script could find exactly what code was at fault, even if eval()s were nested 
ten deep.

Comment 11

[Phil Schwartau]

Verified Fixed in JS shell:

js> var err = new Error()

js> err.lineNumber
0
js> err.fileName

js> typeof err.lineNumber
number
js> typeof err.fileName
string

js> typeof err.__proto__.fileName
string
js> typeof err.__proto__.lineNumber
number

js> err.hasOwnProperty("fileName")
true
js> err.hasOwnProperty("lineNumber")
true

Comment 12

[Phil Schwartau]

Also, from Rob's testcase: 

js> var err = new InternalError ("msg", "file", 2);
js> err.fileName
file
js> err.lineNumber
2

Comment 13

[Phil Schwartau]

Rob's testcase added to JS testsuite:

       mozilla/js/tests/js1_5/Exceptions/regress-50447.js

Have added this to rhino-n.tests skip list, as Rhino does not
implement these properties - 

Comment 14

[Bob Clary [:bc] (inactive)]

mozilla/js/tests/js1_5/Exceptions/regress-50447.js is now rhino only and assumes
exceptions have zero lineNumber and empty fileName by default.

mozilla/js/tests/js1_5/Exceptions/regress-50447-1.js is spidermonkey only and
assumes that lineNumber and fileName are initialized from location where the
exception was created.

rhino-n.tests and spidermonkey-n.tests have been updated.