461917 - [FIX]"ASSERTION: Shouldn't get here when an exception is pending!" with xul:tabs, xul:wizard

Status: VERIFIED FIXED

(Core :: XBL, defect)

Description

[Jesse Ruderman]

Attachment: testcase

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

Comment 1

[Boris Zbarsky [:bzbarsky]]

Here's the problematic stack:

#0  JS_SetPendingException (cx=0x1375c00, v=213086656) at /Users/bzbarsky/mozilla/debug/mozilla/js/src/jsapi.cpp:5892
#1  0x001ff7d0 in js_ErrorToException (cx=0x1375c00, message=0xc5e4de0 "illegal character", reportp=0xbfff8008) at /Users/bzbarsky/mozilla/debug/mozilla/js/src/jsexn.cpp:1255
#2  0x002996dc in js_ReportCompileErrorNumber (cx=0x1375c00, ts=0xbfff882c, pn=0x0, flags=0, errorNumber=144) at /Users/bzbarsky/mozilla/debug/mozilla/js/src/jsscan.cpp:622
#3  0x0029d5e3 in js_GetToken (cx=0x1375c00, ts=0xbfff882c) at /Users/bzbarsky/mozilla/debug/mozilla/js/src/jsscan.cpp:1964
#4  0x0029d860 in js_PeekToken (cx=0x1375c00, ts=0xbfff882c) at /Users/bzbarsky/mozilla/debug/mozilla/js/src/jsscan.cpp:935
#5  0x00285250 in Statements (cx=0x1375c00, ts=0xbfff882c, tc=0xbfff8738) at /Users/bzbarsky/mozilla/debug/mozilla/js/src/jsparse.cpp:1435
#6  0x00285450 in FunctionBody (cx=0x1375c00, ts=0xbfff882c, tc=0xbfff8738) at /Users/bzbarsky/mozilla/debug/mozilla/js/src/jsparse.cpp:838
#7  0x00289a5f in js_CompileFunctionBody (cx=0x1375c00, fun=0xc14a188, principals=0x0, chars=0xbfff8b60, length=1, filename=0xca5b458 "https://bugzilla.mozilla.org/attachment.cgi?id=345048", lineno=1) at /Users/bzbarsky/mozilla/debug/mozilla/js/src/jsparse.cpp:919
#8  0x001c2f4c in JS_CompileUCFunctionForPrincipals (cx=0x1375c00, obj=0x0, principals=0x0, name=0x2150ab7 "onselect", nargs=1, argnames=0x21a553c, chars=0xbfff8b60, length=1, filename=0xca5b458 "https://bugzilla.mozilla.org/attachment.cgi?id=345048", lineno=1) at /Users/bzbarsky/mozilla/debug/mozilla/js/src/jsapi.cpp:4970
#9  0x01dae151 in nsJSContext::CompileEventHandler (this=0xc4eb310, aName=0x1164284, aArgCount=1, aArgNames=0x21a553c, aBody=@0xbfff8b4c, aURL=0xca5b458 "https://bugzilla.mozilla.org/attachment.cgi?id=345048", aLineNo=1, aVersion=0, aHandler=@0xbfff8c44) at /Users/bzbarsky/mozilla/debug/mozilla/dom/src/base/nsJSEnvironment.cpp:1855
#10 0x01f3646f in nsScriptEventHandlerOwnerTearoff::CompileEventHandler (this=0xca761c0, aContext=0xc4eb310, aTarget=0xcae8180, aName=0x1164284, aBody=@0xbfff8b4c, aURL=0xca5b458 "https://bugzilla.mozilla.org/attachment.cgi?id=345048", aLineNo=1, aHandler=@0xbfff8c44) at /Users/bzbarsky/mozilla/debug/mozilla/content/xul/content/src/nsXULElement.cpp:798
#11 0x01bc8b46 in nsEventListenerManager::CompileEventHandlerInternal (this=0x840c2a0, aContext=0xc4eb310, aScope=0xc1a71e0, aObject=0xcae8180, aName=0x1164284, aListenerStruct=0x840c2c8, aCurrentTarget=0xcae8180) at /Users/bzbarsky/mozilla/debug/mozilla/content/events/src/nsEventListenerManager.cpp:1028
#12 0x01bc92b7 in nsEventListenerManager::HandleEventSubType (this=0x840c2a0, aListenerStruct=0x840c2c8, aListener=0xcad9280, aDOMEvent=0xdcda890, aCurrentTarget=0xcae8180, aPhaseFlags=6) at /Users/bzbarsky/mozilla/debug/mozilla/content/events/src/nsEventListenerManager.cpp:1081
#13 0x01bcaed0 in nsEventListenerManager::HandleEvent (this=0x840c2a0, aPresContext=0x12a6600, aEvent=0xc579000, aDOMEvent=0xbfff8f88, aCurrentTarget=0xcae8180, aFlags=6, aEventStatus=0xbfff8f8c) at /Users/bzbarsky/mozilla/debug/mozilla/content/events/src/nsEventListenerManager.cpp:1196
#14 0x01bfed0c in nsEventTargetChainItem::HandleEvent (this=0x1512020, aVisitor=@0xbfff8f80, aFlags=6, aMayHaveNewListenerManagers=1) at /Users/bzbarsky/mozilla/debug/mozilla/content/events/src/nsEventDispatcher.cpp:236
#15 0x01bfef4c in nsEventTargetChainItem::HandleEventTargetChain (this=0x1512200, aVisitor=@0xbfff8f80, aFlags=6, aCallback=0x0, aMayHaveNewListenerManagers=1) at /Users/bzbarsky/mozilla/debug/mozilla/content/events/src/nsEventDispatcher.cpp:300
#16 0x01bff7a2 in nsEventDispatcher::Dispatch (aTarget=0xcae8180, aPresContext=0x12a6600, aEvent=0xc579000, aDOMEvent=0xdcda890, aEventStatus=0xbfff9078, aCallback=0x0) at /Users/bzbarsky/mozilla/debug/mozilla/content/events/src/nsEventDispatcher.cpp:514
#17 0x01bffae0 in nsEventDispatcher::DispatchDOMEvent (aTarget=0xcae8180, aEvent=0x0, aDOMEvent=0xdcda890, aPresContext=0x12a6600, aEventStatus=0xbfff9078) at /Users/bzbarsky/mozilla/debug/mozilla/content/events/src/nsEventDispatcher.cpp:576
#18 0x01bc9634 in nsEventListenerManager::DispatchEvent (this=0x840c2a0, aEvent=0xdcda890, _retval=0xbfff9140) at /Users/bzbarsky/mozilla/debug/mozilla/content/events/src/nsEventListenerManager.cpp:1321
#19 0x01b5977d in nsDOMEventRTTearoff::DispatchEvent (this=0x821d370, aEvt=0xdcda890, _retval=0xbfff9140) at /Users/bzbarsky/mozilla/debug/mozilla/content/base/src/nsGenericElement.cpp:1539
#20 0x00d15f47 in nsIDOMEventTarget_DispatchEvent (cx=0x1375c00, argc=1, vp=0x1619474) at dom_quickstubs.cpp:5631
#21 0x00223885 in js_Interpret (cx=0x1375c00) at /Users/bzbarsky/mozilla/debug/mozilla/js/src/jsinterp.cpp:4998
#22 0x00247dcc in js_Invoke (cx=0x1375c00, argc=1, vp=0x1619458, flags=0) at jsinterp.cpp:1324
#23 0x00248082 in js_InternalInvoke (cx=0x1375c00, obj=0xc619ac0, fval=207712704, flags=0, argc=1, argv=0xbfffb528, rval=0xbfffb528) at jsinterp.cpp:1381
#24 0x002482e3 in js_InternalGetOrSet (cx=0x1375c00, obj=0xc619ac0, id=193925164, fval=207712704, mode=JSACC_WRITE, argc=1, argv=0xbfffb528, rval=0xbfffb528) at jsinterp.cpp:1442
#25 0x0025ee06 in js_SetPropertyHelper (cx=0x1375c00, obj=0xc619ac0, id=193925164, vp=0xbfffb528, entryp=0xbfffb450) at /Users/bzbarsky/mozilla/debug/mozilla/js/src/jsobj.cpp:3938
#26 0x00221725 in js_Interpret (cx=0x1375c00) at /Users/bzbarsky/mozilla/debug/mozilla/js/src/jsinterp.cpp:4624
#27 0x00247dcc in js_Invoke (cx=0x1375c00, argc=0, vp=0x1619420, flags=0) at jsinterp.cpp:1324
#28 0x00248082 in js_InternalInvoke (cx=0x1375c00, obj=0xc619ac0, fval=213097920, flags=0, argc=0, argv=0x0, rval=0xbfffc1a0) at jsinterp.cpp:1381
#29 0x001c3ee7 in JS_CallFunctionValue (cx=0x1375c00, obj=0xc619ac0, fval=213097920, argc=0, argv=0x0, rval=0xbfffc1a0) at /Users/bzbarsky/mozilla/debug/mozilla/js/src/jsapi.cpp:5235
#30 0x01d5a204 in nsXBLProtoImplAnonymousMethod::Execute (this=0xc4f8390, aBoundElement=0xcae8180) at /Users/bzbarsky/mozilla/debug/mozilla/content/xbl/src/nsXBLProtoImplMethod.cpp:334

