Bug 226145

Summary: Drop WTF::tryHoldLock() as it is incompatible with Clang Thread Safety Analysis
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: Web Template FrameworkAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, benjamin, berto, calvaris, cgarcia, changseok, cmarcelo, darin, ddkilzer, dino, eric.carlson, esprehn+autocc, ews-watchlist, ggaren, glenn, graouts, gustavo, gyuyoung.kim, hi, hta, jer.noble, joepeck, keith_miller, kkinnunen, kondapallykalyan, mark.lam, menard, msaboff, philipj, pnormand, saam, sam, sergio, tommyw, tzagallo, vjaquez, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
URL: https://clang.llvm.org/docs/ThreadSafetyAnalysis.html
Bug Depends on:    
Bug Blocks: 226157    
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
none
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch darin: review+

Description Chris Dumez 2021-05-22 14:56:12 PDT 
Drop WTF::tryHoldLock() as it is incompatible with Clang Thread Safety Analysis. Instead, use the following pattern:
```
Locker locker { DeferLock, m_lock };
if (locker.tryLock())
```
Comment 1 Chris Dumez 2021-05-22 15:37:40 PDT 
Created attachment 429429 [details]
Patch
Comment 2 EWS Watchlist 2021-05-22 15:38:23 PDT 
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 3 Chris Dumez 2021-05-22 15:49:43 PDT 
Created attachment 429430 [details]
Patch
Comment 4 Chris Dumez 2021-05-22 16:00:36 PDT 
Created attachment 429432 [details]
Patch
Comment 5 Chris Dumez 2021-05-22 16:16:59 PDT 
Created attachment 429433 [details]
Patch
Comment 6 Chris Dumez 2021-05-22 16:23:56 PDT 
Created attachment 429436 [details]
Patch
Comment 7 Chris Dumez 2021-05-22 16:32:35 PDT 
Created attachment 429437 [details]
Patch
Comment 8 Chris Dumez 2021-05-22 16:43:13 PDT 
Created attachment 429439 [details]
Patch
Comment 9 Chris Dumez 2021-05-22 16:47:56 PDT 
Created attachment 429440 [details]
Patch
Comment 10 Chris Dumez 2021-05-22 16:59:45 PDT 
Not sure what's going on, it builds fine locally :/
Comment 11 Chris Dumez 2021-05-22 18:08:53 PDT 
Created attachment 429443 [details]
Patch
Comment 12 Chris Dumez 2021-05-22 18:51:29 PDT 
Created attachment 429446 [details]
Patch
Comment 13 Chris Dumez 2021-05-22 19:01:06 PDT 
Created attachment 429447 [details]
Patch
Comment 14 Chris Dumez 2021-05-22 19:18:59 PDT 
Created attachment 429448 [details]
Patch
Comment 15 Chris Dumez 2021-05-22 19:20:29 PDT 
Created attachment 429449 [details]
Patch
Comment 16 Darin Adler 2021-05-23 12:44:36 PDT 
Comment on attachment 429449 [details]
Patch

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

> Source/WTF/wtf/Locker.h:59
> +constexpr AdoptLockTag AdoptLock = AdoptLockTag();

I am surprised that we need the "= AdoptLockTag()" here. Can we omit it?
Comment 17 Chris Dumez 2021-05-23 12:53:14 PDT 
(In reply to Darin Adler from comment #16)
> Comment on attachment 429449 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=429449&action=review
> 
> > Source/WTF/wtf/Locker.h:59
> > +constexpr AdoptLockTag AdoptLock = AdoptLockTag();
> 
> I am surprised that we need the "= AdoptLockTag()" here. Can we omit it?

Will check and land without it if it builds. I had copied the definition from <mutex>:
/* _LIBCPP_INLINE_VAR */ constexpr adopt_lock_t  adopt_lock  = adopt_lock_t();
Comment 18 Chris Dumez 2021-05-23 12:55:44 PDT 
Committed r277932 (238062@main): <https://commits.webkit.org/238062@main>
Comment 19 Radar WebKit Bug Importer 2021-05-23 12:56:30 PDT 
<rdar://problem/78371802>
Comment 20 Chris Dumez 2021-05-23 12:56:37 PDT 
(In reply to Chris Dumez from comment #17)
> (In reply to Darin Adler from comment #16)
> > Comment on attachment 429449 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=429449&action=review
> > 
> > > Source/WTF/wtf/Locker.h:59
> > > +constexpr AdoptLockTag AdoptLock = AdoptLockTag();
> > 
> > I am surprised that we need the "= AdoptLockTag()" here. Can we omit it?
> 
> Will check and land without it if it builds. I had copied the definition
> from <mutex>:
> /* _LIBCPP_INLINE_VAR */ constexpr adopt_lock_t  adopt_lock  =
> adopt_lock_t();

Yes, it built fine without it.