Bug 187088 – [Web Animations] Using a Web Animation leaks the Document

• Component: Animations
• Status: RESOLVED FIXED

[Antoine Quint]

[Web Animations] Using a Web Animation leaks the Document

[Antoine Quint]

<rdar://problem/41392046>

[Antoine Quint]

Created attachment 343700 [details]
Patch

[Simon Fraser (smfr)]

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*.

[Antoine Quint]

Committed r233349: <https://trac.webkit.org/changeset/233349>

[Dawei Fenton (:realdawei)]

(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.

[Dawei Fenton (:realdawei)]

Reverted r233349 for reason:

caused 42 crashes on iOS GuardMalloc and iOS ASan tests

Committed r233361: <https://trac.webkit.org/changeset/233361>

[Darin Adler]

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.

[Antoine Quint]

Created attachment 344097 [details]
Patch

[Darin Adler]

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?

[Darin Adler]

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?

[EWS Watchlist]

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

[EWS Watchlist]

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

[Antoine Quint]

(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.

[Antoine Quint]

(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.

[Antoine Quint]

(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()) {
        …
    }

[Antoine Quint]

Created attachment 344167 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Antoine Quint]

Committed r233583: <https://trac.webkit.org/changeset/233583>