Skip to content

Commit 73b4046

Browse files
cdumezcdumez
committed
OfflineAudioContext objects are leaking
https://bugs.webkit.org/show_bug.cgi?id=224279 Reviewed by Darin Adler. Source/WebCore: OfflineAudioContext objects were always leaking due to 2 reference cycles: 1. BaseAudioContext -> m_listener (AudioListener) -> m_positionX (AudioParam) -> m_context (BaseAudioContext) 2. BaseAudioContext -> m_destinationNode (AudioDestinationNode) -> m_context (BaseAudioContext) For reference cycle 1, I made AudioSummingJunction (base class of AudioParam) hold a weak pointer to the AudioContext instead of a Ref<>. I don't think there is a good reason for an AudioSummingJunction (AudioParam or AudioNodeInput) to keep its AudioContext alive. AudioNodes already keep their AudioContext alive. AudioNodeInputs and AudioParams are associated to AudioNodes. For reference cycle 2, I made AudioDestinationNode not hold a strong pointer to its context but instead a weak pointer. Since keeping an AudioDestinationNode alive should keep its AudioContext alive, I made it so that ref'ing the AudioDestinationNode refs its BaseAudioContext. Also, BaseAudioContext::m_destinationNode is now a UniqueRef<> instead of a RefPtr<> to avoid a cycle. Tests: webaudio/OfflineAudioContext/offlineaudiocontext-leak-after-rendering.html webaudio/OfflineAudioContext/offlineaudiocontext-leak.html * Modules/webaudio/AudioContext.cpp: (WebCore::AudioContext::baseLatency): (WebCore::AudioContext::getOutputTimestamp): (WebCore::AudioContext::close): (WebCore::AudioContext::destination): (WebCore::AudioContext::destination const): (WebCore::AudioContext::suspendRendering): (WebCore::AudioContext::resumeRendering): (WebCore::AudioContext::startRendering): (WebCore::AudioContext::lazyInitialize): (WebCore::AudioContext::mediaState const): (WebCore::AudioContext::mayResumePlayback): (WebCore::AudioContext::suspendPlayback): (WebCore::AudioContext::pageMutedStateDidChange): * Modules/webaudio/AudioContext.h: - Drop some null checks now that m_destinationNode can no longer be null. - Capture an ActiveDOMObject::PendingActivity when doing async work that ends up resolving a Promise, so that we keep both the object and its wrapper alive for the duration of the async work. * Modules/webaudio/AudioDestinationNode.cpp: (WebCore::AudioDestinationNode::ref): (WebCore::AudioDestinationNode::deref): * Modules/webaudio/AudioDestinationNode.h: Have AudioDestinationNode override AudioNode's ref() / deref() to forward the refcounting to its BaseAudioContext, instead of using the AudioNode's internal refCount. * Modules/webaudio/AudioListener.cpp: (WebCore::AudioListener::updateValuesIfNeeded): (WebCore::AudioListener::setPosition): (WebCore::AudioListener::setOrientation): Add some null-checks for AudioParam::context() now that AudioParam holds a WeakPtr to its context. * Modules/webaudio/AudioNode.cpp: (WebCore::AudioNode::toWeakOrStrongContext): (WebCore::AudioNode::AudioNode): (WebCore::AudioNode::connect): (WebCore::AudioNode::sampleRate const): (WebCore::AudioNode::markNodeForDeletionIfNecessary): (WebCore::AudioNode::contextForBindings): (WebCore::AudioNode::context): (WebCore::AudioNode::context const): * Modules/webaudio/AudioNode.h: - Hold the BaseAudioContext as a WeakPtr instead of a Ref<> if the AudioNode is an AudioDestinationNode. This avoids a reference cycle since the BaseAudioContext owns the AudioDestinationNode. Even though we are using a WeakPtr, it is safe to assume that the context is not null because ref'ing an AudioDestinationNode refs its BaseAudioContext. - Make sure markNodeForDeletionIfNecessary() has no effect for AudioDestinationNode since BaseAudioContext now owns the AudioDestinationNode when we take care of destroying its destination node when destroyed. * Modules/webaudio/AudioNodeInput.cpp: (WebCore::AudioNodeInput::connect): (WebCore::AudioNodeInput::disconnect): (WebCore::AudioNodeInput::outputEnabledStateChanged): (WebCore::AudioNodeInput::updateInternalBus): (WebCore::AudioNodeInput::bus): (WebCore::AudioNodeInput::internalSummingBus): (WebCore::AudioNodeInput::sumAllConnections): (WebCore::AudioNodeInput::pull): Add assertions that the context is not null. There were already assertions that we are the graph owner, which means we are holding the BaseAudioContext's lock, which means that the audio context is alive. * Modules/webaudio/AudioParam.cpp: (WebCore::AudioParam::value): (WebCore::AudioParam::setValueForBindings): (WebCore::AudioParam::smooth): (WebCore::AudioParam::hasSampleAccurateValues const): (WebCore::AudioParam::calculateSampleAccurateValues): (WebCore::AudioParam::calculateFinalValues): (WebCore::AudioParam::calculateTimelineValues): (WebCore::AudioParam::connect): (WebCore::AudioParam::disconnect): Add null-checks for the AudioContext now that the AudioParam is only holding a WeakPtr to its BaseAudioContext (to avoid a reference cycle). * Modules/webaudio/AudioSummingJunction.cpp: (WebCore::AudioSummingJunction::AudioSummingJunction): (WebCore::AudioSummingJunction::~AudioSummingJunction): (WebCore::AudioSummingJunction::markRenderingStateAsDirty): (WebCore::AudioSummingJunction::addOutput): (WebCore::AudioSummingJunction::removeOutput): (WebCore::AudioSummingJunction::updateRenderingState): (WebCore::AudioSummingJunction::outputEnabledStateChanged): * Modules/webaudio/AudioSummingJunction.h: (WebCore::AudioSummingJunction::context): (WebCore::AudioSummingJunction::context const): - Hold a WeakPtr to the BaseAudioContext to avoid a reference cycle. - Deal with the fact that the audio context may be null now that we're holding a WeakPtr to it (except when holding the graph lock) * Modules/webaudio/BaseAudioContext.cpp: (WebCore::BaseAudioContext::BaseAudioContext): (WebCore::BaseAudioContext::~BaseAudioContext): (WebCore::BaseAudioContext::numberOfInstances): (WebCore::BaseAudioContext::lazyInitialize): (WebCore::BaseAudioContext::clear): (WebCore::BaseAudioContext::uninitialize): (WebCore::BaseAudioContext::stop): (WebCore::BaseAudioContext::sampleRate const): (WebCore::BaseAudioContext::decodeAudioData): (WebCore::BaseAudioContext::markForDeletion): (WebCore::BaseAudioContext::deleteMarkedNodes): (WebCore::BaseAudioContext::setPendingActivity): (WebCore::BaseAudioContext::workletIsReady): * Modules/webaudio/BaseAudioContext.h: (WebCore::BaseAudioContext::destination): (WebCore::BaseAudioContext::destination const): (WebCore::BaseAudioContext::currentSampleFrame const): (WebCore::BaseAudioContext::currentTime const): - Switch m_destinationNode from RefPtr<> to UniqueRef<> since the AudioContext is now the owner of the destinationNode and since refing the destination node actually refs its BaseAudioContext. - Drop some null checks now that m_destinationNode can no longer be null. - Rename makePendingActivity() to setPendingActivity() to avoid a naming conflict with ActiveDOMObject::makePendingActivity(). * Modules/webaudio/DefaultAudioDestinationNode.h: * Modules/webaudio/OfflineAudioDestinationNode.h: - Drop create() factory functions and make the constructor public now that the BaseAudioContext owns its destination node via a UniqueRef<>. - Make some member functions public as they are virtual and they now called on the subclass instead of the base class (and these functions are public in the base class). * Modules/webaudio/OfflineAudioContext.cpp: (WebCore::OfflineAudioContext::startOfflineRendering): (WebCore::OfflineAudioContext::resumeOfflineRendering): * Modules/webaudio/OfflineAudioContext.h: - Drop some null checks now that m_destinationNode can no longer be null. - Capture an ActiveDOMObject::PendingActivity when doing async work that ends up resolving a Promise, so that we keep both the object and its wrapper alive for the duration of the async work. * Modules/webaudio/WebKitAudioContext.cpp: (WebCore::WebKitAudioContext::close): Drop null checks for the destination node now that it can never be null. * dom/ShadowRoot.cpp: * rendering/FloatingObjects.cpp: * rendering/RootInlineBox.cpp: Update classes used of size restrictions since the size of a WeakPtr is not longer the same as the size of a pointer when debug assertions are enabled. As long as they are the same size in release builds, there is no memory use concern. * testing/Internals.cpp: (WebCore::Internals::numberOfBaseAudioContexts const): * testing/Internals.h: * testing/Internals.idl: Add testing function to check how many BaseAudioContexts are alive, so that we can write layout tests and check for leaks. Source/WTF: Add flag that can be passed when constructing a WeakPtr to disable threading assertions. This is useful for cases where we know it is safe due to locking but we'd like to use a WeakPtr instead of a raw pointer because it is safer. * wtf/WeakPtr.h: (WTF::WeakPtr::get const): (WTF::WeakPtr::operator-> const): (WTF::WeakPtr::operator* const): (WTF::WeakPtr::WeakPtr): (WTF::WeakPtrFactory::createWeakPtr const): (WTF::=): (WTF::makeWeakPtr): LayoutTests: Add layout test coverage. * webaudio/OfflineAudioContext/offlineaudiocontext-leak-after-rendering-expected.txt: Added. * webaudio/OfflineAudioContext/offlineaudiocontext-leak-after-rendering.html: Added. * webaudio/OfflineAudioContext/offlineaudiocontext-leak-expected.txt: Added. * webaudio/OfflineAudioContext/offlineaudiocontext-leak.html: Added. Canonical link: https://commits.webkit.org/236304@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@275668 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent 4602fd1 commit 73b4046

