Use CheckedLock more in cases where we try-lock.
Created attachment 429243 [details] Patch
Created attachment 429247 [details] Patch
Created attachment 429258 [details] Patch
Created attachment 429259 [details] Patch
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) ?
(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.
(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.
Created attachment 429290 [details] Patch
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
(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.
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.
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].
<rdar://problem/78322591>