Prevent AudioDeviceThread from starting if clients have called Stop() (round 2).

media/audio/audio_output_device.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_device.h"
 
 #include "base/debug/trace_event.h"
 #include "base/message_loop.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/time.h"
@@ skipping 31 matching lines @@
 
 AudioOutputDevice::AudioOutputDevice(
     AudioOutputIPC* ipc,
     const scoped_refptr<base::MessageLoopProxy>& io_loop)
     : ScopedLoopObserver(io_loop),
       input_channels_(0),
       callback_(NULL),
       ipc_(ipc),
       stream_id_(0),
       play_on_start_(true),
-      is_started_(false) {
+      is_started_(false),
+      stopping_(false) {
   CHECK(ipc_);
 }
 
 void AudioOutputDevice::Initialize(const AudioParameters& params,
                                    RenderCallback* callback) {
   CHECK_EQ(0, stream_id_) <<
       "AudioOutputDevice::Initialize() must be called before Start()";
 
   CHECK(!callback_);  // Calling Initialize() twice?
 
@@ skipping 20 matching lines @@
   DCHECK(callback_) << "Initialize hasn't been called";
   message_loop()->PostTask(FROM_HERE,
       base::Bind(&AudioOutputDevice::CreateStreamOnIOThread, this,
                  audio_parameters_, input_channels_));
 }
 
 void AudioOutputDevice::Stop() {
   {
     base::AutoLock auto_lock(audio_thread_lock_);
     audio_thread_.Stop(MessageLoop::current());
+    stopping_ = true;

[tommi (sloooow) - chröme, 2012/09/20 15:00:22]

instead of the variable, you can use audio_thread_.IsStopped()

[scherkus (not reviewing), 2012/09/20 17:48:28]

Not quite... OnStreamCreated() DCHECKs for audio_thread_.IsStopped() but what we
want to track is that Stop() has been called but we haven't processed
ShutDownOnIOThread() yet

   }
 
   message_loop()->PostTask(FROM_HERE,
       base::Bind(&AudioOutputDevice::ShutDownOnIOThread, this));
 }
 
 void AudioOutputDevice::Play() {
   message_loop()->PostTask(FROM_HERE,
       base::Bind(&AudioOutputDevice::PlayOnIOThread, this));
 }
@@ skipping 56 matching lines @@
     is_started_ = false;
 
     if (ipc_) {
       ipc_->CloseStream(stream_id_);
       ipc_->RemoveDelegate(stream_id_);
     }
 
     stream_id_ = 0;
   }
 
-  // We can run into an issue where ShutDownOnIOThread is called right after
-  // OnStreamCreated is called in cases where Start/Stop are called before we
-  // get the OnStreamCreated callback.  To handle that corner case, we call
-  // Stop(). In most cases, the thread will already be stopped.
   // Another situation is when the IO thread goes away before Stop() is called
   // in which case, we cannot use the message loop to close the thread handle
-  // and can't not rely on the main thread existing either.
+  // and can't rely on the main thread existing either.
+  base::AutoLock auto_lock_(audio_thread_lock_);
   base::ThreadRestrictions::ScopedAllowIO allow_io;
   audio_thread_.Stop(NULL);
   audio_callback_.reset();
+  stopping_ = false;

[tommi (sloooow) - chröme, 2012/09/20 15:00:22]

