Skip to content

Commit b31c920

Browse files
cdumezcdumez
committed
[GPUProcess] Crash under RemoteAudioDestination::render()
https://bugs.webkit.org/show_bug.cgi?id=224688 <rdar://76643365> Reviewed by Eric Carlson. Source/WebKit: 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: LayoutTests: Add layout test that can flakily reproduce the issue. * webaudio/AudioContext/audiocontext-destruction-crash-expected.txt: Added. * webaudio/AudioContext/audiocontext-destruction-crash.html: Added. Canonical link: https://commits.webkit.org/236670@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@276188 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent 3d33c9b commit b31c920

6 files changed

Lines changed: 110 additions & 43 deletions

File tree

‎LayoutTests/ChangeLog‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,16 @@
1+
2021-04-16 Chris Dumez <cdumez@apple.com>
2+
3+
[GPUProcess] Crash under RemoteAudioDestination::render()
4+
https://bugs.webkit.org/show_bug.cgi?id=224688
5+
<rdar://76643365>
6+
7+
Reviewed by Eric Carlson.
8+
9+
Add layout test that can flakily reproduce the issue.
10+
11+
* webaudio/AudioContext/audiocontext-destruction-crash-expected.txt: Added.
12+
* webaudio/AudioContext/audiocontext-destruction-crash.html: Added.
13+
114
2021-04-16 Darin Adler <darin@apple.com>
215

316
font-size with viewport units in calc() doesn't change when viewport resizes

‎LayoutTests/webaudio/AudioContext/audiocontext-destruction-crash-expected.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
This test passes if it does not crash.

‎LayoutTests/webaudio/AudioContext/audiocontext-destruction-crash.html‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
<!DOCTYPE html>
2+
<html>
3+
<body>
4+
<p>This test passes if it does not crash.</p>
5+
<script>
6+
if (window.testRunner) {
7+
testRunner.dumpAsText();
8+
testRunner.waitUntilDone();
9+
}
10+
11+
onload = () => {
12+
for (let i = 0; i < 20; i++)
13+
new AudioContext().createChannelSplitter();
14+
setTimeout(() => {
15+
if (window.testRunner)
16+
testRunner.notifyDone();
17+
}, 10);
18+
};
19+
</script>
20+
</body>
21+
</html>

‎Source/WebKit/ChangeLog‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,55 @@
1+
2021-04-16 Chris Dumez <cdumez@apple.com>
2+
3+
[GPUProcess] Crash under RemoteAudioDestination::render()
4+
https://bugs.webkit.org/show_bug.cgi?id=224688
5+
<rdar://76643365>
6+
7+
Reviewed by Eric Carlson.
8+
9+
When the connection between the GPUProcess and the WebProcess was severed,
10+
GPUConnectionToWebProcess::didClose() would get called and end up destroying
11+
the RemoteAudioDestination object on the main thread. The issue is that the
12+
RemoteAudioDestination may be playing at the time and we would end up
13+
destroying the RemoteAudioDestination object without stopping rendering
14+
first. As a result, we would crash on the background thread in the
15+
RemoteAudioDestination::render() function, trying to use the m_ringBuffer
16+
data member that got destroyed on the main thread.
17+
18+
To address this, I updated the RemoteAudioDestination destructor so that it
19+
stops rendering if necessary. AudioOutputUnitStop() is synchronous so this
20+
ensures render() is done running on the background thread (and won't be
21+
called again) before proceeding with the destruction of the data members.
22+
23+
Test: webaudio/AudioContext/audiocontext-destruction-crash.html
24+
25+
* GPUProcess/media/RemoteAudioDestinationManager.cpp:
26+
Updated the class to stop subclassing ThreadSafeRefCounted. This class does
27+
not need RefCounting at all. I updated the call site to use UniqueRef<>.
28+
29+
(WebKit::RemoteAudioDestination::create): Deleted.
30+
Drop this factory function and made the constructor public now that we no longer
31+
subclass ThreadSafeRefCounted and use makeUniqueRef<>() at the call site.
32+
33+
(WebKit::RemoteAudioDestination::scheduleGracefulShutdownIfNeeded): Deleted.
34+
Stop this function now that the destructor takes care of shutting down gracefully.
35+
36+
(WebKit::RemoteAudioDestination::RemoteAudioDestination):
37+
Made the constructor public.
38+
39+
(WebKit::RemoteAudioDestination::render):
40+
- Stop checking m_protectThisDuringGracefulShutdown on the background thread. This data
41+
member is not needed since stop() is synchronous. It was also not thread-safe since
42+
m_protectThisDuringGracefulShutdown was set on the main thread and we are on the
43+
audio thread here.
44+
- Similarly, drop the check for m_isPlaying. m_isPlaying is not atomic so the check
45+
was not thread safe. Even if m_isPlaying was atomic, m_isPlaying get set to true
46+
*after* calling m_audioOutputUnitAdaptor.start() so render() may early return
47+
even though we were playing. Also, this check is not needed since we set
48+
m_isPlaying to false after calling m_audioOutputUnitAdaptor.stop() and the stop()
49+
call is synchronous and should not return until the audio thread stopped rendering.
50+
51+
* GPUProcess/media/RemoteAudioDestinationManager.h:
52+
153
2021-04-16 Chris Dumez <cdumez@apple.com>
254

