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.
Disabling "CSS Animations via Web Animations" experimental feature seems to make the problem go away.
Created attachment 391417 [details] Memgraph
<rdar://problem/59680143>
Created attachment 391544 [details] WIP Patch
(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.
(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.
(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.
Created attachment 391682 [details] Patch
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 on attachment 391682 [details] Patch Clearing flags on attachment: 391682 Committed r257417: <https://trac.webkit.org/changeset/257417>
All reviewed patches have been landed. Closing bug.
Re-opening so that I may assess why CSSTransition objects persisted in the first place.
(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 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 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 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 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.
We're going to have to revisit this, per my findings in bug 209239. Animations need to keep a strong reference to their timeline.
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).
There is another issue with ElementAnimationRareData::m_animationsCreatedByMarkup, we need to call clear() on it before assigning the new value in ElementAnimationRareData::setAnimationsCreatedByMarkup(CSSAnimationCollection&&).
> > 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.)
Created attachment 394178 [details] Patch
Note that the new Patch attached assumes the previous patch for this bug is reverted as the fix for bug 209239.
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 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.
(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 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.
Created attachment 394221 [details] Patch
(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 on attachment 394221 [details] Patch r=me if ews agrees
Created attachment 394223 [details] Patch
Created attachment 394224 [details] Patch
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?
(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.
Created attachment 394232 [details] Patch
(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 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.
Committed r258826: <https://trac.webkit.org/changeset/258826>
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".
(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.
Committed r258858: <https://trac.webkit.org/changeset/258858>