Fix AudioEntry destruction order. Add debugging CHECKs for racy shutdown.

Destruction order should be harmless, but should be cleaned up. CHECKs
should tell us if the controller is being shutdown while callbacks are in flight.

BUG=349651
TEST=none

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

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

content/browser/renderer_host/media/audio_renderer_host.cc

@@ -74,18 +74,20 @@ class AudioRendererHost::AudioEntry
   // The routing ID of the source render view.
   const int render_view_id_;
 
-  // The AudioOutputController that manages the audio stream.
-  const scoped_refptr<media::AudioOutputController> controller_;
-
-  // Shared memory for transmission of the audio data.
+  // Shared memory for transmission of the audio data.  Used by |reader_|.
   const scoped_ptr<base::SharedMemory> shared_memory_;
 
-  // The synchronous reader to be used by the controller.
+  // The synchronous reader to be used by |controller_|.
   const scoped_ptr<media::AudioOutputController::SyncReader> reader_;
+
+  // The AudioOutputController that manages the audio stream.
+  const scoped_refptr<media::AudioOutputController> controller_;
 };
 
 AudioRendererHost::AudioEntry::AudioEntry(
-    AudioRendererHost* host, int stream_id, int render_view_id,
+    AudioRendererHost* host,
+    int stream_id,
+    int render_view_id,
     const media::AudioParameters& params,
     const std::string& output_device_id,
     scoped_ptr<base::SharedMemory> shared_memory,
@@ -93,10 +95,13 @@ AudioRendererHost::AudioEntry::AudioEntry(
     : host_(host),
       stream_id_(stream_id),
       render_view_id_(render_view_id),
-      controller_(media::AudioOutputController::Create(
-          host->audio_manager_, this, params, output_device_id, reader.get())),
       shared_memory_(shared_memory.Pass()),
-      reader_(reader.Pass()) {
+      reader_(reader.Pass()),
+      controller_(media::AudioOutputController::Create(host->audio_manager_,
+                                                       this,
+                                                       params,
+                                                       output_device_id,
+                                                       reader_.get())) {
   DCHECK(controller_.get());
 }
 

media/audio/audio_output_controller.cc

@@ -45,7 +45,7 @@ AudioOutputController::AudioOutputController(
       diverting_to_stream_(NULL),
       volume_(1.0),
       state_(kEmpty),
-      num_allowed_io_(0),
+      not_currently_in_on_more_io_data_(1),
       sync_reader_(sync_reader),
       message_loop_(audio_manager->GetTaskRunner()),
 #if defined(AUDIO_POWER_MONITORING)
@@ -62,6 +62,8 @@ AudioOutputController::AudioOutputController(
 
 AudioOutputController::~AudioOutputController() {
   DCHECK_EQ(kClosed, state_);
+  // TODO(dalecurtis): Remove debugging for http://crbug.com/349651
+  CHECK(!base::AtomicRefCountDec(&not_currently_in_on_more_io_data_));
 }
 
 // static
@@ -191,8 +193,6 @@ void AudioOutputController::DoPlay() {
   power_poll_callback_.callback().Run();
 #endif
 
-  on_more_io_data_called_ = 0;
-  AllowEntryToOnMoreIOData();
   stream_->Start(this);
 
   // For UMA tracking purposes, start the wedge detection timer.  This allows us
@@ -232,7 +232,6 @@ void AudioOutputController::StopStream() {
   if (state_ == kPlaying) {
     wedge_timer_.reset();
     stream_->Stop();
-    DisallowEntryToOnMoreIOData();
 
 #if defined(AUDIO_POWER_MONITORING)
     power_poll_callback_.Cancel();
@@ -334,7 +333,7 @@ int AudioOutputController::OnMoreData(AudioBus* dest,
 int AudioOutputController::OnMoreIOData(AudioBus* source,
                                         AudioBus* dest,
                                         AudioBuffersState buffers_state) {
-  DisallowEntryToOnMoreIOData();
+  CHECK(!base::AtomicRefCountDec(&not_currently_in_on_more_io_data_));
   TRACE_EVENT0("audio", "AudioOutputController::OnMoreIOData");
 
   // Indicate that we haven't wedged (at least not indefinitely, WedgeCheck()
@@ -354,7 +353,7 @@ int AudioOutputController::OnMoreIOData(AudioBus* source,
   power_monitor_.Scan(*dest, frames);
 #endif
 
-  AllowEntryToOnMoreIOData();
+  base::AtomicRefCountInc(&not_currently_in_on_more_io_data_);
   return frames;
 }
 
@@ -456,16 +455,6 @@ void AudioOutputController::DoStopDiverting() {
   DCHECK(!diverting_to_stream_);
 }
 
-void AudioOutputController::AllowEntryToOnMoreIOData() {
-  DCHECK(base::AtomicRefCountIsZero(&num_allowed_io_));
-  base::AtomicRefCountInc(&num_allowed_io_);
-}
-
-void AudioOutputController::DisallowEntryToOnMoreIOData() {
-  const bool is_zero = !base::AtomicRefCountDec(&num_allowed_io_);
-  DCHECK(is_zero);
-}
-
 void AudioOutputController::WedgeCheck() {
   DCHECK(message_loop_->BelongsToCurrentThread());
 

media/audio/audio_output_controller.h

@@ -215,11 +215,6 @@ class MEDIA_EXPORT AudioOutputController
   // Helper method that stops, closes, and NULLs |*stream_|.
   void DoStopCloseAndClearStream();
 
-  // Sanity-check that entry/exit to OnMoreIOData() by the hardware audio thread
-  // happens only between AudioOutputStream::Start() and Stop().
-  void AllowEntryToOnMoreIOData();
-  void DisallowEntryToOnMoreIOData();
-
   // Checks if a stream was started successfully but never calls OnMoreIOData().
   void WedgeCheck();
 
@@ -244,12 +239,10 @@ class MEDIA_EXPORT AudioOutputController
   // is not required for reading on the audio manager thread.
   State state_;
 
-  // Binary semaphore, used to ensure that only one thread enters the
-  // OnMoreIOData() method, and only when it is valid to do so.  This is for
-  // sanity-checking the behavior of platform implementations of
-  // AudioOutputStream.  In other words, multiple contention is not expected,
-  // nor in the design here.
-  base::AtomicRefCount num_allowed_io_;
+  // Atomic ref count indicating when when we're in the middle of handling an
+  // OnMoreIOData() callback.  Will be CHECK'd to find crashes.
+  // TODO(dalecurtis): Remove debug helpers for http://crbug.com/349651
+  base::AtomicRefCount not_currently_in_on_more_io_data_;
 
   // SyncReader is used only in low latency mode for synchronous reading.
   SyncReader* const sync_reader_;