Crash in EventDispatcher::dispatchEvent entering a location on Google Maps
authorsimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 5 Jun 2015 01:23:56 +0000 (01:23 +0000)
committersimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 5 Jun 2015 01:23:56 +0000 (01:23 +0000)
https://bugs.webkit.org/show_bug.cgi?id=145677
rdar://problem/20698280

Reviewed by Dean Jackson.

If a transition is running on a pseudo-element, and the host element is removed
from the DOM just as the transition ends, and there is a transition event listener,
then we'd crash with a null dereference in event dispatch code.

AnimationController tries to clean up running animations when renderers are destroyed,
but omitted to remove the element from two vectors that store element references.
Elements are only added to these vectors briefly on animation end, before firing
events, but failure to remove the vector entries could result in attempting
to fire an event on a pseudo-element with no host element.

Also convert EventDispatcher code to be more robust to potentially null event
targets, since it's not clear that eventTargetRespectingTargetRules() can always
manage to return a non-null node.

Hard to make a test because this is timing sensitive.

* dom/EventDispatcher.cpp:
(WebCore::eventTargetRespectingTargetRules):
(WebCore::EventDispatcher::dispatchScopedEvent):
(WebCore::EventDispatcher::dispatchEvent):
(WebCore::EventPath::EventPath):
* page/animation/AnimationController.cpp:
(WebCore::AnimationControllerPrivate::clear):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@185232 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/dom/EventDispatcher.cpp
Source/WebCore/page/animation/AnimationController.cpp

index 87a2092..197ac04 100644 (file)
@@ -1,3 +1,35 @@
+2015-06-04  Simon Fraser  <simon.fraser@apple.com>
+
+        Crash in EventDispatcher::dispatchEvent entering a location on Google Maps
+        https://bugs.webkit.org/show_bug.cgi?id=145677
+        rdar://problem/20698280
+
+        Reviewed by Dean Jackson.
+
+        If a transition is running on a pseudo-element, and the host element is removed
+        from the DOM just as the transition ends, and there is a transition event listener,
+        then we'd crash with a null dereference in event dispatch code.
+        
+        AnimationController tries to clean up running animations when renderers are destroyed,
+        but omitted to remove the element from two vectors that store element references.
+        Elements are only added to these vectors briefly on animation end, before firing
+        events, but failure to remove the vector entries could result in attempting
+        to fire an event on a pseudo-element with no host element.
+        
+        Also convert EventDispatcher code to be more robust to potentially null event
+        targets, since it's not clear that eventTargetRespectingTargetRules() can always
+        manage to return a non-null node.
+        
+        Hard to make a test because this is timing sensitive.
+
+        * dom/EventDispatcher.cpp:
+        (WebCore::eventTargetRespectingTargetRules):
+        (WebCore::EventDispatcher::dispatchScopedEvent):
+        (WebCore::EventDispatcher::dispatchEvent):
+        (WebCore::EventPath::EventPath):
+        * page/animation/AnimationController.cpp:
+        (WebCore::AnimationControllerPrivate::clear):
+
 2015-06-04  Hunseop Jeong  <hs85.jeong@samsung.com>
 
         Replace 0 with nullptr in WebCore/Page.
index 8072740..7ca7c34 100644 (file)
@@ -202,26 +202,24 @@ private:
 #endif
 };
 
-inline EventTarget& eventTargetRespectingTargetRules(Node& referenceNode)
+inline EventTarget* eventTargetRespectingTargetRules(Node& referenceNode)
 {
-    if (is<PseudoElement>(referenceNode)) {
-        ASSERT(downcast<PseudoElement>(referenceNode).hostElement());
-        return *downcast<PseudoElement>(referenceNode).hostElement();
-    }
+    if (is<PseudoElement>(referenceNode))
+        return downcast<PseudoElement>(referenceNode).hostElement();
 
     // Events sent to elements inside an SVG use element's shadow tree go to the use element.
     if (is<SVGElement>(referenceNode)) {
         if (auto* useElement = downcast<SVGElement>(referenceNode).correspondingUseElement())
-            return *useElement;
+            return useElement;
     }
 
-    return referenceNode;
+    return &referenceNode;
 }
 
 void EventDispatcher::dispatchScopedEvent(Node& node, PassRefPtr<Event> event)
 {
     // We need to set the target here because it can go away by the time we actually fire the event.
-    event->setTarget(&eventTargetRespectingTargetRules(node));
+    event->setTarget(eventTargetRespectingTargetRules(node));
     ScopedEventQueue::singleton().enqueueEvent(event);
 }
 
