Changeset 233583 in webkit

Timestamp:

Jul 6, 2018, 11:31:12 AM (8 years ago)

Author:

graouts@webkit.org

Message:

[Web Animations] Using a Web Animation leaks the Document
​https://bugs.webkit.org/show_bug.cgi?id=187088
<rdar://problem/41392046>

Reviewed by Darin Adler.

Source/WebCore:

Test: webanimations/leak-document-with-web-animation.html

We need to ensure that any remaining animation is cleared when the DocumentTimeline is detached from its Document.
We rename WebAnimation::prepareAnimationForRemoval() to WebAnimation::remove() since it really actively disassociates
the animation from its timeline.

An earlier version of this patch (r233349) was rolled out due to crashes caught in the ASan configuration. The following
changes were made to make it safe:

• We protect the DocumentAnimationScheduler instance in displayRefreshFired() against code that might run in a

requestAnimationFrame() callback that would trigger the object to be deleted.

• We protect the WebAnimation instance in remove() against setEffectInternal() or setTimelineInternal() potentially

causing the object to be deleted. Similar protections were addede to setEffect() and setTimeline().

• We changed ~DocumentTimeline() to a default implementation to ensure it calls ~DisplayRefreshMonitorClient()

to avoid callbacks after the object has been marked for deletion.

• animation/AnimationTimeline.cpp:

(WebCore::AnimationTimeline::removeAnimationsForElement): We no longer need the call to removeAnimation()
since the new WebAnimation::remove() method will also set the timeline to null which will eventually call
removeAnimation() on the disassociated timeline.

• animation/DeclarativeAnimation.cpp:

(WebCore::DeclarativeAnimation::remove):
(WebCore::DeclarativeAnimation::prepareAnimationForRemoval): Deleted.

• animation/DeclarativeAnimation.h:
• animation/DocumentAnimationScheduler.cpp:

(WebCore::DocumentAnimationScheduler::displayRefreshFired):

• animation/DocumentTimeline.cpp:

(WebCore::DocumentTimeline::detachFromDocument): Call remove() on all known animations.
(WebCore::DocumentTimeline::~DocumentTimeline): Deleted.

• animation/WebAnimation.cpp:

(WebCore::WebAnimation::remove): Set the timeline to null to fully disassociate this animation from its timeline.
(WebCore::WebAnimation::setEffect):
(WebCore::WebAnimation::setEffectInternal):
(WebCore::WebAnimation::setTimeline): Factor the internal timeline-association code out of this JS API method so
that we can call this code without any JS-facing implications.
(WebCore::WebAnimation::setTimelineInternal):
(WebCore::WebAnimation::prepareAnimationForRemoval): Deleted.

• animation/WebAnimation.h:

LayoutTests:

• webanimations/leak-document-with-web-animation-expected.txt: Added.
• webanimations/leak-document-with-web-animation.html: Added.
• webanimations/resources/web-animation-leak-iframe.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/webanimations/leak-document-with-web-animation-expected.txt (added)
• LayoutTests/webanimations/leak-document-with-web-animation.html (added)
• LayoutTests/webanimations/resources (added)
• LayoutTests/webanimations/resources/web-animation-leak-iframe.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/animation/AnimationTimeline.cpp (modified) (1 diff)
• Source/WebCore/animation/DeclarativeAnimation.cpp (modified) (1 diff)
• Source/WebCore/animation/DeclarativeAnimation.h (modified) (2 diffs)
• Source/WebCore/animation/DocumentAnimationScheduler.cpp (modified) (1 diff)
• Source/WebCore/animation/DocumentTimeline.cpp (modified) (2 diffs)
• Source/WebCore/animation/WebAnimation.cpp (modified) (6 diffs)
• Source/WebCore/animation/WebAnimation.h (modified) (3 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2018-07-06  Antoine Quint  <graouts@apple.com>
+
+        [Web Animations] Using a Web Animation leaks the Document
+        https://bugs.webkit.org/show_bug.cgi?id=187088
+        <rdar://problem/41392046>
+
+        Reviewed by Darin Adler.
+
+        * webanimations/leak-document-with-web-animation-expected.txt: Added.
+        * webanimations/leak-document-with-web-animation.html: Added.
+        * webanimations/resources/web-animation-leak-iframe.html: Added.
+
 2018-07-06  Saam Barati  <sbarati@apple.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2018-07-06  Antoine Quint  <graouts@apple.com>
+
+        [Web Animations] Using a Web Animation leaks the Document
+        https://bugs.webkit.org/show_bug.cgi?id=187088
+        <rdar://problem/41392046>
+
+        Reviewed by Darin Adler.
+
+        Test: webanimations/leak-document-with-web-animation.html
+
+        We need to ensure that any remaining animation is cleared when the DocumentTimeline is detached from its Document.
+        We rename WebAnimation::prepareAnimationForRemoval() to WebAnimation::remove() since it really actively disassociates
+        the animation from its timeline.
+
+        An earlier version of this patch (r233349) was rolled out due to crashes caught in the ASan configuration. The following
+        changes were made to make it safe:
+
+        - We protect the DocumentAnimationScheduler instance in displayRefreshFired() against code that might run in a
+        requestAnimationFrame() callback that would trigger the object to be deleted.
+
+        - We protect the WebAnimation instance in remove() against setEffectInternal() or setTimelineInternal() potentially
+        causing the object to be deleted. Similar protections were addede to setEffect() and setTimeline().
+
+        - We changed ~DocumentTimeline() to a default implementation to ensure it calls ~DisplayRefreshMonitorClient()
+        to avoid callbacks after the object has been marked for deletion.
+
+
+        * animation/AnimationTimeline.cpp:
+        (WebCore::AnimationTimeline::removeAnimationsForElement): We no longer need the call to removeAnimation()
+        since the new WebAnimation::remove() method will also set the timeline to null which will eventually call
+        removeAnimation() on the disassociated timeline.
+        * animation/DeclarativeAnimation.cpp:
+        (WebCore::DeclarativeAnimation::remove):
+        (WebCore::DeclarativeAnimation::prepareAnimationForRemoval): Deleted.
+        * animation/DeclarativeAnimation.h:
+        * animation/DocumentAnimationScheduler.cpp:
+        (WebCore::DocumentAnimationScheduler::displayRefreshFired):
+        * animation/DocumentTimeline.cpp:
+        (WebCore::DocumentTimeline::detachFromDocument): Call remove() on all known animations.
+        (WebCore::DocumentTimeline::~DocumentTimeline): Deleted.
+        * animation/WebAnimation.cpp:
+        (WebCore::WebAnimation::remove): Set the timeline to null to fully disassociate this animation from its timeline.
+        (WebCore::WebAnimation::setEffect):
+        (WebCore::WebAnimation::setEffectInternal):
+        (WebCore::WebAnimation::setTimeline): Factor the internal timeline-association code out of this JS API method so
+        that we can call this code without any JS-facing implications.
+        (WebCore::WebAnimation::setTimelineInternal):
+        (WebCore::WebAnimation::prepareAnimationForRemoval): Deleted.
+        * animation/WebAnimation.h:
+
 2018-07-06  Daniel Bates  <dabates@apple.com>
 

trunk/Source/WebCore/animation/AnimationTimeline.cpp

@@ -177 +177 @@
 void AnimationTimeline::removeAnimationsForElement(Element& element)
 {
-    for (auto& animation : animationsForElement(element)) {
-        animation->prepareAnimationForRemoval();
-        removeAnimation(animation.releaseNonNull());
-    }
+    for (auto& animation : animationsForElement(element))
+        animation->remove();
 }
 

trunk/Source/WebCore/animation/DeclarativeAnimation.cpp

@@ -50 +50 @@
 }
 
