Bug 187088 - [Web Animations] Using a Web Animation leaks the Document
Summary: [Web Animations] Using a Web Animation leaks the Document
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-06-27 01:00 PDT by Antoine Quint
Modified: 2018-07-06 11:31 PDT (History)
8 users (show)

See Also:


Attachments
Patch (11.83 KB, patch)
2018-06-27 01:08 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (14.06 KB, patch)
2018-07-02 05:35 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews205 for win-future (12.77 MB, application/zip)
2018-07-02 11:58 PDT, EWS Watchlist
no flags Details
Patch (15.33 KB, patch)
2018-07-03 01:14 PDT, Antoine Quint
darin: review+
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews204 for win-future (12.91 MB, application/zip)
2018-07-03 09:38 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews206 for win-future (12.79 MB, application/zip)
2018-07-03 10:56 PDT, EWS Watchlist
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2018-06-27 01:00:21 PDT
[Web Animations] Using a Web Animation leaks the Document
Comment 1 Antoine Quint 2018-06-27 01:01:01 PDT
<rdar://problem/41392046>
Comment 2 Antoine Quint 2018-06-27 01:08:31 PDT
Created attachment 343700 [details]
Patch
Comment 3 Simon Fraser (smfr) 2018-06-28 16:32:03 PDT
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*.
Comment 4 Antoine Quint 2018-06-28 22:45:42 PDT
Committed r233349: <https://trac.webkit.org/changeset/233349>
Comment 5 Dawei Fenton (:realdawei) 2018-06-29 10:01:16 PDT
(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.
Comment 6 Dawei Fenton (:realdawei) 2018-06-29 10:42:09 PDT
Reverted r233349 for reason:

caused 42 crashes on iOS GuardMalloc and iOS ASan tests

Committed r233361: <https://trac.webkit.org/changeset/233361>
Comment 7 Darin Adler 2018-07-01 19:13:54 PDT
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.
Comment 8 Antoine Quint 2018-07-02 05:35:01 PDT
Created attachment 344097 [details]
Patch
Comment 9 Darin Adler 2018-07-02 09:15:21 PDT
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 10 Darin Adler 2018-07-02 09:24:22 PDT
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 11 EWS Watchlist 2018-07-02 11:58:11 PDT
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
Comment 12 EWS Watchlist 2018-07-02 11:58:22 PDT
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
Comment 13 Antoine Quint 2018-07-02 13:36:09 PDT
(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.
Comment 14 Antoine Quint 2018-07-02 13:52:51 PDT
(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.
Comment 15 Antoine Quint 2018-07-03 00:53:21 PDT
(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()) {
        …
    }
Comment 16 Antoine Quint 2018-07-03 01:14:09 PDT
Created attachment 344167 [details]
Patch
Comment 17 EWS Watchlist 2018-07-03 09:38:02 PDT
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
Comment 18 EWS Watchlist 2018-07-03 09:38:14 PDT
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 19 EWS Watchlist 2018-07-03 10:56:24 PDT
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
Comment 20 EWS Watchlist 2018-07-03 10:56:35 PDT
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
Comment 21 Antoine Quint 2018-07-06 11:31:19 PDT
Committed r233583: <https://trac.webkit.org/changeset/233583>