Crash fix: Remove MessageLoop from AudioPowerMonitor and instead use MessageLoopProxy in AudioOutpu…

Crash fix: Remove MessageLoop from AudioPowerMonitor and instead use MessageLoopProxy in AudioOutputController.

Root cause: AudioPowerMonitor held a reference to the audio thread's MessageLoop and erroneously assumed it would be valid until after the audio stream is closed.  However, at browser shutdown, it's possible for audio streams to be closed by the correct thread, but *after* the MessageLoop associated with the thread is destroyed.

BUG=268629
TEST=media_unittests and manual confirmation by running a browser with the --enable-audible-notifications command-line flag.

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

[miu, 2013-08-12 06:33:11]

Please review.

This should fix the crashing on all platforms.  No more assumptions about when
threads or message loops will be alive.  I would argue things are actually a lot
simpler and cleaner now.

Thanks,
Yuri

[DaleCurtis, 2013-08-12 18:23:29]

Much clearer, but naughty use of races :)

https://codereview.chromium.org/22339024/diff/1/media/audio/audio_power_monit...
File media/audio/audio_power_monitor.cc (right):

https://codereview.chromium.org/22339024/diff/1/media/audio/audio_power_monit...
media/audio/audio_power_monitor.cc:22: ANNOTATE_BENIGN_RACE(
Annotating only keeps TSAN happy, while the impl is still racy / relies on
undefined behavior, which is potentially problematic across the weakly ordered
architectures supported by Chrome.

While not the end of the world in this case, a Lock::Try() in Scan() + Lock in
Reset() + Read() would make this race free w/o a loss of clarity or problematic
overhead.

https://codereview.chromium.org/22339024/diff/1/media/audio/audio_power_monit...
media/audio/audio_power_monitor.cc:68: // Update accumulated results.
if (lock.Try()) {
  average_power_exported_ = average_power_;
  clipped_exported_ |= clipped_;
  lock.Release();
}

Alternatively, check out the AutoTryLock class in WebAudioSourceProviderImpl().

https://codereview.chromium.org/22339024/diff/1/media/audio/audio_power_monit...
media/audio/audio_power_monitor.cc:70: if (clipped)
This is not the same behavior as before and will lead to loss of clipped status,
no?

https://codereview.chromium.org/22339024/diff/1/media/audio/audio_power_monit...
media/audio/audio_power_monitor.cc:78: memcpy(&sampled_power, &average_power_,
sizeof(float));
I don't think memcpy provides you any guarantees, so I assume you're trying to
avoid the compiler optimizing away the local value?  Sketchy :)

https://codereview.chromium.org/22339024/diff/1/media/audio/audio_power_monit...
File media/audio/audio_power_monitor.h (right):

https://codereview.chromium.org/22339024/diff/1/media/audio/audio_power_monit...
media/audio/audio_power_monitor.h:51: // Returns the current power level in
terms of dBFS (see zero_power() and
I recommend you also read clipped and power together and return them as an
std::pair or take bool*, float* output pointers as input.  Otherwise the levels
might not match the clip status due to changes between calls.

[miu, 2013-08-12 19:41:45]

Thanks, Dale.  Took all of your suggestions.  I like the try-locking better. 
Guess I was too focused on removing the word "try" from my code.  ;-)

PTAL.

https://codereview.chromium.org/22339024/diff/1/media/audio/audio_power_monit...
File media/audio/audio_power_monitor.cc (right):

https://codereview.chromium.org/22339024/diff/1/media/audio/audio_power_monit...
media/audio/audio_power_monitor.cc:22: ANNOTATE_BENIGN_RACE(
On 2013/08/12 18:23:29, DaleCurtis wrote:
> Annotating only keeps TSAN happy, while the impl is still racy / relies on
> undefined behavior, which is potentially problematic across the weakly ordered
> architectures supported by Chrome.
> 
> While not the end of the world in this case, a Lock::Try() in Scan() + Lock in
> Reset() + Read() would make this race free w/o a loss of clarity or
problematic
> overhead.

Done.

https://codereview.chromium.org/22339024/diff/1/media/audio/audio_power_monit...
media/audio/audio_power_monitor.cc:68: // Update accumulated results.
On 2013/08/12 18:23:29, DaleCurtis wrote:
> if (lock.Try()) {
>   average_power_exported_ = average_power_;
>   clipped_exported_ |= clipped_;
>   lock.Release();
> }
> 
> Alternatively, check out the AutoTryLock class in
WebAudioSourceProviderImpl().

Done.

https://codereview.chromium.org/22339024/diff/1/media/audio/audio_power_monit...
media/audio/audio_power_monitor.cc:70: if (clipped)
On 2013/08/12 18:23:29, DaleCurtis wrote:
> This is not the same behavior as before and will lead to loss of clipped
status,
> no?

It looks like the same behavior to me.  Can you provide a counter-example?

The change is mostly to make it obvious in the code that the clip flag is set by
one thread, and cleared (i.e., "consumed") by another.

https://codereview.chromium.org/22339024/diff/1/media/audio/audio_power_monit...
media/audio/audio_power_monitor.cc:78: memcpy(&sampled_power, &average_power_,
sizeof(float));
On 2013/08/12 18:23:29, DaleCurtis wrote:
> I don't think memcpy provides you any guarantees, so I assume you're trying to
> avoid the compiler optimizing away the local value?  Sketchy :)

