Makes AudioOutputProxy -> AudioOutputDispatcher reference weak.

Makes AudioOutputProxy -> AudioOutputDispatcher reference weak.

This patch changes the AudioOutputProxy -> AudioOutputDispatcher reference from
raw pointer to a weak pointer to allow the dispatcher to be deleted before
AudioOutputProxy instances can be closed.

BUG=608049
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2621993002
Cr-Commit-Position: refs/heads/master@{#442765}
Committed: https://chromium.googlesource.com/chromium/src/+/28b0c339305fde2065b3d51514d29c1e6280216f

[alokp, 2017-01-10 20:30:08]

Description was changed from

==========
Makes AudioOutputProxy -> AudioOutputDispatcher reference weak.

This patch changes the AudioOutputProxy -> AudioOutputDispatcher reference from
raw pointer to a weak pointer to allow the dispatcher to be deleted before
AudioOutputProxy instances can be closed.

BUG=608049
==========

to

==========
Makes AudioOutputProxy -> AudioOutputDispatcher reference weak.

This patch changes the AudioOutputProxy -> AudioOutputDispatcher reference from
raw pointer to a weak pointer to allow the dispatcher to be deleted before
AudioOutputProxy instances can be closed.

BUG=608049
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[alokp, 2017-01-10 20:33:59]

alokp@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[alokp, 2017-01-10 20:34:00]

Dale: This is what I had in mind to fix the AudioManageBase crashes due to
outstanding AOP instances. If you like the approach, I will add some unittests.

https://codereview.chromium.org/2621993002/diff/1/media/audio/audio_output_di...
File media/audio/audio_output_dispatcher_impl.cc (right):

https://codereview.chromium.org/2621993002/diff/1/media/audio/audio_output_di...
media/audio/audio_output_dispatcher_impl.cc:44: StopStream(stream_proxy);
Do you think it is worthwhile to trigger AudioSourceCallback::OnError to notify
the clients that playback will stop?

IMO it is probably not worth because the browser is already shutting down.

[DaleCurtis, 2017-01-10 20:54:41]

Seems reasonable. It will only help the OSX case since we can't invalidate the
WeakPtrs on the main thread for other platforms.

https://codereview.chromium.org/2621993002/diff/1/media/audio/audio_output_di...
File media/audio/audio_output_dispatcher.h (right):

https://codereview.chromium.org/2621993002/diff/1/media/audio/audio_output_di...
media/audio/audio_output_dispatcher.h:36: : public
base::SupportsWeakPtr<AudioOutputDispatcher> {
Instead of adding this can you make the constructor private and add a Create()
method which returns a WeakPtr? SupportsWeakPtr is a bit too permissive.

https://codereview.chromium.org/2621993002/diff/1/media/audio/audio_output_di...
File media/audio/audio_output_dispatcher_impl.cc (right):

https://codereview.chromium.org/2621993002/diff/1/media/audio/audio_output_di...
media/audio/audio_output_dispatcher_impl.cc:44: StopStream(stream_proxy);
On 2017/01/10 at 20:33:59, alokp wrote:
> Do you think it is worthwhile to trigger AudioSourceCallback::OnError to
notify the clients that playback will stop?
> 
> IMO it is probably not worth because the browser is already shutting down.

No, this should be fine. Also, just use a for each style loop instead?

[alokp, 2017-01-10 22:51:26]

Added unittests. PTAL

https://codereview.chromium.org/2621993002/diff/1/media/audio/audio_output_di...
File media/audio/audio_output_dispatcher.h (right):

https://codereview.chromium.org/2621993002/diff/1/media/audio/audio_output_di...
media/audio/audio_output_dispatcher.h:36: : public
base::SupportsWeakPtr<AudioOutputDispatcher> {
On 2017/01/10 20:54:40, DaleCurtis wrote:
> Instead of adding this can you make the constructor private and add a Create()
> method which returns a WeakPtr? SupportsWeakPtr is a bit too permissive.

A Create method will not work because AMB still needs to hold a
unique_ptr<AudioOutputDispatcher>. We just need the WeakPtr for AOPs. I could
add a virtual AudioOutputDispatcher::GetWeakPtr, but I am not sure if it would
be any better than deriving from SupportsWeakPtr.

https://codereview.chromium.org/2621993002/diff/1/media/audio/audio_output_di...
File media/audio/audio_output_dispatcher_impl.cc (right):

https://codereview.chromium.org/2621993002/diff/1/media/audio/audio_output_di...
media/audio/audio_output_dispatcher_impl.cc:44: StopStream(stream_proxy);
On 2017/01/10 20:54:41, DaleCurtis wrote:
> On 2017/01/10 at 20:33:59, alokp wrote:
> > Do you think it is worthwhile to trigger AudioSourceCallback::OnError to
> notify the clients that playback will stop?
> > 
> > IMO it is probably not worth because the browser is already shutting down.
> 
> No, this should be fine. Also, just use a for each style loop instead?

I cannot use the for-each style because StopStream would invalidate the
iterator. Note the comment.

[alokp, 2017-01-10 22:51:38]

The CQ bit was checked by alokp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 22:52:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[DaleCurtis, 2017-01-10 22:58:08]

https://codereview.chromium.org/2621993002/diff/1/media/audio/audio_output_di...
File media/audio/audio_output_dispatcher.h (right):

https://codereview.chromium.org/2621993002/diff/1/media/audio/audio_output_di...
media/audio/audio_output_dispatcher.h:36: : public
base::SupportsWeakPtr<AudioOutputDispatcher> {
On 2017/01/10 at 22:51:26, alokp wrote:
> On 2017/01/10 20:54:40, DaleCurtis wrote:
> > Instead of adding this can you make the constructor private and add a
Create()
> > method which returns a WeakPtr? SupportsWeakPtr is a bit too permissive.
> 
> A Create method will not work because AMB still needs to hold a
unique_ptr<AudioOutputDispatcher>. We just need the WeakPtr for AOPs. I could
add a virtual AudioOutputDispatcher::GetWeakPtr, but I am not sure if it would
be any better than deriving from SupportsWeakPtr.

what about AOD::CreateProxy() which returns an AudioOutputProxy using the
internal WeakPtr?

[DaleCurtis, 2017-01-10 22:59:05]

https://codereview.chromium.org/2621993002/diff/1/media/audio/audio_output_di...
File media/audio/audio_output_dispatcher_impl.cc (right):

https://codereview.chromium.org/2621993002/diff/1/media/audio/audio_output_di...
media/audio/audio_output_dispatcher_impl.cc:44: StopStream(stream_proxy);
On 2017/01/10 at 22:51:26, alokp wrote:
> On 2017/01/10 20:54:41, DaleCurtis wrote:
> > On 2017/01/10 at 20:33:59, alokp wrote:
> > > Do you think it is worthwhile to trigger AudioSourceCallback::OnError to
> > notify the clients that playback will stop?
> > > 
> > > IMO it is probably not worth because the browser is already shutting down.
> > 
> > No, this should be fine. Also, just use a for each style loop instead?
> 
> I cannot use the for-each style because StopStream would invalidate the
iterator. Note the comment.

Hmm, for some reason I thought for-each is immune to iterator invalidation since
it's not really an iterator. I'll have to go back and check.

[alokp, 2017-01-10 23:31:53]

https://codereview.chromium.org/2621993002/diff/1/media/audio/audio_output_di...
File media/audio/audio_output_dispatcher.h (right):

https://codereview.chromium.org/2621993002/diff/1/media/audio/audio_output_di...
media/audio/audio_output_dispatcher.h:36: : public
base::SupportsWeakPtr<AudioOutputDispatcher> {
On 2017/01/10 22:58:08, DaleCurtis wrote:
> On 2017/01/10 at 22:51:26, alokp wrote:
> > On 2017/01/10 20:54:40, DaleCurtis wrote:
> > > Instead of adding this can you make the constructor private and add a
> Create()
> > > method which returns a WeakPtr? SupportsWeakPtr is a bit too permissive.
> > 
> > A Create method will not work because AMB still needs to hold a
> unique_ptr<AudioOutputDispatcher>. We just need the WeakPtr for AOPs. I could
> add a virtual AudioOutputDispatcher::GetWeakPtr, but I am not sure if it would
> be any better than deriving from SupportsWeakPtr.
> 
> what about AOD::CreateProxy() which returns an AudioOutputProxy using the
> internal WeakPtr?

Yes - this should work. Done.

https://codereview.chromium.org/2621993002/diff/1/media/audio/audio_output_di...
File media/audio/audio_output_dispatcher_impl.cc (right):

https://codereview.chromium.org/2621993002/diff/1/media/audio/audio_output_di...
media/audio/audio_output_dispatcher_impl.cc:44: StopStream(stream_proxy);
On 2017/01/10 22:59:05, DaleCurtis wrote:
> On 2017/01/10 at 22:51:26, alokp wrote:
> > On 2017/01/10 20:54:41, DaleCurtis wrote:
> > > On 2017/01/10 at 20:33:59, alokp wrote:
> > > > Do you think it is worthwhile to trigger AudioSourceCallback::OnError to
> > > notify the clients that playback will stop?
> > > > 
> > > > IMO it is probably not worth because the browser is already shutting
down.
> > > 
> > > No, this should be fine. Also, just use a for each style loop instead?
> > 
> > I cannot use the for-each style because StopStream would invalidate the
> iterator. Note the comment.
> 
> Hmm, for some reason I thought for-each is immune to iterator invalidation
since
> it's not really an iterator. I'll have to go back and check.

AFAICT none - for_each or c++11 range-based for-loop - do not guarantee iterator
to be valid:
http://en.cppreference.com/w/cpp/language/range-for
http://en.cppreference.com/w/cpp/algorithm/for_each

In fact the possible implementation provided in the documentation would clearly
not work.

[DaleCurtis, 2017-01-10 23:46:34]

lgtm % a small cleanup.

https://codereview.chromium.org/2621993002/diff/40001/media/audio/audio_outpu...
File media/audio/audio_output_dispatcher_impl.cc (right):

https://codereview.chromium.org/2621993002/diff/40001/media/audio/audio_outpu...
media/audio/audio_output_dispatcher_impl.cc:43: StopStream(stream_proxy);
Note this call does a search over the proxy map, so how about instead factoring
out an internal StopPhysicalStream method that takes an AudioOutputStream*, you
can then use the for each loop and .clear() at the end instead.

[alokp, 2017-01-11 00:12:50]

https://codereview.chromium.org/2621993002/diff/40001/media/audio/audio_outpu...
File media/audio/audio_output_dispatcher_impl.cc (right):

https://codereview.chromium.org/2621993002/diff/40001/media/audio/audio_outpu...
media/audio/audio_output_dispatcher_impl.cc:43: StopStream(stream_proxy);
On 2017/01/10 23:46:34, DaleCurtis wrote:
> Note this call does a search over the proxy map, so how about instead
factoring
> out an internal StopPhysicalStream method that takes an AudioOutputStream*,
you
> can then use the for each loop and .clear() at the end instead.

Done.

[alokp, 2017-01-11 00:12:54]

The CQ bit was checked by alokp@chromium.org

[alokp, 2017-01-11 00:12:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/2621993002/#ps60001
(title: "adds StopPhysicalStream")

[commit-bot: I haz the power, 2017-01-11 00:13:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 01:46:15]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484093574610510,
"parent_rev": "10fd2deb815a0b4e286651cdc6737413a54a5ad9", "commit_rev":
"28b0c339305fde2065b3d51514d29c1e6280216f"}

[commit-bot: I haz the power, 2017-01-11 01:46:49]

Description was changed from

==========
Makes AudioOutputProxy -> AudioOutputDispatcher reference weak.

This patch changes the AudioOutputProxy -> AudioOutputDispatcher reference from
raw pointer to a weak pointer to allow the dispatcher to be deleted before
AudioOutputProxy instances can be closed.

BUG=608049
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Makes AudioOutputProxy -> AudioOutputDispatcher reference weak.

This patch changes the AudioOutputProxy -> AudioOutputDispatcher reference from
raw pointer to a weak pointer to allow the dispatcher to be deleted before
AudioOutputProxy instances can be closed.

BUG=608049
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2621993002
Cr-Commit-Position: refs/heads/master@{#442765}
Committed:
https://chromium.googlesource.com/chromium/src/+/28b0c339305fde2065b3d51514d2...
==========

[commit-bot: I haz the power, 2017-01-11 01:46:51]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/28b0c339305fde2065b3d51514d2...