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.
Created attachment 392234 [details] Patch
Created attachment 392235 [details] Patch
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 on attachment 392235 [details] Patch Clearing flags on attachment: 392235 Committed r257756: <https://trac.webkit.org/changeset/257756>
All reviewed patches have been landed. Closing bug.
<rdar://problem/59979212>
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?
(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.
(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.