So in this case we call an XBL constructor which sets selectedIndex on the tabbox, which dispatches a "selected" DOM event.  We then try to compile the event handler for that event, whose text is "#".  This is clearly not particularly valid JS, so the compilation fails and sets an exception.  We propagate the error out of the JS engine to nsJSContext::CompileEventHandler, which converts it into an error nsresult and propagates that out in turn.  This all works until we get to nsEventListenerManager::HandleEventSubType, which drops the nsresult on the floor.  So from that point on we think everything is ok, unwind into the JS that called dispatchEvent (all with the exception still on the JSContext), unwind back to XBL with a JS_TRUE return, go on to set up the fields, and hit this assertion.

All this is an awful lot like bug 422009.  I should have done more auditing when I fixed that...

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: This fixes it

This does raise two questions:

1)  Should this check just live in nsJSContext::CompileEventHandler, or are
    there cases when we'd want a caller to eat an exception from
    nsJSContext::CompileEventHandler without reporting it?
2)  The error doesn't get reported, because cx->fp is not a native stack frame
    when we call NS_ScriptErrorReporter.  Of course in this case cx->fp has
    nothing to do with the event handler we're compiling.  Would it make sense
    to set aside the JS stack, or push a native fp or something while reporting
    this error?

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #2)
> 1)  Should this check just live in nsJSContext::CompileEventHandler, or are
>     there cases when we'd want a caller to eat an exception from
>     nsJSContext::CompileEventHandler without reporting it?

