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.
<rdar://problem/39539371>
Created attachment 341114 [details] Patch
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.
(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 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
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 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
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 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
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
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.
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 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.
Created attachment 341201 [details] Patch
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
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
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.
Created attachment 341273 [details] Patch
Running the tests with ASAN uncovered the issue, we didn't close the task queues early enough in DocumentTimeline.
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 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.
Created attachment 341275 [details] Patch
Committed r232185: <https://trac.webkit.org/changeset/232185>