Bug 185917 – [Web Animations] WebAnimation objects never get destroyed

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Antoine Quint]

An AnimationTimeline object stores RefPtr<WebAnimation> in various maps and these don't get cleared when navigating away from a page that contained Animation objects created via the Web Animations API or CSS Animations and CSS Transitions.

[Antoine Quint]

<rdar://problem/39539371>

[Antoine Quint]

Created attachment 341114 [details]
Patch

[Dean Jackson]

Comment on attachment 341114 [details]
Patch

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

> Source/WebCore/animation/AnimationTimeline.cpp:104
> +    if (!animations.contains(&animation))
> +        animations.append(&animation);

Maybe it should be a Set instead of a Vector in the map?

> Source/WebCore/animation/AnimationTimeline.cpp:141
> +        }
> +    } else if (is<CSSTransition>(animation)) {
> +        auto iterator = m_elementToCSSTransitionByCSSPropertyID.find(&element);
> +        if (iterator != m_elementToCSSTransitionByCSSPropertyID.end()) {
> +            auto& cssTransitionsByProperty = iterator->value;
> +            auto property = downcast<CSSTransition>(animation).property();
> +            cssTransitionsByProperty.remove(property);
> +            if (cssTransitionsByProperty.isEmpty())
> +                m_elementToCSSTransitionByCSSPropertyID.remove(&element);
> +        }
> +    }

You can early return from the first one to avoid the else.

[Antoine Quint]

(In reply to Dean Jackson from comment #3)
> Comment on attachment 341114 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=341114&action=review
> 
> > Source/WebCore/animation/AnimationTimeline.cpp:104
> > +    if (!animations.contains(&animation))
> > +        animations.append(&animation);
> 
> Maybe it should be a Set instead of a Vector in the map?

Actually, it would have to be a ListHashSet to preserve the order. Maybe I'll do this.

> > Source/WebCore/animation/AnimationTimeline.cpp:141
> > +        }
> > +    } else if (is<CSSTransition>(animation)) {
> > +        auto iterator = m_elementToCSSTransitionByCSSPropertyID.find(&element);
> > +        if (iterator != m_elementToCSSTransitionByCSSPropertyID.end()) {
> > +            auto& cssTransitionsByProperty = iterator->value;
> > +            auto property = downcast<CSSTransition>(animation).property();
> > +            cssTransitionsByProperty.remove(property);
> > +            if (cssTransitionsByProperty.isEmpty())
> > +                m_elementToCSSTransitionByCSSPropertyID.remove(&element);
> > +        }
> > +    }
> 
> You can early return from the first one to avoid the else.

OK.

[EWS Watchlist]

Comment on attachment 341114 [details]
Patch

Attachment 341114 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/7779975

New failing tests:
imported/mozilla/css-transitions/test_event-dispatch.html

[EWS Watchlist]

Created attachment 341120 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 341114 [details]
Patch

Attachment 341114 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7780005

New failing tests:
imported/mozilla/css-transitions/test_event-dispatch.html

[EWS Watchlist]

Created attachment 341122 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 341114 [details]
Patch

Attachment 341114 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7780033

New failing tests:
animations/play-state-start-paused.html

[EWS Watchlist]

Created attachment 341128 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Antti Koivisto]

You should add a test case for leaks. At least in PLUM case we were leaking full Documents so a test using the existing internals.numberOfLiveDocuments() would cover it. You could also add specific Internals functions for animations.

[Antoine Quint]

The test at animations/play-state-start-paused.html does not crash when run individually but does if run like so:

run-webkit-tests --debug -1 --no-retry-failures --no-sample-on-timeout animations/play-state-paused.html animations/play-state-start-paused.html

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 341114 [details]
Patch

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

> Source/WebCore/animation/WebAnimation.cpp:137
> +    Element* previousTarget;

Totally drive-by, but shouldn't this be initialized to nullptr, and newTarget below as well?

It's totally not clear to me these are not used when not initialized... Sorry if the feedback is not welcome though.

[Antoine Quint]

Created attachment 341201 [details]
Patch

[EWS Watchlist]

Comment on attachment 341201 [details]
Patch

Attachment 341201 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7789988

New failing tests:
animations/play-state-start-paused.html
animations/leak-document-with-css-animation.html

[EWS Watchlist]

Created attachment 341213 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Antoine Quint]

If I change this animation rule:

        @keyframes slide {
            from { transform: translate3d(0, 0, 0); }
            to   { transform: translate3d(400px, 0, 0); }
        }

to:

        @keyframes slide {
            from { margin-left: 0; }
            to   { margin-left: 400px; }
        }

… the animations/play-state-start-paused.html test no longer crashes. So the crash may be related to hardware-accelerated animations.

[Antoine Quint]

Created attachment 341273 [details]
Patch

[Antoine Quint]

Running the tests with ASAN uncovered the issue, we didn't close the task queues early enough in DocumentTimeline.

[Antti Koivisto]

Comment on attachment 341273 [details]
Patch

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

> LayoutTests/animations/leak-document-with-css-animation-expected.txt:9
> +Number of documents prior to loading iframe = 2
> +Number of documents after loading iframe = 3
> +Number of documents after destroying iframe = 2
> +PASS successfullyParsed is true

This may make the test sensitive to problem in other tests that run in the same process. I might be better to print changes in document count instead of the absolute numbers (internals.numberOfLiveDocuments() - documentCountAtStart).

[Antti Koivisto]

Comment on attachment 341273 [details]
Patch

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

> Source/WebCore/animation/WebAnimation.cpp:147
> +    Element* previousTarget;
> +    if (is<KeyframeEffectReadOnly>(oldEffect))
> +        previousTarget = downcast<KeyframeEffectReadOnly>(oldEffect.get())->target();
> +
> +    Element* newTarget;
> +    if (is<KeyframeEffectReadOnly>(m_effect))
> +        newTarget = downcast<KeyframeEffectReadOnly>(m_effect.get())->target();
> +

You need to null-initialize previousTarget and newTarget.

[Antoine Quint]

Created attachment 341275 [details]
Patch

[Antoine Quint]

Committed r232185: <https://trac.webkit.org/changeset/232185>