Bug 864709 - Part 2: Protect accesses to AudioNodeEngine::mNode using a lock; r=padenot

Author: Ehsan Akhgari

content/media/AudioNodeEngine.h

@@ -8,6 +8,7 @@
 
 #include "AudioSegment.h"
 #include "mozilla/dom/AudioParam.h"
+#include "mozilla/Mutex.h"
 
 namespace mozilla {
 
@@ -151,6 +152,7 @@ class AudioNodeEngine {
 public:
   explicit AudioNodeEngine(dom::AudioNode* aNode)
     : mNode(aNode)
+    , mNodeMutex("AudioNodeEngine::mNodeMutex")
   {
     MOZ_ASSERT(mNode, "The engine is constructed with a null node");
     MOZ_COUNT_CTOR(AudioNodeEngine);
@@ -206,15 +208,25 @@ class AudioNodeEngine {
     *aOutput = aInput;
   }
 
+  Mutex& NodeMutex() { return mNodeMutex;}
+
   dom::AudioNode* Node() const
   {
-    MOZ_ASSERT(NS_IsMainThread());
+    mNodeMutex.AssertCurrentThreadOwns();
     return mNode;
   }
 
-protected:
-  friend class dom::AudioNode;
+  void ClearNode()
+  {
+    MOZ_ASSERT(NS_IsMainThread());
+    MOZ_ASSERT(mNode != nullptr);
+    mNodeMutex.AssertCurrentThreadOwns();
+    mNode = nullptr;
+  }
+
+private:
   dom::AudioNode* mNode;
+  Mutex mNodeMutex;
 };
 
 }

content/media/webaudio/AnalyserNode.cpp

@@ -30,7 +30,15 @@ class AnalyserNodeEngine : public AudioNodeEngine
 
     NS_IMETHOD Run()
     {
-      nsRefPtr<AnalyserNode> node = static_cast<AnalyserNode*>(mStream->Engine()->Node());
+      nsRefPtr<AnalyserNode> node;
+      {
+        // No need to keep holding the lock for the whole duration of this
+        // function, since we're holding a strong reference to it, so if
+        // we can obtain the reference, we will hold the node alive in
+        // this function.
+        MutexAutoLock lock(mStream->Engine()->NodeMutex());
+        node = static_cast<AnalyserNode*>(mStream->Engine()->Node());
+      }
       if (node) {
         node->AppendChunk(mChunk);
       }
@@ -56,7 +64,9 @@ class AnalyserNodeEngine : public AudioNodeEngine
   {
     *aOutput = aInput;
 
-    if (mNode &&
+    MutexAutoLock lock(NodeMutex());
+
+    if (Node() &&
         aInput.mChannelData.Length() > 0) {
       nsRefPtr<TransferBuffer> transfer = new TransferBuffer(aStream, aInput);
       NS_DispatchToMainThread(transfer);

content/media/webaudio/AudioNode.cpp

@@ -205,11 +205,17 @@ void
 AudioNode::DestroyMediaStream()
 {
   if (mStream) {
-    // Remove the node reference on the engine
-    AudioNodeStream* ns = static_cast<AudioNodeStream*>(mStream.get());
-    MOZ_ASSERT(ns, "How come we don't have a stream here?");
-    MOZ_ASSERT(ns->Engine()->mNode == this, "Invalid node reference");
-    ns->Engine()->mNode = nullptr;
+    {
+      // Remove the node reference on the engine, and take care to not
+      // hold the lock when the stream gets destroyed, because that will
+      // cause the engine to be destroyed as well, and we don't want to
+      // be holding the lock as we're trying to destroy it!
+      AudioNodeStream* ns = static_cast<AudioNodeStream*>(mStream.get());
+      MutexAutoLock lock(ns->Engine()->NodeMutex());
+      MOZ_ASSERT(ns, "How come we don't have a stream here?");
+      MOZ_ASSERT(ns->Engine()->Node() == this, "Invalid node reference");
+      ns->Engine()->ClearNode();
+    }
 
     mStream->Destroy();
     mStream = nullptr;

content/media/webaudio/DelayNode.cpp

@@ -37,7 +37,15 @@ class DelayNodeEngine : public AudioNodeEngine
 
     NS_IMETHOD Run()
     {
-      nsRefPtr<DelayNode> node = static_cast<DelayNode*>(mStream->Engine()->Node());
+      nsRefPtr<DelayNode> node;
+      {
+        // No need to keep holding the lock for the whole duration of this
+        // function, since we're holding a strong reference to it, so if
+        // we can obtain the reference, we will hold the node alive in
+        // this function.
+        MutexAutoLock lock(mStream->Engine()->NodeMutex());
+        node = static_cast<DelayNode*>(mStream->Engine()->Node());
+      }
       if (node) {
         if (mChange == ADDREF) {
           node->mPlayingRef.Take(node);

content/media/webaudio/DynamicsCompressorNode.cpp

@@ -140,7 +140,15 @@ class DynamicsCompressorNodeEngine : public AudioNodeEngine
 
       NS_IMETHODIMP Run()
       {
-        nsRefPtr<DynamicsCompressorNode> node = static_cast<DynamicsCompressorNode*>(mStream->Engine()->Node());
+        nsRefPtr<DynamicsCompressorNode> node;
+        {
+          // No need to keep holding the lock for the whole duration of this
+          // function, since we're holding a strong reference to it, so if
+          // we can obtain the reference, we will hold the node alive in
+          // this function.
+          MutexAutoLock lock(mStream->Engine()->NodeMutex());
+          node = static_cast<DynamicsCompressorNode*>(mStream->Engine()->Node());
+        }
         if (node) {
           AudioParam* reduction = node->Reduction();
           reduction->CancelAllEvents();

content/media/webaudio/ScriptProcessorNode.cpp

@@ -188,8 +188,10 @@ class ScriptProcessorNodeEngine : public AudioNodeEngine
                                  AudioChunk* aOutput,
                                  bool* aFinished) MOZ_OVERRIDE
   {
+    MutexAutoLock lock(NodeMutex());
+
     // If our node is dead, just output silence
-    if (!mNode) {
+    if (!Node()) {
       aOutput->SetNull(WEBAUDIO_BLOCK_SIZE);
       return;
     }
@@ -275,7 +277,15 @@ class ScriptProcessorNodeEngine : public AudioNodeEngine
           return NS_OK;
         }
 
-        nsRefPtr<ScriptProcessorNode> node = static_cast<ScriptProcessorNode*>(mStream->Engine()->Node());
+        nsRefPtr<ScriptProcessorNode> node;
+        {
+          // No need to keep holding the lock for the whole duration of this
+          // function, since we're holding a strong reference to it, so if
+          // we can obtain the reference, we will hold the node alive in
+          // this function.
+          MutexAutoLock lock(mStream->Engine()->NodeMutex());
+          node = static_cast<ScriptProcessorNode*>(mStream->Engine()->Node());
+        }
         if (!node) {
           return NS_OK;
         }