Android: Fix memory leak for remote MediaStream

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

 /*
  * libjingle
  * Copyright 2013 Google Inc.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are met:
  *
  *  1. Redistributions of source code must retain the above copyright notice,
  *     this list of conditions and the following disclaimer.
  *  2. Redistributions in binary form must reproduce the above copyright notice,
@@ skipping 188 matching lines @@
         j_audio_track_ctor_(GetMethodID(
             jni, *j_audio_track_class_, "<init>", "(J)V")),
         j_video_track_class_(jni, FindClass(jni, "org/webrtc/VideoTrack")),
         j_video_track_ctor_(GetMethodID(
             jni, *j_video_track_class_, "<init>", "(J)V")),
         j_data_channel_class_(jni, FindClass(jni, "org/webrtc/DataChannel")),
         j_data_channel_ctor_(GetMethodID(
             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());
     std::string sdp;
     CHECK(candidate->ToString(&sdp)) << "got so far: " << sdp;
     jclass candidate_class = FindClass(jni(), "org/webrtc/IceCandidate");
     jmethodID ctor = GetMethodID(jni(), candidate_class,
         "<init>", "(Ljava/lang/String;ILjava/lang/String;)V");
     jstring j_mid = JavaStringFromStdString(jni(), candidate->sdp_mid());
     jstring j_sdp = JavaStringFromStdString(jni(), sdp);
@@ skipping 45 matching lines @@
         jni(), *j_observer_class_, "onIceGatheringChange",
         "(Lorg/webrtc/PeerConnection$IceGatheringState;)V");
     jobject new_state_enum = JavaEnumFromIndex(
         jni(), "PeerConnection$IceGatheringState", new_state);
     jni()->CallVoidMethod(*j_observer_global_, m, new_state_enum);
     CHECK_EXCEPTION(jni()) << "error during CallVoidMethod";
   }
 
   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_,
                                             "audioTracks",
                                             "Ljava/util/LinkedList;");
       jobject audio_tracks = GetObjectField(jni(), j_stream, audio_tracks_id);
       jmethodID add = GetMethodID(jni(),
                                   GetObjectClass(jni(), audio_tracks),
                                   "add",
                                   "(Ljava/lang/Object;)Z");
       jboolean added = jni()->CallBooleanMethod(audio_tracks, add, j_track);
       CHECK_EXCEPTION(jni()) << "error during CallBooleanMethod";
       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_,
                                             "videoTracks",
                                             "Ljava/util/LinkedList;");
       jobject video_tracks = GetObjectField(jni(), j_stream, video_tracks_id);
       jmethodID add = GetMethodID(jni(),
                                   GetObjectClass(jni(), video_tracks),
                                   "add",
                                   "(Ljava/lang/Object;)Z");
       jboolean added = jni()->CallBooleanMethod(video_tracks, add, j_track);
       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",
                               "(Lorg/webrtc/MediaStream;)V");
     jni()->CallVoidMethod(*j_observer_global_, m, j_stream);
     CHECK_EXCEPTION(jni()) << "error during CallVoidMethod";
   }
 
   void OnRemoveStream(MediaStreamInterface* stream) override {
     ScopedLocalRefFrame local_ref_frame(jni());
     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 {
     ScopedLocalRefFrame local_ref_frame(jni());
     jobject j_channel = jni()->NewObject(
         *j_data_channel_class_, j_data_channel_ctor_, (jlong)channel);
     CHECK_EXCEPTION(jni()) << "error during NewObject";
 
     jmethodID m = GetMethodID(jni(), *j_observer_class_, "onDataChannel",
                               "(Lorg/webrtc/DataChannel;)V");
@@ skipping 32 matching lines @@
   const ScopedGlobalRef<jobject> j_observer_global_;
   const ScopedGlobalRef<jclass> j_observer_class_;
   const ScopedGlobalRef<jclass> j_media_stream_class_;
   const jmethodID j_media_stream_ctor_;
   const ScopedGlobalRef<jclass> j_audio_track_class_;
   const jmethodID j_audio_track_ctor_;
   const ScopedGlobalRef<jclass> j_video_track_class_;
   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_;
 };
 
 // Wrapper for a Java MediaConstraints object.  Copies all needed data so when
 // the constructor returns the Java object is no longer needed.
 class ConstraintsWrapper : public MediaConstraintsInterface {
  public:
   ConstraintsWrapper(JNIEnv* jni, jobject j_constraints) {
     PopulateConstraintsFromJavaPairList(
         jni, j_constraints, "mandatory", &mandatory_);
@@ skipping 1270 matching lines @@
 }
 
 JOW(void, VideoTrack_nativeRemoveRenderer)(
     JNIEnv* jni, jclass,
     jlong j_video_track_pointer, jlong j_renderer_pointer) {
   reinterpret_cast<VideoTrackInterface*>(j_video_track_pointer)->RemoveRenderer(
       reinterpret_cast<VideoRendererInterface*>(j_renderer_pointer));
 }
 
 }  // namespace webrtc_jni