Skip to content

Commit c8708fe

Browse files
cdumezcdumez
committed
webaudio/AudioListener/audiolistener-set-position.html is leaking PannerNodes
https://bugs.webkit.org/show_bug.cgi?id=224399 Reviewed by Geoffrey Garen. LayoutTests/imported/w3c: Rebaseline existing WPT test. It is still passing but the exception message is different. * web-platform-tests/webaudio/the-audio-api/the-audiocontext-interface/audiocontext-suspend-resume-expected.txt: Source/WebCore: The test was leaking all its nodes and contexts due to several logic issues in our code. Tests: webaudio/OfflineAudioContext/offlineaudiocontext-can-only-render-once.html webaudio/OfflineAudioContext/offlineaudiocontext-leak-after-rendering-with-nodes.html * Modules/webaudio/AudioScheduledSourceNode.cpp: (WebCore::AudioScheduledSourceNode::AudioScheduledSourceNode): (WebCore::AudioScheduledSourceNode::virtualHasPendingActivity const): (WebCore::AudioScheduledSourceNode::finish): * Modules/webaudio/AudioScheduledSourceNode.h: 1. Stop using an ActiveDOMObject::PendingActivity to keep our wrapper alive as this was causing a reference cycle. PendingActivity keeps a ref to |this| and this is ref'ing the PendingActivity. 2 things could break the cycle: - finish() gets called but the audio context may go away without finish getting called. - didBecomeMarkedForDeletion() gets called. However, it was getting called from AudioNode::markNodeForDeletionIfNecessary(). This function would early return if m_normalRefCount is not 0. Here m_normalRefCount could NOT be 0, since PendingActity was ref'ing the Node. 2. Instead of a PendingActivity, we now override ActiveDOMObject::virtualHasPendingActivity() to keep the wrapper alive. The behavior is the same since its return true when the state is not finished and the node has not been marked for deletion. However, I added a condition to make sure it starts returning false as soon as the context is closed. There is also no need to keep the JS wrapper alive if there is no 'ended' event listener. * Modules/webaudio/BaseAudioContext.cpp: (WebCore::BaseAudioContext::lazyInitialize): Early return if lazyInitialize() gets called for an audiocontext is closed. Nothing prevents JS from creating an AudioNode after the AudioContext is closed. Constructing an AudioNode ends up lazy initializing the audio context. In such case, we would already early return in release because if the m_isAudioThreadFinished check. However, we would crash in debug because of the m_isAudioThreadFinished ASSERT(). * Modules/webaudio/BaseAudioContext.h: Make clear() member function protected so it can get called by OfflineAudioContext when rendering is complete. * Modules/webaudio/OfflineAudioContext.cpp: (WebCore::OfflineAudioContext::uninitialize): Stop rejecting the suspend promises when the OfflineAudioContext gets uninitialized. Previously this would only happen on navigation so we did not notice the issue. However, the OfflineAudioContext now gets uninitialized as soon as it is done rendering and rejecting those promises in this case would start causing test failures. (WebCore::OfflineAudioContext::didFinishOfflineRendering): - Stop clearing the m_didStartOfflineRendering flag when rendering is finished. This flag is called [[rendering started]] in the WebAudio specification [1]. As per the specification, it gets set to true in startRendering() and never gets reset to false. This means that an offline audio context cannot start rendering again once it is done rendering. I have added a layout test for this behavior change and verified that this test is passing is both Firefox and Chrome. [1] https://webaudio.github.io/web-audio-api/#dom-offlineaudiocontext-startrendering - Call uninitialize() and clear() when rendering has finished to clear as much memory as possible as soon as we can. This is acceptable because it is no longer possible to start rendering again once it's finished. Previously, uninitialize() / clear() would only happen when navigating away and when the document would get destroyed. LayoutTests: Add layout test coverage. * webaudio/OfflineAudioContext/offlineaudiocontext-can-only-render-once-expected.txt: Added. * webaudio/OfflineAudioContext/offlineaudiocontext-can-only-render-once.html: Added. * webaudio/OfflineAudioContext/offlineaudiocontext-leak-after-rendering-with-nodes-expected.txt: Added. * webaudio/OfflineAudioContext/offlineaudiocontext-leak-after-rendering-with-nodes.html: Added. Canonical link: https://commits.webkit.org/236406@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@275838 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent 5fad646 commit c8708fe

13 files changed

Lines changed: 189 additions & 33 deletions

