Diff
Modified: trunk/LayoutTests/ChangeLog (233582 => 233583)
--- trunk/LayoutTests/ChangeLog 2018-07-06 18:25:59 UTC (rev 233582)
+++ trunk/LayoutTests/ChangeLog 2018-07-06 18:31:12 UTC (rev 233583)
@@ -1,3 +1,15 @@
+2018-07-06 Antoine Quint <[email protected]>
+
+ [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 <[email protected]>
Layout Test editing/selection/navigation-clears-editor-state.html is flaky
Added: trunk/LayoutTests/webanimations/leak-document-with-web-animation-expected.txt (0 => 233583)
--- trunk/LayoutTests/webanimations/leak-document-with-web-animation-expected.txt (rev 0)
+++ trunk/LayoutTests/webanimations/leak-document-with-web-animation-expected.txt 2018-07-06 18:31:12 UTC (rev 233583)
@@ -0,0 +1,13 @@
+This test asserts that Document doesn't leak when a Web Animation is created.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+The iframe has finished loading.
+The iframe has been destroyed.
+PASS internals.numberOfLiveDocuments() is numberOfLiveDocumentsAfterIframeLoaded - 1
+
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: trunk/LayoutTests/webanimations/leak-document-with-web-animation.html (0 => 233583)
--- trunk/LayoutTests/webanimations/leak-document-with-web-animation.html (rev 0)
+++ trunk/LayoutTests/webanimations/leak-document-with-web-animation.html 2018-07-06 18:31:12 UTC (rev 233583)
@@ -0,0 +1,48 @@
+<!DOCTYPE html>
+<html>
+<body _onload_="runTest()">
+<script src=""
+<script>
+description("This test asserts that Document doesn't leak when a Web Animation is created.");
+
+if (window.internals)
+ jsTestIsAsync = true;
+
+gc();
+
+var numberOfLiveDocumentsAfterIframeLoaded = 0;
+
+function runTest() {
+ if (!window.internals)
+ return;
+
+ var frame = document.body.appendChild(document.createElement("iframe"));
+
+ frame._onload_ = function() {
+ if (frame.src ="" 'about:blank')
+ return true;
+
+ numberOfLiveDocumentsAfterIframeLoaded = internals.numberOfLiveDocuments();
+ debug("The iframe has finished loading.");
+
+ frame.remove();
+ frame = null;
+
+ setTimeout(() => {
+ gc();
+ setTimeout(function () {
+ debug("The iframe has been destroyed.");
+ shouldBe("internals.numberOfLiveDocuments()", "numberOfLiveDocumentsAfterIframeLoaded - 1");
+ debug("");
+ finishJSTest();
+ });
+ });
+ }
+
+ frame.src = '';
+}
+
+</script>
+<script src=""
+</body>
+</html>
\ No newline at end of file
Added: trunk/LayoutTests/webanimations/resources/web-animation-leak-iframe.html (0 => 233583)
--- trunk/LayoutTests/webanimations/resources/web-animation-leak-iframe.html (rev 0)
+++ trunk/LayoutTests/webanimations/resources/web-animation-leak-iframe.html 2018-07-06 18:31:12 UTC (rev 233583)
@@ -0,0 +1,4 @@
+<div></div>
+<script type="text/_javascript_">
+document.querySelector("div").animate({ marginLeft: ["0", "100px"] }, 1000);
+</script>
Modified: trunk/Source/WebCore/ChangeLog (233582 => 233583)
--- trunk/Source/WebCore/ChangeLog 2018-07-06 18:25:59 UTC (rev 233582)
+++ trunk/Source/WebCore/ChangeLog 2018-07-06 18:31:12 UTC (rev 233583)
@@ -1,3 +1,53 @@
+2018-07-06 Antoine Quint <[email protected]>
+
+ [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 <[email protected]>
Remove Strong Confirmation Password button
Modified: trunk/Source/WebCore/animation/AnimationTimeline.cpp (233582 => 233583)
--- trunk/Source/WebCore/animation/AnimationTimeline.cpp 2018-07-06 18:25:59 UTC (rev 233582)
+++ trunk/Source/WebCore/animation/AnimationTimeline.cpp 2018-07-06 18:31:12 UTC (rev 233583)
@@ -176,10 +176,8 @@
void AnimationTimeline::removeAnimationsForElement(Element& element)
{
- for (auto& animation : animationsForElement(element)) {
- animation->prepareAnimationForRemoval();
- removeAnimation(animation.releaseNonNull());
- }
+ for (auto& animation : animationsForElement(element))
+ animation->remove();
}
void AnimationTimeline::cancelDeclarativeAnimationsForElement(Element& element)
Modified: trunk/Source/WebCore/animation/DeclarativeAnimation.cpp (233582 => 233583)
--- trunk/Source/WebCore/animation/DeclarativeAnimation.cpp 2018-07-06 18:25:59 UTC (rev 233582)
+++ trunk/Source/WebCore/animation/DeclarativeAnimation.cpp 2018-07-06 18:31:12 UTC (rev 233583)
@@ -49,10 +49,10 @@
{
}
-void DeclarativeAnimation::prepareAnimationForRemoval()
+void DeclarativeAnimation::remove()
{
- WebAnimation::prepareAnimationForRemoval();
m_eventQueue.close();
+ WebAnimation::remove();
}
void DeclarativeAnimation::setBackingAnimation(const Animation& backingAnimation)
Modified: trunk/Source/WebCore/animation/DeclarativeAnimation.h (233582 => 233583)
--- trunk/Source/WebCore/animation/DeclarativeAnimation.h 2018-07-06 18:25:59 UTC (rev 233582)
+++ trunk/Source/WebCore/animation/DeclarativeAnimation.h 2018-07-06 18:31:12 UTC (rev 233583)
@@ -45,7 +45,6 @@
const Animation& backingAnimation() const { return m_backingAnimation; }
void setBackingAnimation(const Animation&);
void invalidateDOMEvents(Seconds elapsedTime = 0_s);
- void prepareAnimationForRemoval() final;
void setTimeline(RefPtr<AnimationTimeline>&&) final;
void cancel() final;
@@ -59,6 +58,7 @@
private:
AnimationEffectReadOnly::Phase phaseWithoutEffect() const;
void enqueueDOMEvent(const AtomicString&, Seconds);
+ void remove() final;
Element& m_target;
Ref<Animation> m_backingAnimation;
Modified: trunk/Source/WebCore/animation/DocumentAnimationScheduler.cpp (233582 => 233583)
--- trunk/Source/WebCore/animation/DocumentAnimationScheduler.cpp 2018-07-06 18:25:59 UTC (rev 233582)
+++ trunk/Source/WebCore/animation/DocumentAnimationScheduler.cpp 2018-07-06 18:31:12 UTC (rev 233583)
@@ -75,6 +75,9 @@
if (!m_document || !m_document->domWindow())
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());
Modified: trunk/Source/WebCore/animation/DocumentTimeline.cpp (233582 => 233583)
--- trunk/Source/WebCore/animation/DocumentTimeline.cpp 2018-07-06 18:25:59 UTC (rev 233582)
+++ trunk/Source/WebCore/animation/DocumentTimeline.cpp 2018-07-06 18:31:12 UTC (rev 233583)
@@ -55,9 +55,7 @@
{
}
-DocumentTimeline::~DocumentTimeline()
-{
-}
+DocumentTimeline::~DocumentTimeline() = default;
void DocumentTimeline::detachFromDocument()
{
@@ -64,6 +62,11 @@
m_invalidationTaskQueue.close();
m_eventDispatchTaskQueue.close();
m_animationScheduleTimer.stop();
+
+ auto& animationsToRemove = animations();
+ while (!animationsToRemove.isEmpty())
+ animationsToRemove.first()->remove();
+
m_document = nullptr;
}
Modified: trunk/Source/WebCore/animation/WebAnimation.cpp (233582 => 233583)
--- trunk/Source/WebCore/animation/WebAnimation.cpp 2018-07-06 18:25:59 UTC (rev 233582)
+++ trunk/Source/WebCore/animation/WebAnimation.cpp 2018-07-06 18:31:12 UTC (rev 233583)
@@ -71,9 +71,12 @@
{
}
-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);
}
void WebAnimation::suspendEffectInvalidation()
@@ -128,6 +131,9 @@
// 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.
+
+ // This object could be deleted after clearing the effect relationship.
+ auto protectedThis = makeRef(*this);
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,
@@ -139,9 +145,10 @@
void WebAnimation::setEffectInternal(RefPtr<AnimationEffectReadOnly>&& newEffect, bool doNotRemoveFromTimeline)
{
- auto oldEffect = m_effect;
+ if (m_effect == newEffect)
+ return;
- m_effect = WTFMove(newEffect);
+ auto oldEffect = std::exchange(m_effect, WTFMove(newEffect));
Element* previousTarget = nullptr;
if (is<KeyframeEffectReadOnly>(oldEffect))
@@ -178,12 +185,6 @@
if (m_startTime)
setHoldTime(std::nullopt);
- if (m_timeline)
- m_timeline->removeAnimation(*this);
-
- if (timeline)
- timeline->addAnimation(*this);
-
if (is<KeyframeEffectReadOnly>(m_effect)) {
auto* keyframeEffect = downcast<KeyframeEffectReadOnly>(m_effect.get());
auto* target = keyframeEffect->target();
@@ -198,7 +199,9 @@
}
}
- 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());
@@ -209,6 +212,20 @@
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);
+}
+
void WebAnimation::effectTargetDidChange(Element* previousTarget, Element* newTarget)
{
if (!m_timeline)
Modified: trunk/Source/WebCore/animation/WebAnimation.h (233582 => 233583)
--- trunk/Source/WebCore/animation/WebAnimation.h 2018-07-06 18:25:59 UTC (rev 233582)
+++ trunk/Source/WebCore/animation/WebAnimation.h 2018-07-06 18:31:12 UTC (rev 233583)
@@ -63,7 +63,6 @@
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>&&);
@@ -117,7 +116,7 @@
void unsuspendEffectInvalidation();
void setSuspended(bool);
bool isSuspended() const { return m_isSuspended; }
- virtual void prepareAnimationForRemoval();
+ virtual void remove();
String description();
@@ -156,7 +155,9 @@
void runPendingPauseTask();
void runPendingPlayTask();
void resetPendingTasks();
-
+ void setEffectInternal(RefPtr<AnimationEffectReadOnly>&&, bool = false);
+ void setTimelineInternal(RefPtr<AnimationTimeline>&&);
+
String m_id;
RefPtr<AnimationEffectReadOnly> m_effect;
RefPtr<AnimationTimeline> m_timeline;
