Attachment 372493 Details for Bug 198964 – Patch

[patch] Patch

bug-198964-20190619143146.patch (text/plain), 12.19 KB, created by youenn fablet on 2019-06-19 14:31:47 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: youenn fablet

Created: 2019-06-19 14:31:47 PDT

Size: 12.19 KB

patch

obsolete

>Subversion Revision: 246549
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 3bbff9910b4ca77264d1e006aa40627675b4fe26..c2b85183b390817f7d9e268953eaf1d3e7126df8 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,40 @@
>+2019-06-19  Youenn Fablet  <youenn@apple.com>
>+
>+        Safari crashes after ~2028 OfflineAudioContext objects are created (they never get garbage collected, consuming a thread each)
>+        https://bugs.webkit.org/show_bug.cgi?id=198964
>+        <rdar://problem/51891520>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Move from setPendingActivity/unsetPendingActivity to an
>+        m_pendingActivity member which is easier to manage.
>+
>+        Keep setting a pending activity for AudioContext at construction time
>+        but do not do that for Offline contexts.
>+        Instead, set the pending activity when startRendering is called.
>+        Unset the pending activity when the rendering activity is finished.
>+
>+        Make m_audioDecoder a unique pointer so that it can lazily be initialized.
>+        This removes the burden of creating an audio decoder thread for each context.
>+
>+        Test: webaudio/offlineaudiocontext-gc.html
>+
>+        * Modules/webaudio/AudioContext.cpp:
>+        (WebCore::AudioContext::AudioContext):
>+        (WebCore::AudioContext::constructCommon):
>+        (WebCore::AudioContext::clear):
>+        (WebCore::AudioContext::decodeAudioData):
>+        (WebCore::AudioContext::startRendering):
>+        (WebCore::AudioContext::finishedRendering):
>+        (WebCore::AudioContext::dispatchEvent):
>+        (WebCore::AudioContext::clearPendingActivity):
>+        (WebCore::AudioContext::makePendingActivity):
>+        To keep it consistent with setPendingActivity/unsetPendingActivity, we
>+        explicitly ref/unref the AudioContext. We should try to remove this ref/unref.
>+        * Modules/webaudio/AudioContext.h:
>+        * Modules/webaudio/OfflineAudioDestinationNode.cpp:
>+        (WebCore::OfflineAudioDestinationNode::startRendering):
>+
> 2019-06-18  Youenn Fablet  <youenn@apple.com>
> 
>         Changing settings of a MediaStreamTrack clone should not alter the settings of the original track
>diff --git a/Source/WebCore/Modules/webaudio/AudioContext.cpp b/Source/WebCore/Modules/webaudio/AudioContext.cpp
>index 0d0500e531f234776a1b3780e3869338c29ef696..e6240e91a0658a3c1e315eb9322ccf493ffceff2 100644
>--- a/Source/WebCore/Modules/webaudio/AudioContext.cpp
>+++ b/Source/WebCore/Modules/webaudio/AudioContext.cpp
>@@ -99,6 +99,7 @@
> #include <wtf/MainThread.h>
> #include <wtf/Ref.h>
> #include <wtf/RefCounted.h>
>+#include <wtf/Scope.h>
> #include <wtf/text/WTFString.h>
> 
> const unsigned MaxPeriodicWaveLength = 4096;
>@@ -141,6 +142,10 @@ AudioContext::AudioContext(Document& document)
>     , m_mediaSession(PlatformMediaSession::create(*this))
>     , m_eventQueue(std::make_unique<GenericEventQueue>(*this))
> {
>+    // According to spec AudioContext must die only after page navigate.
>+    // Lets mark it as ActiveDOMObject with pending activity and unmark it in clear method.
>+    makePendingActivity();
>+
>     constructCommon();
> 
>     m_destinationNode = DefaultAudioDestinationNode::create(*this);
>@@ -172,10 +177,6 @@ AudioContext::AudioContext(Document& document, unsigned numberOfChannels, size_t
> 
> void AudioContext::constructCommon()
> {
>-    // According to spec AudioContext must die only after page navigate.
>-    // Lets mark it as ActiveDOMObject with pending activity and unmark it in clear method.
>-    setPendingActivity(*this);
>-
>     FFTFrame::initialize();
>     
>     m_listener = AudioListener::create();
>@@ -253,8 +254,7 @@ void AudioContext::clear()
>         m_nodesMarkedForDeletion.clear();
>     } while (m_nodesToDelete.size());
> 
>-    // It was set in constructCommon.
>-    unsetPendingActivity(*this);
>+    clearPendingActivity();
> }
> 
> void AudioContext::uninitialize()
>@@ -429,7 +429,9 @@ ExceptionOr<Ref<AudioBuffer>> AudioContext::createBuffer(ArrayBuffer& arrayBuffe
> 
> void AudioContext::decodeAudioData(Ref<ArrayBuffer>&& audioData, RefPtr<AudioBufferCallback>&& successCallback, RefPtr<AudioBufferCallback>&& errorCallback)
> {
>-    m_audioDecoder.decodeAsync(WTFMove(audioData), sampleRate(), WTFMove(successCallback), WTFMove(errorCallback));
>+    if (!m_audioDecoder)
>+        m_audioDecoder = std::make_unique<AsyncAudioDecoder>();
>+    m_audioDecoder->decodeAsync(WTFMove(audioData), sampleRate(), WTFMove(successCallback), WTFMove(errorCallback));
> }
> 
> ExceptionOr<Ref<AudioBufferSourceNode>> AudioContext::createBufferSource()
>@@ -1141,6 +1143,8 @@ void AudioContext::startRendering()
>     if (m_isStopScheduled || !willBeginPlayback())
>         return;
> 
>+    makePendingActivity();
>+
>     destination()->startRendering();
>     setState(State::Running);
> }
>@@ -1176,14 +1180,23 @@ void AudioContext::isPlayingAudioDidChange()
>     });
> }
> 
>-void AudioContext::fireCompletionEvent()
>+void AudioContext::finishedRendering(bool didRendering)
> {
>+    ASSERT(isOfflineContext());
>     ASSERT(isMainThread());
>     if (!isMainThread())
>         return;
> 
>+    auto clearPendingActivityIfExitEarly = WTF::makeScopeExit([this] {
>+        clearPendingActivity();
>+    });
>+
>+
>     ALWAYS_LOG(LOGIDENTIFIER);
>-    
>+
>+    if (!didRendering)
>+        return;
>+
>     AudioBuffer* renderedBuffer = m_renderTarget.get();
>     setState(State::Closed);
> 
>@@ -1192,10 +1205,18 @@ void AudioContext::fireCompletionEvent()
>         return;
> 
>     // Avoid firing the event if the document has already gone away.
>-    if (!m_isStopScheduled) {
>-        // Call the offline rendering completion event listener.
>-        m_eventQueue->enqueueEvent(OfflineAudioCompletionEvent::create(renderedBuffer));
>-    }
>+    if (m_isStopScheduled)
>+        return;
>+
>+    clearPendingActivityIfExitEarly.release();
>+    m_eventQueue->enqueueEvent(OfflineAudioCompletionEvent::create(renderedBuffer));
>+}
>+
>+void AudioContext::dispatchEvent(Event& event)
>+{
>+    EventTarget::dispatchEvent(event);
>+    if (event.eventInterface() == OfflineAudioCompletionEventInterfaceType)
>+        clearPendingActivity();
> }
> 
> void AudioContext::incrementActiveSourceCount()
>@@ -1347,6 +1368,23 @@ void AudioContext::addConsoleMessage(MessageSource source, MessageLevel level, c
>         m_scriptExecutionContext->addConsoleMessage(source, level, message);
> }
> 
>+void AudioContext::clearPendingActivity()
>+{
>+    if (!m_pendingActivity)
>+        return;
>+    m_pendingActivity = nullptr;
>+    // FIXME: Remove this specific unref() and ref() call in makePendingActivity().
>+    unref();
>+}
>+
>+void AudioContext::makePendingActivity()
>+{
>+    if (m_pendingActivity)
>+        return;
>+    m_pendingActivity = makePendingActivity(*this);
>+    ref();
>+}
>+
> #if !RELEASE_LOG_DISABLED
> WTFLogChannel& AudioContext::logChannel() const
> {
>diff --git a/Source/WebCore/Modules/webaudio/AudioContext.h b/Source/WebCore/Modules/webaudio/AudioContext.h
>index f21a8b4e1b93952ee82e063c7d73fa357bbe8574..9d77c9707243f90c3a9f436afd64bd699aa1c638 100644
>--- a/Source/WebCore/Modules/webaudio/AudioContext.h
>+++ b/Source/WebCore/Modules/webaudio/AudioContext.h
>@@ -259,8 +259,8 @@ public:
>     using ThreadSafeRefCounted::deref;
> 
>     void startRendering();
>-    void fireCompletionEvent();
>-    
>+    void finishedRendering(bool didRendering);
>+
>     static unsigned s_hardwareContextCount;
> 
>     // Restrictions to change default behaviors.
>@@ -297,7 +297,9 @@ protected:
>     AudioContext(Document&, unsigned numberOfChannels, size_t numberOfFrames, float sampleRate);
>     
>     static bool isSampleRateRangeGood(float sampleRate);
>-    
>+    void clearPendingActivity();
>+    void makePendingActivity();
>+
> private:
>     void constructCommon();
> 
>@@ -320,6 +322,7 @@ private:
> 
>     // EventTarget
>     ScriptExecutionContext* scriptExecutionContext() const final;
>+    void dispatchEvent(Event&) final;
> 
>     // MediaProducer
>     MediaProducer::MediaStateFlags mediaState() const override;
>@@ -429,7 +432,7 @@ private:
>     Thread* volatile m_audioThread { nullptr };
>     Thread* volatile m_graphOwnerThread { nullptr }; // if the lock is held then this is the thread which owns it, otherwise == nullptr.
> 
>-    AsyncAudioDecoder m_audioDecoder;
>+    std::unique_ptr<AsyncAudioDecoder> m_audioDecoder;
> 
>     // This is considering 32 is large enough for multiple channels audio. 
>     // It is somewhat arbitrary and could be increased if necessary.
>@@ -441,6 +444,7 @@ private:
>     BehaviorRestrictions m_restrictions { NoRestrictions };
> 
>     State m_state { State::Suspended };
>+    RefPtr<PendingActivity<AudioContext>> m_pendingActivity;
> };
> 
> // FIXME: Find out why these ==/!= functions are needed and remove them if possible.
>diff --git a/Source/WebCore/Modules/webaudio/OfflineAudioDestinationNode.cpp b/Source/WebCore/Modules/webaudio/OfflineAudioDestinationNode.cpp
>index 95202258135391229689d442be86a89afd209bd8..ef7b33a86d652d6e6d74e677c6dfffa48346a1a6 100644
>--- a/Source/WebCore/Modules/webaudio/OfflineAudioDestinationNode.cpp
>+++ b/Source/WebCore/Modules/webaudio/OfflineAudioDestinationNode.cpp
>@@ -94,8 +94,7 @@ void OfflineAudioDestinationNode::startRendering()
>     m_renderThread = Thread::create("offline renderer", [this] {
>         bool didRender = offlineRender();
>         callOnMainThread([this, didRender] {
>-            if (didRender)
>-                context().fireCompletionEvent();
>+            context().finishedRendering(didRender);
>             deref();
>         });
>     });
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index ec55a05881677027a94f94312381020f1d554574..1f186b224916ef34787354663846648d39fafef2 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,14 @@
>+2019-06-19  Youenn Fablet  <youenn@apple.com>
>+
>+        Safari crashes after ~2028 OfflineAudioContext objects are created (they never get garbage collected, consuming a thread each)
>+        https://bugs.webkit.org/show_bug.cgi?id=198964
>+        <rdar://problem/51891520>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * webaudio/offlineaudiocontext-gc-expected.txt: Added.
>+        * webaudio/offlineaudiocontext-gc.html: Added.
>+
> 2019-06-18  Youenn Fablet  <youenn@apple.com>
> 
>         Changing settings of a MediaStreamTrack clone should not alter the settings of the original track
>diff --git a/LayoutTests/webaudio/offlineaudiocontext-gc-expected.txt b/LayoutTests/webaudio/offlineaudiocontext-gc-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..974d8f573ac4b476d7a087a1b43071c1b0554c78
>--- /dev/null
>+++ b/LayoutTests/webaudio/offlineaudiocontext-gc-expected.txt
>@@ -0,0 +1,4 @@
>+
>+PASS GC a newly created offline audio context 
>+PASS GC a rendering offline audio context 
>+
>diff --git a/LayoutTests/webaudio/offlineaudiocontext-gc.html b/LayoutTests/webaudio/offlineaudiocontext-gc.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..d009a054e18858e3733b71821711c0fec2e6b153
>--- /dev/null
>+++ b/LayoutTests/webaudio/offlineaudiocontext-gc.html
>@@ -0,0 +1,45 @@
>+<!DOCTYPE html>
>+<meta charset="utf-8">
>+<script src="../resources/testharness.js"></script>
>+<script src="../resources/testharnessreport.js"></script>
>+<script src="../resources/gc.js"></script>
>+<script>
>+
>+function createContext() {
>+    let context = new webkitOfflineAudioContext(2, 1000, 44100);
>+    if (window.internals)
>+        return internals.observeGC(context);
>+}
>+
>+function createRenderingContext(completeCallback) {
>+    let context = new webkitOfflineAudioContext(2, 1000, 44100);
>+    let node = context.createBufferSource();
>+    node.connect(context.destination);
>+    node.start();
>+    context.oncomplete = completeCallback;
>+    context.startRendering();
>+    if (window.internals)
>+        return internals.observeGC(context);
>+}
>+
>+test(() => {
>+    let observer = createContext();
>+    gc();
>+    if (window.internals)
>+        assert_true(observer.wasCollected);
>+}, "GC a newly created offline audio context");
>+
>+promise_test(async () => {
>+    let complete = false;
>+    let observer = createRenderingContext(() => complete = true);
>+    while (!complete) {
>+        gc();
>+        if (window.internals)
>+            assert_false(observer.wasCollected);
>+        await new Promise(resolve => setTimeout(resolve, 50));
>+    }
>+    gc();
>+    if (window.internals)
>+        assert_true(observer.wasCollected);
>+}, "GC a rendering offline audio context");
>+</script>

Actions: View | Formatted Diff | Diff

Attachments on bug 198964: 372487 | 372492 | 372493 | 372494