[Web Animations] Using a Web Animation leaks the Document
<rdar://problem/41392046>
Created attachment 343700 [details] Patch
Comment on attachment 343700 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=343700&action=review > Source/WebCore/animation/WebAnimation.cpp:206 > +void WebAnimation::setTimelineInternal(RefPtr<AnimationTimeline>&& timeline) It's weird to pass a RefPtr by r-value reference here. Normally that's used to transfer ownership. I think you should just pass a AnimationTimeline*.
Committed r233349: <https://trac.webkit.org/changeset/233349>
(In reply to Antoine Quint from comment #4) > Committed r233349: <https://trac.webkit.org/changeset/233349> This change is causing crashes on internal bots. See <rdar://problem/41392046> For these reason we'll need to roll this out.
Reverted r233349 for reason: caused 42 crashes on iOS GuardMalloc and iOS ASan tests Committed r233361: <https://trac.webkit.org/changeset/233361>
Comment on attachment 343700 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=343700&action=review >> Source/WebCore/animation/WebAnimation.cpp:206 >> +void WebAnimation::setTimelineInternal(RefPtr<AnimationTimeline>&& timeline) > > It's weird to pass a RefPtr by r-value reference here. Normally that's used to transfer ownership. I think you should just pass a AnimationTimeline*. But this is transferring ownership of the reference. I think this is OK.
Created attachment 344097 [details] Patch
Comment on attachment 344097 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344097&action=review > Source/WebCore/animation/DeclarativeAnimation.h:51 > + void remove() final; Why public and not private? > Source/WebCore/animation/DocumentAnimationScheduler.cpp:81 > m_isFiring = true; Is there something that prevents this function from being re-entered?
Comment on attachment 344097 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344097&action=review > Source/WebCore/animation/DocumentTimeline.cpp:65 > + const auto& animationsToRemove = animations(); The "const" here is strange, since the code just after this definitely is modifying the collection. I would omit this. > Source/WebCore/animation/WebAnimation.cpp:75 > + // This object could be marked for deletion after either clearing the effect or timeline relationship. The comment is a bit oblique. If clearing the timeline could result in deleting this, then it’s not problem since that’s the last thing in the function. And if clearing the effect "marks" this for deletion, that’s not a problem either. But actually deleting would be a problem. > Source/WebCore/animation/WebAnimation.cpp:147 > auto oldEffect = m_effect; > > + if (oldEffect == newEffect) > + return; Unnecessarily inefficient to check only after copying the old value into a local. I suggest checking before setting up "oldEffect". Also should use WTFMove when assigning m_effect into oldEffect. Might even be best to use std::exchange: auto oldEffect = std::exchange(m_effect, WTFMove(newEffect)); > Source/WebCore/animation/WebAnimation.cpp:200 > + setTimelineInternal(WTFMove(timeline)); What guarantees the object won’t be "marked for deletion" after this? If it’s an issue in WebAnimation::remove, why isn’t it an issue here? > Source/WebCore/animation/WebAnimation.cpp:222 > + if (m_timeline) > + m_timeline->removeAnimation(*this); > + > + if (timeline) > + timeline->addAnimation(*this); > + > + m_timeline = WTFMove(timeline); Is it OK to call addAnimation while m_timeline still points to the old one?
Comment on attachment 344097 [details] Patch Attachment 344097 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8414155 New failing tests: http/tests/security/canvas-remote-read-remote-video-localhost.html
Created attachment 344124 [details] Archive of layout-test-results from ews205 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews205 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
(In reply to Darin Adler from comment #9) > Comment on attachment 344097 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=344097&action=review > > > Source/WebCore/animation/DeclarativeAnimation.h:51 > > + void remove() final; > > Why public and not private? It's matching the superclass's definition in WebAnimation.h which is also public. This allows this method to be called from an AnimationTimeline. > > Source/WebCore/animation/DocumentAnimationScheduler.cpp:81 > > m_isFiring = true; > > Is there something that prevents this function from being re-entered? I didn't double check this, but this method is called through a DisplayRefreshMonitor::scheduleAnimation() call. I expect further calls to DisplayRefreshMonitor::scheduleAnimation() while callbacks are being called are on a different queue.
(In reply to Darin Adler from comment #10) > Comment on attachment 344097 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=344097&action=review > > > Source/WebCore/animation/DocumentTimeline.cpp:65 > > + const auto& animationsToRemove = animations(); > > The "const" here is strange, since the code just after this definitely is > modifying the collection. I would omit this. I will remove it. > > Source/WebCore/animation/WebAnimation.cpp:75 > > + // This object could be marked for deletion after either clearing the effect or timeline relationship. > > The comment is a bit oblique. If clearing the timeline could result in > deleting this, then it’s not problem since that’s the last thing in the > function. And if clearing the effect "marks" this for deletion, that’s not a > problem either. But actually deleting would be a problem. I'm not familiar enough with the internals of RefCounting, but I was definitely getting an issue where the WebAnimation's m_deletionHasBegun was true and accessing its members were yielding crashes when running with ASan. Calling either setEffectInternal() or setTimelineInternal() could get m_deletionHasBegun set to true since AnimationEffectReadOnly and AnimationTimeline are the only two classes referencing WebAnimation. I guess my comment in wrong when saying "marked for deletion". The problem is that it's "being deleted". > > Source/WebCore/animation/WebAnimation.cpp:147 > > auto oldEffect = m_effect; > > > > + if (oldEffect == newEffect) > > + return; > > Unnecessarily inefficient to check only after copying the old value into a > local. I suggest checking before setting up "oldEffect". > > Also should use WTFMove when assigning m_effect into oldEffect. Might even > be best to use std::exchange: > > auto oldEffect = std::exchange(m_effect, WTFMove(newEffect)); Yes, I will fix this. > > Source/WebCore/animation/WebAnimation.cpp:200 > > + setTimelineInternal(WTFMove(timeline)); > > What guarantees the object won’t be "marked for deletion" after this? If > it’s an issue in WebAnimation::remove, why isn’t it an issue here? I don't think that anything guarantees that. I expect we haven't seen this issue (yet) because code calling setTimeline() is either coming from the JS side or when creating WebAnimation instances. I expect we do need to protect the instance in this method as well, just like we do in setEffect(). > > Source/WebCore/animation/WebAnimation.cpp:222 > > + if (m_timeline) > > + m_timeline->removeAnimation(*this); > > + > > + if (timeline) > > + timeline->addAnimation(*this); > > + > > + m_timeline = WTFMove(timeline); > > Is it OK to call addAnimation while m_timeline still points to the old one? Currently, it is OK, but it seems safer to call addAnimation() after we've set m_timeline to the new value.
(In reply to Antoine Quint from comment #13) > (In reply to Darin Adler from comment #9) > > Comment on attachment 344097 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=344097&action=review > > > > > Source/WebCore/animation/DocumentAnimationScheduler.cpp:81 > > > m_isFiring = true; > > > > Is there something that prevents this function from being re-entered? > > I didn't double check this, but this method is called through a > DisplayRefreshMonitor::scheduleAnimation() call. I expect further calls to > DisplayRefreshMonitor::scheduleAnimation() while callbacks are being called > are on a different queue. I checked, DisplayRefreshMonitor::displayDidRefresh() does the right thing: // Copy the hash table and remove clients from it one by one so we don't notify // any client twice, but can respond to removal of clients during the delivery process. HashSet<DisplayRefreshMonitorClient*> clientsToBeNotified = m_clients; m_clientsToBeNotified = &clientsToBeNotified; while (!clientsToBeNotified.isEmpty()) { … }
Created attachment 344167 [details] Patch
Comment on attachment 344167 [details] Patch Attachment 344167 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8425009 New failing tests: http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html
Created attachment 344192 [details] Archive of layout-test-results from ews204 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews204 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 344167 [details] Patch Attachment 344167 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8425553 New failing tests: http/tests/security/canvas-remote-read-remote-video-redirect.html http/tests/security/contentSecurityPolicy/video-redirect-allowed2.html
Created attachment 344197 [details] Archive of layout-test-results from ews206 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Committed r233583: <https://trac.webkit.org/changeset/233583>