Bug 185917 - [Web Animations] WebAnimation objects never get destroyed
Summary: [Web Animations] WebAnimation objects never get destroyed
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-05-23 09:11 PDT by Antoine Quint
Modified: 2018-05-25 06:45 PDT (History)
6 users (show)

See Also:


Attachments
Patch (31.83 KB, patch)
2018-05-23 12:16 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-sierra (2.27 MB, application/zip)
2018-05-23 13:23 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews107 for mac-sierra-wk2 (2.77 MB, application/zip)
2018-05-23 13:29 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews116 for mac-sierra (3.02 MB, application/zip)
2018-05-23 14:06 PDT, EWS Watchlist
no flags Details
Patch (39.31 KB, patch)
2018-05-24 10:10 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews115 for mac-sierra (3.15 MB, application/zip)
2018-05-24 11:38 PDT, EWS Watchlist
no flags Details
Patch (40.99 KB, patch)
2018-05-25 05:35 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (41.22 KB, patch)
2018-05-25 05:56 PDT, Antoine Quint
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2018-05-23 09:11:48 PDT
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.
Comment 1 Antoine Quint 2018-05-23 09:12:09 PDT
<rdar://problem/39539371>
Comment 2 Antoine Quint 2018-05-23 12:16:11 PDT
Created attachment 341114 [details]
Patch
Comment 3 Dean Jackson 2018-05-23 12:24:04 PDT
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.
Comment 4 Antoine Quint 2018-05-23 12:35:02 PDT
(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.
Comment 5 EWS Watchlist 2018-05-23 13:23:23 PDT
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
Comment 6 EWS Watchlist 2018-05-23 13:23:24 PDT
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
Comment 7 EWS Watchlist 2018-05-23 13:29:49 PDT
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
Comment 8 EWS Watchlist 2018-05-23 13:29:51 PDT
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
Comment 9 EWS Watchlist 2018-05-23 14:06:07 PDT
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
Comment 10 EWS Watchlist 2018-05-23 14:06:09 PDT
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
Comment 11 Antti Koivisto 2018-05-24 02:00:16 PDT
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.
Comment 12 Antoine Quint 2018-05-24 03:33:28 PDT
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
Comment 13 Emilio Cobos Álvarez (:emilio) 2018-05-24 06:27:04 PDT
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.
Comment 14 Antoine Quint 2018-05-24 10:10:43 PDT
Created attachment 341201 [details]
Patch
Comment 15 EWS Watchlist 2018-05-24 11:38:13 PDT
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
Comment 16 EWS Watchlist 2018-05-24 11:38:14 PDT
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
Comment 17 Antoine Quint 2018-05-25 03:59:34 PDT
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.
Comment 18 Antoine Quint 2018-05-25 05:35:49 PDT
Created attachment 341273 [details]
Patch
Comment 19 Antoine Quint 2018-05-25 05:36:29 PDT
Running the tests with ASAN uncovered the issue, we didn't close the task queues early enough in DocumentTimeline.
Comment 20 Antti Koivisto 2018-05-25 05:41:27 PDT
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).
Comment 21 Antti Koivisto 2018-05-25 05:43:04 PDT
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.
Comment 22 Antoine Quint 2018-05-25 05:56:15 PDT
Created attachment 341275 [details]
Patch
Comment 23 Antoine Quint 2018-05-25 06:45:22 PDT
Committed r232185: <https://trac.webkit.org/changeset/232185>