‎LayoutTests/ChangeLog‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,17 @@
1+
2021-04-12 Chris Dumez <cdumez@apple.com>
2+
3+
webaudio/AudioListener/audiolistener-set-position.html is leaking PannerNodes
4+
https://bugs.webkit.org/show_bug.cgi?id=224399
5+
6+
Reviewed by Geoffrey Garen.
7+
8+
Add layout test coverage.
9+
10+
* webaudio/OfflineAudioContext/offlineaudiocontext-can-only-render-once-expected.txt: Added.
11+
* webaudio/OfflineAudioContext/offlineaudiocontext-can-only-render-once.html: Added.
12+
* webaudio/OfflineAudioContext/offlineaudiocontext-leak-after-rendering-with-nodes-expected.txt: Added.
13+
* webaudio/OfflineAudioContext/offlineaudiocontext-leak-after-rendering-with-nodes.html: Added.
14+
115
2021-04-12 Zalan Bujtas <zalan@apple.com>
216

317
ASSERTION FAILED: &layoutState().establishedFormattingState(layoutBox.formattingContextRoot()) == this in WebCore::Layout::FormattingState::boxGeometry

‎LayoutTests/imported/w3c/ChangeLog‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,14 @@
1+
2021-04-12 Chris Dumez <cdumez@apple.com>
2+
3+
webaudio/AudioListener/audiolistener-set-position.html is leaking PannerNodes
4+
https://bugs.webkit.org/show_bug.cgi?id=224399
5+
6+
Reviewed by Geoffrey Garen.
7+
8+
Rebaseline existing WPT test. It is still passing but the exception message is different.
9+
10+
* web-platform-tests/webaudio/the-audio-api/the-audiocontext-interface/audiocontext-suspend-resume-expected.txt:
11+
112
2021-04-12 Antoine Quint <graouts@webkit.org>
213

314
border-image-width computed values should be a calc() value if it contains a percentage

‎LayoutTests/imported/w3c/web-platform-tests/webaudio/the-audio-api/the-audiocontext-interface/audiocontext-suspend-resume-expected.txt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ PASS > [test-after-close] Test state after context closed
2222
PASS p3 = offlineContext.startRendering() did not throw an exception.
2323
PASS After close, offlineContext.state is equal to closed.
2424
PASS offlineContext.suspend() rejected correctly with TypeError: Not enough arguments.
25-
PASS offlineContext.resume() rejected correctly with InvalidStateError: Cannot resume an offline audio context that has not started.
25+
PASS offlineContext.resume() rejected correctly with InvalidStateError: Cannot resume an offline audio context that is closed.
2626
PASS < [test-after-close] All assertions passed. (total 4 assertions)
2727
PASS > [resume-running-context] Test resuming a running context
2828
PASS Create online context did not throw an exception.

‎LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-can-only-render-once-expected.txt‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
OfflineAudioContext can only be started once.
2+
3+
On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
4+
5+
6+
PASS OfflineAudioContext threw exception when trying to start it again: InvalidStateError: Rendering was already started
7+
PASS successfullyParsed is true
8+
9+
TEST COMPLETE
10+

‎LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-can-only-render-once.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+
<script src="../../resources/js-test.js"></script>
5+
<script>
6+
description("OfflineAudioContext can only be started once.");
7+
jsTestIsAsync = true;
8+
9+
let context = new OfflineAudioContext(2, 1, 44100);
10+
context.startRendering().then(() => {
11+
context.startRendering().then(() => {
12+
testFailed("OfflineAudioContext did not throw when trying to start it again");
13+
finishJSTest();
14+
}, (e) => {
15+
testPassed("OfflineAudioContext threw exception when trying to start it again: " + e);
16+
finishJSTest();
17+
});
18+
});
19+
</script>
20+
</body>
21+
</html>

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

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

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

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
<!DOCTYPE html>
2+
<html>
3+
<body>
4+
<script src="../../resources/js-test.js"></script>
5+
<script src="../resources/audiocontext-leak-test.js"></script>
6+
<script>
7+
description("Makes sure that the OfflineAudioContext objects are not leaking after rendering with nodes.");
8+
jsTestIsAsync = true;
9+
10+
const instancesToCreate = 100;
11+
let renderingPromises = [];
12+
for (let i = 0; i < instancesToCreate; i++) {
13+
let context = new OfflineAudioContext(2, 1, 44100);
14+
trackContextForLeaks(context);
15+
let src = new ConstantSourceNode(context, {offset: 1});
16+
src.onended = () => { };
17+
let panner = new PannerNode(context);
18+
src.connect(panner).connect(context.destination);
19+
renderingPromises.push(context.startRendering());
20+
}
21+
Promise.all(renderingPromises).then((values) => {
22+
gcAndCheckForContextLeaks();
23+
});
24+
</script>
25+
</body>
26+
</html>

