REGRESSION (r222040): Crash navigating out of gfycat.com url
authorantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 27 Sep 2017 15:32:55 +0000 (15:32 +0000)
committerantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 27 Sep 2017 15:32:55 +0000 (15:32 +0000)
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.

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

LayoutTests/ChangeLog
LayoutTests/fast/animation/animation-element-removal-expected.txt [new file with mode: 0644]
LayoutTests/fast/animation/animation-element-removal.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/dom/Element.cpp
Source/WebCore/dom/Element.h
Source/WebCore/dom/ElementRareData.h
Source/WebCore/page/animation/CSSAnimationController.cpp

index e699cdb..8876e67 100644 (file)
@@ -1,3 +1,13 @@
+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>
 
         Mark accessibility/image-load-on-delay.html as a failure on Windows.
diff --git a/LayoutTests/fast/animation/animation-element-removal-expected.txt b/LayoutTests/fast/animation/animation-element-removal-expected.txt
new file mode 100644 (file)
index 0000000..d06fab5
--- /dev/null
@@ -0,0 +1 @@
+This test passes if it doesn't hit an assert
diff --git a/LayoutTests/fast/animation/animation-element-removal.html b/LayoutTests/fast/animation/animation-element-removal.html
new file mode 100644 (file)
index 0000000..47984a2
--- /dev/null
@@ -0,0 +1,24 @@
+<html>
+<head>
+<style>
+foo { animation: anim 0s infinite alternate;}
+</style>
+<script>
+if (window.testRunner) {
+    testRunner.dumpAsText();
+    testRunner.waitUntilDone();
+}
+function js() {
+var test = document.getElementById("test");
+document.createElement("div").insertAdjacentElement("afterBegin",test);
+window.onpageshow = function() {
+       setTimeout(() => {
+        location.href = `data:text/html,<body onload="testRunner.notifyDone()">This test passes if it doesn't hit an assert`;
+    }, 10);
+};
+}
+</script>
+</head>
+<body onload=js()>
+<svg>
+<foo id="test">
index 6b25d23..ce6023b 100644 (file)
@@ -1,3 +1,44 @@
+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>
 
         [ATK] atk_table_cell_get_position() should return values of aria-rowindex and aria-colindex, if present
index 3cc4583..5142195 100644 (file)
@@ -29,6 +29,7 @@
 #include "AXObjectCache.h"
 #include "Attr.h"
 #include "AttributeChangeInvalidation.h"
+#include "CSSAnimationController.h"
 #include "CSSParser.h"
 #include "Chrome.h"
 #include "ChromeClient.h"
@@ -1764,9 +1765,12 @@ void Element::removedFrom(ContainerNode& insertionPoint)
     if (hasPendingResources())
         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
 }
@@ -3531,7 +3535,26 @@ void Element::setHasPendingResources()
 
 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);
 }
 
 bool Element::canContainRangeEndPoint() const
index 9c1dc4e..c35a183 100644 (file)
@@ -449,6 +449,10 @@ public:
     void clearHasPendingResources();
     virtual void buildPendingResource() { };
 
+    bool hasCSSAnimation() const;
+    void setHasCSSAnimation();
+    void clearHasCSSAnimation();
+
 #if ENABLE(FULLSCREEN_API)
     WEBCORE_EXPORT bool containsFullScreenElement() const;
     void setContainsFullScreenElement(bool);
index ec0886f..f9b46f6 100644 (file)
@@ -107,6 +107,9 @@ public:
     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:
     int m_tabIndex;
     unsigned short m_childIndex;
@@ -118,6 +121,7 @@ private:
     unsigned m_containsFullScreenElement : 1;
 #endif
     unsigned m_hasPendingResources : 1;
+    unsigned m_hasCSSAnimation : 1;
     unsigned m_childrenAffectedByHover : 1;
     unsigned m_childrenAffectedByDrag : 1;
     // Bits for dynamic child matching.
@@ -160,6 +164,7 @@ inline ElementRareData::ElementRareData(RenderElement* renderer)
     , m_containsFullScreenElement(false)
 #endif
     , m_hasPendingResources(false)
+    , m_hasCSSAnimation(false)
     , m_childrenAffectedByHover(false)
     , m_childrenAffectedByDrag(false)
     , m_childrenAffectedByLastChildRules(false)
index d0cd635..83ada17 100644 (file)
@@ -85,6 +85,8 @@ CSSAnimationControllerPrivate::~CSSAnimationControllerPrivate()
 
 CompositeAnimation& CSSAnimationControllerPrivate::ensureCompositeAnimation(Element& element)
 {
+    element.setHasCSSAnimation();
+
     auto result = m_compositeAnimations.ensure(&element, [&] {
         return CompositeAnimation::create(*this);
     });
@@ -97,6 +99,12 @@ CompositeAnimation& CSSAnimationControllerPrivate::ensureCompositeAnimation(Elem
 
 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())
         return false;