Changeset 92658 in webkit

Timestamp:

Aug 8, 2011 5:49:04 PM (13 years ago)

Author:

crogers@google.com

Message:

Fix thread-safety of AudioNode deletion
​https://bugs.webkit.org/show_bug.cgi?id=65888

Reviewed by Kenneth Russell

No new tests - JavaScript API is not affected.

• webaudio/AudioContext.cpp:

(WebCore::AudioContext::AudioContext):
(WebCore::AudioContext::constructCommon):
(WebCore::AudioContext::~AudioContext):
(WebCore::AudioContext::uninitialize):
(WebCore::AudioContext::handlePostRenderTasks):
(WebCore::AudioContext::scheduleNodeDeletion):
(WebCore::AudioContext::deleteMarkedNodesDispatch):
(WebCore::AudioContext::deleteMarkedNodes):

• webaudio/AudioContext.h:

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• webaudio/AudioContext.cpp (modified) (7 diffs)
• webaudio/AudioContext.h (modified) (6 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2011-08-08  Chris Rogers  <crogers@google.com>
+
+        Fix thread-safety of AudioNode deletion
+        https://bugs.webkit.org/show_bug.cgi?id=65888
+
+        Reviewed by Kenneth Russell
+
+        No new tests - JavaScript API is not affected.
+
+        * webaudio/AudioContext.cpp:
+        (WebCore::AudioContext::AudioContext):
+        (WebCore::AudioContext::constructCommon):
+        (WebCore::AudioContext::~AudioContext):
+        (WebCore::AudioContext::uninitialize):
+        (WebCore::AudioContext::handlePostRenderTasks):
+        (WebCore::AudioContext::scheduleNodeDeletion):
+        (WebCore::AudioContext::deleteMarkedNodesDispatch):
+        (WebCore::AudioContext::deleteMarkedNodes):
+        * webaudio/AudioContext.h:
+
 2011-08-08  Chris Marrin  <cmarrin@apple.com>
 

trunk/Source/WebCore/webaudio/AudioContext.cpp

@@ -121 +121 @@
     , m_document(document)
     , m_destinationNode(0)
+    , m_isDeletionScheduled(false)
     , m_connectionCount(0)
     , m_audioThread(0)
@@ -161 +162 @@
 void AudioContext::constructCommon()
 {
-    // Note: because adoptRef() won't be called until we leave this constructor, but code in this constructor needs to reference this context,
-    // relax the check.
-    relaxAdoptionRequirement();
-    
     FFTFrame::initialize();
     
@@ -176 +173 @@
 #if DEBUG_AUDIONODE_REFERENCES
     printf("%p: AudioContext::~AudioContext()\n", this);
-#endif    
+#endif
     // AudioNodes keep a reference to their context, so there should be no way to be in the destructor if there are still AudioNodes around.
     ASSERT(!m_nodesToDelete.size());
@@ -228 +225 @@
         // Get rid of the sources which may still be playing.
         derefUnfinishedSourceNodes();
-        
+
+        deleteMarkedNodes();
+
         // Because the AudioBuffers are garbage collected, we can't delete them here.
         // Instead, at least release the potentially large amount of allocated memory for the audio data.
@@ -567 +566 @@
         derefFinishedSourceNodes();
 
-        // Finally actually delete.
-        deleteMarkedNodes();
+        // Don't delete in the real-time thread. Let the main thread do it.
+        // Ref-counted objects held by certain AudioNodes may not be thread-safe.
+        scheduleNodeDeletion();
 
         // Fixup the state of any dirty AudioNodeInputs and AudioNodeOutputs.
@@ -597 +597 @@
 }
 
+void AudioContext::scheduleNodeDeletion()
+{
+    bool isGood = m_isInitialized && isGraphOwner();
+    ASSERT(isGood);
+    if (!isGood)
+        return;
+
+    // Make sure to call deleteMarkedNodes() on main thread.    
+    if (m_nodesToDelete.size() && !m_isDeletionScheduled) {
+        m_isDeletionScheduled = true;
+
+        // Don't let ourself get deleted before the callback.
+        // See matching deref() in deleteMarkedNodesDispatch().
+        ref();
+        callOnMainThread(deleteMarkedNodesDispatch, this);
+    }
+}
+
+void AudioContext::deleteMarkedNodesDispatch(void* userData)
+{
+    AudioContext* context = reinterpret_cast<AudioContext*>(userData);
+    ASSERT(context);
+    if (!context)
+        return;
+
+    context->deleteMarkedNodes();
+    context->deref();
+}
+
 void AudioContext::deleteMarkedNodes()
 {
-    ASSERT(isGraphOwner() || isAudioThreadFinished());
-
+    ASSERT(isMainThread());
+
+    AutoLocker locker(this);
+    
     // Note: deleting an AudioNode can cause m_nodesToDelete to grow.
-    size_t nodesDeleted = 0;
     while (size_t n = m_nodesToDelete.size()) {
         AudioNode* node = m_nodesToDelete[n - 1];
@@ -619 +649 @@
         // Finally, delete it.
         delete node;
-
-        // Don't delete too many nodes per render quantum since we don't want to do too much work in the realtime audio thread.
-        if (++nodesDeleted > MaxNodesToDeletePerQuantum)
-            break;
-    }
+    }
+    
+    m_isDeletionScheduled = false;
 }
 

