Created attachment 99705 [details] Repro Chromium: https://code.google.com/p/chromium/issues/detail?id=88443 Repro: <body onload="go()"></body> <script> function go() { document.open(); document.addEventListener("load", document.dispatchEvent, true); document.write("<x>"); document.addEventListener("load", window.MouseEvent, true); document.documentElement.innerHTML += "<iframe>"; } </script> id: chrome.dll!WebCore::V8AbstractEventListener::invokeEventHandler ReadAV@NULL (f2d876aa7adcf71aaf4a1a1a3b32680c) description: Attempt to read from unallocated NULL pointer in chrome.dll!WebCore::V8AbstractEventListener::invokeEventHandler application: Chromium 14.0.811.0 stack: chrome.dll!WebCore::V8AbstractEventListener::invokeEventHandler chrome.dll!WebCore::V8AbstractEventListener::handleEvent chrome.dll!WebCore::EventTarget::fireEventListeners chrome.dll!WebCore::EventTarget::fireEventListeners chrome.dll!WebCore::Node::handleLocalEvents chrome.dll!WebCore::EventDispatcher::dispatchEvent chrome.dll!WebCore::EventDispatchMediator::dispatchEvent chrome.dll!WebCore::EventDispatcher::dispatchEvent chrome.dll!WebCore::Node::dispatchEvent chrome.dll!WebCore::EventTarget::dispatchEvent chrome.dll!WebCore::EventSourceInternal::dispatchEventCallback chrome.dll!v8::internal::HandleApiCallHelper<...> chrome.dll!v8::internal::Builtin_HandleApiCall chrome.dll!v8::internal::Invoke chrome.dll!v8::internal::Execution::Call chrome.dll!v8::Function::Call chrome.dll!WebCore::V8Proxy::callFunction chrome.dll!WebCore::V8EventListener::callListenerFunction chrome.dll!WebCore::V8AbstractEventListener::invokeEventHandler ...etc... The repro causes an infinite loop. v8 code detects this using a recursion counter and throws an exception. The event's target is set to NULL, which later causes a NULL ptr exception in V8AbsractEventListener (webkit\source\webcore\bindings\v8\V8AbstractEventListener.cpp@156): event->target()->uncaughtExceptionInEventHandler();
Created attachment 99724 [details] Crash Log from Safari 5 Looks like it crashed (at least an olld copy of ) Safari as well.
Looks like I may have hijacked the v8 bug. It looks like this page causes different bugs in v8 vs. jsc, but both cause crashes. We may need two separate bugs.
Created attachment 99767 [details] Reduced test case This crashes WebKit nightly r90391 on Mac and Chrome 14.0.803.0 (Official Build 90483) dev OS Mac OS WebKit 535.1 (trunk@89703). I am still mystified by the need for the second event listener. window.MouseEvent is not crucial; window.alert and console.log also work. But function () {} does not cause a crash.
Is the V8 crash peculiar to the load event? Because I think I see where the event target is set to null—in EventDispatcher::dispatchEvent(PassRefPtr<Event>) doneWithDefault: event->setTarget(windowContext.target()) sets the event target to 0. There’s a comment in WindowEventContext.cpp that load events aren’t dispatched to the window, and it explicitly declines to initialize m_target (which should be something like the document?)
I now see why the second function is necessary. Any function that throws an exception will do. The built-ins window.alert, console.log, window.MouseEvent, etc. work because they throw TypeError: Illegal Invocation. But function () { throw 'booger'; } works equally well.
Created attachment 99792 [details] Patch
Comment on attachment 99792 [details] Patch Does the new behavior on these tests match Mozilla?
Comment on attachment 99792 [details] Patch This needs dglazkov's scrutiny at the very least, since it touches EventDispatcher logic. Despite the different callstacks in V8 and JSC, I believe event.target being set to null really is the root of all evil. This patch fixes both mac and chromium-mac. The differences between JSC and V8 error messages again bites us and causes this to need platform expectations. I took a look at what Firefox does. It avoids this problem by not redispatching an event if you call dispatchEvent with an event object that is already "in flight."
(In reply to comment #7) > (From update of attachment 99792 [details]) > Does the new behavior on these tests match Mozilla? No. It seems that dispatchEvent(x) where x is an event "in flight" is a no-op in Firefox. Here are a few plausible choices for what WebKit might do here: 1. Update event.target to the most recent target (the most recent receiver of dispatchEvent.) Subsequent event listeners of the original dispatch see the modified event.target. This is the status quo, in that if you write something non-pathological (no exceptions, no stack overflows) that does dispatchEvent-in-dispatchEvent in WebKit today, that is the behavior you get. The patch tries to make the pathological case work the same way. 2. Reset event.target before calling each handler. All event listeners on a given target see the same event.target, even if one of them redispatched the event and event.target was temporarily set to another target. I took an informal straw poll of my officemates and this seems to be the intuition about how it should work. 3. Check the argument to dispatchEvent. If the same event is being dispatched right now, then don’t do anything. This is what Firefox 5.0 seems to do.
(In reply to comment #4) > Is the V8 crash peculiar to the load event? Because I think I see where the event target is set to null—in EventDispatcher::dispatchEvent(PassRefPtr<Event>) doneWithDefault: > > event->setTarget(windowContext.target()) This is bug #1. I shouldn't have used windowContext here. I assumed it is the same as just grabbing the top-of-the-stack target. > > sets the event target to 0. There’s a comment in WindowEventContext.cpp that load events aren’t dispatched to the window, and it explicitly declines to initialize m_target (which should be something like the document?)
> 3. Check the argument to dispatchEvent. If the same event is being dispatched right now, then don’t do anything. This is what Firefox 5.0 seems to do. This is bug #2. We should do this too. Is there anything in the spec that suggests the _right_ way to deal with re-dispatching the same event?
Comment on attachment 99792 [details] Patch great test! ain't the right fix.
(In reply to comment #11) > > 3. Check the argument to dispatchEvent. If the same event is being dispatched right now, then don’t do anything. This is what Firefox 5.0 seems to do. > > This is bug #2. We should do this too. Is there anything in the spec that suggests the _right_ way to deal with re-dispatching the same event? It looks like the WD to the rescue with this: <http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#events-EventTarget-dispatchEvent> dispatchEvent raises EventException DISPATCH_REQUEST_ERR: Raised if the Event object is already being dispatched. Shall I make it so? This will prevent this particular repro from crashing, but I think this bug needs other remediation. For example, presumably I could replace the recursive event dispatch with a badly-behaved handler that overflows the stack on its own. But I have yet to verify this. So do you have advice on how to fix this comprehensively?
(In reply to comment #13) > (In reply to comment #11) > > > 3. Check the argument to dispatchEvent. If the same event is being dispatched right now, then don’t do anything. This is what Firefox 5.0 seems to do. > > > > This is bug #2. We should do this too. Is there anything in the spec that suggests the _right_ way to deal with re-dispatching the same event? > > It looks like the WD to the rescue with this: > > <http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#events-EventTarget-dispatchEvent> > > dispatchEvent raises EventException DISPATCH_REQUEST_ERR: Raised if the Event object is already being dispatched. Yay specs! :) > > Shall I make it so? Yes! > > This will prevent this particular repro from crashing, but I think this bug needs other remediation. For example, presumably I could replace the recursive event dispatch with a badly-behaved handler that overflows the stack on its own. But I have yet to verify this. So do you have advice on how to fix this comprehensively? Keep a reference to a currently dispatched event somewhere on the document?
(In reply to comment #14) > > dispatchEvent raises EventException DISPATCH_REQUEST_ERR: Raised if the Event object is already being dispatched. > > Yay specs! :) > > > > > Shall I make it so? > > Yes! I have filed bug 64150 for making dispatchEvent raise the exception specified in DOM 3 Events. > > This will prevent this particular repro from crashing, but I think this bug needs other remediation. For example, presumably I could replace the recursive event dispatch with a badly-behaved handler that overflows the stack on its own. But I have yet to verify this. So do you have advice on how to fix this comprehensively? > > Keep a reference to a currently dispatched event somewhere on the document? I’m still not confident to know when squirreling the event away on the document makes sense, so I am going to keep staring at this. I may need a tweaked repro post bug 64150 though.
> I’m still not confident to know when squirreling the event away on the document makes sense A (possibly too obvious) question is - what if the recursion uses two JavaScript events?
(In reply to comment #16) > > I’m still not confident to know when squirreling the event away on the document makes sense > > A (possibly too obvious) question is - what if the recursion uses two JavaScript events? Are we talking about fixing bug #1, the fact that a target could be set to 0?
Probably not bug #1, as the suggestion to keep a reference to a currently dispatched event somewhere on the document came when discussion a more general case. Just saying that storing the current event to prevent recursion won't prevent recursion if there are two alternating events that dispatch each other. It's OK if that's irrelevant here :)
(In reply to comment #18) > Probably not bug #1, as the suggestion to keep a reference to a currently dispatched event somewhere on the document came when discussion a more general case. > > Just saying that storing the current event to prevent recursion won't prevent recursion if there are two alternating events that dispatch each other. It's OK if that's irrelevant here :) I believe V8 and JSC stack overflow protection are working just fine, so I expect recursion with two alternating events is not interesting once this bug is fixed. However I will investigate to be sure. For now I am going to retitle this bug because it is much more specific than "event handler infinite recursion" in general.
Also moving this to Event Handling component.
The discussion on bug 64150 notes that resetting m_target to 0 will also allow initEvent to be called a second time, which is against the spec: <http://www.w3.org/TR/DOM-Level-2-Events/events.html#Events-Event-initEvent>
Breaking off another smaller related bug: bug 64847.
Created attachment 101430 [details] WIP Updated failing layout test.
Created attachment 101439 [details] Patch
(In reply to comment #16) > A (possibly too obvious) question is - what if the recursion uses two JavaScript events? To follow up in detail: I don’t think deep recursion is key to this bug. The crash depended on event listeners A and B being registered, A redispatching the event to reset the target to null, and then B throwing an exception. I still played around with recursive cases to try and break this. Recursion with just two JavaScript events is no longer possible since bug 64150 was fixed, because redispatching the events is no longer possible. So I experimented with recursive dispatchEvent with a new event per activation; this does not cause a crash. As I outlined above, redispatch to reset an event’s target to null during dispatch was critical for triggering it. However there is still a bug here—after an event is dispatched, the target should not be null. Attachment 101439 [details] adds a layout test and a fix.
May I suggest following DOM Core instead: http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html
(In reply to comment #26) > May I suggest following DOM Core instead: http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html Thanks. I have filed one bug 64913 and closed another invalid bug 64847. Does DOM Core events have a test suite? I probably need to take a systematic look at this. Regarding this bug, DOM Core events also specifies that the event target should not be set to null after dispatch.
I do not think the DOM Core test suite covers events yet.
Not yet. Could I import your test under the licenses at <http://www.w3.org/Consortium/Legal/2008/04-testsuite-copyright.html>?
(In reply to comment #29) > Not yet. Could I import your test under the licenses at <http://www.w3.org/Consortium/Legal/2008/04-testsuite-copyright.html>? These tests? They are probably too WebKit-specific. Can you share a link to the suite that you have?
Comment on attachment 101439 [details] Patch lovely.
Comment on attachment 101439 [details] Patch Clearing flags on attachment: 101439 Committed r92094: <http://trac.webkit.org/changeset/92094>
All reviewed patches have been landed. Closing bug.
This patch triggered a lot of assertions. ASSERTION FAILED: !m_node->isInShadowTree() Backtrace: WebCore::EventDispatcher::dispatchEvent [0x0186B7AC+1596] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\dom\eventdispatcher.cpp:370) WebCore::EventDispatchMediator::dispatchEvent [0x017E9593+51] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\dom\event.cpp:304) WebCore::EventDispatcher::dispatchEvent [0x0186A470+96] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\dom\eventdispatcher.cpp:54) WebCore::Node::dispatchEvent [0x017F2CBB+59] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\dom\node.cpp:2717) WebCore::ScopedEventQueue::dispatchEvent [0x0184980C+92] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\dom\scopedeventqueue.cpp:80) WebCore::ScopedEventQueue::enqueueEvent [0x018496C2+66] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\dom\scopedeventqueue.cpp:65) WebCore::EventDispatcher::dispatchScopedEvent [0x0186A56C+76] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\dom\eventdispatcher.cpp:93) WebCore::Node::dispatchScopedEvent [0x017F2C5C+44] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\dom\node.cpp:2712) WebCore::dispatchChildInsertionEvents [0x0181F788+248] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\dom\containernode.cpp:1099) WebCore::ContainerNode::appendChild [0x0181DD67+727] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\dom\containernode.cpp:662) WebCore::TextFieldInputType::createShadowSubtree [0x0177DC89+537] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\html\textfieldinputtype.cpp:169) WebCore::HTMLInputElement::createShadowSubtree [0x016C9EEB+59] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\html\htmlinputelement.cpp:111) WebCore::HTMLInputElement::create [0x016C9E48+136] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\html\htmlinputelement.cpp:106) WebCore::inputConstructor [0x0230F525+37] (e:\b\build\slave\webkit_win__dbg__2_\build\src\webkit\debug\obj\global_intermediate\webkit\htmlelementfactory.cpp:306) WebCore::HTMLElementFactory::createHTMLElement [0x0230E516+134] (e:\b\build\slave\webkit_win__dbg__2_\build\src\webkit\debug\obj\global_intermediate\webkit\htmlelementfactory.cpp:649) WebCore::HTMLDocument::createElement [0x017246DB+123] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\html\htmldocument.cpp:313) WebCore::DocumentInternal::createElementCallback [0x02109BA7+183] (e:\b\build\slave\webkit_win__dbg__2_\build\src\webkit\debug\obj\global_intermediate\webcore\bindings\v8document.cpp:1145) v8::internal::HandleApiCallHelper<0> [0x0065E9AC+892] (e:\b\build\slave\webkit_win__dbg__2_\build\src\v8\src\builtins.cc:1105) v8::internal::Builtin_Impl_HandleApiCall [0x006595B4+20] (e:\b\build\slave\webkit_win__dbg__2_\build\src\v8\src\builtins.cc:1122) v8::internal::Builtin_HandleApiCall [0x00659532+66] (e:\b\build\slave\webkit_win__dbg__2_\build\src\v8\src\builtins.cc:1121) (No symbol) [0x352720B6] (No symbol) [0x0EFB8842] (No symbol) [0x0EFB727F] (No symbol) [0x352734C1] (No symbol) [0x1C43FB91] (No symbol) [0x1C43F3D4] (No symbol) [0x0EFB6378] (No symbol) [0x352734C1] (No symbol) [0x1C44DC31] (No symbol) [0x1C44DB30] (No symbol) [0x3528557A] (No symbol) [0x35275E0B] v8::internal::Invoke [0x004DF87C+412] (e:\b\build\slave\webkit_win__dbg__2_\build\src\v8\src\execution.cc:122) v8::internal::Execution::Call [0x004DF66C+140] (e:\b\build\slave\webkit_win__dbg__2_\build\src\v8\src\execution.cc:158) v8::internal::JSObject::SetPropertyWithDefinedSetter [0x005274DE+206] (e:\b\build\slave\webkit_win__dbg__2_\build\src\v8\src\objects.cc:1866) v8::internal::JSObject::SetPropertyWithCallback [0x0052721B+811] (e:\b\build\slave\webkit_win__dbg__2_\build\src\v8\src\objects.cc:1832) v8::internal::JSObject::SetPropertyForResult [0x0052979A+506] (e:\b\build\slave\webkit_win__dbg__2_\build\src\v8\src\objects.cc:2390) v8::internal::JSReceiver::SetProperty [0x00528908+120] (e:\b\build\slave\webkit_win__dbg__2_\build\src\v8\src\objects.cc:2196) v8::internal::JSReceiver::SetProperty [0x00526EAA+90] (e:\b\build\slave\webkit_win__dbg__2_\build\src\v8\src\objects.cc:1779) v8::internal::StoreIC::Store [0x007145BD+1101] (e:\b\build\slave\webkit_win__dbg__2_\build\src\v8\src\ic.cc:1475) v8::internal::StoreIC_Miss [0x00716EE9+217] (e:\b\build\slave\webkit_win__dbg__2_\build\src\v8\src\ic.cc:2012) (No symbol) [0x352720B6] (No symbol) [0x0EFB53B9] (No symbol) [0x352734C1] (No symbol) [0x0EFB2A4C] (No symbol) [0x36C93D2C] (No symbol) [0x35286F29] (No symbol) [0x36C98F22] (No symbol) [0x352734C1] (No symbol) [0x0EFAEA11] (No symbol) [0x35286F29] (No symbol) [0x36C98F22] (No symbol) [0x352734C1] (No symbol) [0x35286F22] (No symbol) [0x3FABB5A1] (No symbol) [0x3528557A] (No symbol) [0x35275E0B] v8::internal::Invoke [0x004DF87C+412] (e:\b\build\slave\webkit_win__dbg__2_\build\src\v8\src\execution.cc:122) v8::internal::Execution::Call [0x004DF66C+140] (e:\b\build\slave\webkit_win__dbg__2_\build\src\v8\src\execution.cc:158)