Changeset 232185 in webkit


Ignore:
Timestamp:
May 25, 2018 6:45:15 AM (6 years ago)
Author:
graouts@webkit.org
Message:

[Web Animations] WebAnimation objects never get destroyed
​https://bugs.webkit.org/show_bug.cgi?id=185917
<rdar://problem/39539371>

Reviewed by Dean Jackson and Antti Koivisto.

Source/WebCore:

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

LayoutTests:

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.
Location:
trunk
Files:
3 added
2 deleted
12 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r232183 r232185  
     12018-05-25  Antoine Quint  <graouts@apple.com>
     2
     3        [Web Animations] WebAnimation objects never get destroyed
     4        https://bugs.webkit.org/show_bug.cgi?id=185917
     5        <rdar://problem/39539371>
     6
     7        Reviewed by Dean Jackson and Antti Koivisto.
     8
     9        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
     10        and is no longer relevant thanks to the tests under imported/mozilla.
     11
     12        * animations/leak-document-with-css-animation-expected.txt: Added.
     13        * animations/leak-document-with-css-animation.html: Added.
     14        * animations/resources/animation-leak-iframe.html: Added.
     15        * platform/win/TestExpectations:
     16        * webanimations/css-transitions-expected.txt: Removed.
     17        * webanimations/css-transitions.html: Removed.
     18
    1192018-05-24  Frederic Wang  <fwang@igalia.com>
    220

  • trunk/LayoutTests/platform/win/TestExpectations

    r232032 r232185  
    40244024
    40254025webkit.org/b/183569 webanimations/css-animations.html [ Failure ]
    4026 webkit.org/b/183569 webanimations/css-transitions.html [ Failure ]
    40274026
    40284027webkit.org/b/183953 imported/mozilla/css-animations/test_animation-cancel.html [ Failure ]

  • trunk/Source/WebCore/ChangeLog

    r232180 r232185  
     12018-05-25  Antoine Quint  <graouts@apple.com>
     2
     3        [Web Animations] WebAnimation objects never get destroyed
     4        https://bugs.webkit.org/show_bug.cgi?id=185917
     5        <rdar://problem/39539371>
     6
     7        Reviewed by Dean Jackson and Antti Koivisto.
     8
     9        The AnimationTimeline class keeps references to WebAnimation objects organized in various ways. First, there
     10        are three main maps across which all animations are stored, one for non-subclass WebAnimation objects
     11        (m_elementToAnimationsMap), one for CSSSAnimation objects (m_elementToCSSAnimationsMap) and one for CSSTranstion
     12        objects (m_elementToCSSTransitionsMap). On top of that, we also keep a map to access CSSAnimation objects for
     13        a given element by CSS animation name (m_elementToCSSAnimationByName) and another map to access CSSTransition
     14        objects for a given element by CSS property (m_elementToCSSTransitionByCSSPropertyID).
     15
     16        None of the RefPtr<WebAnimation> stored in these maps would get cleared when the document would get torn down,
     17        which would also prevent the AnimationTimeline (and its DocumentTimeline subclass) from being destroyed.
     18
     19        We now ensure that element and document tear-down correctly removes animations and clears those maps, which
     20        in turn allows the DocumentTimeline to be destroyed, fixing the significant memory leak introduced by Web Animations
     21        so far.
     22
     23        Finally, we change the collection type for those maps to be ListHashRef instead of Vector to guarantee we only
     24        add an animation once per collection due to changes in how setEffect() and setTimeline() operate.
     25
     26        Test: animations/leak-document-with-css-animation.html
     27
     28        * animation/AnimationTimeline.cpp:
     29        (WebCore::AnimationTimeline::~AnimationTimeline): There is no need to clear those tables as they'll need to be empty
     30        for the AnimationTimeline to even be destroyed.
     31        (WebCore::AnimationTimeline::relevantMapForAnimation): Change to use ListHashRef instead of Vector.
     32        (WebCore::AnimationTimeline::animationWasAddedToElement): Change to use ListHashRef instead of Vector.
     33        (WebCore::AnimationTimeline::animationWasRemovedFromElement): When an animation is removed from an element, ensure that
     34        references to this animation stored in the m_elementToCSSAnimationByName and m_elementToCSSTransitionByCSSPropertyID maps
     35        are cleared.
     36        (WebCore::AnimationTimeline::animationsForElement const): Change to use ListHashRef instead of Vector.
     37        (WebCore::AnimationTimeline::removeAnimationsForElement): Instead of just calling cancel() on all known declarative animations
     38        (this method used to be called cancelDeclarativeAnimationsForElement()), we now set the effect of known animations, declarative
     39        or not, for the provided element which will in turn call animationWasRemovedFromElement() and remove the animation from all
     40        maps that might keep a reference to it.
     41        (WebCore::AnimationTimeline::updateCSSTransitionsForElement): Replace call to removeDeclarativeAnimation() with a simple call
     42        to removeAnimation() which will remove references for this animation from the relevant maps.
     43        (WebCore::AnimationTimeline::cancelOrRemoveDeclarativeAnimation): Ditto.
     44        (WebCore::AnimationTimeline::cancelDeclarativeAnimationsForElement): Deleted.
     45        (WebCore::AnimationTimeline::removeDeclarativeAnimation): Deleted.
     46        * animation/AnimationTimeline.h:
     47        (WebCore::AnimationTimeline::elementToAnimationsMap): Change to use ListHashRef instead of Vector.
     48        (WebCore::AnimationTimeline::elementToCSSAnimationsMap): Change to use ListHashRef instead of Vector.
     49        (WebCore::AnimationTimeline::elementToCSSTransitionsMap): Change to use ListHashRef instead of Vector.
     50        * animation/WebAnimation.cpp:
     51        (WebCore::WebAnimation::setEffect): In the case of a declarative animation, we don't want to remove the animation from the relevant
     52        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
     53        timeline-to-animation relationship.
     54        (WebCore::WebAnimation::setEffectInternal): Factor parts of setEffect() out into a new method that can be called from
     55        AnimationTimeline::removeAnimationsForElement() to reset the m_effect member and correctly call animationWasRemovedFromElement()
     56        without all the Web Animations machinery of setEffect(), which is a public API that has unwanted side effects (such as rejecting
     57        promises).
     58        (WebCore::WebAnimation::setTimeline): In the case of a declarative animation, we don't want to remove the animation from the
     59        relevant maps because, while the timeline was set via the API, the element still has a transition or animation set up and we must
     60        not break the relationship.
     61        * animation/DocumentTimeline.cpp:
     62        (WebCore::DocumentTimeline::~DocumentTimeline):
     63        (WebCore::DocumentTimeline::detachFromDocument): Close the GenericTaskQueues when detaching from the document as it's too late to
     64        perform this work in the destructor. We also cancel the schedule timer which we had forgotten to do before.
     65        * animation/WebAnimation.h:
     66        * dom/Document.cpp:
     67        (WebCore::Document::prepareForDestruction):
     68        * dom/Element.cpp:
     69        (WebCore::Element::removedFromAncestor):
     70        * dom/PseudoElement.cpp:
     71        (WebCore::PseudoElement::clearHostElement):
     72        * rendering/updating/RenderTreeUpdater.cpp:
     73        (WebCore::RenderTreeUpdater::tearDownRenderers):
     74
    1752018-05-24  Chris Dumez  <cdumez@apple.com>
    276

  • trunk/Source/WebCore/animation/AnimationTimeline.cpp

    r232178 r232185  
    5252AnimationTimeline::~AnimationTimeline()
    5353{
    54     m_animations.clear();
    55     m_elementToAnimationsMap.clear();
    56     m_elementToCSSAnimationsMap.clear();
    57     m_elementToCSSTransitionsMap.clear();
    58     m_elementToCSSAnimationByName.clear();
    59     m_elementToCSSTransitionByCSSPropertyID.clear();
    6054}
    6155
    … …  
    8680}
    8781
    88 HashMap<Element*, Vector<RefPtr<WebAnimation>>>& AnimationTimeline::relevantMapForAnimation(WebAnimation& animation)
     82HashMap<Element*, ListHashSet<RefPtr<WebAnimation>>>& AnimationTimeline::relevantMapForAnimation(WebAnimation& animation)
    8983{
    9084    if (animation.isCSSAnimation())
    … …  
    9791void AnimationTimeline::animationWasAddedToElement(WebAnimation& animation, Element& element)
    9892{
    99     auto result = relevantMapForAnimation(animation).ensure(&element, [] {
    100         return Vector<RefPtr<WebAnimation>> { };
    101     });
    102     result.iterator->value.append(&animation);
     93    relevantMapForAnimation(animation).ensure(&element, [] {
     94        return ListHashSet<RefPtr<WebAnimation>> { };
     95    }).iterator->value.add(&animation);
    10396}
    10497
    10598void AnimationTimeline::animationWasRemovedFromElement(WebAnimation& animation, Element& element)
    10699{
     100    // First, we clear this animation from one of the m_elementToCSSAnimationsMap, m_elementToCSSTransitionsMap
     101    // or m_elementToAnimationsMap map, whichever is relevant to this type of animation.
    107102    auto& map = relevantMapForAnimation(animation);
    108103    auto iterator = map.find(&element);
    … …  
    111106
    112107    auto& animations = iterator->value;
    113     animations.removeFirst(&animation);
     108    animations.remove(&animation);
    114109    if (!animations.size())
    115110        map.remove(iterator);
     111
     112    // Now, if we're dealing with a declarative animation, we remove it from either the m_elementToCSSAnimationByName
     113    // or the m_elementToCSSTransitionByCSSPropertyID map, whichever is relevant to this type of animation.
     114    if (is<CSSAnimation>(animation)) {
     115        auto iterator = m_elementToCSSAnimationByName.find(&element);
     116        if (iterator != m_elementToCSSAnimationByName.end()) {
     117            auto& cssAnimationsByName = iterator->value;
     118            auto& name = downcast<CSSAnimation>(animation).animationName();
     119            cssAnimationsByName.remove(name);
     120            if (cssAnimationsByName.isEmpty())
     121                m_elementToCSSAnimationByName.remove(&element);
     122        }
     123    } else if (is<CSSTransition>(animation)) {
     124        auto iterator = m_elementToCSSTransitionByCSSPropertyID.find(&element);
     125        if (iterator != m_elementToCSSTransitionByCSSPropertyID.end()) {
     126            auto& cssTransitionsByProperty = iterator->value;
     127            auto property = downcast<CSSTransition>(animation).property();
     128            cssTransitionsByProperty.remove(property);
     129            if (cssTransitionsByProperty.isEmpty())
     130                m_elementToCSSTransitionByCSSPropertyID.remove(&element);
     131        }
     132    }
    116133}
    117134
    … …  
    119136{
    120137    Vector<RefPtr<WebAnimation>> animations;
    121     if (m_elementToCSSAnimationsMap.contains(&element))
    122         animations.appendVector(m_elementToCSSAnimationsMap.get(&element));
    123     if (m_elementToCSSTransitionsMap.contains(&element))
    124         animations.appendVector(m_elementToCSSTransitionsMap.get(&element));
    125     if (m_elementToAnimationsMap.contains(&element))
    126         animations.appendVector(m_elementToAnimationsMap.get(&element));
     138    if (m_elementToCSSAnimationsMap.contains(&element)) {
     139        const auto& cssAnimations = m_elementToCSSAnimationsMap.get(&element);
     140        animations.appendRange(cssAnimations.begin(), cssAnimations.end());
     141    }
     142    if (m_elementToCSSTransitionsMap.contains(&element)) {
     143        const auto& cssTransitions = m_elementToCSSTransitionsMap.get(&element);
     144        animations.appendRange(cssTransitions.begin(), cssTransitions.end());
     145    }
     146    if (m_elementToAnimationsMap.contains(&element)) {
     147        const auto& webAnimations = m_elementToAnimationsMap.get(&element);
     148        animations.appendRange(webAnimations.begin(), webAnimations.end());
     149    }
    127150    return animations;
    128151}
    129152
    130 void AnimationTimeline::cancelDeclarativeAnimationsForElement(Element& element)
    131 {
    132     for (const auto& animation : animationsForElement(element)) {
    133         if (is<DeclarativeAnimation>(animation))
    134             animation->cancel();
     153void AnimationTimeline::removeAnimationsForElement(Element& element)
     154{
     155    for (auto& animation : animationsForElement(element)) {
     156        animation->setEffectInternal(nullptr);
     157        removeAnimation(animation.releaseNonNull());
    135158    }
    136159}
    … …  
    290313                        if (cssTransitionsByProperty.get(property)->matchesBackingAnimationAndStyles(backingAnimation, oldStyle, newStyle))
    291314                            continue;
    292                         removeDeclarativeAnimation(cssTransitionsByProperty.take(property));
     315                        removeAnimation(cssTransitionsByProperty.take(property).releaseNonNull());
    293316                    }
    294317                    // Now we can create a new CSSTransition with the new backing animation provided it has a valid
    … …  
    316339}
    317340
    318 void AnimationTimeline::removeDeclarativeAnimation(RefPtr<DeclarativeAnimation> animation)
    319 {
    320     animation->setEffect(nullptr);
    321     removeAnimation(animation.releaseNonNull());
    322 }
    323 
    324341void AnimationTimeline::cancelOrRemoveDeclarativeAnimation(RefPtr<DeclarativeAnimation> animation)
    325342{
    … …  
    328345        animation->cancel();
    329346    else
    330         removeDeclarativeAnimation(animation);
     347        removeAnimation(animation.releaseNonNull());
    331348}
    332349

  • trunk/Source/WebCore/animation/AnimationTimeline.h

    r230595 r232185  
    6060    const ListHashSet<RefPtr<WebAnimation>>& animations() const { return m_animations; }
    6161    Vector<RefPtr<WebAnimation>> animationsForElement(Element&) const;
    62     void cancelDeclarativeAnimationsForElement(Element&);
     62    void removeAnimationsForElement(Element&);
    6363    void animationWasAddedToElement(WebAnimation&, Element&);
    6464    void animationWasRemovedFromElement(WebAnimation&, Element&);
    … …  
    8080    bool hasElementAnimations() const { return !m_elementToAnimationsMap.isEmpty() || !m_elementToCSSAnimationsMap.isEmpty() || !m_elementToCSSTransitionsMap.isEmpty(); }
    8181
    82     const HashMap<Element*, Vector<RefPtr<WebAnimation>>>& elementToAnimationsMap() { return m_elementToAnimationsMap; }
    83     const HashMap<Element*, Vector<RefPtr<WebAnimation>>>& elementToCSSAnimationsMap() { return m_elementToCSSAnimationsMap; }
    84     const HashMap<Element*, Vector<RefPtr<WebAnimation>>>& elementToCSSTransitionsMap() { return m_elementToCSSTransitionsMap; }
    85     void removeDeclarativeAnimation(RefPtr<DeclarativeAnimation>);
     82    const HashMap<Element*, ListHashSet<RefPtr<WebAnimation>>>& elementToAnimationsMap() { return m_elementToAnimationsMap; }
     83    const HashMap<Element*, ListHashSet<RefPtr<WebAnimation>>>& elementToCSSAnimationsMap() { return m_elementToCSSAnimationsMap; }
     84    const HashMap<Element*, ListHashSet<RefPtr<WebAnimation>>>& elementToCSSTransitionsMap() { return m_elementToCSSTransitionsMap; }
    8685
    8786private:
    88     HashMap<Element*, Vector<RefPtr<WebAnimation>>>& relevantMapForAnimation(WebAnimation&);
     87    HashMap<Element*, ListHashSet<RefPtr<WebAnimation>>>& relevantMapForAnimation(WebAnimation&);
    8988    void cancelOrRemoveDeclarativeAnimation(RefPtr<DeclarativeAnimation>);
    9089    RefPtr<WebAnimation> cssAnimationForElementAndProperty(Element&, CSSPropertyID);
    … …  
    9291    ClassType m_classType;
    9392    std::optional<Seconds> m_currentTime;
    94     HashMap<Element*, Vector<RefPtr<WebAnimation>>> m_elementToAnimationsMap;
    95     HashMap<Element*, Vector<RefPtr<WebAnimation>>> m_elementToCSSAnimationsMap;
    96     HashMap<Element*, Vector<RefPtr<WebAnimation>>> m_elementToCSSTransitionsMap;
     93    HashMap<Element*, ListHashSet<RefPtr<WebAnimation>>> m_elementToAnimationsMap;
     94    HashMap<Element*, ListHashSet<RefPtr<WebAnimation>>> m_elementToCSSAnimationsMap;
     95    HashMap<Element*, ListHashSet<RefPtr<WebAnimation>>> m_elementToCSSTransitionsMap;
    9796    ListHashSet<RefPtr<WebAnimation>> m_animations;
    9897

  • trunk/Source/WebCore/animation/DocumentTimeline.cpp

    r230703 r232185  
    6262DocumentTimeline::~DocumentTimeline()
    6363{
     64}
     65
     66void DocumentTimeline::detachFromDocument()
     67{
    6468    m_invalidationTaskQueue.close();
    6569    m_eventDispatchTaskQueue.close();
    66 }
    67 
    68 void DocumentTimeline::detachFromDocument()
    69 {
     70    m_animationScheduleTimer.stop();
    7071    m_document = nullptr;
    7172}

  • trunk/Source/WebCore/animation/WebAnimation.cpp

    r230665 r232185  
    120120
    121121    // 7. Let the target effect of animation be new effect.
    122     m_effect = WTFMove(newEffect);
     122    // In the case of a declarative animation, we don't want to remove the animation from the relevant maps because
     123    // while the effect was set via the API, the element still has a transition or animation set up and we must
     124    // not break the timeline-to-animation relationship.
     125    setEffectInternal(WTFMove(newEffect), isDeclarativeAnimation());
    123126
    124127    // 8. Run the procedure to update an animation’s finished state for animation with the did seek flag set to false,
    … …  
    126129    updateFinishedState(DidSeek::No, SynchronouslyNotify::No);
    127130
     131    timingModelDidChange();
     132}
     133
     134void WebAnimation::setEffectInternal(RefPtr<AnimationEffectReadOnly>&& newEffect, bool doNotRemoveFromTimeline)
     135{
     136    auto oldEffect = m_effect;
     137
     138    m_effect = WTFMove(newEffect);
     139
     140    Element* previousTarget = nullptr;
     141    if (is<KeyframeEffectReadOnly>(oldEffect))
     142        previousTarget = downcast<KeyframeEffectReadOnly>(oldEffect.get())->target();
     143
     144    Element* newTarget = nullptr;
     145    if (is<KeyframeEffectReadOnly>(m_effect))
     146        newTarget = downcast<KeyframeEffectReadOnly>(m_effect.get())->target();
     147
    128148    // Update the effect-to-animation relationships and the timeline's animation map.
    129149    if (oldEffect) {
    130150        oldEffect->setAnimation(nullptr);
    131         if (m_timeline && is<KeyframeEffectReadOnly>(oldEffect)) {
    132             if (auto* target = downcast<KeyframeEffectReadOnly>(oldEffect.get())->target())
    133                 m_timeline->animationWasRemovedFromElement(*this, *target);
    134         }
     151        if (!doNotRemoveFromTimeline && m_timeline && previousTarget && previousTarget != newTarget)
     152            m_timeline->animationWasRemovedFromElement(*this, *previousTarget);
    135153    }
    136154
    137155    if (m_effect) {
    138156        m_effect->setAnimation(this);
    139         if (m_timeline && is<KeyframeEffectReadOnly>(m_effect)) {
    140             if (auto* target = downcast<KeyframeEffectReadOnly>(m_effect.get())->target())
    141                 m_timeline->animationWasAddedToElement(*this, *target);
    142         }
    143     }
    144 
    145     timingModelDidChange();
     157        if (m_timeline && newTarget && previousTarget != newTarget)
     158            m_timeline->animationWasAddedToElement(*this, *newTarget);
     159    }
    146160}
    147161
    … …  
    169183        auto* target = keyframeEffect->target();
    170184        if (target) {
    171             if (m_timeline)
     185            // In the case of a declarative animation, we don't want to remove the animation from the relevant maps because
     186            // while the timeline was set via the API, the element still has a transition or animation set up and we must
     187            // not break the relationship.
     188            if (m_timeline && !isDeclarativeAnimation())
    172189                m_timeline->animationWasRemovedFromElement(*this, *target);
    173190            if (timeline)

  • trunk/Source/WebCore/animation/WebAnimation.h

    r230581 r232185  
    6464    AnimationEffectReadOnly* effect() const { return m_effect.get(); }
    6565    void setEffect(RefPtr<AnimationEffectReadOnly>&&);
     66    void setEffectInternal(RefPtr<AnimationEffectReadOnly>&&, bool = false);
    6667    AnimationTimeline* timeline() const { return m_timeline.get(); }
    6768    virtual void setTimeline(RefPtr<AnimationTimeline>&&);

  • trunk/Source/WebCore/dom/Document.cpp

    r232180 r232185  
    23372337        return;
    23382338
    2339     if (m_timeline) {
    2340         m_timeline->detachFromDocument();
    2341         m_timeline = nullptr;
    2342     }
    2343 
    23442339    if (m_frame)
    23452340        m_frame->animation().detachFromDocument(this);
    … …  
    24332428
    24342429    detachFromFrame();
     2430
     2431    if (m_timeline) {
     2432        m_timeline->detachFromDocument();
     2433        m_timeline = nullptr;
     2434    }
    24352435
    24362436    m_hasPreparedForDestruction = true;

  • trunk/Source/WebCore/dom/Element.cpp

    r232178 r232185  
    17981798    if (RuntimeEnabledFeatures::sharedFeatures().webAnimationsCSSIntegrationEnabled()) {
    17991799        if (auto* timeline = document().existingTimeline())
    1800             timeline->cancelDeclarativeAnimationsForElement(*this);
     1800            timeline->removeAnimationsForElement(*this);
    18011801    } else if (frame)
    18021802        frame->animation().cancelAnimations(*this);

  • trunk/Source/WebCore/dom/PseudoElement.cpp

    r232178 r232185  
    9393    if (RuntimeEnabledFeatures::sharedFeatures().webAnimationsCSSIntegrationEnabled()) {
    9494        if (auto* timeline = document().existingTimeline())
    95             timeline->cancelDeclarativeAnimationsForElement(*this);
     95            timeline->removeAnimationsForElement(*this);
    9696    } else if (auto* frame = document().frame())
    9797        frame->animation().cancelAnimations(*this);

  • trunk/Source/WebCore/rendering/updating/RenderTreeUpdater.cpp

    r232178 r232185  
    556556                if (RuntimeEnabledFeatures::sharedFeatures().webAnimationsCSSIntegrationEnabled()) {
    557557                    if (timeline)
    558                         timeline->cancelDeclarativeAnimationsForElement(element);
     558                        timeline->removeAnimationsForElement(element);
    559559                } else
    560560                    animationController.cancelAnimations(element);
Note: See TracChangeset for help on using the changeset viewer.