Bug 226255 – Stop using UncheckedLock in WTF::AutomaticThread

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Chris Dumez]

Stop using UncheckedLock in WTF::AutomaticThread as it is being phased out in favor of Lock, which supports Clang thread safety analysis.

[Chris Dumez]

Created attachment 429725 [details]
Patch

[Chris Dumez]

Created attachment 429726 [details]
Patch

[Chris Dumez]

Created attachment 429727 [details]
Patch

[Keith Miller]

Comment on attachment 429727 [details]
Patch

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

> Source/WTF/wtf/ParallelHelperPool.cpp:189
> +        assertIsHeld(*m_pool.m_lock);

Why not just annotate the poll function? Or does this only work for locks that are a member of the same class as the method?

[Chris Dumez]

Comment on attachment 429727 [details]
Patch

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

>> Source/WTF/wtf/ParallelHelperPool.cpp:189
>> +        assertIsHeld(*m_pool.m_lock);
> 
> Why not just annotate the poll function? Or does this only work for locks that are a member of the same class as the method?

I'll try. I think I already tried that and it didn't work but I don't really remember why.

[Chris Dumez]

Created attachment 429759 [details]
Patch

[Chris Dumez]

(In reply to Keith Miller from comment #4)
> Comment on attachment 429727 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=429727&action=review
> 
> > Source/WTF/wtf/ParallelHelperPool.cpp:189
> > +        assertIsHeld(*m_pool.m_lock);
> 
> Why not just annotate the poll function? Or does this only work for locks
> that are a member of the same class as the method?

Looks like it builds without the assertIsHeld() and the annotation. Made the change.

[Keith Miller]

(In reply to Chris Dumez from comment #7)
> (In reply to Keith Miller from comment #4)
> > Comment on attachment 429727 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=429727&action=review
> > 
> > > Source/WTF/wtf/ParallelHelperPool.cpp:189
> > > +        assertIsHeld(*m_pool.m_lock);
> > 
> > Why not just annotate the poll function? Or does this only work for locks
> > that are a member of the same class as the method?
> 
> Looks like it builds without the assertIsHeld() and the annotation. Made the
> change.

Interesting, I'm surprised it's able to prove that no function changes the m_pool ref. Or maybe it makes the assumption that those values are constant? I guess the same thing would apply to the Box<Lock> that we pass to the AtomaticThread too.

[Chris Dumez]

Created attachment 429767 [details]
Patch

[Chris Dumez]

Comment on attachment 429767 [details]
Patch

Clearing flags on attachment: 429767

Committed r278087 (238164@main): <https://commits.webkit.org/238164@main>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/78517626>