Modified: trunk/Source/WebKit/ChangeLog (276187 => 276188)
--- trunk/Source/WebKit/ChangeLog 2021-04-17 01:08:45 UTC (rev 276187)
+++ trunk/Source/WebKit/ChangeLog 2021-04-17 01:24:04 UTC (rev 276188)
@@ -1,5 +1,57 @@
2021-04-16 Chris Dumez <[email protected]>
+ [GPUProcess] Crash under RemoteAudioDestination::render()
+ https://bugs.webkit.org/show_bug.cgi?id=224688
+ <rdar://76643365>
+
+ Reviewed by Eric Carlson.
+
+ When the connection between the GPUProcess and the WebProcess was severed,
+ GPUConnectionToWebProcess::didClose() would get called and end up destroying
+ the RemoteAudioDestination object on the main thread. The issue is that the
+ RemoteAudioDestination may be playing at the time and we would end up
+ destroying the RemoteAudioDestination object without stopping rendering
+ first. As a result, we would crash on the background thread in the
+ RemoteAudioDestination::render() function, trying to use the m_ringBuffer
+ data member that got destroyed on the main thread.
+
+ To address this, I updated the RemoteAudioDestination destructor so that it
+ stops rendering if necessary. AudioOutputUnitStop() is synchronous so this
+ ensures render() is done running on the background thread (and won't be
+ called again) before proceeding with the destruction of the data members.
+
+ Test: webaudio/AudioContext/audiocontext-destruction-crash.html
+
+ * GPUProcess/media/RemoteAudioDestinationManager.cpp:
+ Updated the class to stop subclassing ThreadSafeRefCounted. This class does
+ not need RefCounting at all. I updated the call site to use UniqueRef<>.
+
+ (WebKit::RemoteAudioDestination::create): Deleted.
+ Drop this factory function and made the constructor public now that we no longer
+ subclass ThreadSafeRefCounted and use makeUniqueRef<>() at the call site.
+
+ (WebKit::RemoteAudioDestination::scheduleGracefulShutdownIfNeeded): Deleted.
+ Stop this function now that the destructor takes care of shutting down gracefully.
+
+ (WebKit::RemoteAudioDestination::RemoteAudioDestination):
+ Made the constructor public.
+
+ (WebKit::RemoteAudioDestination::render):
+ - Stop checking m_protectThisDuringGracefulShutdown on the background thread. This data
+ member is not needed since stop() is synchronous. It was also not thread-safe since
+ m_protectThisDuringGracefulShutdown was set on the main thread and we are on the
+ audio thread here.
+ - Similarly, drop the check for m_isPlaying. m_isPlaying is not atomic so the check
+ was not thread safe. Even if m_isPlaying was atomic, m_isPlaying get set to true
+ *after* calling m_audioOutputUnitAdaptor.start() so render() may early return
+ even though we were playing. Also, this check is not needed since we set
+ m_isPlaying to false after calling m_audioOutputUnitAdaptor.stop() and the stop()
+ call is synchronous and should not return until the audio thread stopped rendering.
+
+ * GPUProcess/media/RemoteAudioDestinationManager.h:
+
+2021-04-16 Chris Dumez <[email protected]>
+
The RemoteRemoteCommandListener destructor should never (re-)launch the GPUProcess
https://bugs.webkit.org/show_bug.cgi?id=224701
Modified: trunk/Source/WebKit/GPUProcess/media/RemoteAudioDestinationManager.cpp (276187 => 276188)
--- trunk/Source/WebKit/GPUProcess/media/RemoteAudioDestinationManager.cpp 2021-04-17 01:08:45 UTC (rev 276187)
+++ trunk/Source/WebKit/GPUProcess/media/RemoteAudioDestinationManager.cpp 2021-04-17 01:24:04 UTC (rev 276188)
@@ -43,27 +43,33 @@
namespace WebKit {
-class RemoteAudioDestination
- : public ThreadSafeRefCounted<RemoteAudioDestination>
+class RemoteAudioDestination final
#if PLATFORM(COCOA)
- , public WebCore::AudioUnitRenderer
+ : public WebCore::AudioUnitRenderer
#endif
{
+ WTF_MAKE_FAST_ALLOCATED;
public:
- static Ref<RemoteAudioDestination> create(GPUConnectionToWebProcess& connection, RemoteAudioDestinationIdentifier identifier,
- const String& inputDeviceId, uint32_t numberOfInputChannels, uint32_t numberOfOutputChannels, float sampleRate, float hardwareSampleRate, IPC::Semaphore&& renderSemaphore)
+ RemoteAudioDestination(GPUConnectionToWebProcess&, RemoteAudioDestinationIdentifier identifier, const String& inputDeviceId, uint32_t numberOfInputChannels, uint32_t numberOfOutputChannels, float sampleRate, float hardwareSampleRate, IPC::Semaphore&& renderSemaphore)
+ : m_id(identifier)
+#if PLATFORM(COCOA)
+ , m_audioOutputUnitAdaptor(*this)
+ , m_ringBuffer(makeUniqueRef<WebCore::CARingBuffer>())
+#endif
+ , m_renderSemaphore(WTFMove(renderSemaphore))
{
- return adoptRef(*new RemoteAudioDestination(connection, identifier, inputDeviceId, numberOfInputChannels, numberOfOutputChannels, sampleRate, hardwareSampleRate, WTFMove(renderSemaphore)));
+ ASSERT(isMainRunLoop());
+#if PLATFORM(COCOA)
+ m_audioOutputUnitAdaptor.configure(hardwareSampleRate, numberOfOutputChannels);
+#endif
}
- virtual ~RemoteAudioDestination() = default;
-
- void scheduleGracefulShutdownIfNeeded()
+ ~RemoteAudioDestination()
{
- if (!m_isPlaying)
- return;
- m_protectThisDuringGracefulShutdown = this;
- stop();
+ ASSERT(isMainRunLoop());
+ // Make sure we stop audio rendering and wait for it to finish before destruction.
+ if (m_isPlaying)
+ stop();
}
#if PLATFORM(COCOA)
@@ -90,11 +96,6 @@
return;
m_isPlaying = false;
-
- if (m_protectThisDuringGracefulShutdown) {
- RELEASE_ASSERT(refCount() == 1);
- m_protectThisDuringGracefulShutdown = nullptr;
- }
#endif
}
@@ -101,29 +102,12 @@
bool isPlaying() const { return m_isPlaying; }
private:
- RemoteAudioDestination(GPUConnectionToWebProcess&, RemoteAudioDestinationIdentifier identifier, const String& inputDeviceId, uint32_t numberOfInputChannels, uint32_t numberOfOutputChannels, float sampleRate, float hardwareSampleRate, IPC::Semaphore&& renderSemaphore)
- : m_id(identifier)
#if PLATFORM(COCOA)
- , m_audioOutputUnitAdaptor(*this)
- , m_ringBuffer(makeUniqueRef<WebCore::CARingBuffer>())
-#endif
- , m_renderSemaphore(WTFMove(renderSemaphore))
- {
-#if PLATFORM(COCOA)
- m_audioOutputUnitAdaptor.configure(hardwareSampleRate, numberOfOutputChannels);
-#endif
- }
-
-#if PLATFORM(COCOA)
OSStatus render(double sampleTime, uint64_t hostTime, UInt32 numberOfFrames, AudioBufferList* ioData)
{
ASSERT(!isMainRunLoop());
OSStatus status = -1;
-
- if (m_protectThisDuringGracefulShutdown || !m_isPlaying)
- return status;
-
if (m_ringBuffer->fetchIfHasEnoughData(ioData, numberOfFrames, m_startFrame)) {
m_startFrame += numberOfFrames;
status = noErr;
@@ -140,8 +124,6 @@
RemoteAudioDestinationIdentifier m_id;
- RefPtr<RemoteAudioDestination> m_protectThisDuringGracefulShutdown;
-
#if PLATFORM(COCOA)
WebCore::AudioOutputUnitAdaptor m_audioOutputUnitAdaptor;
@@ -163,16 +145,14 @@
void RemoteAudioDestinationManager::createAudioDestination(const String& inputDeviceId, uint32_t numberOfInputChannels, uint32_t numberOfOutputChannels, float sampleRate, float hardwareSampleRate, IPC::Semaphore&& renderSemaphore, CompletionHandler<void(const WebKit::RemoteAudioDestinationIdentifier)>&& completionHandler)
{
auto newID = RemoteAudioDestinationIdentifier::generateThreadSafe();
- auto destination = RemoteAudioDestination::create(m_gpuConnectionToWebProcess, newID, inputDeviceId, numberOfInputChannels, numberOfOutputChannels, sampleRate, hardwareSampleRate, WTFMove(renderSemaphore));
- m_audioDestinations.add(newID, destination.copyRef());
+ auto destination = makeUniqueRef<RemoteAudioDestination>(m_gpuConnectionToWebProcess, newID, inputDeviceId, numberOfInputChannels, numberOfOutputChannels, sampleRate, hardwareSampleRate, WTFMove(renderSemaphore));
+ m_audioDestinations.add(newID, WTFMove(destination));
completionHandler(newID);
}
void RemoteAudioDestinationManager::deleteAudioDestination(RemoteAudioDestinationIdentifier identifier, CompletionHandler<void()>&& completionHandler)
{
- auto destination = m_audioDestinations.take(identifier);
- if (destination)
- destination->scheduleGracefulShutdownIfNeeded();
+ m_audioDestinations.remove(identifier);
completionHandler();
if (allowsExitUnderMemoryPressure())
Modified: trunk/Source/WebKit/GPUProcess/media/RemoteAudioDestinationManager.h (276187 => 276188)
--- trunk/Source/WebKit/GPUProcess/media/RemoteAudioDestinationManager.h 2021-04-17 01:08:45 UTC (rev 276187)
+++ trunk/Source/WebKit/GPUProcess/media/RemoteAudioDestinationManager.h 2021-04-17 01:24:04 UTC (rev 276188)
@@ -71,7 +71,7 @@
void audioSamplesStorageChanged(RemoteAudioDestinationIdentifier, const SharedMemory::IPCHandle&, const WebCore::CAAudioStreamDescription&, uint64_t numberOfFrames);
#endif
- HashMap<RemoteAudioDestinationIdentifier, Ref<RemoteAudioDestination>> m_audioDestinations;
+ HashMap<RemoteAudioDestinationIdentifier, UniqueRef<RemoteAudioDestination>> m_audioDestinations;
GPUConnectionToWebProcess& m_gpuConnectionToWebProcess;
};