32 files changed

Lines changed: 561 additions & 143 deletions

‎LayoutTests/ChangeLog‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,17 @@
1+
2021-04-08 Chris Dumez <cdumez@apple.com>
2+
3+
OfflineAudioContext objects are leaking
4+
https://bugs.webkit.org/show_bug.cgi?id=224279
5+
6+
Reviewed by Darin Adler.
7+
8+
Add layout test coverage.
9+
10+
* webaudio/OfflineAudioContext/offlineaudiocontext-leak-after-rendering-expected.txt: Added.
11+
* webaudio/OfflineAudioContext/offlineaudiocontext-leak-after-rendering.html: Added.
12+
* webaudio/OfflineAudioContext/offlineaudiocontext-leak-expected.txt: Added.
13+
* webaudio/OfflineAudioContext/offlineaudiocontext-leak.html: Added.
14+
115
2021-04-08 Andres Gonzalez <andresg_22@apple.com>
216

317
VoiceOver does not echo text insertions and deletions when a contenteditable div has a non editable descendant element with a content editable child

‎LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-leak-after-rendering-expected.txt‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
Makes sure that the OfflineAudioContext objects are not leaking after rendering.
2+
3+
On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
4+
5+
6+
PASS internals.numberOfBaseAudioContexts() >= instancesToCreate is true
7+
PASS internals.numberOfBaseAudioContexts() < instancesToCreate is true
8+
PASS successfullyParsed is true
9+
10+
TEST COMPLETE
11+

