Bug 208069 – DocumentTimeline / CSSTransition objects are leaking on CNN.com

• Component: Animations
• Status: RESOLVED FIXED

[Chris Dumez]

DocumentTimeline / CSSTransition objects are leaking on CNN.com.

Reproduction steps:
1. Visit cnn.com
2. Click links at the top to navigate several times (World, Africa, Americas, Asian)
3. Navigate to about:blank
4. Send a low memory signal (notifyutil -p org.WebKit.lowMemory)

-> Notice that there are still DocumentTimeline / CSSTransition objects alive. Those keeps their Document alive which in turns leaks DOMWindows, Performance objects, Performance entries, ...

There appear to be reference cycles in this code. For e.g. Document has a RefPtr to its DocumentTimeline. DocumentTimeline has a RefPtr to its Document. CSSTransitions seem to keep their DocumentTimeline & Document alive.

[Chris Dumez]

Disabling "CSS Animations via Web Animations" experimental feature seems to make the problem go away.

[Chris Dumez]

Created attachment 391417 [details]
Memgraph

[Radar WebKit Bug Importer]

<rdar://problem/59680143>

[Chris Dumez]

Created attachment 391544 [details]
WIP Patch

[Chris Dumez]

(In reply to Chris Dumez from comment #4)
> Created attachment 391544 [details]
> WIP Patch

Hmm, this patch seems to make it so that Document / Window objects are no longer leaked. However, local testing seems to indicate that CSSTransition & DocumentTimeline may still be leaked.

[Chris Dumez]

(In reply to Chris Dumez from comment #5)
> (In reply to Chris Dumez from comment #4)
> > Created attachment 391544 [details]
> > WIP Patch
> 
> Hmm, this patch seems to make it so that Document / Window objects are no
> longer leaked. However, local testing seems to indicate that CSSTransition &
> DocumentTimeline may still be leaked.

From memgraph, it is clear that the DocumentTimeline objects are staying because of the CSSTransition objects that are ref'ing them. It is however unclear to me what's keeping the CSSTransition objects alive.

[Chris Dumez]

(In reply to Chris Dumez from comment #6)
> (In reply to Chris Dumez from comment #5)
> > (In reply to Chris Dumez from comment #4)
> > > Created attachment 391544 [details]
> > > WIP Patch
> > 
> > Hmm, this patch seems to make it so that Document / Window objects are no
> > longer leaked. However, local testing seems to indicate that CSSTransition &
> > DocumentTimeline may still be leaked.
> 
> From memgraph, it is clear that the DocumentTimeline objects are staying
> because of the CSSTransition objects that are ref'ing them. It is however
> unclear to me what's keeping the CSSTransition objects alive.

Looking at the code though, it seems DocumentTimeline subclasses AnimationTimeline, which has various maps that ref the CSSTransition objects. Given that the CSSTransition objects ref their DocumentTimeline, it is pretty clear there is a cycle. It would be very easy to leak if the cycle does not get broken.

[Chris Dumez]

Created attachment 391682 [details]
Patch

[Antoine Quint]

Comment on attachment 391682 [details]
Patch

This is great, r=me. However, I'd like to keep this open to understand why CSSTransition leaked in the first place because I don't think that is expected, there might be other issues in the animation code.

[WebKit Commit Bot]

Comment on attachment 391682 [details]
Patch

Clearing flags on attachment: 391682

Committed r257417: <https://trac.webkit.org/changeset/257417>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Antoine Quint]

Re-opening so that I may assess why CSSTransition objects persisted in the first place.

[Chris Dumez]

(In reply to Antoine Quint from comment #12)
> Re-opening so that I may assess why CSSTransition objects persisted in the
> first place.

Sounds good, thanks.

[Darin Adler]

Comment on attachment 391682 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=391682&action=review

Seems that WeakPtr needs overloads for is<> and for operator== with other types of pointers such as raw pointers. OK to add get() I suppose, but maybe not an intentional design decision?

> Source/WebCore/animation/WebAnimation.h:72
> -    AnimationTimeline* timeline() const { return m_timeline.get(); }
> +    AnimationTimeline* timeline() const;

Why did this need to change?

[Chris Dumez]

Comment on attachment 391682 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=391682&action=review

>> Source/WebCore/animation/WebAnimation.h:72
>> +    AnimationTimeline* timeline() const;
> 
> Why did this need to change?

I believe it is because AnimationTimeline is only forward declared in the header. I could have fixed this by moving the include to the header too but this seemed better for build time.

[Darin Adler]

Comment on attachment 391682 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=391682&action=review

>>> Source/WebCore/animation/WebAnimation.h:72
>>> +    AnimationTimeline* timeline() const;
>> 
>> Why did this need to change?
> 
> I believe it is because AnimationTimeline is only forward declared in the header. I could have fixed this by moving the include to the header too but this seemed better for build time.

Makes sense.

I had the impression that you could call RefPtr<X>::get with only a forward declaration of X, and I was hoping the same was true for WeakPtr<X>::get. But that might be mistaken.

And I agree with the decision you made about not adding an include at this time to improve inlining a tiny bit.

[Darin Adler]

Comment on attachment 391682 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=391682&action=review

>>>> Source/WebCore/animation/WebAnimation.h:72
>>>> +    AnimationTimeline* timeline() const;
>>> 
>>> Why did this need to change?
>> 
>> I believe it is because AnimationTimeline is only forward declared in the header. I could have fixed this by moving the include to the header too but this seemed better for build time.
> 
> Makes sense.
> 
> I had the impression that you could call RefPtr<X>::get with only a forward declaration of X, and I was hoping the same was true for WeakPtr<X>::get. But that might be mistaken.
> 
> And I agree with the decision you made about not adding an include at this time to improve inlining a tiny bit.

Obviously I was mistaken about WeakPtr, I meant I might be mistaken even about RefPtr.

[Antoine Quint]

We're going to have to revisit this, per my findings in bug 209239. Animations need to keep a strong reference to their timeline.

[Antoine Quint]

I think I know what the problem is: we're not clearing ElementAnimationRareData::m_animationsCreatedByMarkup during teardown if the animation is not also listed in ElementAnimationRareData::m_cssAnimations. However, m_cssAnimations may no longer list an animation listed in m_animationsCreatedByMarkup if the animation was no longer "relevant" in terms of Web Animations (see https://drafts.csswg.org/web-animations-1/#relevant-animation).

[Antoine Quint]

There is another issue with ElementAnimationRareData::m_animationsCreatedByMarkup, we need to call clear() on it before assigning the new value in  ElementAnimationRareData::setAnimationsCreatedByMarkup(CSSAnimationCollection&&).

[Geoffrey Garen]

> > I had the impression that you could call RefPtr<X>::get with only a forward declaration of X, and I was hoping the same was true for WeakPtr<X>::get. But that might be mistaken.

I believe it's still true that RefPtr<X>::get does not require a full declaration.

WeakPtr is different because the load from the WeakPtr needs to know the full type of the stored pointer and the loaded pointer in order to do a correct static_cast. (Each RefPtr instance holds its own pointer, so there is never a cast. WeakPtr instances share a pointer, which might be stored by WeakPtr<Base> and loaded by WeakPtr<Derived>, where static_cast<Derived*>(base) requires pointer fixup. So far, we have been working around the #include problem by moving WeakPtr getters out-of-line.)

[Antoine Quint]

Created attachment 394178 [details]
Patch

[Antoine Quint]

Note that the new Patch attached assumes the previous patch for this bug is reverted as the fix for bug 209239.

[Simon Fraser (smfr)]

Comment on attachment 394178 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394178&action=review

> Source/WebCore/ChangeLog:12
> +        (aka CSSAnimationCollection) member on ElementAnimationRareData being replaced to the new list, but the old list not being cleared from its members.

"being replaced by"?

> Source/WebCore/testing/Internals.cpp:1039
> +bool Internals::animationWithIdExists(const String& id) const

Avoid 'id' because it's a reserved word in ObjectiveC

> Source/WebCore/testing/Internals.h:213
> +    bool animationWithIdExists(const String&) const;

How does the animation get an ID in the first place?

[Geoffrey Garen]

Comment on attachment 394178 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394178&action=review

> Source/WebCore/animation/ElementAnimationRareData.cpp:50
> +    std::exchange(m_animationsCreatedByMarkup, WTFMove(animations)).clear();

Why isn't clearing previous entries the existing behavior of the ListHashSet assignment operator? This code is... suspicious.

[Antoine Quint]

(In reply to Simon Fraser (smfr) from comment #24)
> Comment on attachment 394178 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394178&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        (aka CSSAnimationCollection) member on ElementAnimationRareData being replaced to the new list, but the old list not being cleared from its members.
> 
> "being replaced by"?

Yes :)

> > Source/WebCore/testing/Internals.cpp:1039
> > +bool Internals::animationWithIdExists(const String& id) const
> 
> Avoid 'id' because it's a reserved word in ObjectiveC

I can replace with "identifier", however "id" is the property from WebAnimation.

https://drafts.csswg.org/web-animations-1/#dom-animation-id

> > Source/WebCore/testing/Internals.h:213
> > +    bool animationWithIdExists(const String&) const;
> 
> How does the animation get an ID in the first place?

Through the Animation DOM API, the test does that.

[Darin Adler]

Comment on attachment 394178 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394178&action=review

>>> Source/WebCore/testing/Internals.cpp:1039
>>> +bool Internals::animationWithIdExists(const String& id) const
>> 
>> Avoid 'id' because it's a reserved word in ObjectiveC
> 
> I can replace with "identifier", however "id" is the property from WebAnimation.
> 
> https://drafts.csswg.org/web-animations-1/#dom-animation-id

To be pedantic, it’s not actually a reserved word. If it was we would likely have to be more restrictive in its use. Objective-C adds no reserved keywords. I am not sure there is an actual problem using it.

[Antoine Quint]

Created attachment 394221 [details]
Patch

[Antoine Quint]

(In reply to Darin Adler from comment #27)
> Comment on attachment 394178 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394178&action=review
> 
> >>> Source/WebCore/testing/Internals.cpp:1039
> >>> +bool Internals::animationWithIdExists(const String& id) const
> >> 
> >> Avoid 'id' because it's a reserved word in ObjectiveC
> > 
> > I can replace with "identifier", however "id" is the property from WebAnimation.
> > 
> > https://drafts.csswg.org/web-animations-1/#dom-animation-id
> 
> To be pedantic, it’s not actually a reserved word. If it was we would likely
> have to be more restrictive in its use. Objective-C adds no reserved
> keywords. I am not sure there is an actual problem using it.

I opted not to change it in the updated patch.

[Geoffrey Garen]

Comment on attachment 394221 [details]
Patch

r=me if ews agrees

[Antoine Quint]

Created attachment 394223 [details]
Patch

[Antoine Quint]

Created attachment 394224 [details]
Patch

[Darin Adler]

Comment on attachment 394224 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394224&action=review

> Source/WebCore/dom/Element.h:29
> +#include "ElementAnimationRareData.h"

This is really unfortunate. We don’t want to pull this rare data header into every compilation that uses Element.h. We don’t even do that with ElementRareData.h. Can we find a way to avoid this?

[Antoine Quint]

(In reply to Darin Adler from comment #33)
> Comment on attachment 394224 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394224&action=review
> 
> > Source/WebCore/dom/Element.h:29
> > +#include "ElementAnimationRareData.h"
> 
> This is really unfortunate. We don’t want to pull this rare data header into
> every compilation that uses Element.h. We don’t even do that with
> ElementRareData.h. Can we find a way to avoid this?

Agreed. I'm going to keep looking into that, I don't like the patch the way it is. The change in WTF required some header-related changes that I haven't quite taken the time to fully comprehend yet. Now that I have something working, I'll try to improve on it.

[Antoine Quint]

Created attachment 394232 [details]
Patch

[Antoine Quint]

(In reply to Antoine Quint from comment #34)
> (In reply to Darin Adler from comment #33)
> > > Source/WebCore/dom/Element.h:29
> > > +#include "ElementAnimationRareData.h"
> > 
> > This is really unfortunate. We don’t want to pull this rare data header into
> > every compilation that uses Element.h. We don’t even do that with
> > ElementRareData.h. Can we find a way to avoid this?
> 
> Agreed. I'm going to keep looking into that, I don't like the patch the way
> it is. The change in WTF required some header-related changes that I haven't
> quite taken the time to fully comprehend yet. Now that I have something
> working, I'll try to improve on it.

Newest patch addresses this, just needed to move the implementation of ElementAnimationRareData::setAnimationsCreatedByMarkup() to the .cpp file so that I could #include "CSSAnimation.h"

[Darin Adler]

Comment on attachment 394232 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394232&action=review

> Source/WebCore/testing/Internals.cpp:1042
> +        if (animation && animation->id() == id)

No need for the null check here. The set doesn’t support holding null pointers; that’s the empty value.

[Antoine Quint]

Committed r258826: <https://trac.webkit.org/changeset/258826>

[Darin Adler]

Comment on attachment 394232 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394232&action=review

> Source/WTF/wtf/ListHashSet.h:355
> +    ListHashSet tmp(WTFMove(other));
> +    swap(tmp);

Occurs to me after the fact that we should name this "movedSet" or "moved" instead of "tmp".

[Antoine Quint]

(In reply to Darin Adler from comment #39)
> Comment on attachment 394232 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394232&action=review
> 
> > Source/WTF/wtf/ListHashSet.h:355
> > +    ListHashSet tmp(WTFMove(other));
> > +    swap(tmp);
> 
> Occurs to me after the fact that we should name this "movedSet" or "moved"
> instead of "tmp".

I'll fix this and your final review comment (about null values in sets) in a followup patch.

[Antoine Quint]

Committed r258858: <https://trac.webkit.org/changeset/258858>