Changeset 278288 in webkit

Timestamp:

May 31, 2021, 5:08:20 PM (5 years ago)

Author:

Chris Dumez

Message:

Drop PendingActivity data member from BaseAudioContext
​https://bugs.webkit.org/show_bug.cgi?id=226445

Reviewed by Darin Adler.

Drop PendingActivity data member from BaseAudioContext and instead have AudioContext / OfflineAudioContext
override virtualHasPendingActivity() to keep their JS wrapper alive. I find that PendingActivity data members
are too error prone and a frequent cause of leaks.

• Modules/webaudio/AudioContext.cpp:

(WebCore::AudioContext::AudioContext):
(WebCore::AudioContext::startRendering):
(WebCore::AudioContext::mayResumePlayback):
(WebCore::AudioContext::suspendPlayback):
(WebCore::AudioContext::virtualHasPendingActivity const):

• Modules/webaudio/AudioContext.h:
• Modules/webaudio/BaseAudioContext.cpp:

(WebCore::BaseAudioContext::clear):
(WebCore::BaseAudioContext::clearPendingActivity): Deleted.
(WebCore::BaseAudioContext::setPendingActivity): Deleted.

• Modules/webaudio/BaseAudioContext.h:
• Modules/webaudio/OfflineAudioContext.cpp:

(WebCore::OfflineAudioContext::startRendering):
(WebCore::OfflineAudioContext::resumeRendering):
(WebCore::OfflineAudioContext::didSuspendRendering):
(WebCore::OfflineAudioContext::finishedRendering):
(WebCore::OfflineAudioContext::virtualHasPendingActivity const):
(WebCore::OfflineAudioContext::dispatchEvent): Deleted.

