- Revision
- 275838
- Author
- [email protected]
- Date
- 2021-04-12 14:28:25 -0700 (Mon, 12 Apr 2021)
Log Message
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.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (275837 => 275838)
--- trunk/LayoutTests/ChangeLog 2021-04-12 20:31:23 UTC (rev 275837)
+++ trunk/LayoutTests/ChangeLog 2021-04-12 21:28:25 UTC (rev 275838)
@@ -1,3 +1,17 @@
+2021-04-12 Chris Dumez <[email protected]>
+
+ webaudio/AudioListener/audiolistener-set-position.html is leaking PannerNodes
+ https://bugs.webkit.org/show_bug.cgi?id=224399
+
+ Reviewed by Geoffrey Garen.
+
+ 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.
+
2021-04-12 Zalan Bujtas <[email protected]>
ASSERTION FAILED: &layoutState().establishedFormattingState(layoutBox.formattingContextRoot()) == this in WebCore::Layout::FormattingState::boxGeometry
Modified: trunk/LayoutTests/imported/w3c/ChangeLog (275837 => 275838)
--- trunk/LayoutTests/imported/w3c/ChangeLog 2021-04-12 20:31:23 UTC (rev 275837)
+++ trunk/LayoutTests/imported/w3c/ChangeLog 2021-04-12 21:28:25 UTC (rev 275838)
@@ -1,3 +1,14 @@
+2021-04-12 Chris Dumez <[email protected]>
+
+ webaudio/AudioListener/audiolistener-set-position.html is leaking PannerNodes
+ https://bugs.webkit.org/show_bug.cgi?id=224399
+
+ Reviewed by Geoffrey Garen.
+
+ 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:
+
2021-04-12 Antoine Quint <[email protected]>
border-image-width computed values should be a calc() value if it contains a percentage
Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/webaudio/the-audio-api/the-audiocontext-interface/audiocontext-suspend-resume-expected.txt (275837 => 275838)
--- trunk/LayoutTests/imported/w3c/web-platform-tests/webaudio/the-audio-api/the-audiocontext-interface/audiocontext-suspend-resume-expected.txt 2021-04-12 20:31:23 UTC (rev 275837)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/webaudio/the-audio-api/the-audiocontext-interface/audiocontext-suspend-resume-expected.txt 2021-04-12 21:28:25 UTC (rev 275838)
@@ -22,7 +22,7 @@
PASS p3 = offlineContext.startRendering() did not throw an exception.
PASS After close, offlineContext.state is equal to closed.
PASS offlineContext.suspend() rejected correctly with TypeError: Not enough arguments.
-PASS offlineContext.resume() rejected correctly with InvalidStateError: Cannot resume an offline audio context that has not started.
+PASS offlineContext.resume() rejected correctly with InvalidStateError: Cannot resume an offline audio context that is closed.
PASS < [test-after-close] All assertions passed. (total 4 assertions)
PASS > [resume-running-context] Test resuming a running context
PASS Create online context did not throw an exception.
Added: trunk/LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-can-only-render-once-expected.txt (0 => 275838)
--- trunk/LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-can-only-render-once-expected.txt (rev 0)
+++ trunk/LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-can-only-render-once-expected.txt 2021-04-12 21:28:25 UTC (rev 275838)
@@ -0,0 +1,10 @@
+OfflineAudioContext can only be started once.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS OfflineAudioContext threw exception when trying to start it again: InvalidStateError: Rendering was already started
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: trunk/LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-can-only-render-once.html (0 => 275838)
--- trunk/LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-can-only-render-once.html (rev 0)
+++ trunk/LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-can-only-render-once.html 2021-04-12 21:28:25 UTC (rev 275838)
@@ -0,0 +1,21 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script src=""
+<script>
+description("OfflineAudioContext can only be started once.");
+jsTestIsAsync = true;
+
+let context = new OfflineAudioContext(2, 1, 44100);
+context.startRendering().then(() => {
+ context.startRendering().then(() => {
+ testFailed("OfflineAudioContext did not throw when trying to start it again");
+ finishJSTest();
+ }, (e) => {
+ testPassed("OfflineAudioContext threw exception when trying to start it again: " + e);
+ finishJSTest();
+ });
+});
+</script>
+</body>
+</html>
Added: trunk/LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-leak-after-rendering-with-nodes-expected.txt (0 => 275838)
--- trunk/LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-leak-after-rendering-with-nodes-expected.txt (rev 0)
+++ trunk/LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-leak-after-rendering-with-nodes-expected.txt 2021-04-12 21:28:25 UTC (rev 275838)
@@ -0,0 +1,10 @@
+Makes sure that the OfflineAudioContext objects are not leaking after rendering with nodes.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS didGCAtLeastOneContext() is true
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: trunk/LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-leak-after-rendering-with-nodes.html (0 => 275838)
--- trunk/LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-leak-after-rendering-with-nodes.html (rev 0)
+++ trunk/LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-leak-after-rendering-with-nodes.html 2021-04-12 21:28:25 UTC (rev 275838)
@@ -0,0 +1,26 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script src=""
+<script src=""
+<script>
+description("Makes sure that the OfflineAudioContext objects are not leaking after rendering with nodes.");
+jsTestIsAsync = true;
+
+const instancesToCreate = 100;
+let renderingPromises = [];
+for (let i = 0; i < instancesToCreate; i++) {
+ let context = new OfflineAudioContext(2, 1, 44100);
+ trackContextForLeaks(context);
+ let src = "" ConstantSourceNode(context, {offset: 1});
+ src._onended_ = () => { };
+ let panner = new PannerNode(context);
+ src.connect(panner).connect(context.destination);
+ renderingPromises.push(context.startRendering());
+}
+Promise.all(renderingPromises).then((values) => {
+ gcAndCheckForContextLeaks();
+});
+</script>
+</body>
+</html>
Modified: trunk/Source/WebCore/ChangeLog (275837 => 275838)
--- trunk/Source/WebCore/ChangeLog 2021-04-12 20:31:23 UTC (rev 275837)
+++ trunk/Source/WebCore/ChangeLog 2021-04-12 21:28:25 UTC (rev 275838)
@@ -1,3 +1,79 @@
+2021-04-12 Chris Dumez <[email protected]>
+
+ webaudio/AudioListener/audiolistener-set-position.html is leaking PannerNodes
+ https://bugs.webkit.org/show_bug.cgi?id=224399
+
+ Reviewed by Geoffrey Garen.
+
+ 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.
+
+2021-04-12 Chris Dumez <[email protected]>
+
+ OfflineAudioContext with nodes that completed rendering do not get freed until navigating away
+ https://bugs.webkit.org/show_bug.cgi?id=224439
+
+ Reviewed by Geoffrey Garen.
+
+ Tests: webaudio/OfflineAudioContext/offlineaudiocontext-can-only-render-once.html
+ webaudio/OfflineAudioContext/offlineaudiocontext-leak-after-rendering-with-nodes.html
+
+ * Modules/webaudio/BaseAudioContext.cpp:
+ (WebCore::BaseAudioContext::finishedRendering):
+ * Modules/webaudio/OfflineAudioContext.cpp:
+ (WebCore::OfflineAudioContext::didFinishOfflineRendering):
+
2021-04-12 Ada Chan <[email protected]>
Initial implementation of WebChromeClient::enumerateImmersiveXRDevices() and XRDeviceProxy
Modified: trunk/Source/WebCore/Modules/webaudio/AudioScheduledSourceNode.cpp (275837 => 275838)
--- trunk/Source/WebCore/Modules/webaudio/AudioScheduledSourceNode.cpp 2021-04-12 20:31:23 UTC (rev 275837)
+++ trunk/Source/WebCore/Modules/webaudio/AudioScheduledSourceNode.cpp 2021-04-12 21:28:25 UTC (rev 275838)
@@ -52,7 +52,6 @@
, ActiveDOMObject(context.scriptExecutionContext())
{
suspendIfNeeded();
- m_pendingActivity = makePendingActivity(*this);
}
void AudioScheduledSourceNode::updateSchedulingInfo(size_t quantumFrameSize, AudioBus& outputBus, size_t& quantumFrameOffset, size_t& nonSilentFramesToProcess, double& startFrameOffset)
@@ -182,13 +181,16 @@
return { };
}
-void AudioScheduledSourceNode::didBecomeMarkedForDeletion()
+bool AudioScheduledSourceNode::virtualHasPendingActivity() const
{
- ASSERT(context().isGraphOwner());
- m_pendingActivity = nullptr;
- ASSERT(!hasPendingActivity());
+ return m_hasEndedEventListener && m_playbackState != FINISHED_STATE && !isMarkedForDeletion() && !context().isClosed();
}
+void AudioScheduledSourceNode::eventListenersDidChange()
+{
+ m_hasEndedEventListener = hasEventListeners(eventNames().endedEvent);
+}
+
void AudioScheduledSourceNode::finish()
{
ASSERT(!hasFinished());
@@ -200,12 +202,7 @@
// Heap allocations are forbidden on the audio thread for performance reasons so we need to
// explicitly allow the following allocation(s).
DisableMallocRestrictionsForCurrentThreadScope disableMallocRestrictions;
- callOnMainThread([this, protectedThis = makeRef(*this)] {
- auto release = makeScopeExit([&] () {
- AudioContext::AutoLocker locker(context());
- m_pendingActivity = nullptr;
- ASSERT(!hasPendingActivity());
- });
+ callOnMainThread([this, protectedThis = makeRef(*this), pendingActivity = makePendingActivity(*this)] {
if (context().isStopped())
return;
this->dispatchEvent(Event::create(eventNames().endedEvent, Event::CanBubble::No, Event::IsCancelable::No));
Modified: trunk/Source/WebCore/Modules/webaudio/AudioScheduledSourceNode.h (275837 => 275838)
--- trunk/Source/WebCore/Modules/webaudio/AudioScheduledSourceNode.h 2021-04-12 20:31:23 UTC (rev 275837)
+++ trunk/Source/WebCore/Modules/webaudio/AudioScheduledSourceNode.h 2021-04-12 21:28:25 UTC (rev 275838)
@@ -58,8 +58,6 @@
ExceptionOr<void> startLater(double when);
ExceptionOr<void> stopLater(double when);
- void didBecomeMarkedForDeletion() override;
-
unsigned short playbackState() const { return static_cast<unsigned short>(m_playbackState); }
bool isPlayingOrScheduled() const { return m_playbackState == PLAYING_STATE || m_playbackState == SCHEDULED_STATE; }
bool hasFinished() const { return m_playbackState == FINISHED_STATE; }
@@ -79,11 +77,13 @@
// Called when we have no more sound to play or the noteOff() time has been reached.
virtual void finish();
+ bool virtualHasPendingActivity() const final;
+ void eventListenersDidChange() final;
+
bool requiresTailProcessing() const final { return false; }
PlaybackState m_playbackState { UNSCHEDULED_STATE };
- RefPtr<PendingActivity<AudioScheduledSourceNode>> m_pendingActivity;
// 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").
double m_startTime { 0 }; // in seconds
@@ -91,6 +91,7 @@
// 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
// has been reached.
Optional<double> m_endTime; // in seconds
+ bool m_hasEndedEventListener { false };
};
} // namespace WebCore
Modified: trunk/Source/WebCore/Modules/webaudio/BaseAudioContext.cpp (275837 => 275838)
--- trunk/Source/WebCore/Modules/webaudio/BaseAudioContext.cpp 2021-04-12 20:31:23 UTC (rev 275837)
+++ trunk/Source/WebCore/Modules/webaudio/BaseAudioContext.cpp 2021-04-12 21:28:25 UTC (rev 275838)
@@ -202,7 +202,7 @@
void BaseAudioContext::lazyInitialize()
{
- if (isStopped())
+ if (isStopped() || isClosed())
return;
if (m_isInitialized)
Modified: trunk/Source/WebCore/Modules/webaudio/BaseAudioContext.h (275837 => 275838)
--- trunk/Source/WebCore/Modules/webaudio/BaseAudioContext.h 2021-04-12 20:31:23 UTC (rev 275837)
+++ trunk/Source/WebCore/Modules/webaudio/BaseAudioContext.h 2021-04-12 21:28:25 UTC (rev 275838)
@@ -316,10 +316,9 @@
void setState(State);
virtual void didFinishOfflineRendering(ExceptionOr<Ref<AudioBuffer>>&&) { }
+ void clear();
private:
- void clear();
-
void scheduleNodeDeletion();
void workletIsReady();
Modified: trunk/Source/WebCore/Modules/webaudio/OfflineAudioContext.cpp (275837 => 275838)
--- trunk/Source/WebCore/Modules/webaudio/OfflineAudioContext.cpp 2021-04-12 20:31:23 UTC (rev 275837)
+++ trunk/Source/WebCore/Modules/webaudio/OfflineAudioContext.cpp 2021-04-12 21:28:25 UTC (rev 275838)
@@ -33,6 +33,7 @@
#include "Document.h"
#include "JSAudioBuffer.h"
#include <wtf/IsoMallocInlines.h>
+#include <wtf/Scope.h>
namespace WebCore {
@@ -73,20 +74,7 @@
{
BaseAudioContext::uninitialize();
- Vector<RefPtr<DeferredPromise>> promisesToReject;
-
- {
- AutoLocker locker(*this);
- promisesToReject.reserveInitialCapacity(m_suspendRequests.size());
- for (auto& promise : m_suspendRequests.values())
- promisesToReject.uncheckedAppend(promise);
- m_suspendRequests.clear();
- }
-
- if (m_pendingOfflineRenderingPromise)
- promisesToReject.append(std::exchange(m_pendingOfflineRenderingPromise, nullptr));
-
- for (auto& promise : promisesToReject)
+ if (auto promise = std::exchange(m_pendingOfflineRenderingPromise, nullptr))
promise->reject(Exception { InvalidStateError, "Context is going away"_s });
}
@@ -210,7 +198,10 @@
void OfflineAudioContext::didFinishOfflineRendering(ExceptionOr<Ref<AudioBuffer>>&& result)
{
- m_didStartOfflineRendering = false;
+ auto finishedRenderingScope = WTF::makeScopeExit([this] {
+ uninitialize();
+ clear();
+ });
if (!m_pendingOfflineRenderingPromise)
return;
