Title: [185232] trunk/Source/WebCore
Revision
185232
Author
simon.fra...@apple.com
Date
2015-06-04 18:23:56 -0700 (Thu, 04 Jun 2015)

Log Message

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

Modified Paths

Diff

Modified: trunk/Source/WebCore/ChangeLog (185231 => 185232)


--- trunk/Source/WebCore/ChangeLog	2015-06-05 01:07:51 UTC (rev 185231)
+++ trunk/Source/WebCore/ChangeLog	2015-06-05 01:23:56 UTC (rev 185232)
@@ -1,3 +1,35 @@
+2015-06-04  Simon Fraser  <simon.fra...@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.je...@samsung.com>
 
         Replace 0 with nullptr in WebCore/Page.

Modified: trunk/Source/WebCore/dom/EventDispatcher.cpp (185231 => 185232)


--- trunk/Source/WebCore/dom/EventDispatcher.cpp	2015-06-05 01:07:51 UTC (rev 185231)
+++ trunk/Source/WebCore/dom/EventDispatcher.cpp	2015-06-05 01:23:56 UTC (rev 185232)
@@ -202,26 +202,24 @@
 #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 @@
 
     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 @@
     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 @@
 #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))

Modified: trunk/Source/WebCore/page/animation/AnimationController.cpp (185231 => 185232)


--- trunk/Source/WebCore/page/animation/AnimationController.cpp	2015-06-05 01:07:51 UTC (rev 185231)
+++ trunk/Source/WebCore/page/animation/AnimationController.cpp	2015-06-05 01:23:56 UTC (rev 185232)
@@ -100,6 +100,17 @@
 {
     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);
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to