Remove dangling PlayingState pointers in WebRtcAudioRenderer.

Remove dangling PlayingState pointers in WebRtcAudioRenderer.

This solution is a bit of a hack, but it works. Did manual testing and ran
unit tests. Bug 697256 does not reproduce with this patch.

R=tommi@chromium.org

BUG=697256
Review-Url: https://codereview.chromium.org/2758453004
Cr-Commit-Position: refs/heads/master@{#458025}
Committed: https://chromium.googlesource.com/chromium/src/+/ba21ffd704aebcc8a5e5e1ab1f072e9b79b51fcb

[Max Morin, 2017-03-17 10:53:49]

Tommi: WDYT of this approach? Also, feel free to add more reviewers if you know
someone who should have a look at this.

[Max Morin, 2017-03-17 11:05:19]

Description was changed from

==========
Remove dangling PlayingState pointers in WebRtcAudioRenderer.

This solution is a bit of a hack, but it works. Did manual testing and ran
unit tests. Bug 697256 does not reproduce with this patch.

R=tommi@chromium.org

BUG=697256
==========

to

==========
Remove dangling PlayingState pointers in WebRtcAudioRenderer.

This solution is a bit of a hack, but it works. Did manual testing and ran
unit tests. Bug 697256 does not reproduce with this patch.

R=tommi@chromium.org

BUG=697256
==========

[Max Morin, 2017-03-17 11:19:52]

Also CC Olga, Solis.

[Max Morin, 2017-03-17 11:49:26]

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

[commit-bot: I haz the power, 2017-03-17 11:49:36]

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

[o1ka, 2017-03-17 12:03:30]

https://codereview.chromium.org/2758453004/diff/1/content/renderer/media/webr...
File content/renderer/media/webrtc_audio_renderer.cc (right):

https://codereview.chromium.org/2758453004/diff/1/content/renderer/media/webr...
content/renderer/media/webrtc_audio_renderer.cc:577: // We must ensure that no
references to playing_state remains.
Could you add explanation and reference to a bug?

[Max Morin, 2017-03-17 12:21:44]

https://codereview.chromium.org/2758453004/diff/1/content/renderer/media/webr...
File content/renderer/media/webrtc_audio_renderer.cc (right):

https://codereview.chromium.org/2758453004/diff/1/content/renderer/media/webr...
content/renderer/media/webrtc_audio_renderer.cc:577: // We must ensure that no
references to playing_state remains.
On 2017/03/17 12:03:30, o1ka wrote:
> Could you add explanation and reference to a bug?

Done.

[tommi (sloooow) - chröme, 2017-03-17 12:44:08]

https://codereview.chromium.org/2758453004/diff/1/content/renderer/media/webr...
File content/renderer/media/webrtc_audio_renderer.cc (right):

https://codereview.chromium.org/2758453004/diff/1/content/renderer/media/webr...
content/renderer/media/webrtc_audio_renderer.cc:57: // or SetVolume are called
(whenever the internal |playing_state_| changes).
add documentation for the new parameter

https://codereview.chromium.org/2758453004/diff/1/content/renderer/media/webr...
File content/renderer/media/webrtc_audio_renderer.h (right):

https://codereview.chromium.org/2758453004/diff/1/content/renderer/media/webr...
content/renderer/media/webrtc_audio_renderer.h:196: bool playing_state_deleted);
what about including a TODO here to clean this up. I agree that it doesn't look
pretty.

https://codereview.chromium.org/2758453004/diff/20001/content/renderer/media/...
File content/renderer/media/webrtc_audio_renderer.cc (right):

https://codereview.chromium.org/2758453004/diff/20001/content/renderer/media/...
content/renderer/media/webrtc_audio_renderer.cc:80:
on_play_state_changed_.Run(media_stream_, &playing_state_, true);
did you consider having a separate callback for when a shared renderer goes
away?

Alternatively, since a SharedAudioRenderer does have a pointer to the root
WebRtcAudioRenderer, what about calling a method such as
delegate_->OnSharedAudioRendererGone(this)?  (I suppose we can't call
delegate_->RemovePlayingState()?)

[Max Morin, 2017-03-17 14:08:04]

https://codereview.chromium.org/2758453004/diff/1/content/renderer/media/webr...
File content/renderer/media/webrtc_audio_renderer.h (right):

https://codereview.chromium.org/2758453004/diff/1/content/renderer/media/webr...
content/renderer/media/webrtc_audio_renderer.h:196: bool playing_state_deleted);
On 2017/03/17 12:44:08, tommi - chröme wrote:
> what about including a TODO here to clean this up. I agree that it doesn't
look
> pretty.

I put a TODO in OnPlayStateRemoved.

https://codereview.chromium.org/2758453004/diff/20001/content/renderer/media/...
File content/renderer/media/webrtc_audio_renderer.cc (right):

https://codereview.chromium.org/2758453004/diff/20001/content/renderer/media/...
content/renderer/media/webrtc_audio_renderer.cc:80:
on_play_state_changed_.Run(media_stream_, &playing_state_, true);
On 2017/03/17 12:44:08, tommi - chröme wrote:
> did you consider having a separate callback for when a shared renderer goes
> away?
> 
> Alternatively, since a SharedAudioRenderer does have a pointer to the root
> WebRtcAudioRenderer, what about calling a method such as
> delegate_->OnSharedAudioRendererGone(this)?  (I suppose we can't call
> delegate_->RemovePlayingState()?)

I went ahead and kept delegate_ as a pointer to interface and used a callback,
consistent with how OnPlayStateChanged is treated. Calling RemovePlayingState
directly isn't possible since we don't know which sources to call it with.

[tommi (sloooow) - chröme, 2017-03-17 15:23:58]

lgtm

[Max Morin, 2017-03-20 07:44:03]

The CQ bit was checked by maxmorin@chromium.org

[commit-bot: I haz the power, 2017-03-20 07:44:15]

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-03-20 08:39:35]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1489995843627170,
"parent_rev": "4342d3eacba11f513071dd4bb06b182b4f1245f3", "commit_rev":
"ba21ffd704aebcc8a5e5e1ab1f072e9b79b51fcb"}

[commit-bot: I haz the power, 2017-03-20 08:40:37]

Description was changed from

==========
Remove dangling PlayingState pointers in WebRtcAudioRenderer.

This solution is a bit of a hack, but it works. Did manual testing and ran
unit tests. Bug 697256 does not reproduce with this patch.

R=tommi@chromium.org

BUG=697256
==========

to

==========
Remove dangling PlayingState pointers in WebRtcAudioRenderer.

This solution is a bit of a hack, but it works. Did manual testing and ran
unit tests. Bug 697256 does not reproduce with this patch.

R=tommi@chromium.org

BUG=697256

Review-Url: https://codereview.chromium.org/2758453004
Cr-Commit-Position: refs/heads/master@{#458025}
Committed:
https://chromium.googlesource.com/chromium/src/+/ba21ffd704aebcc8a5e5e1ab1f07...
==========

[commit-bot: I haz the power, 2017-03-20 08:40:38]

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

[Max Morin, 2017-05-03 10:45:28]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2856943003/ by maxmorin@chromium.org.

The reason for reverting is: This causes a memory use increase across the board
on Android, even when WebRTC isn't used..