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

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

My first attempt at a fix (r157378) was no good as it's legal to repeatedly start and stop an AudioOutputDevice. This time around we use flag to track a pending stop so we don't start AudioDeviceThread knowing the client had requested a stop.

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

[scherkus (not reviewing), 2012-09-20 02:34:18]

Can you take a look?

I parameterized the unit tests to run both synchronized and unsynchronzied for
all tests.

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

Thanks Andrew; it looks much better now.

Ping me when landed and I'll test it some extra on Windows.

Please create and link to an issue to do the refactoring you propose in this CL.

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

LGTM w/ nits.

http://codereview.chromium.org/10958004/diff/1/media/audio/audio_output_devic...
File media/audio/audio_output_device.cc (right):

http://codereview.chromium.org/10958004/diff/1/media/audio/audio_output_devic...
media/audio/audio_output_device.cc:228: // Stop() can stop |audio_thread_|
before we receive OnStreamCreated().
Sorry but this sentence is a bit tricky to understand. Could it be a bit more
clear.

http://codereview.chromium.org/10958004/diff/1/media/audio/audio_output_devic...
media/audio/audio_output_device.cc:232: // TODO(scherkus): The real fix is to
have sane ownership semantics. The fact
Seems like a very good idea to add a link to crbug here.
Nice summary.

http://codereview.chromium.org/10958004/diff/1/media/audio/audio_output_devic...
media/audio/audio_output_device.cc:234: // freed memory is a mess.
AudioRendererSink should be non-refcounted that
"that" -> "so that"

http://codereview.chromium.org/10958004/diff/1/media/audio/audio_output_devic...
File media/audio/audio_output_device_unittest.cc (right):

http://codereview.chromium.org/10958004/diff/1/media/audio/audio_output_devic...
media/audio/audio_output_device_unittest.cc:97: public
testing::WithParamInterface<bool> {
nice ;-)

http://codereview.chromium.org/10958004/diff/1/media/audio/audio_output_devic...
media/audio/audio_output_device_unittest.cc:106:
input_channels_(synchronized_io_ ? 2 : 0) {
Is it OK to use a  hard-coded '2' here? Comment perhaps since this choice is
central.

http://codereview.chromium.org/10958004/diff/1/media/audio/audio_output_devic...
media/audio/audio_output_device_unittest.cc:245: audio_device_->Stop();
Was this move required? Just wonder why.

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

http://codereview.chromium.org/10958004/diff/1/media/audio/audio_output_devic...
File media/audio/audio_output_device.cc (right):

http://codereview.chromium.org/10958004/diff/1/media/audio/audio_output_devic...
media/audio/audio_output_device.cc:94: stopping_ = true;
instead of the variable, you can use audio_thread_.IsStopped()

http://codereview.chromium.org/10958004/diff/1/media/audio/audio_output_devic...
media/audio/audio_output_device.cc:178: stopping_ = false;
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.

http://codereview.chromium.org/10958004/diff/1/media/audio/audio_output_devic...
media/audio/audio_output_device.cc:202: base::AutoLock
auto_lock_(audio_thread_lock_);
this isn't needed. IsStopped() does locking internally.
http://code.google.com/searchframe#OAMlx_jo-ck/src/media/audio/audio_device_t...

http://codereview.chromium.org/10958004/diff/1/media/audio/audio_output_devic...
media/audio/audio_output_device.cc:230: // may have point to freed memory.
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?

http://codereview.chromium.org/10958004/diff/1/media/audio/audio_output_devic...
media/audio/audio_output_device.cc:239: if (stopping_)
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]

http://codereview.chromium.org/10958004/diff/1/media/audio/audio_output_devic...
File media/audio/audio_output_device.cc (right):

http://codereview.chromium.org/10958004/diff/1/media/audio/audio_output_devic...
media/audio/audio_output_device.cc:94: stopping_ = true;
On 2012/09/20 15:00:22, tommi wrote:
> instead of the variable, you can use audio_thread_.IsStopped()

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

http://codereview.chromium.org/10958004/diff/1/media/audio/audio_output_devic...
media/audio/audio_output_device.cc:202: base::AutoLock
auto_lock_(audio_thread_lock_);
On 2012/09/20 15:00:22, tommi wrote:
> this isn't needed. IsStopped() does locking internally.
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/media/audio/audio_device_t...

Done.

http://codereview.chromium.org/10958004/diff/1/media/audio/audio_output_devic...
media/audio/audio_output_device.cc:228: // Stop() can stop |audio_thread_|
before we receive OnStreamCreated().
On 2012/09/20 07:54:52, henrika wrote:
> Sorry but this sentence is a bit tricky to understand. Could it be a bit more
> clear.

Done.

http://codereview.chromium.org/10958004/diff/1/media/audio/audio_output_devic...
media/audio/audio_output_device.cc:230: // may have point to freed memory.
On 2012/09/20 15:00:22, tommi wrote:
> 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?

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

http://codereview.chromium.org/10958004/diff/1/media/audio/audio_output_devic...
media/audio/audio_output_device.cc:232: // TODO(scherkus): The real fix is to
have sane ownership semantics. The fact
On 2012/09/20 07:54:52, henrika wrote:
> Seems like a very good idea to add a link to crbug here.
> Nice summary.

Done.

http://codereview.chromium.org/10958004/diff/1/media/audio/audio_output_devic...
media/audio/audio_output_device.cc:234: // freed memory is a mess.
AudioRendererSink should be non-refcounted that
On 2012/09/20 07:54:52, henrika wrote:
> "that" -> "so that"

Done.

http://codereview.chromium.org/10958004/diff/1/media/audio/audio_output_devic...
media/audio/audio_output_device.cc:239: if (stopping_)
On 2012/09/20 15:00:22, tommi wrote:
> 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.

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.

[commit-bot: I haz the power, 2012-09-20 18:19:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scherkus@chromium.org/10958004/8001

[commit-bot: I haz the power, 2012-09-20 21:17:53]

Change committed as 157841