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

Devin Rousso
Reported 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.
Attachments
Patch (4.16 KB, patch)
2020-03-02 18:10 PST, Devin Rousso 		no flags
DetailsFormatted DiffDiff
Patch (4.32 KB, patch)
2020-03-02 18:13 PST, Devin Rousso 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Devin Rousso
Comment 1 2020-03-02 18:10:15 PST
Created attachment 392234 [details] Patch
Devin Rousso
Comment 2 2020-03-02 18:13:44 PST
Created attachment 392235 [details] Patch
Alexey Proskuryakov
Comment 3 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?
WebKit Commit Bot
Comment 4 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>
WebKit Commit Bot
Comment 5 2020-03-02 18:59:37 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 6 2020-03-02 19:00:16 PST
<rdar://problem/59979212>
Joseph Pecoraro
Comment 7 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?
Simon Fraser (smfr)
Comment 8 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.
Joseph Pecoraro
Comment 9 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.
Note You need to log in before you can comment on or make changes to this bug.