355
The RemoteRemoteCommandListener destructor should never (re-)launch the GPUProcess

‎Source/WebKit/GPUProcess/media/RemoteAudioDestinationManager.cpp‎

Lines changed: 22 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -43,27 +43,33 @@
4343

4444
namespace WebKit {
4545

46-
class RemoteAudioDestination
47-
: public ThreadSafeRefCounted<RemoteAudioDestination>
46+
class RemoteAudioDestination final
4847
#if PLATFORM(COCOA)
49-
, public WebCore::AudioUnitRenderer
48+
: public WebCore::AudioUnitRenderer
5049
#endif
5150
{
51+
WTF_MAKE_FAST_ALLOCATED;
5252
public:
53-
static Ref<RemoteAudioDestination> create(GPUConnectionToWebProcess& connection, RemoteAudioDestinationIdentifier identifier,
54-
const String& inputDeviceId, uint32_t numberOfInputChannels, uint32_t numberOfOutputChannels, float sampleRate, float hardwareSampleRate, IPC::Semaphore&& renderSemaphore)
53+
RemoteAudioDestination(GPUConnectionToWebProcess&, RemoteAudioDestinationIdentifier identifier, const String& inputDeviceId, uint32_t numberOfInputChannels, uint32_t numberOfOutputChannels, float sampleRate, float hardwareSampleRate, IPC::Semaphore&& renderSemaphore)
54+
: m_id(identifier)
55+
#if PLATFORM(COCOA)
56+
, m_audioOutputUnitAdaptor(*this)
57+
, m_ringBuffer(makeUniqueRef<WebCore::CARingBuffer>())
58+
#endif
59+
, m_renderSemaphore(WTFMove(renderSemaphore))
5560
{
56-
return adoptRef(*new RemoteAudioDestination(connection, identifier, inputDeviceId, numberOfInputChannels, numberOfOutputChannels, sampleRate, hardwareSampleRate, WTFMove(renderSemaphore)));
61+
ASSERT(isMainRunLoop());
62+
#if PLATFORM(COCOA)
63+
m_audioOutputUnitAdaptor.configure(hardwareSampleRate, numberOfOutputChannels);
64+
#endif
5765
}
5866

59-
virtual ~RemoteAudioDestination() = default;
60-
61-
void scheduleGracefulShutdownIfNeeded()
67+
~RemoteAudioDestination()
6268
{
63-
if (!m_isPlaying)
64-
return;
65-
m_protectThisDuringGracefulShutdown = this;
66-
stop();
69+
ASSERT(isMainRunLoop());
70+
// Make sure we stop audio rendering and wait for it to finish before destruction.
71+
if (m_isPlaying)
72+
stop();
6773
}
6874

6975
#if PLATFORM(COCOA)
@@ -90,40 +96,18 @@ class RemoteAudioDestination
9096
return;
9197

9298
m_isPlaying = false;
93-
94-
if (m_protectThisDuringGracefulShutdown) {
95-
RELEASE_ASSERT(refCount() == 1);
96-
m_protectThisDuringGracefulShutdown = nullptr;
97-
}
9899
#endif
99100
}
100101

101102
bool isPlaying() const { return m_isPlaying; }
102103

103104
private:
104-
RemoteAudioDestination(GPUConnectionToWebProcess&, RemoteAudioDestinationIdentifier identifier, const String& inputDeviceId, uint32_t numberOfInputChannels, uint32_t numberOfOutputChannels, float sampleRate, float hardwareSampleRate, IPC::Semaphore&& renderSemaphore)
105-
: m_id(identifier)
106-
#if PLATFORM(COCOA)
107-
, m_audioOutputUnitAdaptor(*this)
108-
, m_ringBuffer(makeUniqueRef<WebCore::CARingBuffer>())
109-
#endif
110-
, m_renderSemaphore(WTFMove(renderSemaphore))
111-
{
112-
#if PLATFORM(COCOA)
113-
m_audioOutputUnitAdaptor.configure(hardwareSampleRate, numberOfOutputChannels);
114-
#endif
115-
}
116-
117105
#if PLATFORM(COCOA)
118106
OSStatus render(double sampleTime, uint64_t hostTime, UInt32 numberOfFrames, AudioBufferList* ioData)
119107
{
120108
ASSERT(!isMainRunLoop());
121109

122110
OSStatus status = -1;
123-
124-
if (m_protectThisDuringGracefulShutdown || !m_isPlaying)
125-
return status;
126-
127111
if (m_ringBuffer->fetchIfHasEnoughData(ioData, numberOfFrames, m_startFrame)) {
128112
m_startFrame += numberOfFrames;
129113
status = noErr;
@@ -140,8 +124,6 @@ class RemoteAudioDestination
140124

141125
RemoteAudioDestinationIdentifier m_id;
142126

143-
RefPtr<RemoteAudioDestination> m_protectThisDuringGracefulShutdown;
144-
145127
#if PLATFORM(COCOA)
146128
WebCore::AudioOutputUnitAdaptor m_audioOutputUnitAdaptor;
147129

@@ -163,16 +145,14 @@ RemoteAudioDestinationManager::~RemoteAudioDestinationManager() = default;
163145
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)
164146
{
165147
auto newID = RemoteAudioDestinationIdentifier::generateThreadSafe();
166-
auto destination = RemoteAudioDestination::create(m_gpuConnectionToWebProcess, newID, inputDeviceId, numberOfInputChannels, numberOfOutputChannels, sampleRate, hardwareSampleRate, WTFMove(renderSemaphore));
167-
m_audioDestinations.add(newID, destination.copyRef());
148+
auto destination = makeUniqueRef<RemoteAudioDestination>(m_gpuConnectionToWebProcess, newID, inputDeviceId, numberOfInputChannels, numberOfOutputChannels, sampleRate, hardwareSampleRate, WTFMove(renderSemaphore));
149+
m_audioDestinations.add(newID, WTFMove(destination));
168150
completionHandler(newID);
169151
}
170152

171153
void RemoteAudioDestinationManager::deleteAudioDestination(RemoteAudioDestinationIdentifier identifier, CompletionHandler<void()>&& completionHandler)
172154
{
173-
auto destination = m_audioDestinations.take(identifier);
174-
if (destination)
175-
destination->scheduleGracefulShutdownIfNeeded();
155+
m_audioDestinations.remove(identifier);
176156
completionHandler();
177157

178158
if (allowsExitUnderMemoryPressure())

‎Source/WebKit/GPUProcess/media/RemoteAudioDestinationManager.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ class RemoteAudioDestinationManager : private IPC::MessageReceiver {
7171
void audioSamplesStorageChanged(RemoteAudioDestinationIdentifier, const SharedMemory::IPCHandle&, const WebCore::CAAudioStreamDescription&, uint64_t numberOfFrames);
7272
#endif
7373

74-
HashMap<RemoteAudioDestinationIdentifier, Ref<RemoteAudioDestination>> m_audioDestinations;
74+
HashMap<RemoteAudioDestinationIdentifier, UniqueRef<RemoteAudioDestination>> m_audioDestinations;
7575
GPUConnectionToWebProcess& m_gpuConnectionToWebProcess;
7676
};
7777

0 commit comments

Comments
 (0)