• Modules/webaudio/OfflineAudioContext.h:

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• Modules/webaudio/AudioContext.cpp (modified) (6 diffs)
• Modules/webaudio/AudioContext.h (modified) (1 diff)
• Modules/webaudio/BaseAudioContext.cpp (modified) (2 diffs)
• Modules/webaudio/BaseAudioContext.h (modified) (2 diffs)
• Modules/webaudio/OfflineAudioContext.cpp (modified) (5 diffs)
• Modules/webaudio/OfflineAudioContext.h (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2021-05-31  Chris Dumez  <cdumez@apple.com>
+
+        Drop PendingActivity data member from BaseAudioContext
+        https://bugs.webkit.org/show_bug.cgi?id=226445
+
+        Reviewed by Darin Adler.
+
+        Drop PendingActivity data member from BaseAudioContext and instead have AudioContext / OfflineAudioContext
+        override virtualHasPendingActivity() to keep their JS wrapper alive. I find that PendingActivity data members
+        are too error prone and a frequent cause of leaks.
+
+        * Modules/webaudio/AudioContext.cpp:
+        (WebCore::AudioContext::AudioContext):
+        (WebCore::AudioContext::startRendering):
+        (WebCore::AudioContext::mayResumePlayback):
+        (WebCore::AudioContext::suspendPlayback):
+        (WebCore::AudioContext::virtualHasPendingActivity const):
+        * Modules/webaudio/AudioContext.h:
+        * Modules/webaudio/BaseAudioContext.cpp:
+        (WebCore::BaseAudioContext::clear):
+        (WebCore::BaseAudioContext::clearPendingActivity): Deleted.
+        (WebCore::BaseAudioContext::setPendingActivity): Deleted.
+        * Modules/webaudio/BaseAudioContext.h:
+        * Modules/webaudio/OfflineAudioContext.cpp:
+        (WebCore::OfflineAudioContext::startRendering):
+        (WebCore::OfflineAudioContext::resumeRendering):
+        (WebCore::OfflineAudioContext::didSuspendRendering):
+        (WebCore::OfflineAudioContext::finishedRendering):
+        (WebCore::OfflineAudioContext::virtualHasPendingActivity const):
+        (WebCore::OfflineAudioContext::dispatchEvent): Deleted.
+        * Modules/webaudio/OfflineAudioContext.h:
+
 2021-05-31  Chris Dumez  <cdumez@apple.com>
 

trunk/Source/WebCore/Modules/webaudio/AudioContext.cpp

@@ -116 +116 @@
     , m_mediaSession(PlatformMediaSession::create(PlatformMediaSessionManager::sharedManager(), *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.
-    setPendingActivity();
-
     constructCommon();
 
@@ -131 +127 @@
     // Lazy initialization starts rendering so we schedule a task here to make sure lazy initialization
     // ends up happening, even if no audio node gets constructed.
-    postTask([this] {
+    postTask([this, pendingActivity = makePendingActivity(*this)] {
         if (!isStopped())
             lazyInitialize();
@@ -310 +306 @@
         return;
 
-    setPendingActivity();
-
-    lazyInitialize();
-    destination().startRendering([this, protectedThis = makeRef(*this)](std::optional<Exception>&& exception) {
+    lazyInitialize();
+    destination().startRendering([this, protectedThis = makeRef(*this), pendingActivity = makePendingActivity(*this)](std::optional<Exception>&& exception) {
         if (!exception)
             setState(State::Running);
@@ -386 +380 @@
     lazyInitialize();
 
-    destination().resume([this, protectedThis = makeRef(*this)](std::optional<Exception>&& exception) {
+    destination().resume([this, protectedThis = makeRef(*this), pendingActivity = makePendingActivity(*this)](std::optional<Exception>&& exception) {
         setState(exception ? State::Suspended : State::Running);
     });
@@ -472 +466 @@
     lazyInitialize();
 
-    destination().suspend([this, protectedThis = makeRef(*this)](std::optional<Exception>&& exception) {
+    destination().suspend([this, protectedThis = makeRef(*this), pendingActivity = makePendingActivity(*this)](std::optional<Exception>&& exception) {
         if (exception)
             return;
@@ -556 +550 @@
 #endif
 
+bool AudioContext::virtualHasPendingActivity() const
+{
+    return !isClosed();
+}
+
 } // namespace WebCore
 

trunk/Source/WebCore/Modules/webaudio/AudioContext.h

@@ -143 +143 @@
     void suspend(ReasonForSuspension) final;
     void resume() final;
+    bool virtualHasPendingActivity() const final;
 
     UniqueRef<DefaultAudioDestinationNode> m_destinationNode;

trunk/Source/WebCore/Modules/webaudio/BaseAudioContext.cpp

@@ -180 +180 @@
         m_nodesToDelete = std::exchange(m_nodesMarkedForDeletion, { });
     } while (!m_nodesToDelete.isEmpty());
-
-    clearPendingActivity();
 }
 
@@ -878 +876 @@
 }
 
-void BaseAudioContext::clearPendingActivity()
-{
-    m_pendingActivity = nullptr;
-}
-
-void BaseAudioContext::setPendingActivity()
-{
-    if (!m_pendingActivity)
-        m_pendingActivity = makePendingActivity(*this);
-}
-
 PeriodicWave& BaseAudioContext::periodicWave(OscillatorType type)
 {

trunk/Source/WebCore/Modules/webaudio/BaseAudioContext.h

@@ -228 +228 @@
 protected:
     explicit BaseAudioContext(Document&);
-    
-    void clearPendingActivity();
-    void setPendingActivity();
 
     virtual void uninitialize();
@@ -347 +344 @@
     std::unique_ptr<AsyncAudioDecoder> m_audioDecoder;
 
-    RefPtr<PendingActivity<BaseAudioContext>> m_pendingActivity;
-
     AudioIOPosition m_outputPosition;
 

trunk/Source/WebCore/Modules/webaudio/OfflineAudioContext.cpp

@@ -114 +114 @@
         }
 
-        setPendingActivity();
         m_pendingRenderingPromise = WTFMove(promise);
         m_didStartRendering = true;
@@ -176 +175 @@
         }
 
-        setPendingActivity();
         setState(State::Running);
         promise->resolve();
@@ -196 +194 @@
     setState(State::Suspended);
 
-    clearPendingActivity();
-
     RefPtr<DeferredPromise> promise;
     {
@@ -218 +214 @@
     });
 
+    // Make sure our JSwrapper stays alive long enough to resolve the promise and queue the completion event.
+    // Otherwise, setting the state to Closed may cause our JS wrapper to get collected early.
+    auto protectedJSWrapper = makePendingActivity(*this);
     setState(State::Closed);
 
@@ -247 +246 @@
 }
 
-void OfflineAudioContext::dispatchEvent(Event& event)
-{
-    BaseAudioContext::dispatchEvent(event);
-    if (event.eventInterface() == OfflineAudioCompletionEventInterfaceType)
-        clearPendingActivity();
+bool OfflineAudioContext::virtualHasPendingActivity() const
+{
+    return state() == State::Running;
 }
 

trunk/Source/WebCore/Modules/webaudio/OfflineAudioContext.h

@@ -61 +61 @@
     // ActiveDOMObject
     const char* activeDOMObjectName() const final;
-
-    // EventTarget
-    void dispatchEvent(Event&) final;
+    bool virtualHasPendingActivity() const final;
 
     void settleRenderingPromise(ExceptionOr<Ref<AudioBuffer>>&&);