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

media/audio/audio_output_device.cc

Index: media/audio/audio_output_device.cc
diff --git a/media/audio/audio_output_device.cc b/media/audio/audio_output_device.cc
index 363cb4c67b2dac35b80ed5f46cbea4be52e86935..51111a2800b0f1b6c875fb5d950e4a0cc46382d5 100644
--- a/media/audio/audio_output_device.cc
+++ b/media/audio/audio_output_device.cc
@@ -49,7 +49,8 @@ AudioOutputDevice::AudioOutputDevice(
       ipc_(ipc),
       stream_id_(0),
       play_on_start_(true),
-      is_started_(false) {
+      is_started_(false),
+      stopping_(false) {
   CHECK(ipc_);
 }
 
@@ -90,6 +91,7 @@ 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,
@@ -166,16 +168,14 @@ void AudioOutputDevice::ShutDownOnIOThread() {
     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) {
@@ -199,6 +199,7 @@ void AudioOutputDevice::OnStateChanged(AudioOutputIPCDelegate::State state) {
     // 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();
   }
@@ -224,7 +225,19 @@ void AudioOutputDevice::OnStreamCreated(
   // 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(