Android: Fix memory leak for remote MediaStream

Android: Fix memory leak for remote MediaStream

BUG=webrtc:4892
R=glaznev@webrtc.org, tommi@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/1c3dd38cb819733fa3f558063d4b0c135c5be6e7

[magjed_webrtc, 2015-08-25 16:02:12]

The CQ bit was checked by magjed@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-25 16:02:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1308733004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1308733004/60001

[magjed_webrtc, 2015-08-25 16:02:24]

Patchset #3 (id:40001) has been deleted

[magjed_webrtc, 2015-08-25 16:02:29]

Patchset #2 (id:20001) has been deleted

[magjed_webrtc, 2015-08-25 16:02:36]

Patchset #1 (id:1) has been deleted

[magjed_webrtc, 2015-08-25 16:03:11]

magjed@webrtc.org changed reviewers:
+ glaznev@webrtc.org, tommi@webrtc.org

[magjed_webrtc, 2015-08-25 16:37:42]

Please take a look.

[tommi, 2015-08-25 16:45:21]

lgtm

https://codereview.webrtc.org/1308733004/diff/60001/talk/app/webrtc/java/jni/...
File talk/app/webrtc/java/jni/peerconnection_jni.cc (right):

https://codereview.webrtc.org/1308733004/diff/60001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/peerconnection_jni.cc:283:
(jlong)stream_refcounted.release());
reinterpret_cast<>

[AlexG, 2015-08-25 17:16:58]

https://codereview.webrtc.org/1308733004/diff/60001/talk/app/webrtc/java/jni/...
File talk/app/webrtc/java/jni/peerconnection_jni.cc (right):

https://codereview.webrtc.org/1308733004/diff/60001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/peerconnection_jni.cc:211:
OnRemoveStream(streams_.begin()->first);
it is probably not worth to call OnRemoveStream here - it will invoke
unnecessary Java OnStreamRemove() callbacks while object is destroyed. Just call
dispose() for MediaStream and delete global ref or add extra parameter to
OnRemoveStream to not invoke observer callback

https://codereview.webrtc.org/1308733004/diff/60001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/peerconnection_jni.cc:278: // Java MediaStream takes
ownership of one reference. It is released in
Why is this extra change needed? Old code looks shorter and seems like same
functionality for me.

