Bug 224688 – [GPUProcess] Crash under RemoteAudioDestination::render()

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

[Chris Dumez]

Crash under RemoteAudioDestination::render():
Thread[3] EXC_BAD_ACCESS (SIGSEGV) (KERN_INVALID_ADDRESS at 0x0000000000000000)
[  0] 0x00007ff917d41ec0 WebCore`WebCore::CARingBuffer::fetchIfHasEnoughData(AudioBufferList*, unsigned long, unsigned long long, WebCore::CARingBuffer::FetchMode) [inlined] std::__1::unique_ptr<WebCore::CARingBufferStorage, std::__1::default_delete<WebCore::CARingBufferStorage> >::get() const at memory:2361:19

     0x00007ff917d41eb4:     movq %rcx, %r13
     0x00007ff917d41eb7:     movq %rdx, %r15
     0x00007ff917d41eba:     movq %rsi, %r12
     0x00007ff917d41ebd:     movq %rdi, %rbx
 ->  0x00007ff917d41ec0:     movq (%rdi), %rdi
     0x00007ff917d41ec3:     movq (%rdi), %rax
     0x00007ff917d41ec6:    callq *0x38(%rax)
     0x00007ff917d41ec9:     movq (%rbx), %rdi
     0x00007ff917d41ecc:     movq (%rdi), %rax

[  0] 0x00007ff917d41ec0 WebCore`WebCore::CARingBuffer::fetchIfHasEnoughData(AudioBufferList*, unsigned long, unsigned long long, WebCore::CARingBuffer::FetchMode) [inlined] WTF::UniqueRef<WebCore::CARingBufferStorage>::operator->() at UniqueRef.h:70
[  0] 0x00007ff917d41ec0 WebCore`WebCore::CARingBuffer::fetchIfHasEnoughData(AudioBufferList*, unsigned long, unsigned long long, WebCore::CARingBuffer::FetchMode) [inlined] WebCore::CARingBuffer::updateFrameBounds() at CARingBuffer.cpp:329
       325 	}
       326 	
       327 	void CARingBuffer::updateFrameBounds()
       328 	{
    -> 329 	    m_buffers->updateFrameBounds();
       330 	}
       331 	
       332 	bool CARingBuffer::fetchIfHasEnoughData(AudioBufferList* list, size_t frameCount, uint64_t startFrame, FetchMode mode)
       333 	{
    
[  0] 0x00007ff917d41ec0 WebCore`WebCore::CARingBuffer::fetchIfHasEnoughData(AudioBufferList*, unsigned long, unsigned long long, WebCore::CARingBuffer::FetchMode) [inlined] WebCore::CARingBuffer::getCurrentFrameBounds(unsigned long long&, unsigned long long&) at CARingBuffer.cpp:269
       265 	}
       266 	
       267 	void CARingBuffer::getCurrentFrameBounds(uint64_t& startFrame, uint64_t& endFrame)
       268 	{
    -> 269 	    updateFrameBounds();
       270 	    getCurrentFrameBoundsWithoutUpdate(startFrame, endFrame);
       271 	}
       272 	
       273 	void CARingBuffer::getCurrentFrameBoundsWithoutUpdate(uint64_t& startFrame, uint64_t& endFrame)
    
[  0] 0x00007ff917d41ec0 WebCore`WebCore::CARingBuffer::fetchIfHasEnoughData(AudioBufferList*, unsigned long, unsigned long long, WebCore::CARingBuffer::FetchMode) + 32 at CARingBuffer.cpp:336
[  1] 0x00007ff918eb07bc WebKit`WebKit::RemoteAudioDestination::render(double, unsigned long long, unsigned int, AudioBufferList*) + 84 at RemoteAudioDestinationManager.cpp:126:27
[  2] 0x000000010667f217 CoreAudio`ausdk::AUInputElement::PullInput(unsigned int&, AudioTimeStamp const&, unsigned int, unsigned int) [inlined] ausdk::AUInputElement::PullInputWithBufferList(unsigned int&, AudioTimeStamp const&, unsigned int, unsigned int, AudioBufferList&) + 75 at AUInputElement.h:69:15

[Chris Dumez]

<rdar://76643365>

[Chris Dumez]

Created attachment 426263 [details]
Patch

[Peng Liu]

Comment on attachment 426263 [details]
Patch

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

> Source/WebKit/ChangeLog:10
> +        GPUConnectionToWebProcess::didClose() would get called and en up destroying

s/en up/end up/g

> Source/WebKit/ChangeLog:43
> +          was not thread safe. Even if m_isPlaying was atomic, m_isPlaying get set to true

I feel “m_isPlaying" here is the root cause of this crash. But I like your refactoring.

[Chris Dumez]

Created attachment 426268 [details]
Patch

[Chris Dumez]

(In reply to Peng Liu from comment #3)
> Comment on attachment 426263 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=426263&action=review
> 
> > Source/WebKit/ChangeLog:10
> > +        GPUConnectionToWebProcess::didClose() would get called and en up destroying
> 
> s/en up/end up/g
> 
> > Source/WebKit/ChangeLog:43
> > +          was not thread safe. Even if m_isPlaying was atomic, m_isPlaying get set to true
> 
> I feel “m_isPlaying" here is the root cause of this crash. But I like your
> refactoring.

The root cause of this crash is that we weren't calling m_audioOutputUnitAdaptor.stop() in the destructor (or before destroying). There was no way to make this safe using m_isPlaying.

[Peng Liu]

Comment on attachment 426263 [details]
Patch

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

>>> Source/WebKit/ChangeLog:10
>>> +        GPUConnectionToWebProcess::didClose() would get called and en up destroying
>> 
>> s/en up/end up/g
> 
> The root cause of this crash is that we weren't calling m_audioOutputUnitAdaptor.stop() in the destructor (or before destroying). There was no way to make this safe using m_isPlaying.

Hmm, just curious, if we call `m_audioOutputUnitAdaptor.stop()` before destroying and make `m_isPlaying` atomic, will the crash be fixed?

[Chris Dumez]

(In reply to Peng Liu from comment #6)
> Comment on attachment 426263 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=426263&action=review
> 
> >>> Source/WebKit/ChangeLog:10
> >>> +        GPUConnectionToWebProcess::didClose() would get called and en up destroying
> >> 
> >> s/en up/end up/g
> > 
> > The root cause of this crash is that we weren't calling m_audioOutputUnitAdaptor.stop() in the destructor (or before destroying). There was no way to make this safe using m_isPlaying.
> 
> Hmm, just curious, if we call `m_audioOutputUnitAdaptor.stop()` before
> destroying and make `m_isPlaying` atomic, will the crash be fixed?

I you call m_audioOutputUnitAdaptor.stop() before destroying, the crash would be fixed. You would not need to make m_isPlaying atomic to address the crash.

[Chris Dumez]

Comment on attachment 426268 [details]
Patch

Will add a test.

[Chris Dumez]

Created attachment 426284 [details]
Patch

[EWS]

Committed r276188 (236670@main): <https://commits.webkit.org/236670@main>

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