‎Source/WebCore/ChangeLog‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,79 @@
1+
2021-04-12 Chris Dumez <cdumez@apple.com>
2+
3+
webaudio/AudioListener/audiolistener-set-position.html is leaking PannerNodes
4+
https://bugs.webkit.org/show_bug.cgi?id=224399
5+
6+
Reviewed by Geoffrey Garen.
7+
8+
The test was leaking all its nodes and contexts due to several logic issues in our code.
9+
10+
Tests: webaudio/OfflineAudioContext/offlineaudiocontext-can-only-render-once.html
11+
webaudio/OfflineAudioContext/offlineaudiocontext-leak-after-rendering-with-nodes.html
12+
13+
* Modules/webaudio/AudioScheduledSourceNode.cpp:
14+
(WebCore::AudioScheduledSourceNode::AudioScheduledSourceNode):
15+
(WebCore::AudioScheduledSourceNode::virtualHasPendingActivity const):
16+
(WebCore::AudioScheduledSourceNode::finish):
17+
* Modules/webaudio/AudioScheduledSourceNode.h:
18+
1. Stop using an ActiveDOMObject::PendingActivity to keep our wrapper alive as this was causing
19+
a reference cycle. PendingActivity keeps a ref to |this| and this is ref'ing the PendingActivity.
20+
2 things could break the cycle:
21+
- finish() gets called but the audio context may go away without finish getting called.
22+
- didBecomeMarkedForDeletion() gets called. However, it was getting called from
23+
AudioNode::markNodeForDeletionIfNecessary(). This function would early return if m_normalRefCount
24+
is not 0. Here m_normalRefCount could NOT be 0, since PendingActity was ref'ing the Node.
25+
2. Instead of a PendingActivity, we now override ActiveDOMObject::virtualHasPendingActivity() to keep
26+
the wrapper alive. The behavior is the same since its return true when the state is not finished
27+
and the node has not been marked for deletion. However, I added a condition to make sure it starts
28+
returning false as soon as the context is closed. There is also no need to keep the JS wrapper alive
29+
if there is no 'ended' event listener.
30+
31+
* Modules/webaudio/BaseAudioContext.cpp:
32+
(WebCore::BaseAudioContext::lazyInitialize):
33+
Early return if lazyInitialize() gets called for an audiocontext is closed. Nothing prevents JS from
34+
creating an AudioNode after the AudioContext is closed. Constructing an AudioNode ends up lazy
35+
initializing the audio context. In such case, we would already early return in release because if the
36+
m_isAudioThreadFinished check. However, we would crash in debug because of the m_isAudioThreadFinished
37+
ASSERT().
38+
39+
* Modules/webaudio/BaseAudioContext.h:
40+
Make clear() member function protected so it can get called by OfflineAudioContext when rendering is
41+
complete.
42+
43+
* Modules/webaudio/OfflineAudioContext.cpp:
44+
(WebCore::OfflineAudioContext::uninitialize):
45+
Stop rejecting the suspend promises when the OfflineAudioContext gets uninitialized. Previously this
46+
would only happen on navigation so we did not notice the issue. However, the OfflineAudioContext now
47+
gets uninitialized as soon as it is done rendering and rejecting those promises in this case would
48+
start causing test failures.
49+
50+
(WebCore::OfflineAudioContext::didFinishOfflineRendering):
51+
- Stop clearing the m_didStartOfflineRendering flag when rendering is finished. This flag is called
52+
[[rendering started]] in the WebAudio specification [1]. As per the specification, it gets set to true
53+
in startRendering() and never gets reset to false. This means that an offline audio context cannot
54+
start rendering again once it is done rendering. I have added a layout test for this behavior change
55+
and verified that this test is passing is both Firefox and Chrome.
56+
[1] https://webaudio.github.io/web-audio-api/#dom-offlineaudiocontext-startrendering
57+
- Call uninitialize() and clear() when rendering has finished to clear as much memory as possible as
58+
soon as we can. This is acceptable because it is no longer possible to start rendering again once
59+
it's finished. Previously, uninitialize() / clear() would only happen when navigating away and when
60+
the document would get destroyed.
61+
62+
2021-04-12 Chris Dumez <cdumez@apple.com>
63+
64+
OfflineAudioContext with nodes that completed rendering do not get freed until navigating away
65+
https://bugs.webkit.org/show_bug.cgi?id=224439
66+
67+
Reviewed by Geoffrey Garen.
68+
69+
Tests: webaudio/OfflineAudioContext/offlineaudiocontext-can-only-render-once.html
70+
webaudio/OfflineAudioContext/offlineaudiocontext-leak-after-rendering-with-nodes.html
71+
72+
* Modules/webaudio/BaseAudioContext.cpp:
73+
(WebCore::BaseAudioContext::finishedRendering):
74+
* Modules/webaudio/OfflineAudioContext.cpp:
75+
(WebCore::OfflineAudioContext::didFinishOfflineRendering):
76+
177
2021-04-12 Ada Chan <ada.chan@apple.com>
278

