243869 - js_ReportUncaughtException doesn't propagate filename/lineno from error object

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Alex Vincent [:WeirdAl]]

I have a testcase which forcibly sets the fileName and lineNumber properties of 
a constructed Error() object (so as to bypass bug 119719) and rethrow it.  
Unfortunately, the JavaScript Console does not receive notification of these 
properties for some reason.

Comment 1

[Alex Vincent [:WeirdAl]]

Attachment: testcase

I don't know exactly where to assign this bug; I have not found the source code
within Mozilla which actually sets the error reporter.	I cc'd pschwartau,
because although I don't think this is a JSENG problem, he may know where and
to whom this bug really belongs.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Mozilla sets the error reporter at
http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsJSEnvironment.cpp#1403

If I add the following line to the beginning of the NS_ScriptErrorReporter function:

    fprintf(stderr, "message: '%s', report: %p, file: '%s', line: %d\n",
          message, report,
          report ? report->filename : "",
          report ? report->lineno : 0);

and then click the two buttons in that testcase, I see:

message: 'TypeError: "A".toNumber is not a function', report: 0x864d8e8, file:
'http://bugzilla.mozilla.org/attachment.cgi?id=148705&action=view', line: 8
message: 'uncaught exception: Error: Test Error', report: 0xbfffdfe0, file:
'(null)', line: 0

So the JSErrorReport object we're getting has totally ignored the stuff you
passed to Error().  Which sure looks like a JS eng bug to me.

Comment 3

[Brendan Eich [:brendan]]

For some strange reason, jsexn.c's Exception (the common subroutine of the
*Error constructors) does not default to caller filename and line number when
called with no arguments.

/be

Comment 4

[Alex Vincent [:WeirdAl]]

(In reply to comment #3)

I'd blame http://lxr.mozilla.org/seamonkey/source/js/src/jsexn.c#590 for 
that... or possibly lines 1062-1069, if I read the source right.

It appears that, at least under the code around line 590, that if you call Error
() with 0 arguments or 1 argument, you get an empty string for the filename.  
If I'm right (and I really don't know if I am or not), then what you saw there 
was bug 119719 again.  That bug hasn't received any love at all...

Comment 5

[Brendan Eich [:brendan]]

Duping and taking the orphaned bug (which of course has received zero attention,
as khanson has not worked on JS for years).

/be

*** This bug has been marked as a duplicate of 119719 ***

Comment 6

[Boris Zbarsky [:bzbarsky]]

Except in this case Error is getting 3 arguments, so the code linked to in
comment 4 should work fine.

Comment 7

[Brendan Eich [:brendan]]

bz: if by "In this case" you mean attachment 148705 [details], I see new Error() -- no
args passed.  What am I missing?

/be

Comment 8

[Boris Zbarsky [:bzbarsky]]

The button says "new Error()".  The code says:

function bar() {
  try {
    var f = new Error("Test Error", document.location, 19);
    throw f;
  }
  catch(e) {
    test.value = (e.fileName + "#" + e.lineNumber + "\n" + e.message);
    throw e;
  }
}

Comment 9

[Boris Zbarsky [:bzbarsky]]

So the textarea shows that the filename/linenumber on the JS exception object
are set right.  But the the JSErrorReport the error reporter gets never sees
those values.

Given that, and rereading comment 3, it looks like this was dupped incorrectly,
so reopening.

Comment 10

[Brendan Eich [:brendan]]

Well, shoot -- I swear I read the code and saw new Error().

This is mccabe's fault :-/.  See
http://lxr.mozilla.org/mozilla/source/js/src/jsexn.c#1064.

/be

Comment 11

[Alex Vincent [:WeirdAl]]

I'm (usually) not that nuts when it comes to writing testcases.  :D

One thing this bug and bug 119719 have in common: in both cases, the exception 
is fired using a throw statement... and later, we get buggy behavior.  Where is 
the throw statement in JS implemented?  That may give us a big clue on where 
the real fixes need to go.

Comment 12

[Brendan Eich [:brendan]]

Alex: see the lxr link in comment 10 for where the fix to *this* bug goes.  The
throw statement's implementation is fine, and you can't diagnose JS bugs that
way. Source gets turned into bytecode, and APIs come into play.  In this case,
the lack of a catch leaves js_ReportUncaughtException trying to report an error,
but it fails to reconstruct the report from the error object's properties. 
That's all.

