Bug 208493 – Remove the required `LockHolder` when calling `WebAnimation::instances()`

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Devin Rousso]

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.

[Devin Rousso]

Created attachment 392234 [details]
Patch

[Devin Rousso]

Created attachment 392235 [details]
Patch

[Alexey Proskuryakov]

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 on attachment 392235 [details]
Patch

Clearing flags on attachment: 392235

Committed r257756: <https://trac.webkit.org/changeset/257756>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/59979212>

[Joseph Pecoraro]

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)]

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

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