Changeset 222554 in webkit

Timestamp:

Sep 27, 2017, 8:32:55 AM (8 years ago)

Author:

Antti Koivisto

Message:

REGRESSION (r222040): Crash navigating out of gfycat.com url
​https://bugs.webkit.org/show_bug.cgi?id=177531
Source/WebCore:

<rdar://problem/34602601>

Reviewed by Geoff Garen.

Animation structures are normally removed when the render tree is torn down.
However there are cases where we can instantiate animation without creating a renderer
and we need to make sure animations are canceled in these cases too.

CompositeAnimations should also ref the element but that can be done separately.

Test: fast/animation/animation-element-removal.html

• dom/Element.cpp:

(WebCore::Element::removedFrom):

Ensure animations are canceled when element is removed from the tree.

(WebCore::Element::clearHasPendingResources):
(WebCore::Element::hasCSSAnimation const):
(WebCore::Element::setHasCSSAnimation):
(WebCore::Element::clearHasCSSAnimation):

Add a bit so we don't need to do hash lookups for every removal.

• dom/Element.h:
• dom/ElementRareData.h:

(WebCore::ElementRareData::hasCSSAnimation const):
(WebCore::ElementRareData::setHasCSSAnimation):
(WebCore::ElementRareData::ElementRareData):

• page/animation/CSSAnimationController.cpp:

(WebCore::CSSAnimationControllerPrivate::ensureCompositeAnimation):

Test for the bit.

(WebCore::CSSAnimationControllerPrivate::clear):
(WebCore::CSSAnimationController::cancelAnimations):

LayoutTests:

Reviewed by Geoff Garen.

