Closed
Bug 243869
Opened 20 years ago
Closed 20 years ago
js_ReportUncaughtException doesn't propagate filename/lineno from error object
Categories
(Core :: JavaScript Engine, defect, P3)
Tracking
()
VERIFIED
FIXED
mozilla1.8alpha1
People
(Reporter: WeirdAl, Assigned: brendan)
References
()
Details
(Keywords: js1.5, testcase)
Attachments
(4 files, 6 obsolete files)
1.62 KB,
application/xhtml+xml
|
Details | |
2.92 KB,
patch
|
Details | Diff | Splinter Review | |
4.47 KB,
patch
|
shaver
:
review+
|
Details | Diff | Splinter Review |
2.37 KB,
text/plain
|
Details |
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.
Reporter | ||
Comment 1•20 years ago
|
||
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•20 years ago
|
||
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.
Assignee: js-console → general
Component: JavaScript Console → JavaScript Engine
QA Contact: jrgmorrison → pschwartau
Assignee | ||
Comment 3•20 years ago
|
||
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
Assignee: general → brendan
Priority: -- → P3
Target Milestone: --- → mozilla1.8alpha
Reporter | ||
Comment 4•20 years ago
|
||
(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...
Assignee | ||
Comment 5•20 years ago
|
||
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 ***
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → DUPLICATE
Comment 6•20 years ago
|
||
Except in this case Error is getting 3 arguments, so the code linked to in comment 4 should work fine.
Assignee | ||
Comment 7•20 years ago
|
||
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•20 years ago
|
||
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•20 years ago
|
||
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.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Assignee | ||
Comment 10•20 years ago
|
||
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
Status: REOPENED → ASSIGNED
Assignee | ||
Updated•20 years ago
|
Summary: JS doesn't notify JS Console of fileName, lineNumber properties of "throw new Error('foo','bar.xml',19)"; → js_ReportUncaughtException doesn't propagate exception filename/lineno
Reporter | ||
Comment 11•20 years ago
|
||
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.
Assignee | ||
Comment 12•20 years ago
|
||
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
Summary: js_ReportUncaughtException doesn't propagate exception filename/lineno → js_ReportUncaughtException doesn't propagate filename/lineno from error object
Assignee | ||
Comment 13•20 years ago
|
||
*** Bug 246699 has been marked as a duplicate of this bug. ***
URL: javascript:throw 1
Comment 14•20 years ago
|
||
Attachment #150945 -
Attachment description: handle throw 1 → [draft] handles throw 1 (mozilla).
xpcshell would need the same change in JS_ExecuteScript. Change also seems to fit JS_EvaluateUCScriptForPrincipals.
Comment 15•20 years ago
|
||
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)
Attachment #150945 -
Attachment is obsolete: true
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.
Attachment #153388 -
Flags: review+
Assignee | ||
Comment 17•20 years ago
|
||
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•20 years ago
|
||
mozilla/js/src/jsapi.c 3.176
Attachment #153388 -
Attachment is obsolete: true
Assignee | ||
Comment 19•20 years ago
|
||
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
Attachment #153436 -
Flags: review-
Assignee | ||
Comment 20•20 years ago
|
||
Oh, and lose the js_ prefix for the static helper -- no need, not prevailing style, etc. /be
Comment 21•20 years ago
|
||
Attachment #153436 -
Attachment is obsolete: true
Assignee | ||
Comment 22•20 years ago
|
||
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
Attachment #153442 -
Flags: review-
Comment 23•20 years ago
|
||
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.
Assignee | ||
Comment 24•20 years ago
|
||
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•20 years ago
|
||
yes that'd be nice, but i'm having trouble getting a code view that works.
Attachment #153442 -
Attachment is obsolete: true
Assignee | ||
Comment 26•20 years ago
|
||
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
Assignee | ||
Comment 27•20 years ago
|
||
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
Assignee | ||
Comment 28•20 years ago
|
||
No jsapi.c patch here, just the files needed to fix this bug report. /be
Assignee | ||
Comment 29•20 years ago
|
||
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
Attachment #153806 -
Flags: review?(shaver)
Comment 30•20 years ago
|
||
+ 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?
Assignee | ||
Comment 31•20 years ago
|
||
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•20 years ago
|
||
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•20 years ago
|
||
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•20 years ago
|
||
Comment on attachment 167064 [details] [diff] [review] proposal - just changes for js_ReportUncaughtException (bug summary) brendan; what do you think?
Attachment #167064 -
Flags: review?(brendan)
Attachment #153806 -
Flags: review?(shaver) → review+
Assignee | ||
Comment 35•20 years ago
|
||
timeless: was the patch shaver just r+'d not correct, or did you have another reason to attach that proposal? /be
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.
Assignee | ||
Comment 37•20 years ago
|
||
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
Attachment #153806 -
Attachment is obsolete: true
Attachment #153806 -
Flags: review+
Assignee | ||
Updated•20 years ago
|
Attachment #167064 -
Attachment is obsolete: true
Attachment #167064 -
Flags: review?(brendan)
Assignee | ||
Comment 38•20 years ago
|
||
Assignee | ||
Comment 39•20 years ago
|
||
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
Attachment #168260 -
Flags: review?(shaver)
Comment on attachment 168260 [details] [diff] [review] better patch (jsexn.c only) Best yet! (I'm going out to buy your cereal right now!)
Attachment #168260 -
Flags: review?(shaver) → review+
Assignee | ||
Comment 41•20 years ago
|
||
Fixed on trunk. /be
Status: ASSIGNED → RESOLVED
Closed: 20 years ago → 20 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 42•20 years ago
|
||
VERIFIED Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a6) Gecko/20041209
Status: RESOLVED → VERIFIED
Comment 43•19 years ago
|
||
Alex, with your permission this will be included in the javascript test library.
Reporter | ||
Comment 44•19 years ago
|
||
Be my guest. :) I've submitted something that landed in the test suite before.
Comment 45•19 years ago
|
||
js1_5/Regress/regress-243869.js checked in.
Updated•19 years ago
|
Flags: testcase+
You need to log in
before you can comment on or make changes to this bug.
Description
•