Attachment 341273 Details for Bug 185917 – Patch

[patch] Patch

bug-185917-20180525143547.patch (text/plain), 40.99 KB, created by Antoine Quint on 2018-05-25 05:35:49 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Antoine Quint

Created: 2018-05-25 05:35:49 PDT

Size: 40.99 KB

patch

obsolete

>Subversion Revision: 232151
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 348060c76af8e2097de1dc79a08a8919125821de..34e3ed16714b79e4a418067b35c5c9be3db7ed9f 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,77 @@
>+2018-05-25  Antoine Quint  <graouts@apple.com>
>+
>+        [Web Animations] WebAnimation objects never get destroyed
>+        https://bugs.webkit.org/show_bug.cgi?id=185917
>+        <rdar://problem/39539371>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        The AnimationTimeline class keeps references to WebAnimation objects organized in various ways. First, there
>+        are three main maps across which all animations are stored, one for non-subclass WebAnimation objects
>+        (m_elementToAnimationsMap), one for CSSSAnimation objects (m_elementToCSSAnimationsMap) and one for CSSTranstion
>+        objects (m_elementToCSSTransitionsMap). On top of that, we also keep a map to access CSSAnimation objects for
>+        a given element by CSS animation name (m_elementToCSSAnimationByName) and another map to access CSSTransition 
>+        objects for a given element by CSS property (m_elementToCSSTransitionByCSSPropertyID).
>+
>+        None of the RefPtr<WebAnimation> stored in these maps would get cleared when the document would get torn down,
>+        which would also prevent the AnimationTimeline (and its DocumentTimeline subclass) from being destroyed.
>+
>+        We now ensure that element and document tear-down correctly removes animations and clears those maps, which
>+        in turn allows the DocumentTimeline to be destroyed, fixing the significant memory leak introduced by Web Animations
>+        so far.
>+
>+        Finally, we change the collection type for those maps to be ListHashRef instead of Vector to guarantee we only
>+        add an animation once per collection due to changes in how setEffect() and setTimeline() operate.
>+
>+        Test: animations/leak-document-with-css-animation.html
>+
>+        * animation/AnimationTimeline.cpp:
>+        (WebCore::AnimationTimeline::~AnimationTimeline): There is no need to clear those tables as they'll need to be empty
>+        for the AnimationTimeline to even be destroyed.
>+        (WebCore::AnimationTimeline::relevantMapForAnimation): Change to use ListHashRef instead of Vector.
>+        (WebCore::AnimationTimeline::animationWasAddedToElement): Change to use ListHashRef instead of Vector.
>+        (WebCore::AnimationTimeline::animationWasRemovedFromElement): When an animation is removed from an element, ensure that
>+        references to this animation stored in the m_elementToCSSAnimationByName and m_elementToCSSTransitionByCSSPropertyID maps
>+        are cleared.
>+        (WebCore::AnimationTimeline::animationsForElement const): Change to use ListHashRef instead of Vector.
>+        (WebCore::AnimationTimeline::removeAnimationsForElement): Instead of just calling cancel() on all known declarative animations
>+        (this method used to be called cancelDeclarativeAnimationsForElement()), we now set the effect of known animations, declarative
>+        or not, for the provided element which will in turn call animationWasRemovedFromElement() and remove the animation from all
>+        maps that might keep a reference to it.
>+        (WebCore::AnimationTimeline::updateCSSTransitionsForElement): Replace call to removeDeclarativeAnimation() with a simple call
>+        to removeAnimation() which will remove references for this animation from the relevant maps.
>+        (WebCore::AnimationTimeline::cancelOrRemoveDeclarativeAnimation): Ditto.
>+        (WebCore::AnimationTimeline::cancelDeclarativeAnimationsForElement): Deleted.
>+        (WebCore::AnimationTimeline::removeDeclarativeAnimation): Deleted.
>+        * animation/AnimationTimeline.h:
>+        (WebCore::AnimationTimeline::elementToAnimationsMap): Change to use ListHashRef instead of Vector.
>+        (WebCore::AnimationTimeline::elementToCSSAnimationsMap): Change to use ListHashRef instead of Vector.
>+        (WebCore::AnimationTimeline::elementToCSSTransitionsMap): Change to use ListHashRef instead of Vector.
>+        * animation/WebAnimation.cpp:
>+        (WebCore::WebAnimation::setEffect): In the case of a declarative animation, we don't want to remove the animation from the relevant
>+        maps because while the effect was set via the API, the element still has a transition or animation set up and we must not break the
>+        timeline-to-animation relationship.
>+        (WebCore::WebAnimation::setEffectInternal): Factor parts of setEffect() out into a new method that can be called from
>+        AnimationTimeline::removeAnimationsForElement() to reset the m_effect member and correctly call animationWasRemovedFromElement()
>+        without all the Web Animations machinery of setEffect(), which is a public API that has unwanted side effects (such as rejecting
>+        promises).
>+        (WebCore::WebAnimation::setTimeline): In the case of a declarative animation, we don't want to remove the animation from the
>+        relevant maps because, while the timeline was set via the API, the element still has a transition or animation set up and we must
>+        not break the relationship.
>+        * animation/DocumentTimeline.cpp:
>+        (WebCore::DocumentTimeline::~DocumentTimeline):
>+        (WebCore::DocumentTimeline::detachFromDocument): Close the GenericTaskQueues when detaching from the document as it's too late to
>+        perform this work in the destructor. We also cancel the schedule timer which we had forgotten to do before.
>+        * animation/WebAnimation.h:
>+        * dom/Document.cpp:
>+        (WebCore::Document::prepareForDestruction):
>+        * dom/Element.cpp:
>+        (WebCore::Element::removedFromAncestor):
>+        * dom/PseudoElement.cpp:
>+        (WebCore::PseudoElement::clearHostElement):
>+        * rendering/updating/RenderTreeUpdater.cpp:
>+        (WebCore::RenderTreeUpdater::tearDownRenderers):
>+
> 2018-05-23  Joseph Pecoraro  <pecoraro@apple.com>
> 
>         Use ASCIILiteral with applicationBundleIsEqualTo in RuntimeApplicationChecksCocoa
>diff --git a/Source/WebCore/animation/AnimationTimeline.cpp b/Source/WebCore/animation/AnimationTimeline.cpp
>index 1aa8dbc7bb0f5b0780bc741174d705580a871dd1..25255a40c81600d7477ac68f433f74999892c0b6 100644
>--- a/Source/WebCore/animation/AnimationTimeline.cpp
>+++ b/Source/WebCore/animation/AnimationTimeline.cpp
>@@ -51,12 +51,6 @@ AnimationTimeline::AnimationTimeline(ClassType classType)
> 
> AnimationTimeline::~AnimationTimeline()
> {
>-    m_animations.clear();
>-    m_elementToAnimationsMap.clear();
>-    m_elementToCSSAnimationsMap.clear();
>-    m_elementToCSSTransitionsMap.clear();
>-    m_elementToCSSAnimationByName.clear();
>-    m_elementToCSSTransitionByCSSPropertyID.clear();
> }
> 
> void AnimationTimeline::addAnimation(Ref<WebAnimation>&& animation)
>@@ -85,7 +79,7 @@ void AnimationTimeline::setCurrentTime(Seconds currentTime)
>     timingModelDidChange();
> }
> 
>-HashMap<Element*, Vector<RefPtr<WebAnimation>>>& AnimationTimeline::relevantMapForAnimation(WebAnimation& animation)
>+HashMap<Element*, ListHashSet<RefPtr<WebAnimation>>>& AnimationTimeline::relevantMapForAnimation(WebAnimation& animation)
> {
>     if (animation.isCSSAnimation())
>         return m_elementToCSSAnimationsMap;
>@@ -96,42 +90,71 @@ HashMap<Element*, Vector<RefPtr<WebAnimation>>>& AnimationTimeline::relevantMapF
> 
> void AnimationTimeline::animationWasAddedToElement(WebAnimation& animation, Element& element)
> {
>-    auto result = relevantMapForAnimation(animation).ensure(&element, [] {
>-        return Vector<RefPtr<WebAnimation>> { };
>-    });
>-    result.iterator->value.append(&animation);
>+    relevantMapForAnimation(animation).ensure(&element, [] {
>+        return ListHashSet<RefPtr<WebAnimation>> { };
>+    }).iterator->value.add(&animation);
> }
> 
> void AnimationTimeline::animationWasRemovedFromElement(WebAnimation& animation, Element& element)
> {
>+    // First, we clear this animation from one of the m_elementToCSSAnimationsMap, m_elementToCSSTransitionsMap
>+    // or m_elementToAnimationsMap map, whichever is relevant to this type of animation.
>     auto& map = relevantMapForAnimation(animation);
>     auto iterator = map.find(&element);
>     if (iterator == map.end())
>         return;
> 
>     auto& animations = iterator->value;
>-    animations.removeFirst(&animation);
>+    animations.remove(&animation);
>     if (!animations.size())
>         map.remove(iterator);
>+
>+    // Now, if we're dealing with a declarative animation, we remove it from either the m_elementToCSSAnimationByName
>+    // or the m_elementToCSSTransitionByCSSPropertyID map, whichever is relevant to this type of animation.
>+    if (is<CSSAnimation>(animation)) {
>+        auto iterator = m_elementToCSSAnimationByName.find(&element);
>+        if (iterator != m_elementToCSSAnimationByName.end()) {
>+            auto& cssAnimationsByName = iterator->value;
>+            auto& name = downcast<CSSAnimation>(animation).animationName();
>+            cssAnimationsByName.remove(name);
>+            if (cssAnimationsByName.isEmpty())
>+                m_elementToCSSAnimationByName.remove(&element);
>+        }
>+    } else if (is<CSSTransition>(animation)) {
>+        auto iterator = m_elementToCSSTransitionByCSSPropertyID.find(&element);
>+        if (iterator != m_elementToCSSTransitionByCSSPropertyID.end()) {
>+            auto& cssTransitionsByProperty = iterator->value;
>+            auto property = downcast<CSSTransition>(animation).property();
>+            cssTransitionsByProperty.remove(property);
>+            if (cssTransitionsByProperty.isEmpty())
>+                m_elementToCSSTransitionByCSSPropertyID.remove(&element);
>+        }
>+    }
> }
> 
> Vector<RefPtr<WebAnimation>> AnimationTimeline::animationsForElement(Element& element) const
> {
>     Vector<RefPtr<WebAnimation>> animations;
>-    if (m_elementToCSSAnimationsMap.contains(&element))
>-        animations.appendVector(m_elementToCSSAnimationsMap.get(&element));
>-    if (m_elementToCSSTransitionsMap.contains(&element))
>-        animations.appendVector(m_elementToCSSTransitionsMap.get(&element));
>-    if (m_elementToAnimationsMap.contains(&element))
>-        animations.appendVector(m_elementToAnimationsMap.get(&element));
>+    if (m_elementToCSSAnimationsMap.contains(&element)) {
>+        const auto& cssAnimations = m_elementToCSSAnimationsMap.get(&element);
>+        animations.appendRange(cssAnimations.begin(), cssAnimations.end());
>+    }
>+    if (m_elementToCSSTransitionsMap.contains(&element)) {
>+        const auto& cssTransitions = m_elementToCSSTransitionsMap.get(&element);
>+        animations.appendRange(cssTransitions.begin(), cssTransitions.end());
>+    }
>+    if (m_elementToAnimationsMap.contains(&element)) {
>+        const auto& webAnimations = m_elementToAnimationsMap.get(&element);
>+        animations.appendRange(webAnimations.begin(), webAnimations.end());
>+    }
>     return animations;
> }
> 
>-void AnimationTimeline::cancelDeclarativeAnimationsForElement(Element& element)
>+void AnimationTimeline::removeAnimationsForElement(Element& element)
> {
>-    for (const auto& animation : animationsForElement(element)) {
>-        if (is<DeclarativeAnimation>(animation))
>-            animation->cancel();
>+    for (auto& animation : animationsForElement(element)) {
>+        animation->setEffectInternal(nullptr);
>+        removeAnimation(animation.releaseNonNull());
>     }
> }
> 
>@@ -289,7 +312,7 @@ void AnimationTimeline::updateCSSTransitionsForElement(Element& element, const R
>                     if (cssTransitionsByProperty.contains(property)) {
>                         if (cssTransitionsByProperty.get(property)->matchesBackingAnimationAndStyles(backingAnimation, oldStyle, newStyle))
>                             continue;
>-                        removeDeclarativeAnimation(cssTransitionsByProperty.take(property));
>+                        removeAnimation(cssTransitionsByProperty.take(property).releaseNonNull());
>                     }
>                     // Now we can create a new CSSTransition with the new backing animation provided it has a valid
>                     // duration and the from and to values are distinct.
>@@ -315,19 +338,13 @@ void AnimationTimeline::updateCSSTransitionsForElement(Element& element, const R
>         m_elementToCSSTransitionByCSSPropertyID.remove(&element);
> }
> 
>-void AnimationTimeline::removeDeclarativeAnimation(RefPtr<DeclarativeAnimation> animation)
>-{
>-    animation->setEffect(nullptr);
>-    removeAnimation(animation.releaseNonNull());
>-}
>-
> void AnimationTimeline::cancelOrRemoveDeclarativeAnimation(RefPtr<DeclarativeAnimation> animation)
> {
>     auto phase = animation->effect()->phase();
>     if (phase != AnimationEffectReadOnly::Phase::Idle && phase != AnimationEffectReadOnly::Phase::After)
>         animation->cancel();
>     else
>-        removeDeclarativeAnimation(animation);
>+        removeAnimation(animation.releaseNonNull());
> }
> 
> String AnimationTimeline::description()
>diff --git a/Source/WebCore/animation/AnimationTimeline.h b/Source/WebCore/animation/AnimationTimeline.h
>index f3968fe24adff7d3902e2c24569eef5a5a1a0d4f..3680cb0ef697b819fc42824f3810a775ecd34e0d 100644
>--- a/Source/WebCore/animation/AnimationTimeline.h
>+++ b/Source/WebCore/animation/AnimationTimeline.h
>@@ -59,7 +59,7 @@ public:
> 
>     const ListHashSet<RefPtr<WebAnimation>>& animations() const { return m_animations; }
>     Vector<RefPtr<WebAnimation>> animationsForElement(Element&) const;
>-    void cancelDeclarativeAnimationsForElement(Element&);
>+    void removeAnimationsForElement(Element&);
>     void animationWasAddedToElement(WebAnimation&, Element&);
>     void animationWasRemovedFromElement(WebAnimation&, Element&);
> 
>@@ -79,21 +79,20 @@ protected:
> 
>     bool hasElementAnimations() const { return !m_elementToAnimationsMap.isEmpty() || !m_elementToCSSAnimationsMap.isEmpty() || !m_elementToCSSTransitionsMap.isEmpty(); }
> 
>-    const HashMap<Element*, Vector<RefPtr<WebAnimation>>>& elementToAnimationsMap() { return m_elementToAnimationsMap; }
>-    const HashMap<Element*, Vector<RefPtr<WebAnimation>>>& elementToCSSAnimationsMap() { return m_elementToCSSAnimationsMap; }
>-    const HashMap<Element*, Vector<RefPtr<WebAnimation>>>& elementToCSSTransitionsMap() { return m_elementToCSSTransitionsMap; }
>-    void removeDeclarativeAnimation(RefPtr<DeclarativeAnimation>);
>+    const HashMap<Element*, ListHashSet<RefPtr<WebAnimation>>>& elementToAnimationsMap() { return m_elementToAnimationsMap; }
>+    const HashMap<Element*, ListHashSet<RefPtr<WebAnimation>>>& elementToCSSAnimationsMap() { return m_elementToCSSAnimationsMap; }
>+    const HashMap<Element*, ListHashSet<RefPtr<WebAnimation>>>& elementToCSSTransitionsMap() { return m_elementToCSSTransitionsMap; }
> 
> private:
>-    HashMap<Element*, Vector<RefPtr<WebAnimation>>>& relevantMapForAnimation(WebAnimation&);
>+    HashMap<Element*, ListHashSet<RefPtr<WebAnimation>>>& relevantMapForAnimation(WebAnimation&);
>     void cancelOrRemoveDeclarativeAnimation(RefPtr<DeclarativeAnimation>);
>     RefPtr<WebAnimation> cssAnimationForElementAndProperty(Element&, CSSPropertyID);
> 
>     ClassType m_classType;
>     std::optional<Seconds> m_currentTime;
>-    HashMap<Element*, Vector<RefPtr<WebAnimation>>> m_elementToAnimationsMap;
>-    HashMap<Element*, Vector<RefPtr<WebAnimation>>> m_elementToCSSAnimationsMap;
>-    HashMap<Element*, Vector<RefPtr<WebAnimation>>> m_elementToCSSTransitionsMap;
>+    HashMap<Element*, ListHashSet<RefPtr<WebAnimation>>> m_elementToAnimationsMap;
>+    HashMap<Element*, ListHashSet<RefPtr<WebAnimation>>> m_elementToCSSAnimationsMap;
>+    HashMap<Element*, ListHashSet<RefPtr<WebAnimation>>> m_elementToCSSTransitionsMap;
>     ListHashSet<RefPtr<WebAnimation>> m_animations;
> 
>     HashMap<Element*, HashMap<String, RefPtr<CSSAnimation>>> m_elementToCSSAnimationByName;
>diff --git a/Source/WebCore/animation/DocumentTimeline.cpp b/Source/WebCore/animation/DocumentTimeline.cpp
>index 8e8cc23594d2695b0363f6b6c53b56a7ad75d06b..0649696616ad0410a49351a69facb72c5b3efeb7 100644
>--- a/Source/WebCore/animation/DocumentTimeline.cpp
>+++ b/Source/WebCore/animation/DocumentTimeline.cpp
>@@ -61,12 +61,13 @@ DocumentTimeline::DocumentTimeline(Document& document, PlatformDisplayID display
> 
> DocumentTimeline::~DocumentTimeline()
> {
>-    m_invalidationTaskQueue.close();
>-    m_eventDispatchTaskQueue.close();
> }
> 
> void DocumentTimeline::detachFromDocument()
> {
>+    m_invalidationTaskQueue.close();
>+    m_eventDispatchTaskQueue.close();
>+    m_animationScheduleTimer.stop();
>     m_document = nullptr;
> }
> 
>diff --git a/Source/WebCore/animation/WebAnimation.cpp b/Source/WebCore/animation/WebAnimation.cpp
>index 4dcb5e2b3829c8834a15c3d0795437bef5f0d956..82b99a2b96c2ea99f0d6436c9bb2ceacc9e57ecf 100644
>--- a/Source/WebCore/animation/WebAnimation.cpp
>+++ b/Source/WebCore/animation/WebAnimation.cpp
>@@ -119,30 +119,44 @@ void WebAnimation::setEffect(RefPtr<AnimationEffectReadOnly>&& newEffect)
>         newEffect->animation()->setEffect(nullptr);
> 
>     // 7. Let the target effect of animation be new effect.
>-    m_effect = WTFMove(newEffect);
>+    // In the case of a declarative animation, we don't want to remove the animation from the relevant maps because
>+    // while the effect was set via the API, the element still has a transition or animation set up and we must
>+    // not break the timeline-to-animation relationship.
>+    setEffectInternal(WTFMove(newEffect), isDeclarativeAnimation());
> 
>     // 8. Run the procedure to update an animationâs finished state for animation with the did seek flag set to false,
>     // and the synchronously notify flag set to false.
>     updateFinishedState(DidSeek::No, SynchronouslyNotify::No);
> 
>+    timingModelDidChange();
>+}
>+
>+void WebAnimation::setEffectInternal(RefPtr<AnimationEffectReadOnly>&& newEffect, bool doNotRemoveFromTimeline)
>+{
>+    auto oldEffect = m_effect;
>+
>+    m_effect = WTFMove(newEffect);
>+
>+    Element* previousTarget;
>+    if (is<KeyframeEffectReadOnly>(oldEffect))
>+        previousTarget = downcast<KeyframeEffectReadOnly>(oldEffect.get())->target();
>+
>+    Element* newTarget;
>+    if (is<KeyframeEffectReadOnly>(m_effect))
>+        newTarget = downcast<KeyframeEffectReadOnly>(m_effect.get())->target();
>+
>     // Update the effect-to-animation relationships and the timeline's animation map.
>     if (oldEffect) {
>         oldEffect->setAnimation(nullptr);
>-        if (m_timeline && is<KeyframeEffectReadOnly>(oldEffect)) {
>-            if (auto* target = downcast<KeyframeEffectReadOnly>(oldEffect.get())->target())
>-                m_timeline->animationWasRemovedFromElement(*this, *target);
>-        }
>+        if (!doNotRemoveFromTimeline && m_timeline && previousTarget && previousTarget != newTarget)
>+            m_timeline->animationWasRemovedFromElement(*this, *previousTarget);
>     }
> 
>     if (m_effect) {
>         m_effect->setAnimation(this);
>-        if (m_timeline && is<KeyframeEffectReadOnly>(m_effect)) {
>-            if (auto* target = downcast<KeyframeEffectReadOnly>(m_effect.get())->target())
>-                m_timeline->animationWasAddedToElement(*this, *target);
>-        }
>+        if (m_timeline && newTarget && previousTarget != newTarget)
>+            m_timeline->animationWasAddedToElement(*this, *newTarget);
>     }
>-
>-    timingModelDidChange();
> }
> 
> void WebAnimation::setTimeline(RefPtr<AnimationTimeline>&& timeline)
>@@ -168,7 +182,10 @@ void WebAnimation::setTimeline(RefPtr<AnimationTimeline>&& timeline)
>         auto* keyframeEffect = downcast<KeyframeEffectReadOnly>(m_effect.get());
>         auto* target = keyframeEffect->target();
>         if (target) {
>-            if (m_timeline)
>+            // In the case of a declarative animation, we don't want to remove the animation from the relevant maps because
>+            // while the timeline was set via the API, the element still has a transition or animation set up and we must
>+            // not break the relationship.
>+            if (m_timeline && !isDeclarativeAnimation())
>                 m_timeline->animationWasRemovedFromElement(*this, *target);
>             if (timeline)
>                 timeline->animationWasAddedToElement(*this, *target);
>diff --git a/Source/WebCore/animation/WebAnimation.h b/Source/WebCore/animation/WebAnimation.h
>index 3ab99762a60429862a435b8c0ccce978780245c8..5314b7e84187dc0065ef921d20ec575b10f9c585 100644
>--- a/Source/WebCore/animation/WebAnimation.h
>+++ b/Source/WebCore/animation/WebAnimation.h
>@@ -63,6 +63,7 @@ public:
> 
>     AnimationEffectReadOnly* effect() const { return m_effect.get(); }
>     void setEffect(RefPtr<AnimationEffectReadOnly>&&);
>+    void setEffectInternal(RefPtr<AnimationEffectReadOnly>&&, bool = false);
>     AnimationTimeline* timeline() const { return m_timeline.get(); }
>     virtual void setTimeline(RefPtr<AnimationTimeline>&&);
> 
>diff --git a/Source/WebCore/dom/Document.cpp b/Source/WebCore/dom/Document.cpp
>index a321e8725d075b71ca0b209ec3c958269bf9e282..2838b2304c0209afcd006dae0f5f51c8cf37d9a7 100644
>--- a/Source/WebCore/dom/Document.cpp
>+++ b/Source/WebCore/dom/Document.cpp
>@@ -2336,11 +2336,6 @@ void Document::prepareForDestruction()
>     if (m_hasPreparedForDestruction)
>         return;
> 
>-    if (m_timeline) {
>-        m_timeline->detachFromDocument();
>-        m_timeline = nullptr;
>-    }
>-
>     if (m_frame)
>         m_frame->animation().detachFromDocument(this);
> 
>@@ -2433,6 +2428,11 @@ void Document::prepareForDestruction()
> 
>     detachFromFrame();
> 
>+    if (m_timeline) {
>+        m_timeline->detachFromDocument();
>+        m_timeline = nullptr;
>+    }
>+
>     m_hasPreparedForDestruction = true;
> 
>     // Note that m_pageCacheState can be Document::AboutToEnterPageCache if our frame
>diff --git a/Source/WebCore/dom/Element.cpp b/Source/WebCore/dom/Element.cpp
>index b1ff990af4b8b3c668e165d5b467f71f08ed6e20..48c925124ea9306f52cfb039d8a85ab0b6f1abc4 100644
>--- a/Source/WebCore/dom/Element.cpp
>+++ b/Source/WebCore/dom/Element.cpp
>@@ -1797,7 +1797,7 @@ void Element::removedFromAncestor(RemovalType removalType, ContainerNode& oldPar
>     RefPtr<Frame> frame = document().frame();
>     if (RuntimeEnabledFeatures::sharedFeatures().webAnimationsCSSIntegrationEnabled()) {
>         if (auto* timeline = document().existingTimeline())
>-            timeline->cancelDeclarativeAnimationsForElement(*this);
>+            timeline->removeAnimationsForElement(*this);
>     } else if (frame)
>         frame->animation().cancelAnimations(*this);
> 
>diff --git a/Source/WebCore/dom/PseudoElement.cpp b/Source/WebCore/dom/PseudoElement.cpp
>index e82b4475543ceec48ab9a0a0f88abfaa9600eafd..a68567f248d1d8a4fe0034def04a583f390bdf0b 100644
>--- a/Source/WebCore/dom/PseudoElement.cpp
>+++ b/Source/WebCore/dom/PseudoElement.cpp
>@@ -92,7 +92,7 @@ void PseudoElement::clearHostElement()
> 
>     if (RuntimeEnabledFeatures::sharedFeatures().webAnimationsCSSIntegrationEnabled()) {
>         if (auto* timeline = document().existingTimeline())
>-            timeline->cancelDeclarativeAnimationsForElement(*this);
>+            timeline->removeAnimationsForElement(*this);
>     } else if (auto* frame = document().frame())
>         frame->animation().cancelAnimations(*this);
> 
>diff --git a/Source/WebCore/rendering/updating/RenderTreeUpdater.cpp b/Source/WebCore/rendering/updating/RenderTreeUpdater.cpp
>index 1c8c61267d097f3612c2b7fe24474e926f378d8d..282e58712ce4b8ae4406e65a612c13d3c7019e39 100644
>--- a/Source/WebCore/rendering/updating/RenderTreeUpdater.cpp
>+++ b/Source/WebCore/rendering/updating/RenderTreeUpdater.cpp
>@@ -555,7 +555,7 @@ void RenderTreeUpdater::tearDownRenderers(Element& root, TeardownType teardownTy
>             if (teardownType == TeardownType::Full || teardownType == TeardownType::RendererUpdateCancelingAnimations) {
>                 if (RuntimeEnabledFeatures::sharedFeatures().webAnimationsCSSIntegrationEnabled()) {
>                     if (timeline)
>-                        timeline->cancelDeclarativeAnimationsForElement(element);
>+                        timeline->removeAnimationsForElement(element);
>                 } else
>                     animationController.cancelAnimations(element);
>             }
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 7b5cfca156a5318bc07d7234f6d19e142c4ada6e..9fda40f95b0bffd4e6c10a6acb93dc643ef83c77 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,21 @@
>+2018-05-25  Antoine Quint  <graouts@apple.com>
>+
>+        [Web Animations] WebAnimation objects never get destroyed
>+        https://bugs.webkit.org/show_bug.cgi?id=185917
>+        <rdar://problem/39539371>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Add a new test that would fail before this fix since the Document would leak. We also remove a homegrown test that was not correct
>+        and is no longer relevant thanks to the tests under imported/mozilla.
>+
>+        * animations/leak-document-with-css-animation-expected.txt: Added.
>+        * animations/leak-document-with-css-animation.html: Added.
>+        * animations/resources/animation-leak-iframe.html: Added.
>+        * platform/win/TestExpectations:
>+        * webanimations/css-transitions-expected.txt: Removed.
>+        * webanimations/css-transitions.html: Removed.
>+
> 2018-05-23  Chris Dumez  <cdumez@apple.com>
> 
>         Regression(r229831): fast/loader/javascript-url-iframe-remove-on-navigate-async-delegate.html is flaky
>diff --git a/LayoutTests/animations/leak-document-with-css-animation-expected.txt b/LayoutTests/animations/leak-document-with-css-animation-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..f79d5ea06d9687d103df51a5ddc937372977698a
>--- /dev/null
>+++ b/LayoutTests/animations/leak-document-with-css-animation-expected.txt
>@@ -0,0 +1,12 @@
>+This test asserts that Document doesn't leak when a CSS Animation is created.
>+
>+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
>+
>+
>+Number of documents prior to loading iframe = 2
>+Number of documents after loading iframe = 3
>+Number of documents after destroying iframe = 2
>+PASS successfullyParsed is true
>+
>+TEST COMPLETE
>+
>diff --git a/LayoutTests/animations/leak-document-with-css-animation.html b/LayoutTests/animations/leak-document-with-css-animation.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..c01988397582d3bab8cf9877315ac190fe9fa7b5
>--- /dev/null
>+++ b/LayoutTests/animations/leak-document-with-css-animation.html
>@@ -0,0 +1,43 @@
>+<!DOCTYPE html>
>+<html>
>+<body onload="runTest()">
>+<script src="../resources/js-test-pre.js"></script>
>+<script>
>+description("This test asserts that Document doesn't leak when a CSS Animation is created.");
>+
>+if (window.internals)
>+    jsTestIsAsync = true;
>+
>+gc();
>+
>+function runTest() {
>+    if (!window.internals)
>+        return;
>+
>+    debug("Number of documents prior to loading iframe = " + internals.numberOfLiveDocuments());
>+
>+    var frame = document.body.appendChild(document.createElement("iframe"));
>+
>+    frame.onload = function() {
>+        if (frame.src === 'about:blank')
>+            return true;
>+
>+        debug("Number of documents after loading iframe = " + internals.numberOfLiveDocuments());
>+
>+        frame.remove();
>+        frame = null;
>+
>+        gc();
>+        setTimeout(function () {
>+            debug("Number of documents after destroying iframe = " + internals.numberOfLiveDocuments());
>+            finishJSTest();
>+        });
>+    }
>+
>+    frame.src = 'resources/animation-leak-iframe.html';
>+}
>+
>+</script>
>+<script src="../resources/js-test-post.js"></script>
>+</body>
>+</html>
>\ No newline at end of file
>diff --git a/LayoutTests/animations/resources/animation-leak-iframe.html b/LayoutTests/animations/resources/animation-leak-iframe.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..7ae02e008acaaf96d25af61db4da73f7eaacef5d
>--- /dev/null
>+++ b/LayoutTests/animations/resources/animation-leak-iframe.html
>@@ -0,0 +1,25 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+  <title></title>
>+  <style type="text/css" media="screen">
>+
>+      div {
>+          width: 100px;
>+          height: 100px;
>+          background-color: black;
>+          animation: move 1s infinite;
>+      }
>+
>+      @keyframes move {
>+          50% { margin-left: 60px; }
>+      }
>+
>+  </style>
>+</head>
>+<body>
>+
>+<div></div>
>+
>+</body>
>+</html>
>diff --git a/LayoutTests/platform/win/TestExpectations b/LayoutTests/platform/win/TestExpectations
>index a1d828ea449fcd9a2468a68dbe0d8e1da7c052e6..9957062b34ce647ba3c4936c6882a21aca05738a 100644
>--- a/LayoutTests/platform/win/TestExpectations
>+++ b/LayoutTests/platform/win/TestExpectations
>@@ -4023,7 +4023,6 @@ webkit.org/b/183393 fast/loader/redirect-to-invalid-url-using-meta-refresh-disal
> webkit.org/b/183393 fast/loader/window-open-to-invalid-url-disallowed.html [ Skip ]
> 
> webkit.org/b/183569 webanimations/css-animations.html [ Failure ]
>-webkit.org/b/183569 webanimations/css-transitions.html [ Failure ]
> 
> webkit.org/b/183953 imported/mozilla/css-animations/test_animation-cancel.html [ Failure ]
> webkit.org/b/183953 imported/mozilla/css-animations/test_animation-finish.html [ Failure ]
>diff --git a/LayoutTests/webanimations/css-transitions-expected.txt b/LayoutTests/webanimations/css-transitions-expected.txt
>deleted file mode 100644
>index 2cbae40ad0886355f325cf1f1160b48a37366234..0000000000000000000000000000000000000000
>--- a/LayoutTests/webanimations/css-transitions-expected.txt
>+++ /dev/null
>@@ -1,11 +0,0 @@
>-
>-PASS A CSS Transition should be reflected entirely as a CSSTransition object on the timeline. 
>-PASS Web Animations should reflect the transition-delay property. 
>-PASS Web Animations should reflect the transition-duration property. 
>-PASS Web Animations should reflect the transition-property property. 
>-PASS Web Animations should not reflect the transition-timing-function property on the effect's timing. 
>-PASS Calling finish() on the animation no longer lists the animation after it has been running. 
>-PASS Seeking the animation to its end time no longer lists the animation after it has been running. 
>-PASS Setting the target's transition-property to none no longer lists the animation after it has been running. 
>-PASS Seeking the target's transition-duration to 0 no longer lists the animation after it has been running. 
>-
>diff --git a/LayoutTests/webanimations/css-transitions.html b/LayoutTests/webanimations/css-transitions.html
>deleted file mode 100644
>index 052e4c4c39c5121b871ea7960d4fd48a51a38c87..0000000000000000000000000000000000000000
>--- a/LayoutTests/webanimations/css-transitions.html
>+++ /dev/null
>@@ -1,188 +0,0 @@
>-<!DOCTYPE html>
>-<meta charset=utf-8>
>-<title>CSS Transitions</title>
>-<body>
>-<script src="../resources/testharness.js"></script>
>-<script src="../resources/testharnessreport.js"></script>
>-<script>
>-
>-'use strict';
>-
>-function targetTest(testCallback, description)
>-{
>-    test(() => {
>-        const target = document.body.appendChild(document.createElement("div"));
>-        testCallback(target);
>-        target.remove();
>-    }, description);
>-}
>-
>-function forceStyleUpdate(element)
>-{
>-    element.offsetLeft;
>-}
>-
>-targetTest(target => {
>-    target.style.left = "50px";
>-
>-    assert_array_equals(target.getAnimations(), [], "An element should not have any animations initially.");
>-
>-    target.style.transitionProperty = "left";
>-    target.style.transitionDelay = "-1s";
>-    target.style.transitionDuration = "2s";
>-    target.style.transitionTimingFunction = "linear";
>-
>-    assert_array_equals(target.getAnimations(), [], "Setting CSS transitions properties should not yield a CSSTransition until a new target value is specified.");
>-
>-    forceStyleUpdate(target);
>-    target.style.left = "100px";
>-
>-    const animation = target.getAnimations()[0];
>-    assert_true(animation instanceof CSSTransition, "The animation is a CSSTransition.");
>-    assert_true(animation instanceof Animation, "The animation is an Animation.");
>-    assert_equals(animation.timeline, target.ownerDocument.timeline, "The animation's timeline is set to the element's document timeline.");
>-    assert_equals(animation.playState, "running", "The animation is running as soon as it's created.");
>-    assert_equals(animation.transitionProperty, "left", "The animation's transitionProperty is set.");
>-
>-    const effect = animation.effect;
>-    assert_true(effect instanceof KeyframeEffectReadOnly, "The animation's effect is a KeyframeEffectReadOnly.");
>-    assert_false(effect instanceof KeyframeEffect, "The animation's effect is not a KeyframeEffect.");
>-    assert_equals(effect.target, target, "The animation's effect is targeting the target.");
>-
>-    const timing = animation.effect.timing;
>-    assert_equals(timing.delay, -1000, "The animation's delay property matches the transition-delay property.");
>-    assert_equals(timing.duration, 2000, "The animation's duration property matches the transition-duration property.");
>-
>-    const computedTiming = animation.effect.getComputedTiming();
>-    assert_equals(computedTiming.activeDuration, 2000, "The animations's computed timing activeDuration property matches the properties set by CSS");
>-    assert_equals(computedTiming.currentIteration, 0, "The animations's computed timing currentIteration property matches the properties set by CSS");
>-    assert_equals(computedTiming.delay, -1000, "The animations's computed timing delay property matches the properties set by CSS");
>-    assert_equals(computedTiming.duration, 2000, "The animations's computed timing duration property matches the properties set by CSS");
>-    assert_equals(computedTiming.endDelay, 0, "The animations's computed timing endDelay property matches the properties set by CSS");
>-    assert_equals(computedTiming.endTime, 1000, "The animations's computed timing endTime property matches the properties set by CSS");
>-    assert_equals(computedTiming.iterationStart, 0, "The animations's computed timing iterationStart property matches the properties set by CSS");
>-    assert_equals(computedTiming.iterations, 1, "The animations's computed timing iterations property matches the properties set by CSS");
>-    assert_equals(computedTiming.localTime, 0, "The animations's computed timing localTime property matches the properties set by CSS");
>-    assert_equals(computedTiming.progress, 0.5, "The animations's computed timing progress property matches the properties set by CSS");
>-    assert_equals(computedTiming.fill, "backwards", "The animations's computed timing fill property matches the default value set for transitions");
>-    assert_equals(computedTiming.easing, "linear", "The animations's computed timing easing property matches the properties set by CSS");
>-    assert_equals(computedTiming.direction, "normal", "The animations's computed timing direction property matches the properties set by CSS");
>-
>-    const keyframes = animation.effect.getKeyframes();
>-    assert_equals(keyframes.length, 2, "The animation's effect has two keyframes.");
>-    assert_equals(keyframes[0].offset, 0, "The animation's effect's first keyframe has a 0 offset.");
>-    assert_equals(keyframes[0].left, "50px", "The animation's effect's first keyframe has its left property set to 50px.");
>-    assert_equals(keyframes[1].offset, 1, "The animation's effect's first keyframe has a 1 offset.");
>-    assert_equals(keyframes[1].left, "100px", "The animation's effect's first keyframe has its left property set to 100px.");
>-
>-    assert_equals(getComputedStyle(effect.target).left, "75px", "The animation's target's computed style reflects the animation state.");
>-}, "A CSS Transition should be reflected entirely as a CSSTransition object on the timeline.");
>-
>-function transitionProperty(target, propertyName, from, to)
>-{
>-    target.style[propertyName] = from;
>-    forceStyleUpdate(target);
>-    target.style[propertyName] = to;
>-}
>-
>-function transitionPropertyUpdateTest(testCallback, description)
>-{
>-    targetTest(target => {
>-        target.style.transitionProperty = "left";
>-        target.style.transitionDelay = "-500ms";
>-        target.style.transitionDuration = "2s";
>-        transitionProperty(target, "left", "50px", "100px");
>-        testCallback(target);
>-    }, description);
>-}
>-
>-transitionPropertyUpdateTest(target => {
>-    const initialAnimation = target.getAnimations()[0];
>-    assert_equals(initialAnimation.effect.timing.delay, -500, "The animation's delay matches the initial transition-delay property.");
>-
>-    target.style.transitionDelay = 0;
>-    const updatedAnimation = target.getAnimations()[0];
>-    assert_equals(updatedAnimation.effect.timing.delay, 0, "The animation's delay matches the updated transition-delay property.");
>-
>-    assert_not_equals(initialAnimation, updatedAnimation, "The animations before and after updating the transition-delay property differ.");
>-}, "Web Animations should reflect the transition-delay property.");
>-
>-transitionPropertyUpdateTest(target => {
>-    const initialAnimation = target.getAnimations()[0];
>-    assert_equals(initialAnimation.effect.timing.duration, 2000, "The animation's duration matches the initial transition-duration property.");
>-
>-    target.style.transitionDuration = "1s";
>-    const updatedAnimation = target.getAnimations()[0];
>-    assert_equals(updatedAnimation.effect.timing.duration, 1000, "The animation's duration matches the updated transition-duration property.");
>-
>-    assert_not_equals(initialAnimation, updatedAnimation, "The animations before and after updating the transition-duration property differ.");
>-}, "Web Animations should reflect the transition-duration property.");
>-
>-targetTest(target => {
>-    target.style.transitionDuration = "2s";
>-
>-    target.style.transitionProperty = "left";
>-    transitionProperty(target, "left", "50px", "100px");
>-
>-    const initialAnimation = target.getAnimations()[0];
>-    assert_equals(target.getAnimations()[0].transitionProperty, "left", "The animation's property matches the initial transition-property CSS property.");
>-
>-    const initialKeyframes = initialAnimation.effect.getKeyframes();
>-    assert_equals(initialKeyframes.length, 2);
>-    assert_equals(initialKeyframes[0].offset, 0);
>-    assert_equals(initialKeyframes[0].left, "50px");
>-    assert_equals(initialKeyframes[1].offset, 1);
>-    assert_equals(initialKeyframes[1].left, "100px");
>-
>-    target.style.transitionProperty = "top";
>-    transitionProperty(target, "top", "50px", "100px");
>-
>-    const updatedAnimation = target.getAnimations()[0];
>-    assert_equals(updatedAnimation.transitionProperty, "top", "The animation's property matches the updated transition-property CSS property.");
>-
>-    const updatedKeyframes = updatedAnimation.effect.getKeyframes();
>-    assert_equals(updatedKeyframes.length, 2);
>-    assert_equals(updatedKeyframes[0].offset, 0);
>-    assert_equals(updatedKeyframes[0].top, "50px");
>-    assert_equals(updatedKeyframes[1].offset, 1);
>-    assert_equals(updatedKeyframes[1].top, "100px");
>-
>-    assert_not_equals(updatedAnimation, initialAnimation, "Changing the transition-property property generates a different CSSTransition object.");
>-}, "Web Animations should reflect the transition-property property.");
>-
>-transitionPropertyUpdateTest(target => {
>-    const initialAnimation = target.getAnimations()[0];
>-    assert_equals(initialAnimation.effect.timing.easing, "linear", "The animation's easing does not match the default transition-timing-function property.");
>-
>-    target.style.transitionTimingFunction = "ease-in";
>-    const updatedAnimation = target.getAnimations()[0];
>-    assert_equals(updatedAnimation.effect.timing.easing, "linear", "The animation's easing does not match the updated transition-timing-function property.");
>-
>-    target.style.removeProperty("transition-timing-function");
>-    const finalAnimation = target.getAnimations()[0];
>-    assert_equals(target.getAnimations()[0].effect.timing.easing, "linear", "The animation's easing does not match the default transition-timing-function value when the property is not set.");
>-}, "Web Animations should not reflect the transition-timing-function property on the effect's timing.");
>-
>-function runAnimationCompletionTest(finalAssertionCallback, description)
>-{
>-    targetTest(target => {
>-        target.style.transitionProperty = "left";
>-        target.style.transitionDuration = "1s";
>-        transitionProperty(target, "left", "50px", "100px");
>-
>-        assert_equals(target.getAnimations().length, 1, "Seting the transition-duration property on top of the transition-property yields a CSSTransition.");
>-        assert_equals(target.getAnimations()[0].playState, "running", "Seting the transition-duration property on top of the transition-property yields a running CSSTransition.");
>-
>-        finalAssertionCallback(target.getAnimations()[0]);
>-
>-        assert_array_equals(target.getAnimations(), [], `${description} no longer lists the animation.`);
>-    }, `${description} no longer lists the animation after it has been running.`);
>-}
>-
>-runAnimationCompletionTest(animation => animation.finish(), "Calling finish() on the animation");
>-runAnimationCompletionTest(animation => animation.currentTime = animation.effect.timing.duration, "Seeking the animation to its end time");
>-runAnimationCompletionTest(animation => animation.effect.target.style.transitionProperty = "none", "Setting the target's transition-property to none");
>-runAnimationCompletionTest(animation => animation.effect.target.style.transitionDuration = 0, "Seeking the target's transition-duration to 0");
>-
>-</script>
>-</body>
>\ No newline at end of file

Actions: View | Formatted Diff | Diff

Attachments on bug 185917: 341114 | 341120 | 341122 | 341128 | 341201 | 341213 | 341273 | 341275