Bug 208493

Summary: Remove the required `LockHolder` when calling `WebAnimation::instances()`
Product: WebKit Reporter: Devin Rousso <hi>
Component: AnimationsAssignee: Devin Rousso <hi>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dino, ews-watchlist, hi, joepeck, Lawrence.j, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 215940    
Attachments:
Description Flags
Patch
none
Patch none

Description Devin Rousso 2020-03-02 18:08:51 PST 
Since `WebAnimation`s are not accessible from `Worker`s (e.g. main thread only), there's no reason to require that a lock be held.
Comment 1 Devin Rousso 2020-03-02 18:10:15 PST 
Created attachment 392234 [details]
Patch
Comment 2 Devin Rousso 2020-03-02 18:13:44 PST 
Created attachment 392235 [details]
Patch
Comment 3 Alexey Proskuryakov 2020-03-02 18:54:54 PST 
Comment on attachment 392235 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=392235&action=review

> Source/WebCore/ChangeLog:8
> +        Since `WebAnimation`s are not accessible from `Worker`s (e.g. main thread only), there's no

s/e.g./i.e./

> Source/WebCore/animation/WebAnimation.cpp:-116
> -    LockHolder lock(instancesMutex());

So the destructor is also always called from main thread?
Comment 4 WebKit Commit Bot 2020-03-02 18:59:35 PST 
Comment on attachment 392235 [details]
Patch

Clearing flags on attachment: 392235

Committed r257756: <https://trac.webkit.org/changeset/257756>
Comment 5 WebKit Commit Bot 2020-03-02 18:59:37 PST 
All reviewed patches have been landed.  Closing bug.
Comment 6 Radar WebKit Bug Importer 2020-03-02 19:00:16 PST 
<rdar://problem/59979212>
Comment 7 Joseph Pecoraro 2020-03-03 10:12:30 PST 
Comment on attachment 392235 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=392235&action=review

> Source/WebCore/ChangeLog:9
> +        reason to require that a lock be held in order to access `WebAnimation::instances()`.

Can we add an assertion to assert this?
Comment 8 Simon Fraser (smfr) 2020-03-03 11:00:06 PST 
(In reply to Joseph Pecoraro from comment #7)
> Comment on attachment 392235 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=392235&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        reason to require that a lock be held in order to access `WebAnimation::instances()`.
> 
> Can we add an assertion to assert this?

Should we assert for every class that's main thread only? WebAnimation is not ThreadSafeRefCounted, so there's no indication that it's thread safe.
Comment 9 Joseph Pecoraro 2020-03-03 11:42:30 PST 
(In reply to Simon Fraser (smfr) from comment #8)
> (In reply to Joseph Pecoraro from comment #7)
> > Comment on attachment 392235 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=392235&action=review
> > 
> > > Source/WebCore/ChangeLog:9
> > > +        reason to require that a lock be held in order to access `WebAnimation::instances()`.
> > 
> > Can we add an assertion to assert this?
> 
> Should we assert for every class that's main thread only? WebAnimation is
> not ThreadSafeRefCounted, so there's no indication that it's thread safe.

That seems fair. The idea would be if WebAnimation becomes ThreadSafeRefCounted in the future, then we might overlook adding locks to this all-animations list and the assertion would catch that. I guess WebAnimations are not something we are going to expose to Workers / Worklets anytime soon though.