‎LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-leak-after-rendering.html‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
<!DOCTYPE html>
2+
<html>
3+
<body>
4+
<script src="../../resources/js-test.js"></script>
5+
<script>
6+
description("Makes sure that the OfflineAudioContext objects are not leaking after rendering.");
7+
jsTestIsAsync = true;
8+
9+
const instancesToCreate = 100;
10+
11+
function test() {
12+
let promises = [];
13+
for (let i = 0; i < instancesToCreate; i++) {
14+
let context = new OfflineAudioContext(2, 1, 44100);
15+
promises.push(context.startRendering());
16+
}
17+
shouldBeTrue("internals.numberOfBaseAudioContexts() >= instancesToCreate");
18+
Promise.all(promises).then((values) => {
19+
gc();
20+
setTimeout(() => {
21+
gc();
22+
shouldBeTrue("internals.numberOfBaseAudioContexts() < instancesToCreate");
23+
finishJSTest();
24+
}, 0);
25+
});
26+
}
27+
28+
test();
29+
</script>
30+
</body>
31+
</html>

‎LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-leak-expected.txt‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
Makes sure that the OfflineAudioContext objects are not leaking.
2+
3+
On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
4+
5+
6+
PASS internals.numberOfBaseAudioContexts() >= instancesToCreate is true
7+
PASS internals.numberOfBaseAudioContexts() < instancesToCreate is true
8+
PASS successfullyParsed is true
9+
10+
TEST COMPLETE
11+

