Ensure AudioPropertyListener callbacks happen on the browser thread.

Per discussion with shess, there may be issues when issuing calls
which modify OSX's HAL state while it's in the middle of calling us
back on another thread.

Adding some print statements shows that the property callbacks are
driven by the NSApplication pump on the base thread (in this case
that's BrowserMainLoop).  Creating the listeners on the audio thread
did not cause callbacks to occur on the audio thread.

As such this change simply ensures that our device notification won't
run until after the NSApplication pump completes its current cycle.

BUG=158170
TEST=device changes continue to work.

Review URL: https://codereview.chromium.org/11348081

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

2 files changed, 42 insertions, 18 deletions

diff --git a/media/audio/mac/audio_manager_mac.cc b/media/audio/mac/audio_manager_mac.cc
index 9cd8924..4197c8f 100644
--- a/media/audio/mac/audio_manager_mac.cc
+++ b/media/audio/mac/audio_manager_mac.cc
@@ -2,8 +2,9 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include <CoreAudio/AudioHardware.h>
+#include "media/audio/mac/audio_manager_mac.h"
 
+#include <CoreAudio/AudioHardware.h>
 #include <string>
 
 #include "base/bind.h"
@@ -14,7 +15,6 @@
 #include "media/audio/mac/audio_input_mac.h"
 #include "media/audio/mac/audio_low_latency_input_mac.h"
 #include "media/audio/mac/audio_low_latency_output_mac.h"
-#include "media/audio/mac/audio_manager_mac.h"
 #include "media/audio/mac/audio_output_mac.h"
 #include "media/audio/mac/audio_synchronized_mac.h"
 #include "media/audio/mac/audio_unified_mac.h"
@@ -239,9 +239,8 @@ static const AudioObjectPropertyAddress kDeviceChangePropertyAddress = {
   kAudioObjectPropertyElementMaster
 };
 
-// Callback from the system when the default device changes.  This can be called
-// either on the main thread or on an audio thread managed by the system
-// depending on what kAudioHardwarePropertyRunLoop is set to.
+// Callback from the system when the default device changes; this must be called
+// on the MessageLoop that created the AudioManager.
 static OSStatus OnDefaultDeviceChangedCallback(
     AudioObjectID object,
     UInt32 num_addresses,
@@ -255,40 +254,49 @@ static OSStatus OnDefaultDeviceChangedCallback(
         addresses[i].mScope == kDeviceChangePropertyAddress.mScope &&
         addresses[i].mElement == kDeviceChangePropertyAddress.mElement &&
         context) {
-      static_cast<base::Closure*>(context)->Run();
+      static_cast<AudioManagerMac*>(context)->OnDeviceChange();
+      break;
     }
   }
 
   return noErr;
 }
 
-AudioManagerMac::AudioManagerMac() {
+AudioManagerMac::AudioManagerMac()
+    : listener_registered_(false),
+      creating_message_loop_(base::MessageLoopProxy::current()) {
   SetMaxOutputStreamsAllowed(kMaxOutputStreams);
 
-  // Register a callback for device changes.
-  listener_cb_ = BindToLoop(GetMessageLoop(), base::Bind(
-      &AudioManagerMac::NotifyAllOutputDeviceChangeListeners,
-      base::Unretained(this)));
+  // AudioManagerMac is expected to be created by the root platform thread, this
+  // is generally BrowserMainLoop, it's MessageLoop will drive the NSApplication
+  // pump which in turn fires the property listener callbacks.
+  if (!creating_message_loop_)
+    return;
 
   OSStatus result = AudioObjectAddPropertyListener(
       kAudioObjectSystemObject,
       &kDeviceChangePropertyAddress,
       &OnDefaultDeviceChangedCallback,
-      &listener_cb_);
+      this);
 
   if (result != noErr) {
     OSSTATUS_DLOG(ERROR, result) << "AudioObjectAddPropertyListener() failed!";
-    listener_cb_.Reset();
+    return;
   }
+
+  listener_registered_ = true;
 }
 
 AudioManagerMac::~AudioManagerMac() {
-  if (!listener_cb_.is_null()) {
+  if (listener_registered_) {
+    // TODO(dalecurtis): CHECK destruction happens on |creating_message_loop_|,
+    // should be true, but currently several unit tests perform destruction in
+    // odd places so we can't CHECK here currently.
     OSStatus result = AudioObjectRemovePropertyListener(
         kAudioObjectSystemObject,
         &kDeviceChangePropertyAddress,
         &OnDefaultDeviceChangedCallback,
-        &listener_cb_);
+        this);
     OSSTATUS_DLOG_IF(ERROR, result != noErr, result)
         << "AudioObjectRemovePropertyListener() failed!";
   }
@@ -363,6 +371,18 @@ AudioInputStream* AudioManagerMac::MakeLowLatencyInputStream(
   return stream;
 }
 
+void AudioManagerMac::OnDeviceChange() {
+  CHECK(creating_message_loop_->BelongsToCurrentThread());
+
+  // Post the task to the |creating_message_loop_| to execute our listener
+  // callback.  The callback is created using BindToLoop() so will hop over
+  // to the audio thread upon execution.
+  creating_message_loop_->PostTask(FROM_HERE, BindToLoop(
+      GetMessageLoop(), base::Bind(
+          &AudioManagerMac::NotifyAllOutputDeviceChangeListeners,
+          base::Unretained(this))));
+}
+
 AudioManager* CreateAudioManager() {
   return new AudioManagerMac();
 }
diff --git a/media/audio/mac/audio_manager_mac.h b/media/audio/mac/audio_manager_mac.h
index 085c679..80db260 100644
--- a/media/audio/mac/audio_manager_mac.h
+++ b/media/audio/mac/audio_manager_mac.h
@@ -6,8 +6,8 @@
 #define MEDIA_AUDIO_MAC_AUDIO_MANAGER_MAC_H_
 
 #include "base/basictypes.h"
-#include "base/callback.h"
 #include "base/compiler_specific.h"
+#include "base/message_loop_proxy.h"
 #include "media/audio/audio_manager_base.h"
 
 namespace media {
@@ -35,12 +35,16 @@ class MEDIA_EXPORT AudioManagerMac : public AudioManagerBase {
   virtual AudioInputStream* MakeLowLatencyInputStream(
       const AudioParameters& params, const std::string& device_id) OVERRIDE;
 
+  // Called by an internal device change listener.  Must be called on
+  // |creating_message_loop_|.
+  void OnDeviceChange();
+
  protected:
   virtual ~AudioManagerMac();
 
  private:
-  // Listens for output device changes.
-  base::Closure listener_cb_;
+  bool listener_registered_;
+  scoped_refptr<base::MessageLoopProxy> creating_message_loop_;
 
   DISALLOW_COPY_AND_ASSIGN(AudioManagerMac);
 };