It doesn't look like any of the callers of CompileEventHandler are set up to deal with exceptions from said compilation. I think we should move the check there.

> 2)  The error doesn't get reported, because cx->fp is not a native stack frame
>     when we call NS_ScriptErrorReporter.  Of course in this case cx->fp has
>     nothing to do with the event handler we're compiling.  Would it make sense
>     to set aside the JS stack, or push a native fp or something while reporting
>     this error?

Yeah, let's do that.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: With those things updated

I left the ReportPendingException() on nsIScriptContext though now we have no external callers.  If preferred, I can remove that too (and un-rev the IID).

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 350268 [details] [diff] [review]
With those things updated

sr=jst. I don't feel strongly about removing ReportPendingException() from the interface at this point, but if you don't, please file a bug to remove it after 3.1...

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 350268 [details] [diff] [review]
With those things updated

I guess we should in fact take this on 1.9.1.  I'll file a followup for m-c to remove the function.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/9c8ba21dc1cc for the fix and http://hg.mozilla.org/mozilla-central/rev/bfd996bd103f for the test.

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 350268 [details] [diff] [review]
With those things updated

a191=beltzner, please make sure the tests go along with it

Comment 9

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7942e981f25b for patch+tests on 1.9.1.

Comment 10

[Aakash Desai [:aakashd]]

Seeing that there haven't been any duplicate sightings for this issue for 3 months, I'm going to go ahead and verify this unless someone has an issue with that.