@@ -340,9 +338,13 @@ bool EventDispatcher::dispatchEvent(Node* origin, PassRefPtr<Event> prpEvent)
 
     ChildNodesLazySnapshot::takeChildNodesLazySnapshot();
 
-    event->setTarget(&eventTargetRespectingTargetRules(*node));
+    EventTarget* target = eventTargetRespectingTargetRules(*node);
+    event->setTarget(target);
+    if (!event->target())
+        return true;
+
     ASSERT_WITH_SECURITY_IMPLICATION(!NoEventDispatchAssertion::isEventDispatchForbidden());
-    ASSERT(event->target());
+
     WindowEventContext windowEventContext(node.get(), eventPath.lastContextIfExists());
 
     InputElementClickState clickHandlingState;
@@ -352,7 +354,7 @@ bool EventDispatcher::dispatchEvent(Node* origin, PassRefPtr<Event> prpEvent)
     if (!event->propagationStopped() && !eventPath.isEmpty())
         dispatchEventInDOM(*event, eventPath, windowEventContext);
 
-    event->setTarget(&eventTargetRespectingTargetRules(*node));
+    event->setTarget(eventTargetRespectingTargetRules(*node));
     event->setCurrentTarget(nullptr);
     event->setEventPhase(0);
 
@@ -419,22 +421,22 @@ EventPath::EventPath(Node& targetNode, Event& event)
 #if ENABLE(TOUCH_EVENTS) && !PLATFORM(IOS)
     bool isTouchEvent = event.isTouchEvent();
 #endif
-    EventTarget* target = 0;
+    EventTarget* target = nullptr;
 
     Node* node = nodeOrHostIfPseudoElement(&targetNode);
     while (node) {
         if (!target || !isSVGElement) // FIXME: This code doesn't make sense once we've climbed out of the SVG subtree in a HTML document.
-            target = &eventTargetRespectingTargetRules(*node);
+            target = eventTargetRespectingTargetRules(*node);
         for (; node; node = node->parentNode()) {
-            EventTarget& currentTarget = eventTargetRespectingTargetRules(*node);
+            EventTarget* currentTarget = eventTargetRespectingTargetRules(*node);
             if (isMouseOrFocusEvent)
-                m_path.append(std::make_unique<MouseOrFocusEventContext>(node, &currentTarget, target));
+                m_path.append(std::make_unique<MouseOrFocusEventContext>(node, currentTarget, target));
 #if ENABLE(TOUCH_EVENTS) && !PLATFORM(IOS)
             else if (isTouchEvent)
-                m_path.append(std::make_unique<TouchEventContext>(node, &currentTarget, target));
+                m_path.append(std::make_unique<TouchEventContext>(node, currentTarget, target));
 #endif
             else
-                m_path.append(std::make_unique<EventContext>(node, &currentTarget, target));
+                m_path.append(std::make_unique<EventContext>(node, currentTarget, target));
             if (!inDocument)
                 return;
             if (is<ShadowRoot>(*node))
index 7885cac..3f73d36 100644 (file)
@@ -100,6 +100,17 @@ bool AnimationControllerPrivate::clear(RenderElement& renderer)
 {
     ASSERT(renderer.isCSSAnimating());
     ASSERT(m_compositeAnimations.contains(&renderer));
+
+    Element* element = renderer.element();
+
+    m_eventsToDispatch.removeAllMatching([element] (const EventToDispatch& info) {
+        return info.element == element;
+    });
+
+    m_elementChangesToDispatch.removeAllMatching([element] (const Ref<Element>& currElement) {
+        return &currElement.get() == element;
+    });
+    
     // Return false if we didn't do anything OR we are suspended (so we don't try to
     // do a setNeedsStyleRecalc() when suspended).
     RefPtr<CompositeAnimation> animation = m_compositeAnimations.take(&renderer);