Stop the AudioDeviceThread when the IO loop goes away.

Stop the AudioDeviceThread when the IO loop goes away.

When shutting down, the pipeline code can take a long time to terminate.
In some rare cases, the IO loop terminates before the pipeline thread and
the audio device classes would attempt to use the io loop to perform tasks on.
I checked in a candidate fix for this a couple of weeks ago which monitors the
lifetime of the IO loop and uses MessageLoopProxy* instead of MessageLoop* to
avoid issues with using the NULL loop.  That fixes part of the problem.
What was missing was also stopping the audio thread in this case since the IO
thread will run teardown before the Pipeline thread calls AudioDevice::Stop().
In that period, the audio thread will still be running but the callback object
has been deleted.

Additionally, I'm adding a lock to guard the thread_ variable in AudioDeviceThread.
Before I was relying on external synchronization in the AudioDevice and
AudioInputDevice state machine.  However, it doesn't hurt to go belt and
suspenders although it might not look cool ;)

BUG=115299
TEST=Follow repro steps in bug report.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=124330

[scherkus (not reviewing), 2012-02-29 21:10:15]

https://chromiumcodereview.appspot.com/9534002/diff/7001/content/renderer/med...
File content/renderer/media/audio_device.cc (right):

https://chromiumcodereview.appspot.com/9534002/diff/7001/content/renderer/med...
content/renderer/media/audio_device.cc:102:
audio_thread_.Stop(MessageLoop::current());
remind me again.. what's the advantage to stopping the audio thread on current
thread as opposed to the IO thread? (see my other comment below)

https://chromiumcodereview.appspot.com/9534002/diff/7001/content/renderer/med...
content/renderer/media/audio_device.cc:185:
audio_thread_.Stop(MessageLoop::current());
hold the phone...

combined with the change to WillDestroyCurrentMessageLoop() to synchronously
stop audio_thread_... perhaps we should always synchronously stop audio_thread_
inside of ShutDownOnIOThread()?

Here's my thinking... there are two spots where ShutDownOnIOThread can get
called:
  1) Stop() via PostTask
  2) WillDestroyCurrentMessageLoop()

In (1) we already call audio_thread_.Stop(MessageLoop::current()) prior to
posting the task for ShutDownOnIOThread(). If it doesn't matter which thread is
used to stop audio_thread_, then why not always stop it inside this method?
(hopefully I'm not missing anything here!)

If that all makes sense I wonder if audio_thread_.Stop() even needs to accept a
MessageLoop parameter anymore if synchronous stopping is always used.

https://chromiumcodereview.appspot.com/9534002/diff/7001/content/renderer/med...
File content/renderer/media/audio_device_thread.h (right):

https://chromiumcodereview.appspot.com/9534002/diff/7001/content/renderer/med...
content/renderer/media/audio_device_thread.h:103: mutable base::Lock
thread_lock_;
nit: mutable = sadface

when would an ADT be const? how about dropping const+mutable, especially since
IsStopped() is only used for DCHECK purposes

https://chromiumcodereview.appspot.com/9534002/diff/7001/content/renderer/med...
File content/renderer/media/audio_input_device.cc (right):

https://chromiumcodereview.appspot.com/9534002/diff/7001/content/renderer/med...
content/renderer/media/audio_input_device.cc:266: void
AudioInputDevice::WillDestroyCurrentMessageLoop() {
ditto

[tommi (sloooow) - chröme, 2012-02-29 21:22:13]

https://chromiumcodereview.appspot.com/9534002/diff/7001/content/renderer/med...
File content/renderer/media/audio_device.cc (right):

https://chromiumcodereview.appspot.com/9534002/diff/7001/content/renderer/med...
content/renderer/media/audio_device.cc:102:
audio_thread_.Stop(MessageLoop::current());
On 2012/02/29 21:10:15, scherkus wrote:
> remind me again.. what's the advantage to stopping the audio thread on current
> thread as opposed to the IO thread? (see my other comment below)

The IO thread isn't allowed to join threads.

However, we're in a bit of a pickle with the pipeline thread so we have to break
that rule below basically because we're out of threads to wait on.

https://chromiumcodereview.appspot.com/9534002/diff/7001/content/renderer/med...
content/renderer/media/audio_device.cc:185:
audio_thread_.Stop(MessageLoop::current());
On 2012/02/29 21:10:15, scherkus wrote:
> hold the phone...
> 
> combined with the change to WillDestroyCurrentMessageLoop() to synchronously
> stop audio_thread_... perhaps we should always synchronously stop
audio_thread_
> inside of ShutDownOnIOThread()?

We shouldn't do that on the IO thread due to thread restrictions.  However, when
we hit the WillDestroyCurrentMessageLoop case, that means that the Pipeline
thread is still running while the IO is ending and as we've seen in crash
reports, the main thread has also gone.  That's why I added the synchronous wait
there.  It would be good if the pipeline thread has been properly terminated
before the main and io threads go down.

> 
> Here's my thinking... there are two spots where ShutDownOnIOThread can get
> called:
>   1) Stop() via PostTask
>   2) WillDestroyCurrentMessageLoop()
> 
> In (1) we already call audio_thread_.Stop(MessageLoop::current()) prior to
> posting the task for ShutDownOnIOThread(). If it doesn't matter which thread
is
> used to stop audio_thread_, then why not always stop it inside this method?
> (hopefully I'm not missing anything here!)
> 
> If that all makes sense I wonder if audio_thread_.Stop() even needs to accept
a
> MessageLoop parameter anymore if synchronous stopping is always used.

https://chromiumcodereview.appspot.com/9534002/diff/7001/content/renderer/med...
File content/renderer/media/audio_device_thread.h (right):

https://chromiumcodereview.appspot.com/9534002/diff/7001/content/renderer/med...
content/renderer/media/audio_device_thread.h:103: mutable base::Lock
thread_lock_;
On 2012/02/29 21:10:15, scherkus wrote:
> nit: mutable = sadface

why? I always figured it would be fair game for synchronization and cache?

> 
> when would an ADT be const? how about dropping const+mutable, especially since
> IsStopped() is only used for DCHECK purposes

OK with me.

[scherkus (not reviewing), 2012-02-29 21:40:53]

cleared up a lot of my confusion over IM -- ping me again when you want me to
review w/ the scoped IO changes

https://chromiumcodereview.appspot.com/9534002/diff/7001/content/renderer/med...
File content/renderer/media/audio_device_thread.h (right):

https://chromiumcodereview.appspot.com/9534002/diff/7001/content/renderer/med...
content/renderer/media/audio_device_thread.h:103: mutable base::Lock
thread_lock_;
On 2012/02/29 21:22:13, tommi wrote:
> On 2012/02/29 21:10:15, scherkus wrote:
> > nit: mutable = sadface
> 
> why? I always figured it would be fair game for synchronization and cache?
> 
> > 
> > when would an ADT be const? how about dropping const+mutable, especially
since
> > IsStopped() is only used for DCHECK purposes
> 
> OK with me.

ah it was sadface for the reasons I mentioned (object isn't const and it's a
dcheck), i.e., the mutable doesn't really seem necessary here

[tommi (sloooow) - chröme, 2012-02-29 22:00:14]

changes uploaded.  mutable is gone and there's a single call to the synchronous
Stop() + improved documentation.

[scherkus (not reviewing), 2012-02-29 22:40:57]

LGTM!

[commit-bot: I haz the power, 2012-02-29 22:41:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommi@chromium.org/9534002/15003

[commit-bot: I haz the power, 2012-03-01 01:57:09]

Change committed as 124330