379
Initial implementation of WebChromeClient::enumerateImmersiveXRDevices() and XRDeviceProxy

‎Source/WebCore/Modules/webaudio/AudioScheduledSourceNode.cpp‎

Lines changed: 8 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,6 @@ AudioScheduledSourceNode::AudioScheduledSourceNode(BaseAudioContext& context, No
5252
, ActiveDOMObject(context.scriptExecutionContext())
5353
{
5454
suspendIfNeeded();
55-
m_pendingActivity = makePendingActivity(*this);
5655
}
5756

5857
void AudioScheduledSourceNode::updateSchedulingInfo(size_t quantumFrameSize, AudioBus& outputBus, size_t& quantumFrameOffset, size_t& nonSilentFramesToProcess, double& startFrameOffset)
@@ -182,11 +181,14 @@ ExceptionOr<void> AudioScheduledSourceNode::stopLater(double when)
182181
return { };
183182
}
184183

185-
void AudioScheduledSourceNode::didBecomeMarkedForDeletion()
184+
bool AudioScheduledSourceNode::virtualHasPendingActivity() const
186185
{
187-
ASSERT(context().isGraphOwner());
188-
m_pendingActivity = nullptr;
189-
ASSERT(!hasPendingActivity());
186+
return m_hasEndedEventListener && m_playbackState != FINISHED_STATE && !isMarkedForDeletion() && !context().isClosed();
187+
}
188+
189+
void AudioScheduledSourceNode::eventListenersDidChange()
190+
{
191+
m_hasEndedEventListener = hasEventListeners(eventNames().endedEvent);
190192
}
191193

192194
void AudioScheduledSourceNode::finish()
@@ -200,12 +202,7 @@ void AudioScheduledSourceNode::finish()
200202
// Heap allocations are forbidden on the audio thread for performance reasons so we need to
201203
// explicitly allow the following allocation(s).
202204
DisableMallocRestrictionsForCurrentThreadScope disableMallocRestrictions;
203-
callOnMainThread([this, protectedThis = makeRef(*this)] {
204-
auto release = makeScopeExit([&] () {
205-
AudioContext::AutoLocker locker(context());
206-
m_pendingActivity = nullptr;
207-
ASSERT(!hasPendingActivity());
208-
});
205+
callOnMainThread([this, protectedThis = makeRef(*this), pendingActivity = makePendingActivity(*this)] {
209206
if (context().isStopped())
210207
return;
211208
this->dispatchEvent(Event::create(eventNames().endedEvent, Event::CanBubble::No, Event::IsCancelable::No));

‎Source/WebCore/Modules/webaudio/AudioScheduledSourceNode.h‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -58,8 +58,6 @@ class AudioScheduledSourceNode : public AudioNode, public ActiveDOMObject {
5858
ExceptionOr<void> startLater(double when);
5959
ExceptionOr<void> stopLater(double when);
6060

61-
void didBecomeMarkedForDeletion() override;
62-
6361
unsigned short playbackState() const { return static_cast<unsigned short>(m_playbackState); }
6462
bool isPlayingOrScheduled() const { return m_playbackState == PLAYING_STATE || m_playbackState == SCHEDULED_STATE; }
6563
bool hasFinished() const { return m_playbackState == FINISHED_STATE; }
@@ -79,18 +77,21 @@ class AudioScheduledSourceNode : public AudioNode, public ActiveDOMObject {
7977
// Called when we have no more sound to play or the noteOff() time has been reached.
8078
virtual void finish();
8179

80+
bool virtualHasPendingActivity() const final;
81+
void eventListenersDidChange() final;
82+
8283
bool requiresTailProcessing() const final { return false; }
8384

8485
PlaybackState m_playbackState { UNSCHEDULED_STATE };
8586

86-
RefPtr<PendingActivity<AudioScheduledSourceNode>> m_pendingActivity;
8787
// m_startTime is the time to start playing based on the context's timeline (0 or a time less than the context's current time means "now").
8888
double m_startTime { 0 }; // in seconds
8989

9090
// m_endTime is the time to stop playing based on the context's timeline (0 or a time less than the context's current time means "now").
9191
// If it hasn't been set explicitly, then the sound will not stop playing (if looping) or will stop when the end of the AudioBuffer
9292
// has been reached.
9393
Optional<double> m_endTime; // in seconds
94+
bool m_hasEndedEventListener { false };
9495
};
9596

9697
} // namespace WebCore

0 commit comments

Comments
 (0)