Bug 208069

Summary: DocumentTimeline / CSSTransition objects are leaking on CNN.com
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: AnimationsAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, benjamin, cmarcelo, commit-queue, darin, dino, esprehn+autocc, ews-watchlist, ggaren, graouts, graouts, gyuyoung.kim, jonlee, kangil.han, koivisto, rniwa, ryuan.choi, sergio, simon.fraser, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=208079
Bug Depends on: 208145    
Bug Blocks: 209239    
Attachments:
Description Flags
Memgraph
none
WIP Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch darin: review+

Description Chris Dumez 2020-02-21 11:48:38 PST
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.
Comment 1 Chris Dumez 2020-02-21 11:52:29 PST
Disabling "CSS Animations via Web Animations" experimental feature seems to make the problem go away.
Comment 2 Chris Dumez 2020-02-21 11:55:23 PST
Created attachment 391417 [details]
Memgraph
Comment 3 Radar WebKit Bug Importer 2020-02-21 13:21:39 PST
<rdar://problem/59680143>
Comment 4 Chris Dumez 2020-02-24 08:28:40 PST
Created attachment 391544 [details]
WIP Patch
Comment 5 Chris Dumez 2020-02-24 08:56:32 PST
(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.
Comment 6 Chris Dumez 2020-02-24 09:04:45 PST
(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.
Comment 7 Chris Dumez 2020-02-24 09:12:54 PST
(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.
Comment 8 Chris Dumez 2020-02-25 14:03:40 PST
Created attachment 391682 [details]
Patch
Comment 9 Antoine Quint 2020-02-26 01:35:36 PST
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.
Comment 10 WebKit Commit Bot 2020-02-26 02:21:35 PST
Comment on attachment 391682 [details]
Patch

Clearing flags on attachment: 391682

Committed r257417: <https://trac.webkit.org/changeset/257417>
Comment 11 WebKit Commit Bot 2020-02-26 02:21:37 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Antoine Quint 2020-02-26 02:25:11 PST
Re-opening so that I may assess why CSSTransition objects persisted in the first place.
Comment 13 Chris Dumez 2020-02-26 08:02:34 PST
(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.
Comment 14 Darin Adler 2020-02-26 08:41:43 PST
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?
Comment 15 Chris Dumez 2020-02-26 08:43:11 PST
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.
Comment 16 Darin Adler 2020-02-26 09:03:10 PST
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.
Comment 17 Darin Adler 2020-02-26 09:04:02 PST
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.
Comment 18 Antoine Quint 2020-03-19 11:01:02 PDT
We're going to have to revisit this, per my findings in bug 209239. Animations need to keep a strong reference to their timeline.
Comment 19 Antoine Quint 2020-03-20 08:26:11 PDT
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).
Comment 20 Antoine Quint 2020-03-21 06:20:40 PDT
There is another issue with ElementAnimationRareData::m_animationsCreatedByMarkup, we need to call clear() on it before assigning the new value in  ElementAnimationRareData::setAnimationsCreatedByMarkup(CSSAnimationCollection&&).
Comment 21 Geoffrey Garen 2020-03-21 11:19:33 PDT
> > 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.)
Comment 22 Antoine Quint 2020-03-21 14:18:37 PDT
Created attachment 394178 [details]
Patch
Comment 23 Antoine Quint 2020-03-21 14:21:47 PDT
Note that the new Patch attached assumes the previous patch for this bug is reverted as the fix for bug 209239.
Comment 24 Simon Fraser (smfr) 2020-03-21 15:23:15 PDT
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?
Comment 25 Geoffrey Garen 2020-03-21 16:06:30 PDT
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.
Comment 26 Antoine Quint 2020-03-22 04:56:36 PDT
(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.
Comment 27 Darin Adler 2020-03-22 09:34:50 PDT
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.
Comment 28 Antoine Quint 2020-03-22 11:47:11 PDT
Created attachment 394221 [details]
Patch
Comment 29 Antoine Quint 2020-03-22 11:49:13 PDT
(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.
Comment 30 Geoffrey Garen 2020-03-22 11:50:06 PDT
Comment on attachment 394221 [details]
Patch

r=me if ews agrees
Comment 31 Antoine Quint 2020-03-22 12:17:02 PDT
Created attachment 394223 [details]
Patch
Comment 32 Antoine Quint 2020-03-22 12:24:30 PDT
Created attachment 394224 [details]
Patch
Comment 33 Darin Adler 2020-03-22 13:32:21 PDT
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?
Comment 34 Antoine Quint 2020-03-22 15:06:11 PDT
(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.
Comment 35 Antoine Quint 2020-03-22 15:24:57 PDT
Created attachment 394232 [details]
Patch
Comment 36 Antoine Quint 2020-03-22 15:25:53 PDT
(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"
Comment 37 Darin Adler 2020-03-22 16:12:39 PDT
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.
Comment 38 Antoine Quint 2020-03-23 01:17:35 PDT
Committed r258826: <https://trac.webkit.org/changeset/258826>
Comment 39 Darin Adler 2020-03-23 10:13:24 PDT
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".
Comment 40 Antoine Quint 2020-03-23 10:46:02 PDT
(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.
Comment 41 Antoine Quint 2020-03-23 10:50:15 PDT
Committed r258858: <https://trac.webkit.org/changeset/258858>