Bug 226001

Summary:

Use CheckedLock more in WebKit2 code

Product:

WebKit

Reporter:

Chris Dumez <cdumez>

Component:

WebKit2

Assignee:

Chris Dumez <cdumez>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

achristensen, benjamin, cmarcelo, darin, ddkilzer, ews-watchlist, ggaren, kkinnunen, sam, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=226021

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

Chris Dumez

Reported 2021-05-19 19:29:07 PDT

Use CheckedLock more in WebKit2 code to benefit for Clang Thread Safety Analysis.

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
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Chris Dumez

Comment 1 2021-05-19 19:30:59 PDT

Created attachment 429127 [details] Patch

Alex Christensen

Comment 2 2021-05-19 20:52:55 PDT

Comment on attachment 429127 [details] Patch I like WTF_GUARDED_BY_LOCK

Chris Dumez

Comment 3 2021-05-19 23:09:39 PDT

Created attachment 429140 [details] Patch

Kimmo Kinnunen

Comment 4 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);

Chris Dumez

Comment 5 2021-05-20 07:37:17 PDT

Created attachment 429171 [details] Patch

Chris Dumez

Comment 6 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.

EWS

Comment 7 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].

Radar WebKit Bug Importer

Comment 8 2021-05-20 09:12:17 PDT

<rdar://problem/78261485>

Note You need to log in before you can comment on or make changes to this bug.