Removed.  Not needed anymore now that we're using locking.

https://codereview.chromium.org/22339024/diff/1/media/audio/audio_power_monit...
File media/audio/audio_power_monitor.h (right):

https://codereview.chromium.org/22339024/diff/1/media/audio/audio_power_monit...
media/audio/audio_power_monitor.h:51: // Returns the current power level in
terms of dBFS (see zero_power() and
On 2013/08/12 18:23:29, DaleCurtis wrote:
> I recommend you also read clipped and power together and return them as an
> std::pair or take bool*, float* output pointers as input.  Otherwise the
levels
> might not match the clip status due to changes between calls.

Done.

[DaleCurtis, 2013-08-12 20:43:56]

lgtm, you probably will need to release-block-beta that bug so you can get this
merged if it misses the branch cut.

https://codereview.chromium.org/22339024/diff/1/media/audio/audio_power_monit...
File media/audio/audio_power_monitor.cc (right):

https://codereview.chromium.org/22339024/diff/1/media/audio/audio_power_monit...
media/audio/audio_power_monitor.cc:70: if (clipped)
On 2013/08/12 19:41:46, Yuri wrote:
> On 2013/08/12 18:23:29, DaleCurtis wrote:
> > This is not the same behavior as before and will lead to loss of clipped
> status,
> > no?
> 
> It looks like the same behavior to me.  Can you provide a counter-example?
> 
> The change is mostly to make it obvious in the code that the clip flag is set
by
> one thread, and cleared (i.e., "consumed") by another.

Ah you're right.  The | is clearer to me apparently :p

https://codereview.chromium.org/22339024/diff/18001/media/audio/audio_power_m...
File media/audio/audio_power_monitor.cc (right):

https://codereview.chromium.org/22339024/diff/18001/media/audio/audio_power_m...
media/audio/audio_power_monitor.cc:28: power_reading_ = average_power_ = 0.0f;
Needs documentation that it can't be called while scanning.

https://codereview.chromium.org/22339024/diff/18001/media/audio/audio_power_m...
media/audio/audio_power_monitor.cc:43: bool clipped = false;
Just use has_clipped_ directly?

https://codereview.chromium.org/22339024/diff/18001/media/audio/audio_power_m...
media/audio/audio_power_monitor.cc:89: clipped_reading_ = false;
No need for if.  If you're so inclined:

bool clipped = false;
std::swap(clipped, clipped_reading_);

[miu, 2013-08-12 21:10:54]

Thanks for the quick turnaround, Dale.

https://codereview.chromium.org/22339024/diff/18001/media/audio/audio_power_m...
File media/audio/audio_power_monitor.cc (right):

https://codereview.chromium.org/22339024/diff/18001/media/audio/audio_power_m...
media/audio/audio_power_monitor.cc:28: power_reading_ = average_power_ = 0.0f;
On 2013/08/12 20:43:56, DaleCurtis wrote:
> Needs documentation that it can't be called while scanning.

Done.

https://codereview.chromium.org/22339024/diff/18001/media/audio/audio_power_m...
media/audio/audio_power_monitor.cc:43: bool clipped = false;
On 2013/08/12 20:43:56, DaleCurtis wrote:
> Just use has_clipped_ directly?

Done.

https://codereview.chromium.org/22339024/diff/18001/media/audio/audio_power_m...
media/audio/audio_power_monitor.cc:89: clipped_reading_ = false;
On 2013/08/12 20:43:56, DaleCurtis wrote:
> No need for if.  If you're so inclined:
> 
> bool clipped = false;
> std::swap(clipped, clipped_reading_);

Done.

[commit-bot: I haz the power, 2013-08-12 21:14:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miu@chromium.org/22339024/23001

[commit-bot: I haz the power, 2013-08-13 00:50:17]

Change committed as 217142