Bug 226131 - Use CheckedLock in SpeechRecognitionCaptureSourceImpl
Summary: Use CheckedLock in SpeechRecognitionCaptureSourceImpl
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-05-21 19:09 PDT by Chris Dumez
Modified: 2021-05-25 16:06 PDT (History)
9 users (show)

See Also:


Attachments
Patch (8.35 KB, patch)
2021-05-21 19:16 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (8.39 KB, patch)
2021-05-21 20:17 PDT, Chris Dumez
darin: review+
Details | Formatted Diff | Diff
Patch for landing (8.20 KB, patch)
2021-05-21 22:10 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (7.70 KB, patch)
2021-05-22 09:52 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-21 19:09:50 PDT
Use CheckedLock in SpeechRecognitionCaptureSourceImpl to benefit from Clang Thread Safety Analysis.
Comment 1 Chris Dumez 2021-05-21 19:16:00 PDT
Created attachment 429389 [details]
Patch
Comment 2 Chris Dumez 2021-05-21 20:17:02 PDT
Created attachment 429395 [details]
Patch
Comment 3 Darin Adler 2021-05-21 22:03:49 PDT
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
Comment 4 Chris Dumez 2021-05-21 22:06:26 PDT
(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.
Comment 5 Chris Dumez 2021-05-21 22:10:27 PDT
Created attachment 429402 [details]
Patch for landing
Comment 6 Chris Dumez 2021-05-21 22:26:52 PDT
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.
Comment 7 Chris Dumez 2021-05-22 09:52:37 PDT
Created attachment 429415 [details]
Patch
Comment 8 EWS 2021-05-22 13:10:48 PDT
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].
Comment 9 Radar WebKit Bug Importer 2021-05-22 13:11:16 PDT
<rdar://problem/78354801>