Summary: | Use CheckedLock in SpeechRecognitionCaptureSourceImpl | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||
Component: | Media | Assignee: | Chris Dumez <cdumez> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | achristensen, darin, eric.carlson, ggaren, jer.noble, sam, sihui_liu, webkit-bug-importer, youennf | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=226247 | ||||||||||||
Attachments: |
|
Description
Chris Dumez
2021-05-21 19:09:50 PDT
Created attachment 429389 [details]
Patch
Created attachment 429395 [details]
Patch
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 (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. Created attachment 429402 [details]
Patch for landing
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. Created attachment 429415 [details]
Patch
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]. |