Bug 63934

Summary:

REGRESSION(Safari 5.0.4–r91186) Event target set to null post event dispatch for some events (eg. "load")

Product:

WebKit

Reporter:

Berend-Jan Wever <skylined>

Component:

UI Events

Assignee:

Dominic Cooney <dominicc>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, annevk, ap, ayg, dglazkov, dominicc, eric, Ms2ger, sam, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

64150, 65443

Bug Blocks:

Attachments:

Description	Flags
Repro	none
Crash Log from Safari 5	none
Reduced test case	none
Patch	none
WIP Updated failing layout test.	none
Patch	none

Berend-Jan Wever

Reported 2011-07-05 05:35:42 PDT

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();

Attachments
Repro (303 bytes, text/html) 2011-07-05 05:35 PDT, Berend-Jan Wever	no flags	Details
Crash Log from Safari 5 (112.03 KB, text/plain) 2011-07-05 10:25 PDT, Eric Seidel (no email)	no flags	Details
Reduced test case (166 bytes, text/html) 2011-07-05 17:38 PDT, Dominic Cooney	no flags	Details
Patch (5.73 KB, patch) 2011-07-05 22:17 PDT, Dominic Cooney	no flags	Details Formatted Diff Diff
WIP Updated failing layout test. (1.65 KB, patch) 2011-07-19 23:57 PDT, Dominic Cooney	no flags	Details Formatted Diff Diff
Patch (4.11 KB, patch) 2011-07-20 01:44 PDT, Dominic Cooney	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Eric Seidel (no email)

Comment 1 2011-07-05 10:25:47 PDT

Created attachment 99724 [details] Crash Log from Safari 5 Looks like it crashed (at least an olld copy of ) Safari as well.

Eric Seidel (no email)

Comment 2 2011-07-05 11:11:12 PDT

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.

Dominic Cooney

Comment 3 2011-07-05 17:38:40 PDT

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.

Dominic Cooney

Comment 4 2011-07-05 18:40:45 PDT

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?)

Dominic Cooney

Comment 5 2011-07-05 18:44:59 PDT

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.

Dominic Cooney

Comment 6 2011-07-05 22:17:05 PDT

Created attachment 99792 [details] Patch

Alexey Proskuryakov

Comment 7 2011-07-05 22:20:14 PDT

Comment on attachment 99792 [details] Patch Does the new behavior on these tests match Mozilla?

Dominic Cooney

Comment 8 2011-07-05 22:21:40 PDT

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."

Dominic Cooney

Comment 9 2011-07-05 23:21:34 PDT

(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.

Dimitri Glazkov (Google)

Comment 10 2011-07-07 08:49:00 PDT

(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?)

Dimitri Glazkov (Google)

Comment 11 2011-07-07 08:50:03 PDT

> 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?

Dimitri Glazkov (Google)

Comment 12 2011-07-07 08:50:27 PDT

Comment on attachment 99792 [details] Patch great test! ain't the right fix.

Dominic Cooney

Comment 13 2011-07-07 09:06:46 PDT

(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?

Dimitri Glazkov (Google)

Comment 14 2011-07-07 09:12:03 PDT

(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?

Dominic Cooney

Comment 15 2011-07-08 01:15:59 PDT

(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.

Alexey Proskuryakov

Comment 16 2011-07-08 09:00:34 PDT

> 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?

Dimitri Glazkov (Google)

Comment 17 2011-07-08 09:14:36 PDT

(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?

Alexey Proskuryakov

Comment 18 2011-07-08 09:56:30 PDT

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 :)

Dominic Cooney

Comment 19 2011-07-09 18:36:40 PDT

(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.

Dominic Cooney

Comment 20 2011-07-09 18:39:24 PDT

Also moving this to Event Handling component.

Dominic Cooney

Comment 21 2011-07-12 19:29:04 PDT

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>

Dominic Cooney

Comment 22 2011-07-19 23:45:51 PDT

Breaking off another smaller related bug: bug 64847.

Dominic Cooney

Comment 23 2011-07-19 23:57:06 PDT

Created attachment 101430 [details] WIP Updated failing layout test.

Dominic Cooney

Comment 24 2011-07-20 01:44:31 PDT

Created attachment 101439 [details] Patch

Dominic Cooney

Comment 25 2011-07-20 02:04:24 PDT

(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.

Anne van Kesteren

Comment 26 2011-07-20 04:48:57 PDT

May I suggest following DOM Core instead: http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html

Dominic Cooney

Comment 27 2011-07-20 17:59:20 PDT

(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.

Anne van Kesteren

Comment 28 2011-07-20 22:33:55 PDT

I do not think the DOM Core test suite covers events yet.

Ms2ger (he/him; ⌚ UTC+1/+2)

Comment 29 2011-07-21 01:38:06 PDT

Not yet. Could I import your test under the licenses at <http://www.w3.org/Consortium/Legal/2008/04-testsuite-copyright.html>?

Dominic Cooney

Comment 30 2011-07-31 17:28:17 PDT

(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?

Dimitri Glazkov (Google)

Comment 31 2011-07-31 18:54:41 PDT

Comment on attachment 101439 [details] Patch lovely.

WebKit Review Bot

Comment 32 2011-07-31 19:54:10 PDT

Comment on attachment 101439 [details] Patch Clearing flags on attachment: 101439 Committed r92094: <http://trac.webkit.org/changeset/92094>

WebKit Review Bot

Comment 33 2011-07-31 19:54:17 PDT

All reviewed patches have been landed. Closing bug.

Adam Barth

Comment 34 2011-07-31 21:17:22 PDT

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)

Note You need to log in before you can comment on or make changes to this bug.