Bug 226001 - Use CheckedLock more in WebKit2 code
Summary: Use CheckedLock more in WebKit2 code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-05-19 19:29 PDT by Chris Dumez
Modified: 2021-05-20 09:12 PDT (History)
10 users (show)

See Also:


Attachments
Patch (48.00 KB, patch)
2021-05-19 19:30 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (50.13 KB, patch)
2021-05-19 23:09 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (49.05 KB, patch)
2021-05-20 07:37 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2021-05-19 19:29:07 PDT
Use CheckedLock more in WebKit2 code to benefit for Clang Thread Safety Analysis.
Comment 1 Chris Dumez 2021-05-19 19:30:59 PDT
Created attachment 429127 [details]
Patch
Comment 2 Alex Christensen 2021-05-19 20:52:55 PDT
Comment on attachment 429127 [details]
Patch

I like WTF_GUARDED_BY_LOCK
Comment 3 Chris Dumez 2021-05-19 23:09:39 PDT
Created attachment 429140 [details]
Patch
Comment 4 Kimmo Kinnunen 2021-05-20 01:21:52 PDT
Comment on attachment 429140 [details]
Patch

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

great work.
maybe the lockable() could be reverted / not added.

> Source/WTF/ChangeLog:8
> +        Add lockable() getter to Locker<CheckedLock> to match the regular Locker<T>.

I didn't see the use case to add that.
That enables the anti-pattern that is being invoked also in this patch.

> Source/WebKit/WebProcess/GPU/webrtc/LibWebRTCCodecs.cpp:177
> +void LibWebRTCCodecs::ensureGPUProcessConnectionOnMainThread(Locker<CheckedLock>& locker)

For CheckedLock, this sort of unused parameter pattern is now an anti-pattern.

The annotation in the header file would be :
   void LibWebRTCCodecs::ensureGPUProcessConnectionOnMainThread() WTF_REQUIRES_LOCK(m_connectionLock);
Comment 5 Chris Dumez 2021-05-20 07:37:17 PDT
Created attachment 429171 [details]
Patch
Comment 6 Chris Dumez 2021-05-20 07:38:48 PDT
(In reply to Kimmo Kinnunen from comment #4)
> Comment on attachment 429140 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=429140&action=review
> 
> great work.
> maybe the lockable() could be reverted / not added.
> 
> > Source/WTF/ChangeLog:8
> > +        Add lockable() getter to Locker<CheckedLock> to match the regular Locker<T>.
> 
> I didn't see the use case to add that.
> That enables the anti-pattern that is being invoked also in this patch.

Yes, definitely not needed if we get rid of the "Passing Locker as parameter" pattern. I dropped this.

> > Source/WebKit/WebProcess/GPU/webrtc/LibWebRTCCodecs.cpp:177
> > +void LibWebRTCCodecs::ensureGPUProcessConnectionOnMainThread(Locker<CheckedLock>& locker)
> 
> For CheckedLock, this sort of unused parameter pattern is now an
> anti-pattern.
> 
> The annotation in the header file would be :
>    void LibWebRTCCodecs::ensureGPUProcessConnectionOnMainThread()
> WTF_REQUIRES_LOCK(m_connectionLock);

Fixed.
Comment 7 EWS 2021-05-20 09:11:32 PDT
Committed r277787 (237948@main): <https://commits.webkit.org/237948@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 429171 [details].
Comment 8 Radar WebKit Bug Importer 2021-05-20 09:12:17 PDT
<rdar://problem/78261485>