this is one place that's functionally different from IsStopped, but I don't know
if it makes a difference.
When the thread has truly exited, then IsStopped() returns true whereas this
variable will be false.

 }
 
 void AudioOutputDevice::SetVolumeOnIOThread(double volume) {
   DCHECK(message_loop()->BelongsToCurrentThread());
   if (stream_id_)
     ipc_->SetVolume(stream_id_, volume);
 }
 
 void AudioOutputDevice::OnStateChanged(AudioOutputIPCDelegate::State state) {
   DCHECK(message_loop()->BelongsToCurrentThread());
 
   // Do nothing if the stream has been closed.
   if (!stream_id_)
     return;
 
   if (state == AudioOutputIPCDelegate::kError) {
     DLOG(WARNING) << "AudioOutputDevice::OnStateChanged(kError)";
     // Don't dereference the callback object if the audio thread
     // is stopped or stopping.  That could mean that the callback
     // object has been deleted.
     // TODO(tommi): Add an explicit contract for clearing the callback
     // object.  Possibly require calling Initialize again or provide
     // a callback object via Start() and clear it in Stop().
+    base::AutoLock auto_lock_(audio_thread_lock_);

[tommi (sloooow) - chröme, 2012/09/20 15:00:22]

this isn't needed. IsStopped() does locking internally.
http://code.google.com/searchframe#OAMlx_jo-ck/src/media/audio/audio_device_t...

[scherkus (not reviewing), 2012/09/20 17:48:28]

Done.

     if (!audio_thread_.IsStopped())
       callback_->OnRenderError();
   }
 }
 
 void AudioOutputDevice::OnStreamCreated(
     base::SharedMemoryHandle handle,
     base::SyncSocket::Handle socket_handle,
     int length) {
   DCHECK(message_loop()->BelongsToCurrentThread());
 #if defined(OS_WIN)
   DCHECK(handle);
   DCHECK(socket_handle);
 #else
   DCHECK_GE(handle.fd, 0);
   DCHECK_GE(socket_handle, 0);
 #endif
 
   // We should only get this callback if stream_id_ is valid.  If it is not,
   // the IPC layer should have closed the shared memory and socket handles
   // for us and not invoked the callback.  The basic assertion is that when
   // stream_id_ is 0 the AudioOutputDevice instance is not registered as a
   // delegate and hence it should not receive callbacks.
   DCHECK(stream_id_);
 
+  // Stop() can stop |audio_thread_| before we receive OnStreamCreated().

[henrika (OOO until Aug 14), 2012/09/20 07:54:52]

Sorry but this sentence is a bit tricky to understand. Could it be a bit more
clear.

[scherkus (not reviewing), 2012/09/20 17:48:28]

Done.

+  // In that case we don't risk starting the thread using |callback_|, which
+  // may have point to freed memory.

[tommi (sloooow) - chröme, 2012/09/20 15:00:22]

If AudioDeviceThread::Stop() has been called, then the callback_ that belongs to
the thread has been set to NULL so at least the AudioOutputDevice's callback
won't be called.  However, I see that it's possible to create a _new_
AudioThreadCallback object, based on an old, bogus callback pointer.

I think that the design bug is that the callback is set via Initialize, and not
Start().  It should be set with Start() and freed in Stop().  As is, it's set
once and assumed that the same callback _might_ be used across multiple
Start/Stop calls.

If we change Stop() to clear the callback, Initialize() to accept no parameter
and Start() to accept the callback parameter, then would that help resolve this?

[scherkus (not reviewing), 2012/09/20 17:48:28]

I like all of those ideas but considering this has to get merged I'm preferring
the short-term hack :(

+  //
+  // TODO(scherkus): The real fix is to have sane ownership semantics. The fact

[henrika (OOO until Aug 14), 2012/09/20 07:54:52]

Seems like a very good idea to add a link to crbug here.
Nice summary.

[scherkus (not reviewing), 2012/09/20 17:48:28]

Done.

+  // that |callback_| (which should own and outlive this object!) can point to
+  // freed memory is a mess. AudioRendererSink should be non-refcounted that

[henrika (OOO until Aug 14), 2012/09/20 07:54:52]

"that" -> "so that"

[scherkus (not reviewing), 2012/09/20 17:48:28]

Done.

+  // owners (WebRtcAudioDeviceImpl, AudioRendererImpl, etc...) can Stop() and
+  // delete as they see fit. AudioOutputDevice should internally use WeakPtr
+  // to handle teardown and thread hopping.
   base::AutoLock auto_lock(audio_thread_lock_);
+  if (stopping_)

[tommi (sloooow) - chröme, 2012/09/20 15:00:22]

If we change the Start() vs Initialize() contract, then presumably we could
check callback_ here instead?
We could also just decided to force single-use AudioOutputDevices? :) Depending
on how well that sits with webrtc.

[scherkus (not reviewing), 2012/09/20 17:48:28]

I prototyped a change and it appears webrtc can use single-use AOD objects, but
it's a larger change I don't want to commit to at the moment.

+    return;
 
   DCHECK(audio_thread_.IsStopped());
   audio_callback_.reset(new AudioOutputDevice::AudioThreadCallback(
       audio_parameters_, input_channels_, handle, length, callback_));
   audio_thread_.Start(audio_callback_.get(), socket_handle,
       "AudioOutputDevice");
 
   // We handle the case where Play() and/or Pause() may have been called
   // multiple times before OnStreamCreated() gets called.
   is_started_ = true;
@@ skipping 85 matching lines @@
   // TODO(dalecurtis): Technically this is not always correct.  Due to channel
   // padding for alignment, there may be more data available than this.  We're
   // relying on AudioSyncReader::Read() to parse this with that in mind.  Rename
   // these methods to Set/GetActualFrameCount().
   SetActualDataSizeInBytes(
       &shared_memory_, memory_length_,
       num_frames * sizeof(*output_bus_->channel(0)) * output_bus_->channels());
 }
 
 }  // namespace media.