Bug 226056 – Use CheckedLock more in cases where we try-lock

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Chris Dumez]

Use CheckedLock more in cases where we try-lock.

[Chris Dumez]

Created attachment 429243 [details]
Patch

[Chris Dumez]

Created attachment 429247 [details]
Patch

[Chris Dumez]

Created attachment 429258 [details]
Patch

[Chris Dumez]

Created attachment 429259 [details]
Patch

[Kimmo Kinnunen]

Comment on attachment 429259 [details]
Patch

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

> Source/WebCore/Modules/speech/SpeechRecognitionCaptureSourceImpl.h:75
>      RefPtr<AudioSampleDataSource> m_dataSource;

Are we able to use WTF_GUARDED_BY_LOCK(m_dataSourceLock) ?

[Chris Dumez]

(In reply to Kimmo Kinnunen from comment #5)
> Comment on attachment 429259 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=429259&action=review
> 
> > Source/WebCore/Modules/speech/SpeechRecognitionCaptureSourceImpl.h:75
> >      RefPtr<AudioSampleDataSource> m_dataSource;
> 
> Are we able to use WTF_GUARDED_BY_LOCK(m_dataSourceLock) ?

Probably, my bet is that I just inadvertently missed it since I refactored the code to support it. I'll try and add it and build to make sure.

[Chris Dumez]

(In reply to Chris Dumez from comment #6)
> (In reply to Kimmo Kinnunen from comment #5)
> > Comment on attachment 429259 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=429259&action=review
> > 
> > > Source/WebCore/Modules/speech/SpeechRecognitionCaptureSourceImpl.h:75
> > >      RefPtr<AudioSampleDataSource> m_dataSource;
> > 
> > Are we able to use WTF_GUARDED_BY_LOCK(m_dataSourceLock) ?
> 
> Probably, my bet is that I just inadvertently missed it since I refactored
> the code to support it. I'll try and add it and build to make sure.

Actually, I'll revert the changes to this file for now. Not all call paths grab the lock before using m_dataSource. I am not convinced this code is actually thread safe. It will likely require its own patch.

[Chris Dumez]

Created attachment 429290 [details]
Patch

[Alex Christensen]

Comment on attachment 429290 [details]
Patch

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

> Source/WTF/wtf/Logger.h:359
> -        auto lock = tryHoldLock(observerLock());
> -        if (!lock)
> +        if (!observerLock().tryLock())
>              return;
>  
> +        Locker locker { AdoptLockTag { }, observerLock() };

Can we not use something similar to tryHoldLock?  I'm not sure if this does the same thing, but to me it looks like we're giving something else a chance to get the lock between the call to tryLock and observerLock

[Chris Dumez]

(In reply to Alex Christensen from comment #9)
> Comment on attachment 429290 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=429290&action=review
> 
> > Source/WTF/wtf/Logger.h:359
> > -        auto lock = tryHoldLock(observerLock());
> > -        if (!lock)
> > +        if (!observerLock().tryLock())
> >              return;
> >  
> > +        Locker locker { AdoptLockTag { }, observerLock() };
> 
> Can we not use something similar to tryHoldLock?  I'm not sure if this does
> the same thing, but to me it looks like we're giving something else a chance
> to get the lock between the call to tryLock and observerLock

No, tryHoldLock and TryLocker-types objects currently don't work with Clang thread-safety analysis unfortunately. I agree that the new code is not as nice but at least we get Clang's thread-safety analysis. As far as I can tell though, there is no race here. I do not understand your concern on that front. The new code is just as safe AFAICT.

[Alex Christensen]

Comment on attachment 429290 [details]
Patch

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

>>> Source/WTF/wtf/Logger.h:359
>>> +        Locker locker { AdoptLockTag { }, observerLock() };
>> 
>> Can we not use something similar to tryHoldLock?  I'm not sure if this does the same thing, but to me it looks like we're giving something else a chance to get the lock between the call to tryLock and observerLock
> 
> No, tryHoldLock and TryLocker-types objects currently don't work with Clang thread-safety analysis unfortunately. I agree that the new code is not as nice but at least we get Clang's thread-safety analysis. As far as I can tell though, there is no race here. I do not understand your concern on that front. The new code is just as safe AFAICT.

Now I see.  If the tryLock call returns true then the lock is in a locked state, and then we adopt it with a raii object.  It would be nice if tryLock could return the raii object, but that's not how std::unique_lock works.

[EWS]

Committed r277875 (238013@main): <https://commits.webkit.org/238013@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 429290 [details].

[Radar WebKit Bug Importer]

<rdar://problem/78322591>