Android: Fix memory leak for remote MediaStream

talk/app/webrtc/java/jni/peerconnection_jni.cc

Index: talk/app/webrtc/java/jni/peerconnection_jni.cc
diff --git a/talk/app/webrtc/java/jni/peerconnection_jni.cc b/talk/app/webrtc/java/jni/peerconnection_jni.cc
index c326ccff6bede1e3e93e5605be72be86a672df7d..f65e42ddec97f31221479d56095656819c02c450 100644
--- a/talk/app/webrtc/java/jni/peerconnection_jni.cc
+++ b/talk/app/webrtc/java/jni/peerconnection_jni.cc
@@ -206,7 +206,10 @@ class PCOJava : public PeerConnectionObserver {
             jni, *j_data_channel_class_, "<init>", "(J)V")) {
   }
 
-  virtual ~PCOJava() {}
+  virtual ~PCOJava() {
+    while (!streams_.empty())
+      OnRemoveStream(streams_.begin()->first);

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

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

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

Done.

+  }
 
   void OnIceCandidate(const IceCandidateInterface* candidate) override {
     ScopedLocalRefFrame local_ref_frame(jni());
@@ -272,16 +275,22 @@ class PCOJava : public PeerConnectionObserver {
 
   void OnAddStream(MediaStreamInterface* stream) override {
     ScopedLocalRefFrame local_ref_frame(jni());
-    jobject j_stream = jni()->NewObject(
-        *j_media_stream_class_, j_media_stream_ctor_, (jlong)stream);
+    // Java MediaStream takes ownership of one reference. It is released in

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

Why is this extra change needed? Old code looks shorter and seems like same
functionality for me.

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

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.

+    // MediaStream_free.
+    rtc::scoped_refptr<MediaStreamInterface> stream_refcounted(stream);
+    jobject j_stream =
+        jni()->NewObject(*j_media_stream_class_, j_media_stream_ctor_,
+                         (jlong)stream_refcounted.release());

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

reinterpret_cast<>

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

Done.

     CHECK_EXCEPTION(jni()) << "error during NewObject";
 
-    AudioTrackVector audio_tracks = stream->GetAudioTracks();
-    for (size_t i = 0; i < audio_tracks.size(); ++i) {
-      AudioTrackInterface* track = audio_tracks[i];
+    for (rtc::scoped_refptr<AudioTrackInterface> track :

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

ditto - do we need this change?

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

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.

+         stream->GetAudioTracks()) {
       jstring id = JavaStringFromStdString(jni(), track->id());
-      jobject j_track = jni()->NewObject(
-          *j_audio_track_class_, j_audio_track_ctor_, (jlong)track, id);
+      // Java AudioTrack takes ownership of one reference. It is released in
+      // MediaStreamTrack_free.
+      jobject j_track =
+          jni()->NewObject(*j_audio_track_class_, j_audio_track_ctor_,
+                           (jlong)track.release(), id);
       CHECK_EXCEPTION(jni()) << "error during NewObject";
       jfieldID audio_tracks_id = GetFieldID(jni(),
                                             *j_media_stream_class_,
@@ -297,12 +306,14 @@ class PCOJava : public PeerConnectionObserver {
       CHECK(added);
     }
 
-    VideoTrackVector video_tracks = stream->GetVideoTracks();
-    for (size_t i = 0; i < video_tracks.size(); ++i) {
-      VideoTrackInterface* track = video_tracks[i];
+    for (rtc::scoped_refptr<VideoTrackInterface> track :

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

ditto - no need in this change?

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

ditto.

+         stream->GetVideoTracks()) {
       jstring id = JavaStringFromStdString(jni(), track->id());
-      jobject j_track = jni()->NewObject(
-          *j_video_track_class_, j_video_track_ctor_, (jlong)track, id);
+      // Java VideoTrack takes ownership of one reference. It is released in
+      // MediaStreamTrack_free.
+      jobject j_track =
+          jni()->NewObject(*j_video_track_class_, j_video_track_ctor_,
+                           (jlong)track.release(), id);
       CHECK_EXCEPTION(jni()) << "error during NewObject";
       jfieldID video_tracks_id = GetFieldID(jni(),
                                             *j_media_stream_class_,
@@ -317,7 +328,7 @@ class PCOJava : public PeerConnectionObserver {
       CHECK_EXCEPTION(jni()) << "error during CallBooleanMethod";
       CHECK(added);
     }
-    streams_[stream] = jni()->NewWeakGlobalRef(j_stream);
+    streams_[stream] = jni()->NewGlobalRef(j_stream);
     CHECK_EXCEPTION(jni()) << "error during NewWeakGlobalRef";
 
     jmethodID m = GetMethodID(jni(), *j_observer_class_, "onAddStream",
@@ -331,15 +342,18 @@ class PCOJava : public PeerConnectionObserver {
     NativeToJavaStreamsMap::iterator it = streams_.find(stream);
     CHECK(it != streams_.end()) << "unexpected stream: " << std::hex << stream;
 
-    WeakRef s(jni(), it->second);
+    jobject j_stream = it->second;

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

Remove WekRef class from jni_helpers.h - I don't think it is used anymore now.

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

Good catch.

     streams_.erase(it);
-    if (!s.obj())
-      return;
 
     jmethodID m = GetMethodID(jni(), *j_observer_class_, "onRemoveStream",
                               "(Lorg/webrtc/MediaStream;)V");
-    jni()->CallVoidMethod(*j_observer_global_, m, s.obj());
+    jni()->CallVoidMethod(*j_observer_global_, m, j_stream);
     CHECK_EXCEPTION(jni()) << "error during CallVoidMethod";
+
+    jni()->CallVoidMethod(

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

With this change no need to call stream.videoTracks.get(0).dispose() in
PeerConnectionClient.onRemoveStream()?

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

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

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

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

+        j_stream, GetMethodID(jni(), *j_media_stream_class_, "dispose", "()V"));
+    CHECK_EXCEPTION(jni()) << "error during MediaStream.dispose()";
+    DeleteGlobalRef(jni(), j_stream);
   }
 
   void OnDataChannel(DataChannelInterface* channel) override {
@@ -392,8 +406,10 @@ class PCOJava : public PeerConnectionObserver {
   const jmethodID j_video_track_ctor_;
   const ScopedGlobalRef<jclass> j_data_channel_class_;
   const jmethodID j_data_channel_ctor_;
-  typedef std::map<void*, jweak> NativeToJavaStreamsMap;
-  NativeToJavaStreamsMap streams_;  // C++ -> Java streams.
+  typedef std::map<MediaStreamInterface*, jobject> NativeToJavaStreamsMap;
+  // C++ -> Java streams. The stored jobects are global refs and must be

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

nit: add "remote" for clarity - i.e. "C++ -> Java remote streams"

[magjed_webrtc, 2015/08/25 21:30:51]

Done.

+  // manually deleted upon removal.
+  NativeToJavaStreamsMap streams_;

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

nit: streams_ -> remote_streams_

[magjed_webrtc, 2015/08/25 21:30:51]

Done.

   scoped_ptr<ConstraintsWrapper> constraints_;
 };