410010 - Event handling allows exceptions to stay on a context for too long

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P1)

Description

[Blake Kaplan (:mrbkap) (inactive)]

Currently, I have a stack that looks like the following:
#0  XPCNativeWrapperCtor (cx=0xafa08058, obj=0x0, argc=4, argv=0xafb7c1e0, rval=0xbff78bc8)
    at /home/mrbkap/src/3mozbuild/mozilla/js/src/xpconnect/src/XPCNativeWrapper.cpp:788
#1  0xb7e426c6 in js_Invoke (cx=0xafa08058, argc=4, vp=0xafb7c1d8, flags=1)
    at /home/mrbkap/src/3mozbuild/mozilla/js/src/jsinterp.c:1032
...
#7  0xb4c2a8ee in nsJSContext::CallEventHandler (this=0xafa5fc40, aTarget=0xafac44e8, aScope=0xaf1dac40, 
    aHandler=0xaf1d3660, aargv=0x8ad1188, arv=0xbff795fc)
    at /home/mrbkap/src/3mozbuild/mozilla/dom/src/base/nsJSEnvironment.cpp:1958
#8  0xb4ca35bc in nsJSEventListener::HandleEvent (this=0x8c73798, aEvent=0x8bd34f8)
    at /home/mrbkap/src/3mozbuild/mozilla/dom/src/events/nsJSEventListener.cpp:235
#9  0xb4a42b9f in nsEventListenerManager::HandleEventSubType (this=0x8c73758, aListenerStruct=0x8c737e0, 
    aListener=0x8c73798, aDOMEvent=0x8bd34f8, aCurrentTarget=0xafac44e8, aPhaseFlags=6)
    at /home/mrbkap/src/3mozbuild/mozilla/content/events/src/nsEventListenerManager.cpp:1081
...
#19 0xb49f7f7d in nsNodeUtils::ContentRemoved (aContainer=0x8abbb60, aChild=0x8bdc460, aIndexInContainer=0)
    at /home/mrbkap/src/3mozbuild/mozilla/content/base/src/nsNodeUtils.cpp:166
#20 0xb4b0f160 in nsHTMLDocument::OpenCommon (this=0x8abbb60, aContentType=@0xbff79d8c, aReplace=0)
    at /home/mrbkap/src/3mozbuild/mozilla/content/html/document/src/nsHTMLDocument.cpp:2215
...
#27 0xb6869759 in XPCWrappedNative::CallMethod (ccx=@0xbff7a2e4, mode=XPCWrappedNative::CALL_METHOD)
    at /home/mrbkap/src/3mozbuild/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:2342
...
#53 0xb7e533e9 in js_Interpret (cx=0xafa08058, pc=0xafc65e2c "6", result=0xbff7bdb0)
    at /home/mrbkap/src/3mozbuild/mozilla/js/src/jsinterp.c:3598

What's happening is that XPCNativeWrapperCtor is throwing an exception (it's being passed a null object) which propagates up to nsJSContext::CallEventHandler. CallEventHandler sees that we have failed with an exception and calls nsContentUtils::NotifyXPCIfExceptionPending. At this point, we should probably report the error because nobody is going to bother looking at our return value. However, because we're being called by an XPConnect called function, there is a native call context so we just tell XPConnect about the exception. Then, nsEventListenerManager::HandleEventSubType calls another JS event handler, causing the engine to print:

  JS INTERPRETER CALLED WITH PENDING EXCEPTION af1c8920

which means we don't actually run the handler. Currently, this is non-fatal, however, I have some code that ends up hitting a JS assertion because of the pending exception (we end up calling into an XPCSafeJSObjectWrapper, I think).

I think we can solve this by setting aside the XPCCallContext when we start handling the event (much the same way that the JS engine sets aside the current stack frame during a call to JS_EvaluateScript or similar). Does that sound reasonable?

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

smaug pointed out to me on IRC that we could do this when we push a context. It seems that we could probably do this in XPCJSContextStack::Push where we set aside the previous context's stack frame or in nsCxPusher.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Easier fix

I'm not sure if this is really right or not, but it should fix this bug. My idea is that event handlers are conceptually called asynchronously, there's no reason to propagate an exception back to the caller. If people don't agree with this, I can go back to figuring out how to mark XPCCallContexts as being dormant.

Comment 4

[Johnny Stenback (:jst)]

Comment on attachment 300215 [details] [diff] [review]
Easier fix

Yeah, I see no reason not to do this.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

This needs to block because it blocks bug 344494.

Comment 6

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

Comment on attachment 300215 [details] [diff] [review]
Easier fix

This is blocking so it doesn't need approval ... but heck, a=beltzner

Comment 7

[Johnny Stenback (:jst)]

Fix checked in.