Bug 864709 - Part 2: Protect accesses to AudioNodeEngine::mNode using a lock; r=padenot
authorEhsan Akhgari <ehsan@mozilla.com>
Wed, 24 Apr 2013 12:13:45 -0400
changeset 129767 5eeaecafb9896020e3ed7d9fc8ae41f6967322bb
parent 129766 4e929e5050330a207e2abfbde5a0a32d4834ac84
child 129768 f87f14e8949faf0c40f219aeeacc00784f011918
push id1
push userroot
push dateMon, 20 Oct 2014 17:29:22 +0000
reviewerspadenot
bugs864709
milestone23.0a1
Bug 864709 - Part 2: Protect accesses to AudioNodeEngine::mNode using a lock; r=padenot
content/media/AudioNodeEngine.h
content/media/webaudio/AnalyserNode.cpp
content/media/webaudio/AudioNode.cpp
content/media/webaudio/DelayNode.cpp
content/media/webaudio/DynamicsCompressorNode.cpp
content/media/webaudio/ScriptProcessorNode.cpp
--- a/content/media/AudioNodeEngine.h
+++ b/content/media/AudioNodeEngine.h
@@ -3,16 +3,17 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 #ifndef MOZILLA_AUDIONODEENGINE_H_
 #define MOZILLA_AUDIONODEENGINE_H_
 
 #include "AudioSegment.h"
 #include "mozilla/dom/AudioParam.h"
+#include "mozilla/Mutex.h"
 
 namespace mozilla {
 
 namespace dom {
 class AudioNode;
 struct ThreeDPoint;
 }
 
@@ -146,16 +147,17 @@ AudioBlockPanStereoToStereo(const float 
 /**
  * All methods of this class and its subclasses are called on the
  * MediaStreamGraph thread.
  */
 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);
   }
   virtual ~AudioNodeEngine()
   {
     MOZ_ASSERT(!mNode, "The node reference must be already cleared");
     MOZ_COUNT_DTOR(AudioNodeEngine);
@@ -201,22 +203,32 @@ public:
   virtual void ProduceAudioBlock(AudioNodeStream* aStream,
                                  const AudioChunk& aInput,
                                  AudioChunk* aOutput,
                                  bool* aFinished)
   {
     *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;
 };
 
 }
 
 #endif /* MOZILLA_AUDIONODEENGINE_H_ */
--- a/content/media/webaudio/AnalyserNode.cpp
+++ b/content/media/webaudio/AnalyserNode.cpp
@@ -25,17 +25,25 @@ class AnalyserNodeEngine : public AudioN
                    const AudioChunk& aChunk)
       : mStream(aStream)
       , mChunk(aChunk)
     {
     }
 
     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);
       }
       return NS_OK;
     }
 
   private:
     nsRefPtr<AudioNodeStream> mStream;
@@ -51,17 +59,19 @@ public:
 
   virtual void ProduceAudioBlock(AudioNodeStream* aStream,
                                  const AudioChunk& aInput,
                                  AudioChunk* aOutput,
                                  bool* aFinished)
   {
     *aOutput = aInput;
 
-    if (mNode &&
+    MutexAutoLock lock(NodeMutex());
+
+    if (Node() &&
         aInput.mChannelData.Length() > 0) {
       nsRefPtr<TransferBuffer> transfer = new TransferBuffer(aStream, aInput);
       NS_DispatchToMainThread(transfer);
     }
   }
 };
 
 AnalyserNode::AnalyserNode(AudioContext* aContext)
--- a/content/media/webaudio/AudioNode.cpp
+++ b/content/media/webaudio/AudioNode.cpp
@@ -200,21 +200,27 @@ AudioNode::Disconnect(uint32_t aOutput, 
   // This disconnection may have disconnected a panner and a source.
   Context()->UpdatePannerSource();
 }
 
 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;
   }
 }
 
 }
 }
--- a/content/media/webaudio/DelayNode.cpp
+++ b/content/media/webaudio/DelayNode.cpp
@@ -32,17 +32,25 @@ class DelayNodeEngine : public AudioNode
     PlayingRefChanged(AudioNodeStream* aStream, ChangeType aChange)
       : mStream(aStream)
       , mChange(aChange)
     {
     }
 
     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);
         } else if (mChange == RELEASE) {
           node->mPlayingRef.Drop(node);
         }
       }
       return NS_OK;
--- a/content/media/webaudio/DynamicsCompressorNode.cpp
+++ b/content/media/webaudio/DynamicsCompressorNode.cpp
@@ -135,17 +135,25 @@ private:
       Command(AudioNodeStream* aStream, float aReduction)
         : mStream(aStream)
         , mReduction(aReduction)
       {
       }
 
       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();
           reduction->SetValue(mReduction);
         }
         return NS_OK;
       }
 
--- a/content/media/webaudio/ScriptProcessorNode.cpp
+++ b/content/media/webaudio/ScriptProcessorNode.cpp
@@ -183,18 +183,20 @@ public:
     mSource = aSource;
   }
 
   virtual void ProduceAudioBlock(AudioNodeStream* aStream,
                                  const AudioChunk& aInput,
                                  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;
     }
 
     // First, record our input buffer
     for (uint32_t i = 0; i < mInputChannels.Length(); ++i) {
       if (aInput.IsNull()) {
         PodZero(mInputChannels[i] + mInputWriteIndex,
@@ -270,17 +272,25 @@ private:
       NS_IMETHODIMP Run()
       {
         // If it's not safe to run scripts right now, schedule this to run later
         if (!nsContentUtils::IsSafeToRunScript()) {
           nsContentUtils::AddScriptRunner(this);
           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;
         }
 
         AutoPushJSContext cx(node->Context()->GetJSContext());
         if (cx) {
           JSAutoRequest ar(cx);