fix crash in AUAudioOutputStream::Render()

AUAudioOutputStream::Stop() first sets the source_ to NULL then calls AudioOutputUnitStop(), since AudioOutputUnitStop() is called by AudioManager thread, it will do a synchronous stop which means that AUAudioOutputStream::Render() will keep running until it is done. so that it crashes when it hits source_ which has been set to NULL. The solution is to stop the HAL first and then set the source_ to NULL.


BUG=120193
TEST=None
TAB=tommi

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=129590

[no longer working on chromium, 2012-03-27 10:59:17]

Chris, could you please take a look at it?

[no longer working on chromium, 2012-03-27 11:01:32]

https://chromiumcodereview.appspot.com/9834098/diff/1/media/audio/mac/audio_l...
File media/audio/mac/audio_low_latency_output_mac.cc (right):

https://chromiumcodereview.appspot.com/9834098/diff/1/media/audio/mac/audio_l...
media/audio/mac/audio_low_latency_output_mac.cc:221: if (stopped_)
I don't use atomic operation here since there is comment requiring not to add
locks in this real-time thread.

[no longer working on chromium, 2012-03-28 11:31:13]

Ping.

Hi Chris, do you have time to take a look it?

BR,
/SX

On Tue, Mar 27, 2012 at 1:01 PM, <xians@chromium.org> wrote:

>
> https://chromiumcodereview.**appspot.com/9834098/diff/1/**
>
media/audio/mac/audio_low_**latency_output_mac.cc<https://chromiumcodereview.appspot.com/9834098/diff/1/media/audio/mac/audio_low_latency_output_mac.cc>
> File media/audio/mac/audio_low_**latency_output_mac.cc (right):
>
> https://chromiumcodereview.**appspot.com/9834098/diff/1/**
>
media/audio/mac/audio_low_**latency_output_mac.cc#**newcode221<https://chromiumcodereview.appspot.com/9834098/diff/1/media/audio/mac/audio_low_latency_output_mac.cc#newcode221>
> media/audio/mac/audio_low_**latency_output_mac.cc:221: if (stopped_)
> I don't use atomic operation here since there is comment requiring not
> to add locks in this real-time thread.
>
>
https://chromiumcodereview.**appspot.com/9834098/<https://chromiumcodereview....
>

[Chris Rogers, 2012-03-28 18:55:10]

LGTM with small comments below

Thanks for fixing this - this is great!

https://chromiumcodereview.appspot.com/9834098/diff/1/media/audio/mac/audio_l...
File media/audio/mac/audio_low_latency_output_mac.cc (right):

https://chromiumcodereview.appspot.com/9834098/diff/1/media/audio/mac/audio_l...
media/audio/mac/audio_low_latency_output_mac.cc:194: stopped_ = true;
Should this maybe be set to "true" only after AudioOutputUnitStop() has
returned?

and you can also remove the check down below for stopped_ in the Render() method

https://chromiumcodereview.appspot.com/9834098/diff/1/media/audio/mac/audio_l...
media/audio/mac/audio_low_latency_output_mac.cc:221: if (stopped_)
Strictly speaking, I don't think it's even necessary to do this check here,
since you're now calling AudioOutputStop() (which is synchronous) before setting
source_ = NULL 

On 2012/03/27 11:01:32, xians1 wrote:
> I don't use atomic operation here since there is comment requiring not to add
> locks in this real-time thread.

[no longer working on chromium, 2012-03-28 19:25:31]

https://chromiumcodereview.appspot.com/9834098/diff/1/media/audio/mac/audio_l...
File media/audio/mac/audio_low_latency_output_mac.cc (right):

https://chromiumcodereview.appspot.com/9834098/diff/1/media/audio/mac/audio_l...
media/audio/mac/audio_low_latency_output_mac.cc:194: stopped_ = true;
On 2012/03/28 18:55:10, Chris Rogers wrote:
> Should this maybe be set to "true" only after AudioOutputUnitStop() has
> returned?
> 
> and you can also remove the check down below for stopped_ in the Render()
method

Done.

https://chromiumcodereview.appspot.com/9834098/diff/1/media/audio/mac/audio_l...
media/audio/mac/audio_low_latency_output_mac.cc:221: if (stopped_)
On 2012/03/28 18:55:10, Chris Rogers wrote:
> Strictly speaking, I don't think it's even necessary to do this check here,
> since you're now calling AudioOutputStop() (which is synchronous) before
setting
> source_ = NULL 
> 
> On 2012/03/27 11:01:32, xians1 wrote:
> > I don't use atomic operation here since there is comment requiring not to
add
> > locks in this real-time thread.
> 

