Fix AudioEntry destruction order. Add debugging CHECKs for racy shutdown.

Fix AudioEntry destruction order.  Add debugging CHECKs for racy shutdown.

Destruction order should be harmless, but should be cleaned up. CHECKs
should tell us if the controller is being shutdown while callbacks are in flight.

BUG=349651
TEST=none

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

[DaleCurtis, 2014-03-06 01:16:32]

-ooo scherkus, +acolwell

[acolwell GONE FROM CHROMIUM, 2014-03-06 20:54:09]

https://codereview.chromium.org/188243002/diff/20001/media/audio/audio_output...
File media/audio/audio_output_controller.cc (right):

https://codereview.chromium.org/188243002/diff/20001/media/audio/audio_output...
media/audio/audio_output_controller.cc:66:
CHECK(base::AtomicRefCountIsZero(&currently_in_on_more_io_data_));
I'm a little unclear why we need this. Who owns this object and how are
OnMoreIOData() calls allowed without something holding a reference to this? The
need for this makes me think we don't really have a good understanding of
ownership & lifetime here.

https://codereview.chromium.org/188243002/diff/20001/media/audio/audio_output...
media/audio/audio_output_controller.cc:336:
base::AtomicRefCountInc(&currently_in_on_more_io_data_);
If you make the initial value 1 and dec instead of inc, you could CHECK here and
crash before allowing access to any of the vars. You could then dec & check in
the destructor which would cause all future calls here to crash immediately.

[DaleCurtis, 2014-03-06 21:21:11]

https://codereview.chromium.org/188243002/diff/20001/media/audio/audio_output...
File media/audio/audio_output_controller.cc (right):

https://codereview.chromium.org/188243002/diff/20001/media/audio/audio_output...
media/audio/audio_output_controller.cc:66:
CHECK(base::AtomicRefCountIsZero(&currently_in_on_more_io_data_));
On 2014/03/06 20:54:10, acolwell wrote:
> I'm a little unclear why we need this. Who owns this object and how are
> OnMoreIOData() calls allowed without something holding a reference to this?
The
> need for this makes me think we don't really have a good understanding of
> ownership & lifetime here.

See audio_io.h and the AudioSourceCallback interface.  For better or worse,
we're handing out raw pointers back to this class which are used by clients
closer to the hardware to request audio data.

I'd love to remove ref-counting from AudioOutputController, but there are
various consumers still relying on this behavior; notably WebrtcAudioPrivate :( 
Cleaning these up would be non-trivial.

What I suspect is happening is that there's a stale SourceCallback lingering, I
want the destruction trace to get crash reports that have both the offender and
the destruction on the stack.

https://codereview.chromium.org/188243002/diff/20001/media/audio/audio_output...
media/audio/audio_output_controller.cc:336:
base::AtomicRefCountInc(&currently_in_on_more_io_data_);
On 2014/03/06 20:54:10, acolwell wrote:
> If you make the initial value 1 and dec instead of inc, you could CHECK here
and
> crash before allowing access to any of the vars. You could then dec & check in
> the destructor which would cause all future calls here to crash immediately.

Done.

[acolwell GONE FROM CHROMIUM, 2014-03-07 00:05:35]

lgtm

https://codereview.chromium.org/188243002/diff/40001/media/audio/audio_output...
File media/audio/audio_output_controller.cc (right):

https://codereview.chromium.org/188243002/diff/40001/media/audio/audio_output...
media/audio/audio_output_controller.cc:66:
CHECK(base::AtomicRefCountIsOne(&not_currently_in_on_more_io_data_));
nit: If you make this
CHECK(!base::AtomicRefCountDec(&not_currently_in_on_more_io_data_)); you should
be able to detect you are in the expected state and prevent future
OnMoreIOData() from executing. (Assuming the memory doesn't get reused
immediately)

[DaleCurtis, 2014-03-07 00:09:04]

https://codereview.chromium.org/188243002/diff/40001/media/audio/audio_output...
File media/audio/audio_output_controller.cc (right):

https://codereview.chromium.org/188243002/diff/40001/media/audio/audio_output...
media/audio/audio_output_controller.cc:66:
CHECK(base::AtomicRefCountIsOne(&not_currently_in_on_more_io_data_));
On 2014/03/07 00:05:35, acolwell wrote:
> nit: If you make this
> CHECK(!base::AtomicRefCountDec(&not_currently_in_on_more_io_data_)); you
should
> be able to detect you are in the expected state and prevent future
> OnMoreIOData() from executing. (Assuming the memory doesn't get reused
> immediately)

Done.

[DaleCurtis, 2014-03-07 00:09:16]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2014-03-07 00:11:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/188243002/60001

[commit-bot: I haz the power, 2014-03-07 13:21:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-07 13:21:50]

Retried try job too often on win_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[DaleCurtis, 2014-03-07 19:03:36]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2014-03-07 19:04:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/188243002/60001

[commit-bot: I haz the power, 2014-03-07 20:25:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/188243002/60001

[commit-bot: I haz the power, 2014-03-08 10:51:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/188243002/60001

[commit-bot: I haz the power, 2014-03-08 11:45:04]

Change committed as 255757