‎LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-leak.html‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
<!DOCTYPE html>
2+
<html>
3+
<body>
4+
<script src="../../resources/js-test.js"></script>
5+
<script>
6+
description("Makes sure that the OfflineAudioContext objects are not leaking.");
7+
jsTestIsAsync = true;
8+
9+
const instancesToCreate = 100;
10+
11+
function allocateContexts() {
12+
let contexts = [];
13+
for (let i = 0; i < instancesToCreate; i++)
14+
contexts.push(new OfflineAudioContext(2, 1, 44100));
15+
shouldBeTrue("internals.numberOfBaseAudioContexts() >= instancesToCreate");
16+
}
17+
18+
allocateContexts();
19+
gc();
20+
setTimeout(() => {
21+
gc();
22+
shouldBeTrue("internals.numberOfBaseAudioContexts() < instancesToCreate");
23+
finishJSTest();
24+
}, 0);
25+
</script>
26+
</body>
27+
</html>

‎Source/WTF/ChangeLog‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,23 @@
1+
2021-04-08 Chris Dumez <cdumez@apple.com>
2+
3+
OfflineAudioContext objects are leaking
4+
https://bugs.webkit.org/show_bug.cgi?id=224279
5+
6+
Reviewed by Darin Adler.
7+
8+
Add flag that can be passed when constructing a WeakPtr to disable threading assertions.
9+
This is useful for cases where we know it is safe due to locking but we'd like to use a
10+
WeakPtr instead of a raw pointer because it is safer.
11+
12+
* wtf/WeakPtr.h:
13+
(WTF::WeakPtr::get const):
14+
(WTF::WeakPtr::operator-> const):
15+
(WTF::WeakPtr::operator* const):
16+
(WTF::WeakPtr::WeakPtr):
17+
(WTF::WeakPtrFactory::createWeakPtr const):
18+
(WTF::=):
19+
(WTF::makeWeakPtr):
20+
121
2021-04-08 Simon Fraser <simon.fraser@apple.com>
222

323
Copy-constructed Vectors should not have excess capacity

‎Source/WTF/wtf/WeakPtr.h‎

Lines changed: 20 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,8 @@ struct EmptyCounter {
3838
static void decrement() { }
3939
};
4040