True, thanks for pointing out. It just feels safer to use a check to make sure
we quit the callback as quickly as possible after Stop() is called. But I agree
this is not necessary.

[Chris Rogers, 2012-03-28 19:52:16]

Thanks Shijing, still LGTM.  By the way, this is an important enough crash fix
that it would be good to merge this fix into the last branch once this lands.

On 2012/03/28 19:25:31, xians1 wrote:
>
https://chromiumcodereview.appspot.com/9834098/diff/1/media/audio/mac/audio_l...
> File media/audio/mac/audio_low_latency_output_mac.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9834098/diff/1/media/audio/mac/audio_l...
> media/audio/mac/audio_low_latency_output_mac.cc:194: stopped_ = true;
> On 2012/03/28 18:55:10, Chris Rogers wrote:
> > Should this maybe be set to "true" only after AudioOutputUnitStop() has
> > returned?
> > 
> > and you can also remove the check down below for stopped_ in the Render()
> method
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/9834098/diff/1/media/audio/mac/audio_l...
> media/audio/mac/audio_low_latency_output_mac.cc:221: if (stopped_)
> On 2012/03/28 18:55:10, Chris Rogers wrote:
> > Strictly speaking, I don't think it's even necessary to do this check here,
> > since you're now calling AudioOutputStop() (which is synchronous) before
> setting
> > source_ = NULL 
> > 
> > On 2012/03/27 11:01:32, xians1 wrote:
> > > I don't use atomic operation here since there is comment requiring not to
> add
> > > locks in this real-time thread.
> > 
> 
> True, thanks for pointing out. It just feels safer to use a check to make sure
> we quit the callback as quickly as possible after Stop() is called. But I
agree
> this is not necessary.

[no longer working on chromium, 2012-03-28 20:00:20]

sure things.
definitely, i will merge it to the release branch.

sx
On Mar 28, 2012 9:52 PM, <crogers@google.com> wrote:

> Thanks Shijing, still LGTM.  By the way, this is an important enough crash
> fix
> that it would be good to merge this fix into the last branch once this
> lands.
>
> On 2012/03/28 19:25:31, xians1 wrote:
>
> https://chromiumcodereview.**appspot.com/9834098/diff/1/**
>
media/audio/mac/audio_low_**latency_output_mac.cc<https://chromiumcodereview.appspot.com/9834098/diff/1/media/audio/mac/audio_low_latency_output_mac.cc>
>
>> File media/audio/mac/audio_low_**latency_output_mac.cc (right):
>>
>
>
> https://chromiumcodereview.**appspot.com/9834098/diff/1/**
>
media/audio/mac/audio_low_**latency_output_mac.cc#**newcode194<https://chromiumcodereview.appspot.com/9834098/diff/1/media/audio/mac/audio_low_latency_output_mac.cc#newcode194>
>
>> media/audio/mac/audio_low_**latency_output_mac.cc:194: stopped_ = true;
>> On 2012/03/28 18:55:10, Chris Rogers wrote:
>> > Should this maybe be set to "true" only after AudioOutputUnitStop() has
>> > returned?
>> >
>> > and you can also remove the check down below for stopped_ in the
>> Render()
>> method
>>
>
>  Done.
>>
>
>
> https://chromiumcodereview.**appspot.com/9834098/diff/1/**
>
media/audio/mac/audio_low_**latency_output_mac.cc#**newcode221<https://chromiumcodereview.appspot.com/9834098/diff/1/media/audio/mac/audio_low_latency_output_mac.cc#newcode221>
>
>> media/audio/mac/audio_low_**latency_output_mac.cc:221: if (stopped_)
>> On 2012/03/28 18:55:10, Chris Rogers wrote:
>> > Strictly speaking, I don't think it's even necessary to do this check
>> here,
>> > since you're now calling AudioOutputStop() (which is synchronous) before
>> setting
>> > source_ = NULL
>> >
>> > On 2012/03/27 11:01:32, xians1 wrote:
>> > > I don't use atomic operation here since there is comment requiring
>> not to
>> add
>> > > locks in this real-time thread.
>> >
>>
>
>  True, thanks for pointing out. It just feels safer to use a check to make
>> sure
>> we quit the callback as quickly as possible after Stop() is called. But I
>>
> agree
>
>> this is not necessary.
>>
>
>
>
>
https://chromiumcodereview.**appspot.com/9834098/<https://chromiumcodereview....
>

[commit-bot: I haz the power, 2012-03-29 07:39:49]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.