Bug 225935 – Use WTF::Locker for locking BaseAudioContext's graph lock

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Audio

[Chris Dumez]

Use WTF::Locker for locking BaseAudioContext's graph lock instead of our own AutoLocker.

[Chris Dumez]

Created attachment 428986 [details]
Patch

[Sam Weinig]

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.

[Chris Dumez]

(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?

[Chris Dumez]

(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.

[Sam Weinig]

(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.

[Chris Dumez]

(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()?

[Sam Weinig]

(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?

[Sam Weinig]

(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.

[Chris Dumez]

(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?

[Chris Dumez]

Created attachment 428997 [details]
Patch

[Sam Weinig]

(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 };

[Sam Weinig]

> 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

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/78184315>