Bug 63934 - REGRESSION(Safari 5.0.4–r91186) Event target set to null post event dispatch for some events (eg. "load")
Summary: REGRESSION(Safari 5.0.4–r91186) Event target set to null post event dispatch ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: Dominic Cooney
URL:
Keywords:
Depends on: 64150 65443
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-05 05:35 PDT by Berend-Jan Wever
Modified: 2011-07-31 21:20 PDT (History)
10 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Berend-Jan Wever 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();
Comment 1 Eric Seidel (no email) 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.
Comment 2 Eric Seidel (no email) 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.
Comment 3 Dominic Cooney 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.
Comment 4 Dominic Cooney 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?)
Comment 5 Dominic Cooney 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.
Comment 6 Dominic Cooney 2011-07-05 22:17:05 PDT
Created attachment 99792 [details]
Patch
Comment 7 Alexey Proskuryakov 2011-07-05 22:20:14 PDT
Comment on attachment 99792 [details]
Patch

Does the new behavior on these tests match Mozilla?
Comment 8 Dominic Cooney 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."
Comment 9 Dominic Cooney 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.
Comment 10 Dimitri Glazkov (Google) 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?)
Comment 11 Dimitri Glazkov (Google) 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?
Comment 12 Dimitri Glazkov (Google) 2011-07-07 08:50:27 PDT
Comment on attachment 99792 [details]
Patch

great test! ain't the right fix.
Comment 13 Dominic Cooney 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?
Comment 14 Dimitri Glazkov (Google) 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?
Comment 15 Dominic Cooney 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.
Comment 16 Alexey Proskuryakov 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?
Comment 17 Dimitri Glazkov (Google) 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?
Comment 18 Alexey Proskuryakov 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 :)
Comment 19 Dominic Cooney 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.
Comment 20 Dominic Cooney 2011-07-09 18:39:24 PDT
Also moving this to Event Handling component.
Comment 21 Dominic Cooney 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>
Comment 22 Dominic Cooney 2011-07-19 23:45:51 PDT
Breaking off another smaller related bug: bug 64847.
Comment 23 Dominic Cooney 2011-07-19 23:57:06 PDT
Created attachment 101430 [details]
WIP Updated failing layout test.
Comment 24 Dominic Cooney 2011-07-20 01:44:31 PDT
Created attachment 101439 [details]
Patch
Comment 25 Dominic Cooney 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.
Comment 26 Anne van Kesteren 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
Comment 27 Dominic Cooney 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.
Comment 28 Anne van Kesteren 2011-07-20 22:33:55 PDT
I do not think the DOM Core test suite covers events yet.
Comment 29 Ms2ger (he/him; ⌚ UTC+1/+2) 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>?
Comment 30 Dominic Cooney 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?
Comment 31 Dimitri Glazkov (Google) 2011-07-31 18:54:41 PDT
Comment on attachment 101439 [details]
Patch

lovely.
Comment 32 WebKit Review Bot 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>
Comment 33 WebKit Review Bot 2011-07-31 19:54:17 PDT
All reviewed patches have been landed.  Closing bug.
Comment 34 Adam Barth 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)