From 1c3dd38cb819733fa3f558063d4b0c135c5be6e7 Mon Sep 17 00:00:00 2001 From: Magnus Jedvert Date: Thu, 27 Aug 2015 13:39:58 +0200 Subject: [PATCH] Android: Fix memory leak for remote MediaStream BUG=webrtc:4892 R=glaznev@webrtc.org, tommi@webrtc.org Review URL: https://codereview.webrtc.org/1308733004 . Cr-Commit-Position: refs/heads/master@{#9797} --- talk/app/webrtc/java/jni/jni_helpers.cc | 11 --- talk/app/webrtc/java/jni/jni_helpers.h | 14 ---- .../app/webrtc/java/jni/peerconnection_jni.cc | 72 ++++++++++++------- .../appspot/apprtc/PeerConnectionClient.java | 4 -- 4 files changed, 46 insertions(+), 55 deletions(-) diff --git a/talk/app/webrtc/java/jni/jni_helpers.cc b/talk/app/webrtc/java/jni/jni_helpers.cc index a2cbd61971..448d1ad91b 100644 --- a/talk/app/webrtc/java/jni/jni_helpers.cc +++ b/talk/app/webrtc/java/jni/jni_helpers.cc @@ -273,17 +273,6 @@ void DeleteGlobalRef(JNIEnv* jni, jobject o) { CHECK_EXCEPTION(jni) << "error during DeleteGlobalRef"; } -WeakRef::WeakRef(JNIEnv* jni, jweak ref) - : jni_(jni), obj_(jni_->NewLocalRef(ref)) { - CHECK_EXCEPTION(jni) << "error during NewLocalRef"; -} -WeakRef::~WeakRef() { - if (obj_) { - jni_->DeleteLocalRef(obj_); - CHECK_EXCEPTION(jni_) << "error during DeleteLocalRef"; - } -} - // Scope Java local references to the lifetime of this object. Use in all C++ // callbacks (i.e. entry points that don't originate in a Java callstack // through a "native" method call). diff --git a/talk/app/webrtc/java/jni/jni_helpers.h b/talk/app/webrtc/java/jni/jni_helpers.h index 03ac5298c5..dde713728b 100644 --- a/talk/app/webrtc/java/jni/jni_helpers.h +++ b/talk/app/webrtc/java/jni/jni_helpers.h @@ -108,20 +108,6 @@ jobject NewGlobalRef(JNIEnv* jni, jobject o); void DeleteGlobalRef(JNIEnv* jni, jobject o); -// Given a jweak reference, allocate a (strong) local reference scoped to the -// lifetime of this object if the weak reference is still valid, or NULL -// otherwise. -class WeakRef { - public: - WeakRef(JNIEnv* jni, jweak ref); - ~WeakRef(); - jobject obj() { return obj_; } - - private: - JNIEnv* const jni_; - jobject const obj_; -}; - // Scope Java local references to the lifetime of this object. Use in all C++ // callbacks (i.e. entry points that don't originate in a Java callstack // through a "native" method call). diff --git a/talk/app/webrtc/java/jni/peerconnection_jni.cc b/talk/app/webrtc/java/jni/peerconnection_jni.cc index c326ccff6b..503a75ee88 100644 --- a/talk/app/webrtc/java/jni/peerconnection_jni.cc +++ b/talk/app/webrtc/java/jni/peerconnection_jni.cc @@ -206,7 +206,11 @@ class PCOJava : public PeerConnectionObserver { jni, *j_data_channel_class_, "", "(J)V")) { } - virtual ~PCOJava() {} + virtual ~PCOJava() { + ScopedLocalRefFrame local_ref_frame(jni()); + while (!remote_streams_.empty()) + DisposeRemoteStream(remote_streams_.begin()); + } void OnIceCandidate(const IceCandidateInterface* candidate) override { ScopedLocalRefFrame local_ref_frame(jni()); @@ -272,16 +276,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 holds one reference. Corresponding Release() is in + // MediaStream_free, triggered by MediaStream.dispose(). + stream->AddRef(); + jobject j_stream = + jni()->NewObject(*j_media_stream_class_, j_media_stream_ctor_, + reinterpret_cast(stream)); 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 (const auto& track : 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 holds one reference. Corresponding Release() is in + // MediaStreamTrack_free, triggered by AudioTrack.dispose(). + track->AddRef(); + jobject j_track = + jni()->NewObject(*j_audio_track_class_, j_audio_track_ctor_, + reinterpret_cast(track.get()), id); CHECK_EXCEPTION(jni()) << "error during NewObject"; jfieldID audio_tracks_id = GetFieldID(jni(), *j_media_stream_class_, @@ -297,12 +307,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 (const auto& track : 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 holds one reference. Corresponding Release() is in + // MediaStreamTrack_free, triggered by VideoTrack.dispose(). + track->AddRef(); + jobject j_track = + jni()->NewObject(*j_video_track_class_, j_video_track_ctor_, + reinterpret_cast(track.get()), id); CHECK_EXCEPTION(jni()) << "error during NewObject"; jfieldID video_tracks_id = GetFieldID(jni(), *j_media_stream_class_, @@ -317,8 +329,7 @@ class PCOJava : public PeerConnectionObserver { CHECK_EXCEPTION(jni()) << "error during CallBooleanMethod"; CHECK(added); } - streams_[stream] = jni()->NewWeakGlobalRef(j_stream); - CHECK_EXCEPTION(jni()) << "error during NewWeakGlobalRef"; + remote_streams_[stream] = NewGlobalRef(jni(), j_stream); jmethodID m = GetMethodID(jni(), *j_observer_class_, "onAddStream", "(Lorg/webrtc/MediaStream;)V"); @@ -328,18 +339,15 @@ class PCOJava : public PeerConnectionObserver { 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); - streams_.erase(it); - if (!s.obj()) - return; - + NativeToJavaStreamsMap::iterator it = remote_streams_.find(stream); + CHECK(it != remote_streams_.end()) << "unexpected stream: " << std::hex + << stream; + jobject j_stream = it->second; 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"; + DisposeRemoteStream(it); } void OnDataChannel(DataChannelInterface* channel) override { @@ -378,6 +386,17 @@ class PCOJava : public PeerConnectionObserver { const ConstraintsWrapper* constraints() { return constraints_.get(); } private: + typedef std::map NativeToJavaStreamsMap; + + void DisposeRemoteStream(const NativeToJavaStreamsMap::iterator& it) { + jobject j_stream = it->second; + remote_streams_.erase(it); + jni()->CallVoidMethod( + j_stream, GetMethodID(jni(), *j_media_stream_class_, "dispose", "()V")); + CHECK_EXCEPTION(jni()) << "error during MediaStream.dispose()"; + DeleteGlobalRef(jni(), j_stream); + } + JNIEnv* jni() { return AttachCurrentThreadIfNeeded(); } @@ -392,8 +411,9 @@ class PCOJava : public PeerConnectionObserver { const jmethodID j_video_track_ctor_; const ScopedGlobalRef j_data_channel_class_; const jmethodID j_data_channel_ctor_; - typedef std::map NativeToJavaStreamsMap; - NativeToJavaStreamsMap streams_; // C++ -> Java streams. + // C++ -> Java remote streams. The stored jobects are global refs and must be + // manually deleted upon removal. Use DisposeRemoteStream(). + NativeToJavaStreamsMap remote_streams_; scoped_ptr constraints_; }; diff --git a/webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java b/webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java index 51ef32ea53..e720fe7414 100644 --- a/webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java +++ b/webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java @@ -911,11 +911,7 @@ public class PeerConnectionClient { executor.execute(new Runnable() { @Override public void run() { - if (peerConnection == null || isError) { - return; - } remoteVideoTrack = null; - stream.videoTracks.get(0).dispose(); } }); }