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

media/audio/audio_output_controller.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/audio/audio_output_controller.h"
 
 #include "base/bind.h"
 #include "base/debug/trace_event.h"
 #include "base/metrics/histogram.h"
 #include "base/task_runner_util.h"
@@ skipping 27 matching lines @@
     const std::string& output_device_id,
     SyncReader* sync_reader)
     : audio_manager_(audio_manager),
       params_(params),
       handler_(handler),
       output_device_id_(output_device_id),
       stream_(NULL),
       diverting_to_stream_(NULL),
       volume_(1.0),
       state_(kEmpty),
-      num_allowed_io_(0),
+      currently_in_on_more_io_data_(0),
       sync_reader_(sync_reader),
       message_loop_(audio_manager->GetTaskRunner()),
 #if defined(AUDIO_POWER_MONITORING)
       power_monitor_(
           params.sample_rate(),
           TimeDelta::FromMilliseconds(kPowerMeasurementTimeConstantMillis)),
 #endif
       on_more_io_data_called_(0) {
   DCHECK(audio_manager);
   DCHECK(handler_);
   DCHECK(sync_reader_);
   DCHECK(message_loop_.get());
 }
 
 AudioOutputController::~AudioOutputController() {
   DCHECK_EQ(kClosed, state_);
+  // TODO(dalecurtis): Remove debugging for http://crbug.com/349651
+  CHECK(base::AtomicRefCountIsZero(&currently_in_on_more_io_data_));

[acolwell GONE FROM CHROMIUM, 2014/03/06 20:54:10]

I'm a little unclear why we need this. Who owns this object and how are
OnMoreIOData() calls allowed without something holding a reference to this? The
need for this makes me think we don't really have a good understanding of
ownership & lifetime here.

[DaleCurtis, 2014/03/06 21:21:11]

See audio_io.h and the AudioSourceCallback interface.  For better or worse,
we're handing out raw pointers back to this class which are used by clients
closer to the hardware to request audio data.

I'd love to remove ref-counting from AudioOutputController, but there are
various consumers still relying on this behavior; notably WebrtcAudioPrivate :( 
Cleaning these up would be non-trivial.

What I suspect is happening is that there's a stale SourceCallback lingering, I
want the destruction trace to get crash reports that have both the offender and
the destruction on the stack.

 }
 
 // static
 scoped_refptr<AudioOutputController> AudioOutputController::Create(
     AudioManager* audio_manager,
     EventHandler* event_handler,
     const AudioParameters& params,
     const std::string& output_device_id,
     SyncReader* sync_reader) {
   DCHECK(audio_manager);
@@ skipping 109 matching lines @@
 #if defined(AUDIO_POWER_MONITORING)
   power_monitor_.Reset();
   power_poll_callback_.Reset(
       base::Bind(&AudioOutputController::ReportPowerMeasurementPeriodically,
                  this));
   // Run the callback to send an initial notification that we're starting in
   // silence, and to schedule periodic callbacks.
   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
   // to record statistics about the number of wedged playbacks in the field.
   //
   // WedgeCheck() will look to see if |on_more_io_data_called_| is true after
   // the timeout expires.  Care must be taken to ensure the wedge check delay is
   // large enough that the value isn't queried while OnMoreDataIO() is setting
   // it.
   //
@@ skipping 19 matching lines @@
       TimeDelta::FromSeconds(1) / kPowerMeasurementsPerSecond);
 }
 #endif
 
 void AudioOutputController::StopStream() {
   DCHECK(message_loop_->BelongsToCurrentThread());
 
   if (state_ == kPlaying) {
     wedge_timer_.reset();
     stream_->Stop();
-    DisallowEntryToOnMoreIOData();
 
 #if defined(AUDIO_POWER_MONITORING)
     power_poll_callback_.Cancel();
 #endif
 
     state_ = kPaused;
   }
 }
 
 void AudioOutputController::DoPause() {
@@ skipping 81 matching lines @@
 }
 
 int AudioOutputController::OnMoreData(AudioBus* dest,
                                       AudioBuffersState buffers_state) {
   return OnMoreIOData(NULL, dest, buffers_state);
 }
 
 int AudioOutputController::OnMoreIOData(AudioBus* source,
                                         AudioBus* dest,
                                         AudioBuffersState buffers_state) {
-  DisallowEntryToOnMoreIOData();
+  base::AtomicRefCountInc(&currently_in_on_more_io_data_);

[acolwell GONE FROM CHROMIUM, 2014/03/06 20:54:10]

If you make the initial value 1 and dec instead of inc, you could CHECK here and
crash before allowing access to any of the vars. You could then dec & check in
the destructor which would cause all future calls here to crash immediately.

[DaleCurtis, 2014/03/06 21:21:11]

Done.

   TRACE_EVENT0("audio", "AudioOutputController::OnMoreIOData");
 
   // Indicate that we haven't wedged (at least not indefinitely, WedgeCheck()
   // may have already fired if OnMoreIOData() took an abnormal amount of time).
   // Since this thread is the only writer of |on_more_io_data_called_| once the
   // thread starts, its safe to compare and then increment.
   if (base::AtomicRefCountIsZero(&on_more_io_data_called_))
     base::AtomicRefCountInc(&on_more_io_data_called_);
 
   sync_reader_->Read(source, dest);
 
   const int frames = dest->frames();
   sync_reader_->UpdatePendingBytes(
       buffers_state.total_bytes() + frames * params_.GetBytesPerFrame());
 
 #if defined(AUDIO_POWER_MONITORING)
   power_monitor_.Scan(*dest, frames);
 #endif
 
-  AllowEntryToOnMoreIOData();
+  CHECK(!base::AtomicRefCountDec(&currently_in_on_more_io_data_));
   return frames;
 }
 
 void AudioOutputController::OnError(AudioOutputStream* stream) {
   // Handle error on the audio controller thread.
   message_loop_->PostTask(FROM_HERE, base::Bind(
       &AudioOutputController::DoReportError, this));
 }
 
 void AudioOutputController::DoStopCloseAndClearStream() {
@@ skipping 81 matching lines @@
   if (state_ == kClosed)
     return;
 
   // Note: OnDeviceChange() will cause the existing stream (the consumer of the
   // diverted audio data) to be closed, and diverting_to_stream_ will be set
   // back to NULL.
   OnDeviceChange();
   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());
 
   // If we should be playing and we haven't, that's a wedge.
   if (state_ == kPlaying) {
     const bool playback_success =
         base::AtomicRefCountIsOne(&on_more_io_data_called_);
 
     UMA_HISTOGRAM_BOOLEAN(
         "Media.AudioOutputControllerPlaybackStartupSuccess", playback_success);
 
     // Let the AudioManager try and fix it.
     if (!playback_success)
       audio_manager_->FixWedgedAudio();
   }
 }
 
 }  // namespace media