OfflineAudioDestinationNode::startRendering leaks OfflineAudioDestinationNode if...
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 10 Apr 2019 20:31:04 +0000 (20:31 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 10 Apr 2019 20:31:04 +0000 (20:31 +0000)
https://bugs.webkit.org/show_bug.cgi?id=196759

Reviewed by Eric Carlson.

OfflineAudioDestinationNode::startRendering unconditionally ref's itself before invoking offlineRender() in a new thread.
But offlineRender can early exit without ever calling deref() in the main thread, leading to the leak of
OfflineAudioDestinationNode. Fixed the leak by always calling deref in the main thread after calling offlineRender().

Also removed the debug assertion in offlineRender which always hits when we run the relevant test.

Test: imported/w3c/web-platform-tests/webaudio/the-audio-api/the-offlineaudiocontext-interface/current-time-block-size.html

* Modules/webaudio/OfflineAudioDestinationNode.cpp:
(WebCore::OfflineAudioDestinationNode::startRendering):
(WebCore::OfflineAudioDestinationNode::offlineRender):
(WebCore::OfflineAudioDestinationNode::notifyComplete): Merged into startRendering.
* Modules/webaudio/OfflineAudioDestinationNode.h:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@244145 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/Modules/webaudio/OfflineAudioDestinationNode.cpp
Source/WebCore/Modules/webaudio/OfflineAudioDestinationNode.h

index 7bb9bba..902a6fd 100644 (file)
@@ -1,3 +1,24 @@
+2019-04-09  Ryosuke Niwa  <rniwa@webkit.org>
+
+        OfflineAudioDestinationNode::startRendering leaks OfflineAudioDestinationNode if offlineRender exists early
+        https://bugs.webkit.org/show_bug.cgi?id=196759
+
+        Reviewed by Eric Carlson.
+
+        OfflineAudioDestinationNode::startRendering unconditionally ref's itself before invoking offlineRender() in a new thread.
+        But offlineRender can early exit without ever calling deref() in the main thread, leading to the leak of
+        OfflineAudioDestinationNode. Fixed the leak by always calling deref in the main thread after calling offlineRender().
+
+        Also removed the debug assertion in offlineRender which always hits when we run the relevant test.
+
+        Test: imported/w3c/web-platform-tests/webaudio/the-audio-api/the-offlineaudiocontext-interface/current-time-block-size.html
+
+        * Modules/webaudio/OfflineAudioDestinationNode.cpp:
+        (WebCore::OfflineAudioDestinationNode::startRendering):
+        (WebCore::OfflineAudioDestinationNode::offlineRender):
+        (WebCore::OfflineAudioDestinationNode::notifyComplete): Merged into startRendering.
+        * Modules/webaudio/OfflineAudioDestinationNode.h:
+
 2019-04-10  Megan Gardner  <megan_gardner@apple.com>
 
         Fix text autoscrolling when typing in modern webkit
index 7a377e9..9520225 100644 (file)
@@ -84,37 +84,46 @@ void OfflineAudioDestinationNode::startRendering()
     if (!m_renderTarget.get())
         return;
     
-    if (!m_startedRendering) {
-        m_startedRendering = true;
-        ref(); // See corresponding deref() call in notifyCompleteDispatch().
-        m_renderThread = Thread::create("offline renderer", [this] {
-            offlineRender();
+    if (m_startedRendering)
+        return;
+
+    m_startedRendering = true;
+    ref();
+    // FIXME: Should we call lazyInitialize here?
+    // FIXME: We should probably limit the number of threads we create for offline audio.
+    m_renderThread = Thread::create("offline renderer", [this] {
+        bool didRender = offlineRender();
+        callOnMainThread([this, didRender] {
+            if (didRender)
+                context().fireCompletionEvent();
+            deref();
         });
-    }
+    });
 }
 
-void OfflineAudioDestinationNode::offlineRender()
+bool OfflineAudioDestinationNode::offlineRender()
 {
     ASSERT(!isMainThread());
     ASSERT(m_renderBus.get());
     if (!m_renderBus.get())
-        return;
+        return false;
 
     bool isAudioContextInitialized = context().isInitialized();
-    ASSERT(isAudioContextInitialized);
+    // FIXME: We used to assert that isAudioContextInitialized is true here.
+    // But it's trivially false in imported/w3c/web-platform-tests/webaudio/the-audio-api/the-offlineaudiocontext-interface/current-time-block-size.html 
     if (!isAudioContextInitialized)
-        return;
+        return false;
 
     bool channelsMatch = m_renderBus->numberOfChannels() == m_renderTarget->numberOfChannels();
     ASSERT(channelsMatch);
     if (!channelsMatch)
-        return;
-        
+        return false;
+
     bool isRenderBusAllocated = m_renderBus->length() >= renderQuantumSize;
     ASSERT(isRenderBusAllocated);
     if (!isRenderBusAllocated)
-        return;
-        
+        return false;
+
     // Break up the render target into smaller "render quantize" sized pieces.
     // Render until we're finished.
     size_t framesToProcess = m_renderTarget->length();
@@ -136,17 +145,8 @@ void OfflineAudioDestinationNode::offlineRender()
         n += framesAvailableToCopy;
         framesToProcess -= framesAvailableToCopy;
     }
-    
-    // Our work is done. Let the AudioContext know.
-    callOnMainThread([this] {
-        notifyComplete();
-        deref();
-    });
-}
 
-void OfflineAudioDestinationNode::notifyComplete()
-{
-    context().fireCompletionEvent();
+    return true;
 }
 
 } // namespace WebCore
index f030f3b..da24957 100644 (file)
@@ -66,10 +66,7 @@ private:
     // Rendering thread.
     RefPtr<Thread> m_renderThread;
     bool m_startedRendering;
-    void offlineRender();
-    
-    // For completion callback on main thread.
-    void notifyComplete();
+    bool offlineRender();
 };
 
 } // namespace WebCore