Bug 226131 – Use CheckedLock in SpeechRecognitionCaptureSourceImpl

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Chris Dumez]

Use CheckedLock in SpeechRecognitionCaptureSourceImpl to benefit from Clang Thread Safety Analysis.

[Chris Dumez]

Created attachment 429389 [details]
Patch

[Chris Dumez]

Created attachment 429395 [details]
Patch

[Darin Adler]

Comment on attachment 429395 [details]
Patch

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

I’m not sure I have the skills to know that the WTF_IGNORES_THREAD_SAFETY_ANALYSIS is safe because we must not lock on the audio thread, but to understand why locking in updateDataSource is also OK. But I will take your word for it. Refactoring looks like it was done correctly without introducing errors.

> Source/WebCore/Modules/speech/SpeechRecognitionCaptureSourceImpl.cpp:128
> +// This function uses m_dataSource without locking. It is safe becuase this function only get called on the audio thread and m_dataSource only gets modified on the

Typo in because

[Chris Dumez]

(In reply to Darin Adler from comment #3)
> Comment on attachment 429395 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=429395&action=review
> 
> I’m not sure I have the skills to know that the
> WTF_IGNORES_THREAD_SAFETY_ANALYSIS is safe because we must not lock on the
> audio thread, but to understand why locking in updateDataSource is also OK.
> But I will take your word for it. Refactoring looks like it was done
> correctly without introducing errors.

tryLock() on the audio thread is OK, lock() isn't. We never want to be have the audio thread wait on a lock.

> 
> > Source/WebCore/Modules/speech/SpeechRecognitionCaptureSourceImpl.cpp:128
> > +// This function uses m_dataSource without locking. It is safe becuase this function only get called on the audio thread and m_dataSource only gets modified on the
> 
> Typo in because

Will fix.

[Chris Dumez]

Created attachment 429402 [details]
Patch for landing

[Chris Dumez]

For the record, the reason I think the current code is thread-safe is that:
- m_dataSource pointer only gets set on the audio thread (with a lock)
- we always grab the lock when using m_dataSource pointer on the main thread
- We don't grab the lock when *using* m_dataSource pointer on the audio thread but I believe this is fine given that the pointer only gets set on the same audio thread.

So at least, for the pointer access point of view, I believe we are OK. Then, comes the other question, is it OK to call AudioSampleDataSource::pushSamples() & AudioSampleDataSource::pullSamples() on different threads without synchronization. This part is a bit harder. AudioSampleDataSource is using a CARingBuffer and my understanding is that it is safe to push & push on different threads on a CARingBuffer.

However, AudioSampleDataSource has some additional logic besides using the CARingBuffer and that seems a lot more fragile. Both push & pull seem to be using m_inputSampleOffset & m_lastPushedSampleCount without synchronization for e.g.
I am curious what our media experts think about that.

The patch definitely doesn't make things less safe. But since I had to use WTF_IGNORES_THREAD_SAFETY_ANALYSIS, it is hard to guarantee that the code was thread-safe in the first place.

[Chris Dumez]

Created attachment 429415 [details]
Patch

[EWS]

Committed r277921 (238054@main): <https://commits.webkit.org/238054@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/78354801>