-void DeclarativeAnimation::prepareAnimationForRemoval()
-{
-    WebAnimation::prepareAnimationForRemoval();
+void DeclarativeAnimation::remove()
+{
     m_eventQueue.close();
+    WebAnimation::remove();
 }
 

trunk/Source/WebCore/animation/DeclarativeAnimation.h

@@ -46 +46 @@
     void setBackingAnimation(const Animation&);
     void invalidateDOMEvents(Seconds elapsedTime = 0_s);
-    void prepareAnimationForRemoval() final;
 
     void setTimeline(RefPtr<AnimationTimeline>&&) final;
@@ -60 +59 @@
     AnimationEffectReadOnly::Phase phaseWithoutEffect() const;
     void enqueueDOMEvent(const AtomicString&, Seconds);
+    void remove() final;
 
     Element& m_target;

trunk/Source/WebCore/animation/DocumentAnimationScheduler.cpp

@@ -76 +76 @@
         return;
 
+    // This object could be deleted after scripts in the the requestAnimationFrame callbacks are executed.
+    auto protectedThis = makeRef(*this);
+
     m_isFiring = true;
     m_lastTimestamp = Seconds(m_document->domWindow()->nowTimestamp());

trunk/Source/WebCore/animation/DocumentTimeline.cpp

@@ -56 +56 @@
 }
 
-DocumentTimeline::~DocumentTimeline()
-{
-}
+DocumentTimeline::~DocumentTimeline() = default;
 
 void DocumentTimeline::detachFromDocument()
@@ -65 +63 @@
     m_eventDispatchTaskQueue.close();
     m_animationScheduleTimer.stop();
+
+    auto& animationsToRemove = animations();
+    while (!animationsToRemove.isEmpty())
+        animationsToRemove.first()->remove();
+
     m_document = nullptr;
 }

trunk/Source/WebCore/animation/WebAnimation.cpp

@@ -72 +72 @@
 }
 
-void WebAnimation::prepareAnimationForRemoval()
-{
+void WebAnimation::remove()
+{
+    // This object could be deleted after either clearing the effect or timeline relationship.
+    auto protectedThis = makeRef(*this);
     setEffectInternal(nullptr);
+    setTimelineInternal(nullptr);
 }
 
@@ -129 +132 @@
     // 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.
+
+    // This object could be deleted after clearing the effect relationship.
+    auto protectedThis = makeRef(*this);
     setEffectInternal(WTFMove(newEffect), isDeclarativeAnimation());
 
@@ -140 +146 @@
 void WebAnimation::setEffectInternal(RefPtr<AnimationEffectReadOnly>&& newEffect, bool doNotRemoveFromTimeline)
 {
-    auto oldEffect = m_effect;
-
-    m_effect = WTFMove(newEffect);
+    if (m_effect == newEffect)
+        return;
+
+    auto oldEffect = std::exchange(m_effect, WTFMove(newEffect));
 
     Element* previousTarget = nullptr;
@@ -178 +185 @@
     if (m_startTime)
         setHoldTime(std::nullopt);
-
-    if (m_timeline)
-        m_timeline->removeAnimation(*this);
-
-    if (timeline)
-        timeline->addAnimation(*this);
 
     if (is<KeyframeEffectReadOnly>(m_effect)) {
@@ -199 +200 @@
     }
 
-    m_timeline = WTFMove(timeline);
+    // This object could be deleted after clearing the timeline relationship.
+    auto protectedThis = makeRef(*this);
+    setTimelineInternal(WTFMove(timeline));
 
     setSuspended(is<DocumentTimeline>(m_timeline) && downcast<DocumentTimeline>(*m_timeline).animationsAreSuspended());
@@ -208 +211 @@
     // and the synchronously notify flag set to false.
     updateFinishedState(DidSeek::No, SynchronouslyNotify::No);
+}
+
+void WebAnimation::setTimelineInternal(RefPtr<AnimationTimeline>&& timeline)
+{
+    if (m_timeline == timeline)
+        return;
+
+    if (m_timeline)
+        m_timeline->removeAnimation(*this);
+
+    m_timeline = WTFMove(timeline);
+
+    if (m_timeline)
+        m_timeline->addAnimation(*this);
 }
 

trunk/Source/WebCore/animation/WebAnimation.h

@@ -64 +64 @@
     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>&&);
@@ -118 +117 @@
     void setSuspended(bool);
     bool isSuspended() const { return m_isSuspended; }
-    virtual void prepareAnimationForRemoval();
+    virtual void remove();
 
     String description();
@@ -157 +156 @@
     void runPendingPlayTask();
     void resetPendingTasks();
-    
+    void setEffectInternal(RefPtr<AnimationEffectReadOnly>&&, bool = false);
+    void setTimelineInternal(RefPtr<AnimationTimeline>&&);
+
     String m_id;
     RefPtr<AnimationEffectReadOnly> m_effect;