Closed Bug 410010 Opened 14 years ago Closed 14 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla1.9beta4

People

(Reporter: mrbkap, Assigned: mrbkap)

References

Details

Attachments

(1 file)

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?
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.
Attached patch Easier fixSplinter Review
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.
Assignee: nobody → mrbkap
Status: NEW → ASSIGNED
Attachment #300215 - Flags: review?(jst)
Comment on attachment 300215 [details] [diff] [review]
Easier fix

Yeah, I see no reason not to do this.
Attachment #300215 - Flags: superreview+
Attachment #300215 - Flags: review?(jst)
Attachment #300215 - Flags: review+
This needs to block because it blocks bug 344494.
Flags: blocking1.9?
Attachment #300215 - Flags: approval1.9?
Flags: blocking1.9? → blocking1.9+
OS: Linux → All
Priority: -- → P1
Hardware: PC → All
Target Milestone: --- → mozilla1.9beta4
Comment on attachment 300215 [details] [diff] [review]
Easier fix

This is blocking so it doesn't need approval ... but heck, a=beltzner
Attachment #300215 - Flags: approval1.9? → approval1.9+
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Version: unspecified → Trunk
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.