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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

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?"

Comment 1

[Brian Crowder]

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?

Comment 2

[Brian Crowder]

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

Comment 3

[Brian Crowder]

Attachment: 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.

Comment 4

[Brian Crowder]

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?

Comment 5

[Igor Bukanov]

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.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Does this still need to be checked in?

Comment 7

[Brian Crowder]

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

Comment 8

[Brian Crowder]

Attachment: 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

Comment 9

[Brian Crowder]

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

Comment 10

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/Error/regress-354246.js,v  <--  regress-354246.
s

Comment 11

[Bob Clary [:bc] (inactive)]

verified fixed 1.9.0 2007-01-23 win/mac*/linux