WebCore::AudioSourceProviderAVFObjC::process() is calling MTAudioProcessingTapGetSourceAudio() where the value of the parameter MTAudioProcessingTapRef is NULL. Crashed Thread: 29 AUDeferredRenderer-0x7fe86d4e20 Exception Type: EXC_BAD_ACCESS (SIGSEGV) Exception Codes: KERN_INVALID_ADDRESS at 0x0000000000000010 Exception Note: EXC_CORPSE_NOTIFY Thread 29 Crashed:: AUDeferredRenderer-0x7fe86d4e20 0 com.apple.MediaToolbox 0x7fffa8aabb53 MTAudioProcessingTapGetSourceAudio + 129 (/Library/Caches/com.apple.xbs/Sources/CoreMedia/CoreMedia-1873/Prototypes/MediaConverter/MTAudioProcessingTap.c:869) 1 com.apple.WebCore 0x00007fffac05b580 WebCore::AudioSourceProviderAVFObjC::process(long, unsigned int, AudioBufferList*, long*, unsigned int*) + 64 2 com.apple.MediaToolbox 0x7fffa8aacaab aptap_AudioQueueProcessingTapCallback + 158 (/Library/Caches/com.apple.xbs/Sources/CoreMedia/CoreMedia-1873/Prototypes/MediaConverter/MTAudioProcessingTap.c:466) 3 com.apple.audio.toolbox.AudioToolbox 0x00007fffa450c97f AQClientCallProcessingTap(void*, unsigned int, AudioTimeStamp*, unsigned int*, unsigned int*, AudioBufferList*, int, double, unsigned int, double, unsigned int, unsigned char const*, unsigned int) + 93 4 com.apple.audio.toolbox.AudioToolbox 0x00007fffa45934b0 AQProcessingTap::DoCallout(unsigned int&, AudioTimeStamp&, unsigned int&, AudioBufferList&) + 188 5 com.apple.audio.toolbox.AudioToolbox 0x00007fffa45932af AudioQueueObject::PerformTapInternal(AudioTimeStamp&, unsigned int&, unsigned int&) + 133 6 com.apple.audio.toolbox.AudioToolbox 0x00007fffa45931b5 AudioQueueObject::PerformProcessingTap(int (*)(void*, unsigned int&, AudioTimeStamp const&, unsigned int, AudioBufferList&, double&, unsigned int&), void*, AudioTimeStamp&, unsigned int&, AudioBufferList&, unsigned int&) + 197 7 com.apple.audio.toolbox.AudioToolbox 0x00007fffa459ddd9 MEMixerChannel::TapDownstream(void*, unsigned int*, AudioTimeStamp const*, unsigned int, unsigned int, AudioBufferList*) + 129 8 com.apple.audio.units.Components 0x000000011a51c6b5 AUInputElement::PullInput(unsigned int&, AudioTimeStamp const&, unsigned int, unsigned int) + 185 9 com.apple.audio.units.Components 0x000000011a45f374 AUDeferredRenderer::Produce() + 576 10 com.apple.audio.units.Components 0x000000011a45f0ab AUDeferredRenderer::ProducerThreadProc() + 49 11 com.apple.audio.units.Components 0x000000011a45eaab AUDeferredRenderer::ProducerThreadProc(void*) + 31 12 com.apple.audio.units.Components 0x000000011a532737 CAPThread::Entry(CAPThread*) + 85 13 libsystem_pthread.dylib 0x00007fffb9accc4f _pthread_body + 176 14 libsystem_pthread.dylib 0x00007fffb9accb9f _pthread_start + 274 15 libsystem_pthread.dylib 0x00007fffb9acc3d5 thread_start + 13
<rdar://problem/27446589>
Created attachment 310115 [details] Patch
Comment on attachment 310115 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310115&action=review > Source/WebCore/platform/graphics/avfoundation/AudioSourceProviderAVFObjC.mm:364 > + return; I wonder how we get into this state? We should probably ASSERT in "initCallback" if the passed 'tap' is nullptr, since I don't think this is supposed to happen. It seems like this might happen if 'destroyMix()' is called, and then something attempts to use the AudioSourceProviderAVFObjC again (some queued 'process' callback gets invoked).
Created attachment 310250 [details] Patch
(In reply to Brent Fulgham from comment #3) > Comment on attachment 310115 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=310115&action=review > > > Source/WebCore/platform/graphics/avfoundation/AudioSourceProviderAVFObjC.mm:364 > > + return; > > I wonder how we get into this state? We should probably ASSERT in > "initCallback" if the passed 'tap' is nullptr, since I don't think this is > supposed to happen. > > It seems like this might happen if 'destroyMix()' is called, and then > something attempts to use the AudioSourceProviderAVFObjC again (some queued > 'process' callback gets invoked). Thanks for reviewing! I have added an ASSERT in initCallback, and included a scenario of how this might happen in the changelog.
Created attachment 310251 [details] Patch
Comment on attachment 310251 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310251&action=review I think this looks fine, but I'd like to hear from Jer or Eric before landing. > Source/WebCore/ChangeLog:19 > + will crash. Should we be asserting that the calls are being made on the main thread?
Comment on attachment 310251 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310251&action=review >> Source/WebCore/ChangeLog:19 >> + will crash. > > Should we be asserting that the calls are being made on the main thread? If it's possible to destroy the tap simultaneous to using it on separate threads, then this code might reduce NULL-deref crashes, but might still lead to calling into destroyed objects. > Source/WebCore/platform/graphics/avfoundation/AudioSourceProviderAVFObjC.mm:366 > + if (!m_tap) > + return; > + However, it should be possible to eliminate that possibility by doing something like: RetainPtr<MTAudioProcessingTapRef> tap = m_tap; if (!tap) return; And then: > Source/WebCore/platform/graphics/avfoundation/AudioSourceProviderAVFObjC.mm:369 > OSStatus status = MTAudioProcessingTapGetSourceAudio(m_tap.get(), numberOfFrames, bufferListInOut, flagsOut, &rangeOut, &itemCount); Using tap instead of m_tap in this call. This is an unfortunate amount of ref-count churn, but the only other solution here would be to add a bunch of locks, which have their own performance problems.
Created attachment 310267 [details] Patch
(In reply to Jer Noble from comment #8) > Comment on attachment 310251 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=310251&action=review > > >> Source/WebCore/ChangeLog:19 > >> + will crash. > > > > Should we be asserting that the calls are being made on the main thread? > > If it's possible to destroy the tap simultaneous to using it on separate > threads, then this code might reduce NULL-deref crashes, but might still > lead to calling into destroyed objects. > > > Source/WebCore/platform/graphics/avfoundation/AudioSourceProviderAVFObjC.mm:366 > > + if (!m_tap) > > + return; > > + > > However, it should be possible to eliminate that possibility by doing > something like: > > RetainPtr<MTAudioProcessingTapRef> tap = m_tap; Do we still need locking to make sure this assignment is thread safe? > if (!tap) > return; > > And then: > > > Source/WebCore/platform/graphics/avfoundation/AudioSourceProviderAVFObjC.mm:369 > > OSStatus status = MTAudioProcessingTapGetSourceAudio(m_tap.get(), numberOfFrames, bufferListInOut, flagsOut, &rangeOut, &itemCount); > > Using tap instead of m_tap in this call. > > This is an unfortunate amount of ref-count churn, but the only other > solution here would be to add a bunch of locks, which have their own > performance problems. Thanks for reviewing! I updated the patch.
View in context: https://bugs.webkit.org/attachment.cgi?id=310251&action=review >>>> Source/WebCore/ChangeLog:19 >>>> + will crash. >>> >>> Should we be asserting that the calls are being made on the main thread? >> >> If it's possible to destroy the tap simultaneous to using it on separate threads, then this code might reduce NULL-deref crashes, but might still lead to calling into destroyed objects. > > Do we still need locking to make sure this assignment is thread safe? I would file a follow-up bug to see if we need a blocking mechanism to ensure that the tap isn't freed while in the middle of a process(). We may need to add something.
(In reply to Jer Noble from comment #11) > View in context: > https://bugs.webkit.org/attachment.cgi?id=310251&action=review > > >>>> Source/WebCore/ChangeLog:19 > >>>> + will crash. > >>> > >>> Should we be asserting that the calls are being made on the main thread? > >> > >> If it's possible to destroy the tap simultaneous to using it on separate threads, then this code might reduce NULL-deref crashes, but might still lead to calling into destroyed objects. > > > > Do we still need locking to make sure this assignment is thread safe? > > I would file a follow-up bug to see if we need a blocking mechanism to > ensure that the tap isn't freed while in the middle of a process(). We may > need to add something. I see that the MTAudioProcessingTapRef is passed in the processCallback() function. What do you think about just passing this on to the process() method, and use it there instead of using m_tap? This parameter will not be null even if the main thread sets m_tap to null, and I assume it is retained by MediaToolbox, so it can be used safely?
Comment on attachment 310267 [details] Patch Clearing flags on attachment: 310267 Committed r217005: <http://trac.webkit.org/changeset/217005>
All reviewed patches have been landed. Closing bug.