trunk/Source/WebCore/webaudio/AudioContext.h

@@ -39 +39 @@
 #include <wtf/RefCounted.h>
 #include <wtf/RefPtr.h>
+#include <wtf/ThreadSafeRefCounted.h>
 #include <wtf/Threading.h>
 #include <wtf/Vector.h>
@@ -68 +69 @@
 // For thread safety between the audio thread and the main thread, it has a rendering graph locking mechanism. 
 
-class AudioContext : public ActiveDOMObject, public RefCounted<AudioContext>, public EventTarget {
+class AudioContext : public ActiveDOMObject, public ThreadSafeRefCounted<AudioContext>, public EventTarget {
 public:
     // Create an AudioContext for rendering to the audio hardware.
@@ -137 +138 @@
     void derefFinishedSourceNodes();
 
-    // We reap all marked nodes at the end of each realtime render quantum in deleteMarkedNodes().
+    // We schedule deletion of all marked nodes at the end of each realtime render quantum.
     void markForDeletion(AudioNode*);
     void deleteMarkedNodes();
-
+    
     // Keeps track of the number of connections made.
     void incrementConnectionCount()
@@ -210 +211 @@
     DEFINE_ATTRIBUTE_EVENT_LISTENER(complete);
 
-    // Reconcile ref/deref which are defined both in AudioNode and EventTarget.
-    using RefCounted<AudioContext>::ref;
-    using RefCounted<AudioContext>::deref;
+    // Reconcile ref/deref which are defined both in ThreadSafeRefCounted and EventTarget.
+    using ThreadSafeRefCounted<AudioContext>::ref;
+    using ThreadSafeRefCounted<AudioContext>::deref;
 
     void startRendering();
@@ -226 +227 @@
     void lazyInitialize();
     void uninitialize();
+
+    void scheduleNodeDeletion();
+    static void deleteMarkedNodesDispatch(void* userData);
     
     bool m_isInitialized;
@@ -258 +262 @@
     Vector<AudioNode*> m_referencedNodes;
 
-    // Accumulate nodes which need to be deleted at the end of a render cycle (in realtime thread) here.
+    // Accumulate nodes which need to be deleted here.
+    // They will be scheduled for deletion (on the main thread) at the end of a render cycle (in realtime thread).
     Vector<AudioNode*> m_nodesToDelete;
+    bool m_isDeletionScheduled;
 
     // Only accessed when the graph lock is held.