Fix the ownership of media_log_ in WebMediaPlayerAndroid

Fix the ownership of media_log_ in WebMediaPlayerAndroid

WebMediaPlayer should own media_log_, fix the leak there.

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

[qinmin, 2014-08-21 20:53:51]

PTAL

[acolwell GONE FROM CHROMIUM, 2014-08-21 21:46:13]

Please don't do this. I'm actively working on a patch to make it possible to
create WebMediaPlayer's outside of content/ and putting RenderMediaLog in the
WebMediaPlayer implementations actually adds a content/ dependency to these
classes. Please keep the allocation in render_frame_impl and just fix the
ownership issues.

[qinmin, 2014-08-21 22:01:11]

On 2014/08/21 21:46:13, acolwell wrote:
> Please don't do this. I'm actively working on a patch to make it possible to
> create WebMediaPlayer's outside of content/ and putting RenderMediaLog in the
> WebMediaPlayer implementations actually adds a content/ dependency to these
> classes. Please keep the allocation in render_frame_impl and just fix the
> ownership issues.

Ok, updated the change to only fix the ownership issue.

[xhwang, 2014-08-21 22:40:28]

https://codereview.chromium.org/496103002/diff/20001/content/renderer/media/a...
File content/renderer/media/android/webmediaplayer_android.h (right):

https://codereview.chromium.org/496103002/diff/20001/content/renderer/media/a...
content/renderer/media/android/webmediaplayer_android.h:454:
scoped_refptr<media::MediaLog> media_log_;
Since now WMPA partially owns media_log_, does it make sense to move this up
(before l.437) so that during destruction media_source_delegate_ won't use
media_log_ after media_log_ is already destructed?

[qinmin, 2014-08-22 01:03:59]

https://codereview.chromium.org/496103002/diff/20001/content/renderer/media/a...
File content/renderer/media/android/webmediaplayer_android.h (right):

https://codereview.chromium.org/496103002/diff/20001/content/renderer/media/a...
content/renderer/media/android/webmediaplayer_android.h:454:
scoped_refptr<media::MediaLog> media_log_;
Currently, we pass a raw pointer to media_source_delegate_ and let
media_source_delegate_ taking the ownership. This will leak this pointer for
non-MSE case.
So if both webmediaplayerandroid and MediaSourceDelegate references the
media_log_, reference counting will release the pointer when both WMPA and MSD
are destroyed. As a result, putting media_log_ in front of
media_source_delegate_ doesn't help, since the pointer will still hanging around
until media_source_delegate_ gets deleted .  
Modified the code to pass a refptr instead of a raw pointer to MSD.
Or, alternatively, we can call swap inside MSD. In that case, WMPA will no
longer own the ptr in MSE case, and owns it in non-MSE case.
On 2014/08/21 22:40:28, xhwang wrote:
> Since now WMPA partially owns media_log_, does it make sense to move this up
> (before l.437) so that during destruction media_source_delegate_ won't use
> media_log_ after media_log_ is already destructed?

[xhwang, 2014-08-22 02:34:06]

lgtm % nit

https://codereview.chromium.org/496103002/diff/60001/content/renderer/media/a...
File content/renderer/media/android/media_source_delegate.h (right):

https://codereview.chromium.org/496103002/diff/60001/content/renderer/media/a...
content/renderer/media/android/media_source_delegate.h:59:
scoped_refptr<media::MediaLog> media_log);
nit: in media code, we pass scoped_refptr by const ref, as you can see in the
previous line.

[qinmin, 2014-08-22 16:32:09]

https://codereview.chromium.org/496103002/diff/60001/content/renderer/media/a...
File content/renderer/media/android/media_source_delegate.h (right):

https://codereview.chromium.org/496103002/diff/60001/content/renderer/media/a...
content/renderer/media/android/media_source_delegate.h:59:
scoped_refptr<media::MediaLog> media_log);
On 2014/08/22 02:34:06, xhwang wrote:
> nit: in media code, we pass scoped_refptr by const ref, as you can see in the
> previous line.

Done.

[qinmin, 2014-08-22 16:32:11]

The CQ bit was checked by qinmin@chromium.org

[commit-bot: I haz the power, 2014-08-22 16:35:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/qinmin@chromium.org/496103002/80001

[commit-bot: I haz the power, 2014-08-22 17:54:42]

Committed patchset #4 (80001) as 291443