Use WTF::Locker for locking BaseAudioContext's graph lock instead of our own AutoLocker.
Created attachment 428986 [details] Patch
Comment on attachment 428986 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428986&action=review > Source/WebCore/Modules/webaudio/AudioBufferSourceNode.cpp:419 > + auto contextLocker = holdLock(context().graphLock()); I believe we are trying to move away from holdLock() and just use: Locker locker { context().graphLock() }; because holdLock does not play nice with ThreadSafetyAnalysis. We really should just do a global replace of holdLock() to get this over with.
(In reply to Sam Weinig from comment #2) > Comment on attachment 428986 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=428986&action=review > > > Source/WebCore/Modules/webaudio/AudioBufferSourceNode.cpp:419 > > + auto contextLocker = holdLock(context().graphLock()); > > I believe we are trying to move away from holdLock() and just use: > > Locker locker { context().graphLock() }; > > because holdLock does not play nice with ThreadSafetyAnalysis. > > We really should just do a global replace of holdLock() to get this over > with. holdLock() looks nicer and there is no harm in using it here since we're not using a Lock type that supports static analysis. My understanding was that we just can use holdLock() with StaticLock but it was OK to use holdLock() for other types of locks?
(In reply to Chris Dumez from comment #3) > (In reply to Sam Weinig from comment #2) > > Comment on attachment 428986 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=428986&action=review > > > > > Source/WebCore/Modules/webaudio/AudioBufferSourceNode.cpp:419 > > > + auto contextLocker = holdLock(context().graphLock()); > > > > I believe we are trying to move away from holdLock() and just use: > > > > Locker locker { context().graphLock() }; > > > > because holdLock does not play nice with ThreadSafetyAnalysis. > > > > We really should just do a global replace of holdLock() to get this over > > with. > > holdLock() looks nicer and there is no harm in using it here since we're not > using a Lock type that supports static analysis. My understanding was that > we just can use holdLock() with StaticLock but it was OK to use holdLock() > for other types of locks? Also note that it wouldn't be: Locker locker { context().graphLock() }; It would be: Locker<RecursiveLock> locker { context().graphLock() }; Which would look even worse. I much prefer using auto that having to name the lock type.
(In reply to Chris Dumez from comment #3) > (In reply to Sam Weinig from comment #2) > > Comment on attachment 428986 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=428986&action=review > > > > > Source/WebCore/Modules/webaudio/AudioBufferSourceNode.cpp:419 > > > + auto contextLocker = holdLock(context().graphLock()); > > > > I believe we are trying to move away from holdLock() and just use: > > > > Locker locker { context().graphLock() }; > > > > because holdLock does not play nice with ThreadSafetyAnalysis. > > > > We really should just do a global replace of holdLock() to get this over > > with. > > holdLock() looks nicer and there is no harm in using it here since we're not > using a Lock type that supports static analysis. My understanding was that > we just can use holdLock() with StaticLock but it was OK to use holdLock() > for other types of locks? I think the only reason CheckedLock exists is that we holdLock. There is comment that states: // FIXME: Maybe should be folded back to Lock once holdLock is not used globally.
(In reply to Sam Weinig from comment #5) > (In reply to Chris Dumez from comment #3) > > (In reply to Sam Weinig from comment #2) > > > Comment on attachment 428986 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=428986&action=review > > > > > > > Source/WebCore/Modules/webaudio/AudioBufferSourceNode.cpp:419 > > > > + auto contextLocker = holdLock(context().graphLock()); > > > > > > I believe we are trying to move away from holdLock() and just use: > > > > > > Locker locker { context().graphLock() }; > > > > > > because holdLock does not play nice with ThreadSafetyAnalysis. > > > > > > We really should just do a global replace of holdLock() to get this over > > > with. > > > > holdLock() looks nicer and there is no harm in using it here since we're not > > using a Lock type that supports static analysis. My understanding was that > > we just can use holdLock() with StaticLock but it was OK to use holdLock() > > for other types of locks? > > I think the only reason CheckedLock exists is that we holdLock. There is > comment that states: > > // FIXME: Maybe should be folded back to Lock once holdLock is not used > globally. And what are you proposing for tryLock()? Keep using tryHoldLock()?
(In reply to Chris Dumez from comment #4) > (In reply to Chris Dumez from comment #3) > > (In reply to Sam Weinig from comment #2) > > > Comment on attachment 428986 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=428986&action=review > > > > > > > Source/WebCore/Modules/webaudio/AudioBufferSourceNode.cpp:419 > > > > + auto contextLocker = holdLock(context().graphLock()); > > > > > > I believe we are trying to move away from holdLock() and just use: > > > > > > Locker locker { context().graphLock() }; > > > > > > because holdLock does not play nice with ThreadSafetyAnalysis. > > > > > > We really should just do a global replace of holdLock() to get this over > > > with. > > > > holdLock() looks nicer and there is no harm in using it here since we're not > > using a Lock type that supports static analysis. My understanding was that > > we just can use holdLock() with StaticLock but it was OK to use holdLock() > > for other types of locks? > > Also note that it wouldn't be: > Locker locker { context().graphLock() }; > > It would be: > Locker<RecursiveLock> locker { context().graphLock() }; > > Which would look even worse. I much prefer using auto that having to name > the lock type. Why do you have to specify RecursiveLock here? Is the argument deduction not working?
(In reply to Chris Dumez from comment #6) > (In reply to Sam Weinig from comment #5) > > (In reply to Chris Dumez from comment #3) > > > (In reply to Sam Weinig from comment #2) > > > > Comment on attachment 428986 [details] > > > > Patch > > > > > > > > View in context: > > > > https://bugs.webkit.org/attachment.cgi?id=428986&action=review > > > > > > > > > Source/WebCore/Modules/webaudio/AudioBufferSourceNode.cpp:419 > > > > > + auto contextLocker = holdLock(context().graphLock()); > > > > > > > > I believe we are trying to move away from holdLock() and just use: > > > > > > > > Locker locker { context().graphLock() }; > > > > > > > > because holdLock does not play nice with ThreadSafetyAnalysis. > > > > > > > > We really should just do a global replace of holdLock() to get this over > > > > with. > > > > > > holdLock() looks nicer and there is no harm in using it here since we're not > > > using a Lock type that supports static analysis. My understanding was that > > > we just can use holdLock() with StaticLock but it was OK to use holdLock() > > > for other types of locks? > > > > I think the only reason CheckedLock exists is that we holdLock. There is > > comment that states: > > > > // FIXME: Maybe should be folded back to Lock once holdLock is not used > > globally. > > And what are you proposing for tryLock()? Keep using tryHoldLock()? I guess I would recommend we add a TryLocker class or something, not sure. But since this doesn't seem as settled as I thought it was, perhaps we should continue with holdLock/tryHoldLock for now.
(In reply to Sam Weinig from comment #7) > (In reply to Chris Dumez from comment #4) > > (In reply to Chris Dumez from comment #3) > > > (In reply to Sam Weinig from comment #2) > > > > Comment on attachment 428986 [details] > > > > Patch > > > > > > > > View in context: > > > > https://bugs.webkit.org/attachment.cgi?id=428986&action=review > > > > > > > > > Source/WebCore/Modules/webaudio/AudioBufferSourceNode.cpp:419 > > > > > + auto contextLocker = holdLock(context().graphLock()); > > > > > > > > I believe we are trying to move away from holdLock() and just use: > > > > > > > > Locker locker { context().graphLock() }; > > > > > > > > because holdLock does not play nice with ThreadSafetyAnalysis. > > > > > > > > We really should just do a global replace of holdLock() to get this over > > > > with. > > > > > > holdLock() looks nicer and there is no harm in using it here since we're not > > > using a Lock type that supports static analysis. My understanding was that > > > we just can use holdLock() with StaticLock but it was OK to use holdLock() > > > for other types of locks? > > > > Also note that it wouldn't be: > > Locker locker { context().graphLock() }; > > > > It would be: > > Locker<RecursiveLock> locker { context().graphLock() }; > > > > Which would look even worse. I much prefer using auto that having to name > > the lock type. > > Why do you have to specify RecursiveLock here? Is the argument deduction not > working? My bad, I am surprised but `Locker locker { context().graphLock() };` appears to be compiling fine. I thought this only used with a CheckedLock for some reason. For tryHoldLock(), it looks like one alternative would be: auto locker = Locker<RecursiveLock>::tryLock(context().graphLock()); Should I adopt that?
Created attachment 428997 [details] Patch
(In reply to Chris Dumez from comment #9) > (In reply to Sam Weinig from comment #7) > > (In reply to Chris Dumez from comment #4) > > > (In reply to Chris Dumez from comment #3) > > > > (In reply to Sam Weinig from comment #2) > > > > > Comment on attachment 428986 [details] > > > > > Patch > > > > > > > > > > View in context: > > > > > https://bugs.webkit.org/attachment.cgi?id=428986&action=review > > > > > > > > > > > Source/WebCore/Modules/webaudio/AudioBufferSourceNode.cpp:419 > > > > > > + auto contextLocker = holdLock(context().graphLock()); > > > > > > > > > > I believe we are trying to move away from holdLock() and just use: > > > > > > > > > > Locker locker { context().graphLock() }; > > > > > > > > > > because holdLock does not play nice with ThreadSafetyAnalysis. > > > > > > > > > > We really should just do a global replace of holdLock() to get this over > > > > > with. > > > > > > > > holdLock() looks nicer and there is no harm in using it here since we're not > > > > using a Lock type that supports static analysis. My understanding was that > > > > we just can use holdLock() with StaticLock but it was OK to use holdLock() > > > > for other types of locks? > > > > > > Also note that it wouldn't be: > > > Locker locker { context().graphLock() }; > > > > > > It would be: > > > Locker<RecursiveLock> locker { context().graphLock() }; > > > > > > Which would look even worse. I much prefer using auto that having to name > > > the lock type. > > > > Why do you have to specify RecursiveLock here? Is the argument deduction not > > working? > > My bad, I am surprised but `Locker locker { context().graphLock() };` > appears to be compiling fine. I thought this only used with a CheckedLock > for some reason. > > For tryHoldLock(), it looks like one alternative would be: > auto locker = Locker<RecursiveLock>::tryLock(context().graphLock()); > > Should I adopt that? That looks good for now. I will add something to make this nicer soon and fix this up. Probably make it something like: Locker locker { context().graphLock(), Locker::Try };
> My bad, I am surprised but `Locker locker { context().graphLock() };` > appears to be compiling fine. I thought this only used with a CheckedLock > for some reason. The wonders of CTAD! https://en.cppreference.com/w/cpp/language/class_template_argument_deduction
Committed r277709 (237890@main): <https://commits.webkit.org/237890@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 428997 [details].
<rdar://problem/78184315>