Giving "new Error" an obj with bogus toString halts JavaScript execution

VERIFIED FIXED

Status

()

Core
JavaScript Engine
VERIFIED FIXED
12 years ago
11 years ago

People

(Reporter: Jesse Ruderman, Assigned: Brian Crowder)

Tracking

(Blocks: 1 bug, {testcase})

Trunk
testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

12 years ago
Split from bug 352742 comment 16:

js> print(1); try{new Error({toString: function() { x.y } })}catch(e){} print(3) 
1
typein:5: x is not defined


Bug 352742 comment 19, from Brian Crowder:

"Jesse, I think the bug you demonstrate in comment #16 is a different bug.  I
need to check the spec on this but I -think- (thanks to shaver for clarifying
this for me) that the core problem is that were are suppressing exceptions
anytime we are building an Exception object, without any regard to whether we
are building one as an implicit response to an error or building one explicitly
(not yet in an error-state).  Can you file as a new bug?"
(Assignee)

Comment 1

12 years ago
Thanks, Jesse.  I haven't got a patch for this yet, and the spec seems to grant me a lot of leeway.

As I perceive it, here is the problem:  the function call is happening inside an Interpret stack which is inside of the Error (really an Exception) constructor.  The Exception constructor makes the sweeping assumption that if it is being called, an exception is being thrown, and so further exceptions should be surpressed.  Combine that with the fact (maybe another bug) that TryMethod explicitly nulls out the context error reporter, and you end up with a silent failure.  The Error object doesn't get built because an error (for which an exception should have been thrown, or at least a report generated) happened during construction, and no error is reported because exceptions can't be propogated from inside of an Exception constructor.

We should fix this bogus assumption that the act of building an Exception object should suppress throwing exceptions.  Instead, error-handlers that build Exception objects implicitly should set this flag themselves, as should the act of actually throwing an exception.

Anyone else have any thoughts on this?
Status: NEW → ASSIGNED
Assignee: general → crowder
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
(Assignee)

Comment 2

12 years ago
FmyI:  https://bugzilla.mozilla.org/show_bug.cgi?id=127136 is the bug for which this recursion-prevention code was originally written.
(Assignee)

Comment 3

12 years ago
Created attachment 241670 [details] [diff] [review]
Altered recursion-prevention (doesn't happen during Exception CTOR anymore)

I think the original patch was overzealous in suppressing exceptions while in Exception().  Exceptions are now only suppressed by this logic during js_ErrorToException and JS_ReportPendingException, so it should be possible to generate an exception while constructing an Exception() object, instead of getting a silent failure.
Attachment #241670 - Flags: superreview?(brendan)
Attachment #241670 - Flags: review?(mrbkap)
(Assignee)

Updated

12 years ago
OS: Mac OS X 10.4 → All
Hardware: Macintosh → All
(Assignee)

Comment 4

12 years ago
Comment on attachment 241670 [details] [diff] [review]
Altered recursion-prevention (doesn't happen during Exception CTOR anymore)

Taking mrbkollege and brendan off r= detail.  Igor, mind taking a look?
Attachment #241670 - Flags: superreview?(brendan)
Attachment #241670 - Flags: review?(mrbkap)
Attachment #241670 - Flags: review?(igor.bukanov)

Comment 5

12 years ago
Comment on attachment 241670 [details] [diff] [review]
Altered recursion-prevention (doesn't happen during Exception CTOR anymore)

> static JSBool
> Exception(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)
> {
> 
>   out:
>-    cx->creatingException = JS_FALSE;
>     return ok;
> }

After the change the out label is not necessary and all the pairs out = false; goto out; can be replaced by return false.
Attachment #241670 - Flags: review?(igor.bukanov) → review+
Does this still need to be checked in?
(Assignee)

Comment 7

12 years ago
mrbkap:  If you want to check it out yourself, and then land it for me on the trunk that would be much appreciated.
(Assignee)

Comment 8

12 years ago
Created attachment 251288 [details] [diff] [review]
exn patch v2

Igor:  Cleaned up Exception() a bit to match your earlier nit and hammered some rust off this patch.  Enough changes to be worth re-review.  Here's the result:

js> print(1); try{new Error({toString: function() { x.y } })}catch(e){ print("Caught: " + e) }
1
Caught: ReferenceError: x is not defined
Attachment #241670 - Attachment is obsolete: true
Attachment #251288 - Flags: review?(igor)

Updated

12 years ago
Attachment #251288 - Flags: review?(igor) → review+
(Assignee)

Comment 9

12 years ago
Landed on trunk:

Checking in jsapi.c;
/cvsroot/mozilla/js/src/jsapi.c,v  <--  jsapi.c
new revision: 3.306; previous revision: 3.305
done
Checking in jscntxt.h;
/cvsroot/mozilla/js/src/jscntxt.h,v  <--  jscntxt.h
new revision: 3.138; previous revision: 3.137
done
Checking in jsexn.c;
/cvsroot/mozilla/js/src/jsexn.c,v  <--  jsexn.c
new revision: 3.81; previous revision: 3.80
done
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 10

12 years ago
/cvsroot/mozilla/js/tests/js1_5/Error/regress-354246.js,v  <--  regress-354246.
s
Flags: in-testsuite+

Comment 11

12 years ago
verified fixed 1.9.0 2007-01-23 win/mac*/linux
Status: RESOLVED → VERIFIED
(Reporter)

Updated

11 years ago
No longer blocks: 349611
(Reporter)

Updated

11 years ago
Blocks: 349611
You need to log in before you can comment on or make changes to this bug.