Bug 226056 - Use CheckedLock more in cases where we try-lock
Summary: Use CheckedLock more in cases where we try-lock
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-05-20 16:57 PDT by Chris Dumez
Modified: 2021-05-21 12:25 PDT (History)
17 users (show)

See Also:


Attachments
Patch (30.81 KB, patch)
2021-05-20 16:59 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (30.82 KB, patch)
2021-05-20 17:16 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (25.89 KB, patch)
2021-05-20 19:34 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (26.51 KB, patch)
2021-05-20 19:36 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (21.61 KB, patch)
2021-05-21 08:08 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2021-05-20 16:57:29 PDT
Use CheckedLock more in cases where we try-lock.
Comment 1 Chris Dumez 2021-05-20 16:59:17 PDT
Created attachment 429243 [details]
Patch
Comment 2 Chris Dumez 2021-05-20 17:16:28 PDT
Created attachment 429247 [details]
Patch
Comment 3 Chris Dumez 2021-05-20 19:34:30 PDT
Created attachment 429258 [details]
Patch
Comment 4 Chris Dumez 2021-05-20 19:36:18 PDT
Created attachment 429259 [details]
Patch
Comment 5 Kimmo Kinnunen 2021-05-20 23:02:07 PDT
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) ?
Comment 6 Chris Dumez 2021-05-21 07:41:08 PDT
(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.
Comment 7 Chris Dumez 2021-05-21 08:06:03 PDT
(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.
Comment 8 Chris Dumez 2021-05-21 08:08:38 PDT
Created attachment 429290 [details]
Patch
Comment 9 Alex Christensen 2021-05-21 09:11:41 PDT
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
Comment 10 Chris Dumez 2021-05-21 09:23:29 PDT
(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 11 Alex Christensen 2021-05-21 12:03:15 PDT
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.
Comment 12 EWS 2021-05-21 12:24:14 PDT
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].
Comment 13 Radar WebKit Bug Importer 2021-05-21 12:25:20 PDT
<rdar://problem/78322591>