The other bug, bug 119719, is obviously different.  It's reporting a lack of
code to set default arguments to useful values.

/be

Comment 13

[Brendan Eich [:brendan]]

*** Bug 246699 has been marked as a duplicate of this bug. ***

Comment 14

[timeless]

Attachment: [draft] handles throw 1 (mozilla). xpcshell would need the same change in JS_ExecuteScript. Change also seems to fit JS_EvaluateUCScriptForPrincipals.

Comment 15

[timeless]

Attachment: [draft] crashes less, provides some error reporting and stability

Testcase:
http://www.mvdsl.com/downloads/soapbox.html

click the link in a standard build and you get:
Error: uncaught exception: [Exception... "Component returned failure code:
0x80004005 (NS_ERROR_FAILURE) [nsISOAPCall.invoke]"  nsresult: "0x80004005
(NS_ERROR_FAILURE)"  location: "JS frame ::
http://www.mvdsl.com/downloads/soapbox.js :: test :: line 8"  data: no]

click the link w/ my patch and i get:
Error: uncaught exception: [Exception... "Component returned failure code:
0x804b000d [nsISOAPCall.invoke]"  nsresult: "0x804b000d (<unknown>)"  location:
"JS frame :: http://www.mvdsl.com/downloads/soapbox.js :: test :: line 8" 
data: no]
Source File: javascript:test('It worked!')
Line: 1

http://viper.haque.net/~timeless/nsError.js says:
js> analyze(0x804b000d)
nsresult([/*unsigned:*/2152398861, /*signed:*/-2142568435, /*hex:*/0x804b000d])

Module: NS_ERROR_MODULE_NETWORK
Severity: 1
Code: 13
IsSuccessCode: false
Name: NS_ERROR_CONNECTION_REFUSED

Which is indeed the error that is internally floated around (not sure why, i
was asked about this by a guy on #soap)

Comment 16

[Mike Shaver (:shaver emeritus)]

Comment on attachment 153388 [details] [diff] [review]
[draft] crashes less, provides some error reporting and stability

I dislike the repetitive code, but commonizing to a function that takes &frame,
 and optionally-NULL script would probably not be worth the effort.

Comment 17

[Brendan Eich [:brendan]]

Comment on attachment 153388 [details] [diff] [review]
[draft] crashes less, provides some error reporting and stability

I'd prefer it if you made a static wrapper for js_ReportUncaughtException, just
for jsapi.c, that did the repetitive code and took (cx, script) params.

/be

Comment 18

[timeless]

Attachment: with static function

mozilla/js/src/jsapi.c	3.176

Comment 19

[Brendan Eich [:brendan]]

Comment on attachment 153436 [details] [diff] [review]
with static function

>+++ jsapi.c
>@@ -93,6 +93,20 @@

Hmm, way early in the file.

> #define CHECK_REQUEST(cx)       ((void)0)
> #endif
> 
>+static JSBool
>+js_ReportUncaughtExceptionForScript(JSContext *cx, JSScript *script)
>+{
>+    JSStackFrame frame;
>+    JSBool ok;

Empty line here, please -- you know prevailing style, adhere to it.

>+    memset(&frame, 0, sizeof frame);
>+    frame.script = script;
>+    frame.pc = script->main;
>+    cx->fp = &frame;
>+    ok = js_ReportUncaughtException(cx);
>+    cx->fp = NULL;
>+    return ok;
>+}
>+
> JS_PUBLIC_API(int64)
> JS_Now()
> {
>@@ -3473,7 +3487,7 @@ JS_ExecuteScript(JSContext *cx, JSObject

Wow, way later in the file.  How about moving the static helper down to just
before JS_ExecuteScript?  That's also prevailing style, and a good idea.

>@@ -3630,8 +3644,13 @@ JS_CallFunctionValue(JSContext *cx, JSOb
>     CHECK_REQUEST(cx);
>     if (!js_InternalCall(cx, obj, fval, argc, argv, rval)) {
> #if JS_HAS_EXCEPTIONS
>-        if (!cx->fp)
>-            js_ReportUncaughtException(cx);
>+        if (!cx->fp) {
>+            JSScript *script = NULL;
>+            JSFunction *fun = JS_ValueToFunction(cx, fval);
>+            if (fun)
>+                script = fun->u.script;

Oops, if !fun there was an error.

>+            js_ReportUncaughtExceptionForScript(cx, script);

But really, this case is different.  The jsval fval denotes a callable object,
and it needs to be protected during JS_CallFunctionVCalue from GC.  That
callable object may or may not be a function object with a script, but we must
protect it, not just some private data hanging off one particular case of "it".

So I would change js_ReportUncaughtExceptionForScript to take (JSContext *cx,
JSScript *script, jsval fval) and have it set frame.rval = fval in addition to
frame.script = script.	That will protect the callable object including any
JSScript hanging off its private data struct.

Then you don't need any block-local ugliness in JS_CallFunctionValue's if
(!cx->fp) case.

/be

Comment 20

[Brendan Eich [:brendan]]

Oh, and lose the js_ prefix for the static helper -- no need, not prevailing
style, etc.

/be

Comment 21

[timeless]

Attachment: private static function with jsval arg

Comment 22

[Brendan Eich [:brendan]]

Comment on attachment 153442 [details] [diff] [review]
private static function with jsval arg

>Index: jsapi.c
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsapi.c,v
>retrieving revision 3.175
>diff -upw -r3.175 jsapi.c
>--- jsapi.c
>+++ jsapi.c
>@@ -3466,6 +3466,29 @@ JS_DecompileFunctionBody(JSContext *cx, 
>     return str;
> }
> 
>+#if JS_HAS_EXCEPTIONS
>+static JSBool
>+ReportUncaughtExceptionForScript(JSContext *cx, JSScript *script, jsval fval)
>+{
>+    JSStackFrame frame;
>+    JSBool ok;

Empty line here, again, please -- separate declarations from statements.

>+    JSFunction *fun = NULL;
>+    if (fval) {
>+        fun = JS_ValueToFunction(cx, fval);
>+        if (fun)
>+            script = fun->u.script;
>+    }

Delete all of the above hunk, again there is no need for this if you root fval
by storing it in cx->fp->rval.

>@@ -3473,7 +3496,7 @@ JS_ExecuteScript(JSContext *cx, JSObject
>     if (!js_Execute(cx, obj, script, NULL, 0, rval)) {
> #if JS_HAS_EXCEPTIONS
>         if (!cx->fp)
>-            js_ReportUncaughtException(cx);
>+            ReportUncaughtExceptionForScript(cx, script, 0);

JSVAL_NULL would be nicer than 0.

>@@ -3582,7 +3605,7 @@ JS_EvaluateUCScriptForPrincipals(JSConte
>     ok = js_Execute(cx, obj, script, NULL, 0, rval);
> #if JS_HAS_EXCEPTIONS
>     if (!ok && !cx->fp)
>-        js_ReportUncaughtException(cx);
>+        ReportUncaughtExceptionForScript(cx, script, 0);

Ditto.

/be

Comment 23

[timeless]

brendan: this code was custom shaped so that it would arrive at
<http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/jscntxt.c&rev=3.51&mark=597-599,614-615,617-620#596>
with the required elements.

a lack of fp->script means that this code is useless.

Comment 24

[Brendan Eich [:brendan]]

timeless: that's not the problem.  You don't want to use the script's filename
and line number, because the error-as-exception could have been thrown from some
function called deep within a call graph reached from the top-level script.

The right fix is over in jsexn.c, line 1064 and below, as mentioned in comment 10.

/be

Comment 25

[timeless]

Attachment: recover from current crashers

yes that'd be nice, but i'm having trouble getting a code view that works.

Comment 26

[Brendan Eich [:brendan]]

So shaver r='d hastily, and I neg'd/obsoleted, but it didn't prevent the
checkin. timeless kindly backed out, but not before the patch caused bug 252032.
 Now that we are back to status-quo ante, I'll patch.

/be

Comment 27

[Brendan Eich [:brendan]]

Fix the URL.  If you throw 1 or anything other than a *Error instance, you won't
get filename/lineno propagation.  There is no property storage in primitive
types such as number, and there's no internal properties for filename and lineno
in an arbitrary object that could be thrown.  Only the *Error classes support
keeping such props.

/be

Comment 28

[Brendan Eich [:brendan]]

Attachment: proposed fix

No jsapi.c patch here, just the files needed to fix this bug report.

/be

Comment 29

[Brendan Eich [:brendan]]

Comment on attachment 153806 [details] [diff] [review]
proposed fix

shaver, I fixed the indentation glitch at

+	 (reportp->errorNumber == JSMSG_UNCAUGHT_EXCEPTION ||
+	 reportp->errorNumber == JSMSG_UNCAUGHT_EXCEPTION_EX)) {

/be

Comment 30

[timeless]

+            filename = JS_NewStringCopyZ(cx, fp->script->filename);
+            if (!filename) {
+                ok = JS_FALSE;
+                goto out;
+            }
+        } else {
+            filename = cx->runtime->emptyString;

are there other allocs past this one? if not, couldn't you at least propogate 
what was available?

Comment 31

[Brendan Eich [:brendan]]

timeless: what do you mean?  Propagate what?  Failure must not be hidden here,
so if JS_NewStringCopyZ fails, we need to bail right away.

/be

Comment 32

[timeless]

I suspect your patch will lead to output like:

Error: uncaught exception thrown from
chrome://inspector/content/viewers/dom/dom.js:829: [Exception... "Node was not
found" code: "8" nsresult: "0x80530008 (NS_ERROR_DOM_NOT_FOUND_ERR)" location:
"chrome://inspector/content/viewers/dom/dom.js Line: 829"]

if your patch fills the filename and line# in the object, then the javascript
console will show the filename/line number 3 times. note it's fairly important
that the javascript console have access to the filename/line number fields.

that said, some variant of some patch for this really should be pushed for 1.8a6.

Comment 33

[timeless]

Attachment: proposal - just changes for js_ReportUncaughtException (bug summary)

this patch results in:
Error: Node was not found
Source File: chrome://inspector/content/viewers/dom/dom.js
Line: 829
in the jsconsole. yes it means the jsconsole loses all those pretty xpconnect
error codes. if people want them, they can try to use venkman, but i wish them
luck, all i get from venkman is crashes like
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=TB2181824E
:)

Comment 34

[timeless]

Comment on attachment 167064 [details] [diff] [review]
proposal - just changes for js_ReportUncaughtException (bug summary)

brendan; what do you think?

Comment 35

[Brendan Eich [:brendan]]

timeless: was the patch shaver just r+'d not correct, or did you have another
reason to attach that proposal?

/be

Comment 36

[Mike Shaver (:shaver emeritus)]

Brendan: you gonna check that patch in, or what?

Forcing people to go to venkman for XPConnect error codes is a non-starter, but
thanks for the attempt to tidy the output.

Comment 37

[Brendan Eich [:brendan]]

Comment on attachment 153806 [details] [diff] [review]
proposed fix

I'm not checking this one in -- we don't want JSMSG_UNCAUGHT_EXCEPTION_EX, or
needed it.

/be

Comment 38

[Brendan Eich [:brendan]]

Attachment: better patch (jsexn.c only)

Comment 39

[Brendan Eich [:brendan]]

Comment on attachment 168260 [details] [diff] [review]
better patch (jsexn.c only)

This gives us the right joy in the JS console (linked Source: URI, line number,
and original message one line up).

/be

Comment 40

[Mike Shaver (:shaver emeritus)]

Comment on attachment 168260 [details] [diff] [review]
better patch (jsexn.c only)

Best yet!

(I'm going out to buy your cereal right now!)

Comment 41

[Brendan Eich [:brendan]]

Fixed on trunk.

/be

Comment 42

[Alex Vincent [:WeirdAl]]

VERIFIED Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a6) Gecko/20041209

Comment 43

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/Regress/regress-243869.js

Alex, with your permission this will be included in the javascript test
library.

Comment 44

[Alex Vincent [:WeirdAl]]

Be my guest.  :)  I've submitted something that landed in the test suite before.

Comment 45

[Bob Clary [:bc] (inactive)]

js1_5/Regress/regress-243869.js checked in.