• fast/animation/animation-element-removal-expected.txt: Added.
• fast/animation/animation-element-removal.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/animation/animation-element-removal-expected.txt (added)
• LayoutTests/fast/animation/animation-element-removal.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/dom/Element.cpp (modified) (3 diffs)
• Source/WebCore/dom/Element.h (modified) (1 diff)
• Source/WebCore/dom/ElementRareData.h (modified) (3 diffs)
• Source/WebCore/page/animation/CSSAnimationController.cpp (modified) (2 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2017-09-27  Antti Koivisto  <antti@apple.com>
+
+        REGRESSION (r222040): Crash navigating out of gfycat.com url
+        https://bugs.webkit.org/show_bug.cgi?id=177531
+
+        Reviewed by Geoff Garen.
+
+        * fast/animation/animation-element-removal-expected.txt: Added.
+        * fast/animation/animation-element-removal.html: Added.
+
 2017-09-27  Per Arne Vollan  <pvollan@apple.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2017-09-27  Antti Koivisto  <antti@apple.com>
+
+        REGRESSION (r222040): Crash navigating out of gfycat.com url
+        https://bugs.webkit.org/show_bug.cgi?id=177531
+        <rdar://problem/34602601>
+
+        Reviewed by Geoff Garen.
+
+        Animation structures are normally removed when the render tree is torn down.
+        However there are cases where we can instantiate animation without creating a renderer
+        and we need to make sure animations are canceled in these cases too.
+
+        CompositeAnimations should also ref the element but that can be done separately.
+
+        Test: fast/animation/animation-element-removal.html
+
+        * dom/Element.cpp:
+        (WebCore::Element::removedFrom):
+
+            Ensure animations are canceled when element is removed from the tree.
+
+        (WebCore::Element::clearHasPendingResources):
+        (WebCore::Element::hasCSSAnimation const):
+        (WebCore::Element::setHasCSSAnimation):
+        (WebCore::Element::clearHasCSSAnimation):
+
+            Add a bit so we don't need to do hash lookups for every removal.
+
+        * dom/Element.h:
+        * dom/ElementRareData.h:
+        (WebCore::ElementRareData::hasCSSAnimation const):
+        (WebCore::ElementRareData::setHasCSSAnimation):
+        (WebCore::ElementRareData::ElementRareData):
+        * page/animation/CSSAnimationController.cpp:
+        (WebCore::CSSAnimationControllerPrivate::ensureCompositeAnimation):
+
+            Test for the bit.
+
+        (WebCore::CSSAnimationControllerPrivate::clear):
+        (WebCore::CSSAnimationController::cancelAnimations):
+
 2017-09-27  Joanmarie Diggs  <jdiggs@igalia.com>
 

trunk/Source/WebCore/dom/Element.cpp

@@ -30 +30 @@
 #include "Attr.h"
 #include "AttributeChangeInvalidation.h"
+#include "CSSAnimationController.h"
 #include "CSSParser.h"
 #include "Chrome.h"
@@ -1765 +1766 @@
         document().accessSVGExtensions().removeElementFromPendingResources(this);
 
+    Frame* frame = document().frame();
+    if (frame)
+        frame->animation().cancelAnimations(*this);
 
 #if PLATFORM(MAC)
-    if (Frame* frame = document().frame())
+    if (frame)
         frame->mainFrame().removeLatchingStateForTarget(*this);
 #endif
@@ -3532 +3536 @@
 void Element::clearHasPendingResources()
 {
-    ensureElementRareData().setHasPendingResources(false);
+    if (!hasRareData())
+        return;
+    elementRareData()->setHasPendingResources(false);
+}
+
+bool Element::hasCSSAnimation() const
+{
+    return hasRareData() && elementRareData()->hasCSSAnimation();
+}
+
+void Element::setHasCSSAnimation()
+{
+    ensureElementRareData().setHasCSSAnimation(true);
+}
+
+void Element::clearHasCSSAnimation()
+{
+    if (!hasRareData())
+        return;
+    elementRareData()->setHasCSSAnimation(false);
 }
 

trunk/Source/WebCore/dom/Element.h

@@ -450 +450 @@
     virtual void buildPendingResource() { };
 
+    bool hasCSSAnimation() const;
+    void setHasCSSAnimation();
+    void clearHasCSSAnimation();
+
 #if ENABLE(FULLSCREEN_API)
     WEBCORE_EXPORT bool containsFullScreenElement() const;

trunk/Source/WebCore/dom/ElementRareData.h

@@ -107 +107 @@
     bool hasPendingResources() const { return m_hasPendingResources; }
     void setHasPendingResources(bool has) { m_hasPendingResources = has; }
+
+    bool hasCSSAnimation() const { return m_hasCSSAnimation; }
+    void setHasCSSAnimation(bool value) { m_hasCSSAnimation = value; }
 
 private:
@@ -119 +122 @@
 #endif
     unsigned m_hasPendingResources : 1;
+    unsigned m_hasCSSAnimation : 1;
     unsigned m_childrenAffectedByHover : 1;
     unsigned m_childrenAffectedByDrag : 1;
@@ -161 +165 @@
 #endif
     , m_hasPendingResources(false)
+    , m_hasCSSAnimation(false)
     , m_childrenAffectedByHover(false)
     , m_childrenAffectedByDrag(false)

trunk/Source/WebCore/page/animation/CSSAnimationController.cpp

@@ -86 +86 @@
 CompositeAnimation& CSSAnimationControllerPrivate::ensureCompositeAnimation(Element& element)
 {
+    element.setHasCSSAnimation();
+
     auto result = m_compositeAnimations.ensure(&element, [&] {
         return CompositeAnimation::create(*this);
@@ -98 +100 @@
 bool CSSAnimationControllerPrivate::clear(Element& element)
 {
+    ASSERT(element.hasCSSAnimation() == m_compositeAnimations.contains(&element));
+
+    if (!element.hasCSSAnimation())
+        return false;
+    element.clearHasCSSAnimation();
+
     auto it = m_compositeAnimations.find(&element);
     if (it == m_compositeAnimations.end())