Crash/stability fix for device change logic in AudioOutputController. The outstanding weak pointers need to be invalidated when streams are closed/re-created in order to auto-cancel the "polling starts."

Note: This is part one of a two-part patch.  This encompasses changes which can only be applied to M24 and later.  See http://codereview.chromium.org/11741010 for a more detailed description of the root cause and solution.

BUG=167920
TEST=Ran media_unittests.
TBR=dalecurtis@chromium.org


Review URL: https://chromiumcodereview.appspot.com/11753008

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

2 files changed, 15 insertions, 18 deletions

diff --git a/media/audio/audio_output_controller.cc b/media/audio/audio_output_controller.cc
index 50850c9..a9dd2c8 100644
--- a/media/audio/audio_output_controller.cc
+++ b/media/audio/audio_output_controller.cc
@@ -119,7 +119,8 @@ void AudioOutputController::DoCreate() {
     return;
   DCHECK(state_ == kEmpty || state_ == kRecreating) << state_;
 
-  DoStopCloseAndClearStream(NULL);
+  DoStopCloseAndClearStream(NULL);  // Calls RemoveOutputDeviceChangeListener().
+
   stream_ = audio_manager_->MakeAudioOutputStreamProxy(params_);
   if (!stream_) {
     state_ = kError;
@@ -140,8 +141,7 @@ void AudioOutputController::DoCreate() {
 
   // Everything started okay, so register for state change callbacks if we have
   // not already done so.
-  if (state_ != kRecreating)
-    audio_manager_->AddOutputDeviceChangeListener(this);
+  audio_manager_->AddOutputDeviceChangeListener(this);
 
   // We have successfully opened the stream. Set the initial volume.
   stream_->SetVolume(volume_);
@@ -177,6 +177,7 @@ void AudioOutputController::DoPlay() {
   // TODO(vrk): The polling here and in WaitTillDataReady() is pretty clunky.
   // Refine the API such that polling is no longer needed. (crbug.com/112196)
   number_polling_attempts_left_ = kPollNumAttempts;
+  DCHECK(!weak_this_.HasWeakPtrs());
   message_loop_->PostDelayedTask(
       FROM_HERE,
       base::Bind(&AudioOutputController::PollAndStartIfDataReady,
@@ -348,13 +349,12 @@ void AudioOutputController::DoStopCloseAndClearStream(WaitableEvent* done) {
 
   // Allow calling unconditionally and bail if we don't have a stream_ to close.
   if (stream_) {
+    audio_manager_->RemoveOutputDeviceChangeListener(this);
+
     stream_->Stop();
     stream_->Close();
     stream_ = NULL;
 
-    audio_manager_->RemoveOutputDeviceChangeListener(this);
-    audio_manager_ = NULL;
-
     weak_this_.InvalidateWeakPtrs();
   }
 
@@ -369,13 +369,9 @@ void AudioOutputController::OnDeviceChange() {
   // We should always have a stream by this point.
   CHECK(stream_);
 
-  // Preserve the original state and shutdown the stream.
-  State original_state = state_;
-  stream_->Stop();
-  stream_->Close();
-  stream_ = NULL;
-
-  // Recreate the stream, exit if we ran into an error.
+  // Recreate the stream (DoCreate() will first shut down an existing stream).
+  // Exit if we ran into an error.
+  const State original_state = state_;
   state_ = kRecreating;
   DoCreate();
   if (!stream_ || state_ == kError)
diff --git a/media/audio/audio_output_controller.h b/media/audio/audio_output_controller.h
index 762a948..61718a3 100644
--- a/media/audio/audio_output_controller.h
+++ b/media/audio/audio_output_controller.h
@@ -196,10 +196,13 @@ class MEDIA_EXPORT AudioOutputController
   // Signals event when done if it is not NULL.
   void DoStopCloseAndClearStream(base::WaitableEvent *done);
 
-  AudioManager* audio_manager_;
+  AudioManager* const audio_manager_;
 
   // |handler_| may be called only if |state_| is not kClosed.
   EventHandler* handler_;
+
+  // Note: It's important to invalidate the weak pointers whenever stream_ is
+  // changed.  See comment for weak_this_.
   AudioOutputStream* stream_;
 
   // The current volume of the audio stream.
@@ -226,10 +229,8 @@ class MEDIA_EXPORT AudioOutputController
 
   AudioParameters params_;
 
-  // Used to post delayed tasks to ourselves that we can cancel.
-  // We don't want the tasks to hold onto a reference as it will slow down
-  // shutdown and force it to wait for the most delayed task.
-  // Also, if we're shutting down, we do not want to poll for more data.
+  // Used to auto-cancel the delayed tasks that are created to poll for data
+  // (when starting-up a stream).
   base::WeakPtrFactory<AudioOutputController> weak_this_;
 
   DISALLOW_COPY_AND_ASSIGN(AudioOutputController);