Add a lock around getting and setting of source_ to avoid possible compiler reorderings.

Also makes it very explicit that multi-threaded access is occuring.

BUG=24801
TEST=will see how TSan handles this.


Review URL: http://codereview.chromium.org/7888055

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@102067 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 32 insertions, 11 deletions

diff --git a/media/audio/mac/audio_output_mac.cc b/media/audio/mac/audio_output_mac.cc
index a81dae0..02e8c63 100644
--- a/media/audio/mac/audio_output_mac.cc
+++ b/media/audio/mac/audio_output_mac.cc
@@ -91,7 +91,7 @@ void PCMQueueOutAudioOutputStream::HandleError(OSStatus err) {
   // source_ can be set to NULL from another thread. We need to cache its
   // pointer while we operate here. Note that does not mean that the source
   // has been destroyed.
-  AudioSourceCallback* source = source_;
+  AudioSourceCallback* source = GetSource();
   if (source)
     source->OnError(this, static_cast<int>(err));
   NOTREACHED() << "error code " << err;
@@ -157,7 +157,7 @@ bool PCMQueueOutAudioOutputStream::Open() {
   // Create the actual queue object and let the OS use its own thread to
   // run its CFRunLoop.
   err = AudioQueueNewOutput(&format_, RenderCallback, this, NULL,
-                                     kCFRunLoopCommonModes, 0, &audio_queue_);
+                            kCFRunLoopCommonModes, 0, &audio_queue_);
   if (err != noErr) {
     HandleError(err);
     return false;
@@ -314,7 +314,8 @@ void PCMQueueOutAudioOutputStream::Close() {
 void PCMQueueOutAudioOutputStream::Stop() {
   // We request a synchronous stop, so the next call can take some time. In
   // the windows implementation we block here as well.
-  source_ = NULL;
+  SetSource(NULL);
+
   // We set the source to null to signal to the data queueing thread it can stop
   // queueing data, however at most one callback might still be in flight which
   // could attempt to enqueue right after the next call. Rather that trying to
@@ -378,10 +379,10 @@ bool PCMQueueOutAudioOutputStream::CheckForAdjustedLayout(
   return false;
 }
 
-// Note to future hackers of this function: Do not add locks here because we
-// call out to third party source that might do crazy things including adquire
-// external locks or somehow re-enter here because its legal for them to call
-// some audio functions.
+// Note to future hackers of this function: Do not add locks to this function
+// that are held through any calls made back into AudioQueue APIs, or other
+// OS audio functions.  This is because the OS dispatch may grab external
+// locks, or possibly re-enter this function which can lead to a deadlock.
 void PCMQueueOutAudioOutputStream::RenderCallback(void* p_this,
                                                   AudioQueueRef queue,
                                                   AudioQueueBufferRef buffer) {
@@ -391,7 +392,7 @@ void PCMQueueOutAudioOutputStream::RenderCallback(void* p_this,
       static_cast<PCMQueueOutAudioOutputStream*>(p_this);
   // Call the audio source to fill the free buffer with data. Not having a
   // source means that the queue has been closed. This is not an error.
-  AudioSourceCallback* source = audio_stream->source_;
+  AudioSourceCallback* source = audio_stream->GetSource();
   if (!source)
     return;
 
@@ -467,8 +468,9 @@ void PCMQueueOutAudioOutputStream::RenderCallback(void* p_this,
     if (err == kAudioQueueErr_EnqueueDuringReset) {
       // This is the error you get if you try to enqueue a buffer and the
       // queue has been closed. Not really a problem if indeed the queue
-      // has been closed.
-      if (!audio_stream->source_)
+      // has been closed.  We recheck the value of source now to see if it has
+      // indeed been closed.
+      if (!audio_stream->GetSource())
         return;
     }
     audio_stream->HandleError(err);
@@ -478,7 +480,7 @@ void PCMQueueOutAudioOutputStream::RenderCallback(void* p_this,
 void PCMQueueOutAudioOutputStream::Start(AudioSourceCallback* callback) {
   DCHECK(callback);
   OSStatus err = noErr;
-  source_ = callback;
+  SetSource(callback);
   pending_bytes_ = 0;
   // Ask the source to pre-fill all our buffers before playing.
   for (uint32 ix = 0; ix != kNumBuffers; ++ix) {
@@ -500,3 +502,14 @@ void PCMQueueOutAudioOutputStream::Start(AudioSourceCallback* callback) {
     return;
   }
 }
+
+void PCMQueueOutAudioOutputStream::SetSource(AudioSourceCallback* source) {
+  base::AutoLock lock(source_lock_);
+  source_ = source;
+}
+
+AudioOutputStream::AudioSourceCallback*
+PCMQueueOutAudioOutputStream::GetSource() {
+  base::AutoLock lock(source_lock_);
+  return source_;
+}
diff --git a/media/audio/mac/audio_output_mac.h b/media/audio/mac/audio_output_mac.h
index 031dc3f..8d18c3e 100644
--- a/media/audio/mac/audio_output_mac.h
+++ b/media/audio/mac/audio_output_mac.h
@@ -9,6 +9,7 @@
 #include <AudioToolbox/AudioQueue.h>
 #include <AudioUnit/AudioUnit.h>
 
+#include "base/synchronization/lock.h"
 #include "media/audio/audio_io.h"
 #include "media/audio/audio_parameters.h"
 
@@ -52,12 +53,19 @@ class PCMQueueOutAudioOutputStream : public AudioOutputStream {
   // Called when an error occurs.
   void HandleError(OSStatus err);
 
+  // Atomic operations for setting/getting the source callback.
+  void SetSource(AudioSourceCallback* source);
+  AudioSourceCallback* GetSource();
+
   // Structure that holds the stream format details such as bitrate.
   AudioStreamBasicDescription format_;
   // Handle to the OS audio queue object.
   AudioQueueRef audio_queue_;
   // Array of pointers to the OS managed audio buffers.
   AudioQueueBufferRef buffer_[kNumBuffers];
+  // Mutex for the |source_| to implment atomic set and get.
+  // It is important to NOT wait on any other locks while this is held.
+  base::Lock source_lock_;
   // Pointer to the object that will provide the audio samples.
   AudioSourceCallback* source_;
   // Our creator, the audio manager needs to be notified when we close.