https://codereview.webrtc.org/1308733004/diff/60001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/peerconnection_jni.cc:286: for
(rtc::scoped_refptr<AudioTrackInterface> track :
ditto - do we need this change?

https://codereview.webrtc.org/1308733004/diff/60001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/peerconnection_jni.cc:309: for
(rtc::scoped_refptr<VideoTrackInterface> track :
ditto - no need in this change?

https://codereview.webrtc.org/1308733004/diff/60001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/peerconnection_jni.cc:345: jobject j_stream =
it->second;
Remove WekRef class from jni_helpers.h - I don't think it is used anymore now.

https://codereview.webrtc.org/1308733004/diff/60001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/peerconnection_jni.cc:353: jni()->CallVoidMethod(
With this change no need to call stream.videoTracks.get(0).dispose() in
PeerConnectionClient.onRemoveStream()?

https://codereview.webrtc.org/1308733004/diff/60001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/peerconnection_jni.cc:410: // C++ -> Java streams. The
stored jobects are global refs and must be
nit: add "remote" for clarity - i.e. "C++ -> Java remote streams"

https://codereview.webrtc.org/1308733004/diff/60001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/peerconnection_jni.cc:412: NativeToJavaStreamsMap
streams_;
nit: streams_ -> remote_streams_

[AlexG, 2015-08-25 17:19:59]

https://codereview.webrtc.org/1308733004/diff/60001/talk/app/webrtc/java/jni/...
File talk/app/webrtc/java/jni/peerconnection_jni.cc (right):

https://codereview.webrtc.org/1308733004/diff/60001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/peerconnection_jni.cc:353: jni()->CallVoidMethod(
On 2015/08/25 17:16:58, AlexG wrote:
> With this change no need to call stream.videoTracks.get(0).dispose() in
> PeerConnectionClient.onRemoveStream()?

Sorry - ignore this comment - didn't notice you've already done the change.

[commit-bot: I haz the power, 2015-08-25 17:48:21]

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

[commit-bot: I haz the power, 2015-08-25 17:48:21]

Dry run: Try jobs failed on following builders:
  mac_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[magjed_webrtc, 2015-08-25 21:30:52]

https://codereview.webrtc.org/1308733004/diff/60001/talk/app/webrtc/java/jni/...
File talk/app/webrtc/java/jni/peerconnection_jni.cc (right):

https://codereview.webrtc.org/1308733004/diff/60001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/peerconnection_jni.cc:211:
OnRemoveStream(streams_.begin()->first);
On 2015/08/25 17:16:58, AlexG wrote:
> it is probably not worth to call OnRemoveStream here - it will invoke
> unnecessary Java OnStreamRemove() callbacks while object is destroyed. Just
call
> dispose() for MediaStream and delete global ref or add extra parameter to
> OnRemoveStream to not invoke observer callback

Done.

https://codereview.webrtc.org/1308733004/diff/60001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/peerconnection_jni.cc:278: // Java MediaStream takes
ownership of one reference. It is released in
On 2015/08/25 17:16:58, AlexG wrote:
> Why is this extra change needed? Old code looks shorter and seems like same
> functionality for me.

I wanted to avoid using raw pointers for ref counted objects. The only
functional change is that an extra AddRef() is called. I tried to make it more
explicit in the new patch.

https://codereview.webrtc.org/1308733004/diff/60001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/peerconnection_jni.cc:283:
(jlong)stream_refcounted.release());
On 2015/08/25 16:45:21, tommi (webrtc) wrote:
> reinterpret_cast<>

Done.

https://codereview.webrtc.org/1308733004/diff/60001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/peerconnection_jni.cc:286: for
(rtc::scoped_refptr<AudioTrackInterface> track :
On 2015/08/25 17:16:58, AlexG wrote:
> ditto - do we need this change?

Same here, need to balance the Release() in MediaStreamTrack_free that wasn't
called before with an AddRef(). The tracks we create here are currently not safe
because they might be deallocated when C++ releases its last ref.

https://codereview.webrtc.org/1308733004/diff/60001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/peerconnection_jni.cc:309: for
(rtc::scoped_refptr<VideoTrackInterface> track :
On 2015/08/25 17:16:58, AlexG wrote:
> ditto - no need in this change?

ditto.

https://codereview.webrtc.org/1308733004/diff/60001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/peerconnection_jni.cc:345: jobject j_stream =
it->second;
On 2015/08/25 17:16:58, AlexG wrote:
> Remove WekRef class from jni_helpers.h - I don't think it is used anymore now.

Good catch.

https://codereview.webrtc.org/1308733004/diff/60001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/peerconnection_jni.cc:353: jni()->CallVoidMethod(
On 2015/08/25 17:16:58, AlexG wrote:
> With this change no need to call stream.videoTracks.get(0).dispose() in
> PeerConnectionClient.onRemoveStream()?

Exactly, MediaStream.dispose() will tear down the whole hierarchy.

https://codereview.webrtc.org/1308733004/diff/60001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/peerconnection_jni.cc:410: // C++ -> Java streams. The
stored jobects are global refs and must be
On 2015/08/25 17:16:58, AlexG wrote:
> nit: add "remote" for clarity - i.e. "C++ -> Java remote streams"

Done.

https://codereview.webrtc.org/1308733004/diff/60001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/peerconnection_jni.cc:412: NativeToJavaStreamsMap
streams_;
On 2015/08/25 17:16:58, AlexG wrote:
> nit: streams_ -> remote_streams_

Done.

[AlexG, 2015-08-26 17:18:07]

lgtm

https://codereview.webrtc.org/1308733004/diff/80001/talk/app/webrtc/java/jni/...
File talk/app/webrtc/java/jni/peerconnection_jni.cc (right):

https://codereview.webrtc.org/1308733004/diff/80001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/peerconnection_jni.cc:332: remote_streams_[stream] =
jni()->NewGlobalRef(j_stream);
nit: use NewGlobalRef(jni, j_stream) and remove or rename CHECK_EXCEPTION below
- it prints no longer used "NewWeakGlobalRef".

[magjed_webrtc, 2015-08-27 08:52:57]

The CQ bit was checked by magjed@webrtc.org

[magjed_webrtc, 2015-08-27 08:52:57]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org,
glaznev@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1308733004/#ps100001 (title:
"use helper NewGlobalRef function instead")

[commit-bot: I haz the power, 2015-08-27 08:53:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1308733004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1308733004/100001

[commit-bot: I haz the power, 2015-08-27 10:53:18]

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

[commit-bot: I haz the power, 2015-08-27 10:53:19]

Try jobs failed on following builders:
  android on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[magjed_webrtc, 2015-08-27 11:40:15]

Committed patchset #3 (id:100001) manually as
1c3dd38cb819733fa3f558063d4b0c135c5be6e7 (presubmit successful).