422009 - [FIX]"Assertion failure: !cx->throwing" with XBL constructor that throws and syntax error in event handler

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Loading the testcase triggers:

Assertion failure: !cx->throwing, at /Users/jruderman/trunk/mozilla/js/src/jsinterp.c:6275

I think this bug can also trigger:

###!!! ASSERTION: Shouldn't get here when an exception is pending!: '!::JS_IsExceptionPending(cx)', file /Users/jruderman/trunk/mozilla/content/xbl/src/nsXBLProtoImplField.cpp, line 124

Comment 1

[Johnny Stenback (:jst)]

bz, didn't you just add this assertion?

Comment 2

[Boris Zbarsky [:bzbarsky]]

I added the second one.  I didn't add the first one...

Comment 3

[Boris Zbarsky [:bzbarsky]]

We go to compile the event handler.  We call JS_CompileUCFunctionForPrincipals.  This sets a pending exception (shouldn't it also report it?  It's a compile, not runtime, error).  Then we return an error to nsEventListenerManager::AddScriptEventListener, which converts NS_ERROR_ILLEGAL_VALUE into NS_SUCCESS_LOSS_OF_UNSIGNIFICANT_DATA (with an NS_WARNING about a likely syntax error, etc).

Then we unwind from the SetAttr() call with a success value, back into the JS code that called setAttribute().  That would be the onload handler calling the boom() function.

Then later, off an event, we run constructors, and assert at the JS_CallFunctionValue call from the constructor (well, once it gets down into js_Interpret).

I'm guessing that nsEventListenerManager::AddScriptEventListener should report the exception, right?  And perhaps set aside any existing exception if there is one before trying to compile the listener?

Comment 4

[Brendan Eich [:brendan]]

SpiderMonkey pre-dates ECMA-262 Edition 3. It used to have only errors, not catchable exceptions. Errors were reported promptly. When exceptions were added, errors became thrown exceptions, catchable by any active JS on the stack. But if an exception reached the oldest active API call as it was about to return, for backward compatibility it was then reported.

(OOM is always a promptly reported error, never an exception.)

Sounds like there's a setAttr active when the failing JS_Compile* API call is made, so if you don't want that exception propagating to setAttr's caller, then yes: JS_ReportPendingException(cx).

/be

Comment 5

[Boris Zbarsky [:bzbarsky]]

Yeah, I don't think we want this exception propagating to the setAttribute() caller.

Comment 6

[Johnny Stenback (:jst)]

Correct. And I don't think we need to worry about setting aside pending exceptions, we shouldn't ever get to nsEventListenerManager::AddScriptEventListener() with a pending exception (we could assert that).

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This just seems like "XBL sucks", not something that can cause security issues or break existing stuff (since it's not a regression). So not blocking.

Comment 8

[Boris Zbarsky [:bzbarsky]]

You mean "DOM events impl sucks", right?  ;)

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 310421 [details] [diff] [review]
Fix

This is a pretty straightforward change to not leave a pending exception just hanging out on the JSContext.

Comment 11

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

Comment on attachment 310421 [details] [diff] [review]
Fix

a=shaver

Comment 12

[Boris Zbarsky [:bzbarsky]]

Checked in, with the crashtest.