41+
enum class EnableWeakPtrThreadingAssertions : bool { No, Yes };
42+
4143
template<typename Counter = EmptyCounter> class WeakPtrImpl : public ThreadSafeRefCounted<WeakPtrImpl<Counter>> {
4244
WTF_MAKE_NONCOPYABLE(WeakPtrImpl);
4345
WTF_MAKE_FAST_ALLOCATED;
@@ -91,7 +93,7 @@ template<typename T, typename Counter> class WeakPtr {
9193
T* get() const
9294
{
9395
// FIXME: Our GC threads currently need to get opaque pointers from WeakPtrs and have to be special-cased.
94-
ASSERT(!m_impl || Thread::mayBeGCThread() || m_impl->wasConstructedOnMainThread() == isMainThread());
96+
ASSERT(!m_impl || !m_shouldEnableAssertions || Thread::mayBeGCThread() || m_impl->wasConstructedOnMainThread() == isMainThread());
9597
return m_impl ? static_cast<T*>(m_impl->template get<T>()) : nullptr;
9698
}
9799

@@ -104,13 +106,13 @@ template<typename T, typename Counter> class WeakPtr {
104106

105107
T* operator->() const
106108
{
107-
ASSERT(!m_impl || m_impl->wasConstructedOnMainThread() == isMainThread());
109+
ASSERT(!m_impl || !m_shouldEnableAssertions || m_impl->wasConstructedOnMainThread() == isMainThread());
108110
return get();
109111
}
110112

111113
T& operator*() const
112114
{
113-
ASSERT(!m_impl || m_impl->wasConstructedOnMainThread() == isMainThread());
115+
ASSERT(!m_impl || !m_shouldEnableAssertions || m_impl->wasConstructedOnMainThread() == isMainThread());
114116
return *get();
115117
}
116118

@@ -121,12 +123,19 @@ template<typename T, typename Counter> class WeakPtr {
121123
template<typename, typename> friend class WeakPtr;
122124
template<typename, typename> friend class WeakPtrFactory;
123125

124-
explicit WeakPtr(Ref<WeakPtrImpl<Counter>>&& ref)
126+
explicit WeakPtr(Ref<WeakPtrImpl<Counter>>&& ref, EnableWeakPtrThreadingAssertions shouldEnableAssertions)
125127
: m_impl(WTFMove(ref))
128+
#if ASSERT_ENABLED
129+
, m_shouldEnableAssertions(shouldEnableAssertions == EnableWeakPtrThreadingAssertions::Yes)
130+
#endif
126131
{
132+
UNUSED_PARAM(shouldEnableAssertions);
127133
}
128134

129135
RefPtr<WeakPtrImpl<Counter>> m_impl;
136+
#if ASSERT_ENABLED
137+
bool m_shouldEnableAssertions { true };
138+
#endif
130139
};
131140

132141
// Note: you probably want to inherit from CanMakeWeakPtr rather than use this directly.
@@ -159,12 +168,12 @@ template<typename T, typename Counter = EmptyCounter> class WeakPtrFactory {
159168
m_impl = WeakPtrImpl<Counter>::create(const_cast<T*>(&object));
160169
}
161170

162-
template<typename U> WeakPtr<U, Counter> createWeakPtr(U& object) const
171+
template<typename U> WeakPtr<U, Counter> createWeakPtr(U& object, EnableWeakPtrThreadingAssertions enableWeakPtrThreadingAssertions = EnableWeakPtrThreadingAssertions::Yes) const
163172
{
164173
initializeIfNeeded(object);
165174

166175
ASSERT(&object == m_impl->template get<T>());
167-
return WeakPtr<U, Counter>(makeRef(*m_impl));
176+
return WeakPtr<U, Counter>(makeRef(*m_impl), enableWeakPtrThreadingAssertions);
168177
}
169178

170179
void revokeAll()
@@ -244,16 +253,16 @@ template<typename T, typename Counter> template<typename U> inline WeakPtr<T, Co
244253
return *this;
245254
}
246255

247-
template<typename T> inline auto makeWeakPtr(T& object)
256+
template<typename T> inline auto makeWeakPtr(T& object, EnableWeakPtrThreadingAssertions enableWeakPtrThreadingAssertions = EnableWeakPtrThreadingAssertions::Yes)
248257
{
249-
return object.weakPtrFactory().template createWeakPtr<T>(object);
258+
return object.weakPtrFactory().template createWeakPtr<T>(object, enableWeakPtrThreadingAssertions);
250259
}
251260

252-
template<typename T> inline auto makeWeakPtr(T* ptr) -> decltype(makeWeakPtr(*ptr))
261+
template<typename T> inline auto makeWeakPtr(T* ptr, EnableWeakPtrThreadingAssertions enableWeakPtrThreadingAssertions = EnableWeakPtrThreadingAssertions::Yes) -> decltype(makeWeakPtr(*ptr))
253262
{
254263
if (!ptr)
255264
return { };
256-
return makeWeakPtr(*ptr);
265+
return makeWeakPtr(*ptr, enableWeakPtrThreadingAssertions);
257266
}
258267

259268
template<typename T, typename U, typename Counter> inline bool operator==(const WeakPtr<T, Counter>& a, const WeakPtr<U, Counter>& b)
@@ -289,6 +298,7 @@ template<typename T, typename U, typename Counter> inline bool operator!=(T* a,
289298
} // namespace WTF
290299

291300
using WTF::CanMakeWeakPtr;
301+
using WTF::EnableWeakPtrThreadingAssertions;
292302
using WTF::WeakPtr;
293303
using WTF::WeakPtrFactory;
294304
using WTF::WeakPtrFactoryInitialization;

0 commit comments

Comments
 (0)