From ee61f9440a4929cea43526eaf32e6a1674fba524 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sami=20Kalliom=C3=A4ki?= Date: Mon, 4 Feb 2019 13:42:11 +0100 Subject: [PATCH] Fix a bug in video_encoder_wrapper where int array was not freed properly. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit JNI_COMMIT doesn't actually free the buffer. From JNI docs: 0: copy back the content and free the elems buffer JNI_COMMIT: copy back the content but do not free the elems buffer JNI_ABORT: free the buffer without copying back the possible changes Also introduces helper methods to help avoid this problem in the future. Bug: webrtc:10132 Change-Id: I769df286d3bd186fdf39ee2363e9002f36454509 Reviewed-on: https://webrtc-review.googlesource.com/c/120600 Reviewed-by: Magnus Jedvert Commit-Queue: Sami Kalliomäki Cr-Commit-Position: refs/heads/master@{#26529} --- sdk/android/BUILD.gn | 1 + sdk/android/native_api/jni/java_types.cc | 43 +++++++++++++++++++ sdk/android/native_api/jni/java_types.h | 13 ++++++ .../native_unittests/java_types_unittest.cc | 41 ++++++++++++++++++ .../src/jni/android_network_monitor.cc | 14 +++--- sdk/android/src/jni/pc/data_channel.cc | 7 ++- sdk/android/src/jni/video_encoder_wrapper.cc | 12 ++---- 7 files changed, 110 insertions(+), 21 deletions(-) diff --git a/sdk/android/BUILD.gn b/sdk/android/BUILD.gn index 2974e1a0f9..f666c3d427 100644 --- a/sdk/android/BUILD.gn +++ b/sdk/android/BUILD.gn @@ -860,6 +860,7 @@ if (is_android) { ":generated_external_classes_jni", ":generated_native_api_jni", ":internal_jni", + "//api:array_view", "//rtc_base:checks", "//rtc_base:rtc_base_approved", "//third_party/abseil-cpp/absl/types:optional", diff --git a/sdk/android/native_api/jni/java_types.cc b/sdk/android/native_api/jni/java_types.cc index df0a22905e..5617b18640 100644 --- a/sdk/android/native_api/jni/java_types.cc +++ b/sdk/android/native_api/jni/java_types.cc @@ -206,6 +206,49 @@ ScopedJavaLocalRef NativeToJavaString( return str ? NativeToJavaString(jni, *str) : nullptr; } +ScopedJavaLocalRef NativeToJavaByteArray( + JNIEnv* env, + rtc::ArrayView container) { + ScopedJavaLocalRef jarray(env, + env->NewByteArray(container.size())); + int8_t* array_ptr = + env->GetByteArrayElements(jarray.obj(), /*isCopy=*/nullptr); + memcpy(array_ptr, container.data(), container.size() * sizeof(int8_t)); + env->ReleaseByteArrayElements(jarray.obj(), array_ptr, /*mode=*/0); + return jarray; +} + +ScopedJavaLocalRef NativeToJavaIntArray( + JNIEnv* env, + rtc::ArrayView container) { + ScopedJavaLocalRef jarray(env, env->NewIntArray(container.size())); + int32_t* array_ptr = + env->GetIntArrayElements(jarray.obj(), /*isCopy=*/nullptr); + memcpy(array_ptr, container.data(), container.size() * sizeof(int32_t)); + env->ReleaseIntArrayElements(jarray.obj(), array_ptr, /*mode=*/0); + return jarray; +} + +std::vector JavaToNativeByteArray(JNIEnv* env, + const JavaRef& jarray) { + int8_t* array_ptr = + env->GetByteArrayElements(jarray.obj(), /*isCopy=*/nullptr); + size_t array_length = env->GetArrayLength(jarray.obj()); + std::vector container(array_ptr, array_ptr + array_length); + env->ReleaseByteArrayElements(jarray.obj(), array_ptr, /*mode=*/JNI_ABORT); + return container; +} + +std::vector JavaToNativeIntArray(JNIEnv* env, + const JavaRef& jarray) { + int32_t* array_ptr = + env->GetIntArrayElements(jarray.obj(), /*isCopy=*/nullptr); + size_t array_length = env->GetArrayLength(jarray.obj()); + std::vector container(array_ptr, array_ptr + array_length); + env->ReleaseIntArrayElements(jarray.obj(), array_ptr, /*mode=*/JNI_ABORT); + return container; +} + ScopedJavaLocalRef NativeToJavaBooleanArray( JNIEnv* env, const std::vector& container) { diff --git a/sdk/android/native_api/jni/java_types.h b/sdk/android/native_api/jni/java_types.h index 3b857049cc..c8f0c60f38 100644 --- a/sdk/android/native_api/jni/java_types.h +++ b/sdk/android/native_api/jni/java_types.h @@ -23,6 +23,7 @@ #include #include "absl/types/optional.h" +#include "api/array_view.h" #include "rtc_base/checks.h" #include "rtc_base/thread_checker.h" #include "sdk/android/native_api/jni/scoped_java_ref.h" @@ -220,6 +221,18 @@ ScopedJavaLocalRef NativeToJavaObjectArray( return j_container; } +ScopedJavaLocalRef NativeToJavaByteArray( + JNIEnv* env, + rtc::ArrayView container); +ScopedJavaLocalRef NativeToJavaIntArray( + JNIEnv* env, + rtc::ArrayView container); + +std::vector JavaToNativeByteArray(JNIEnv* env, + const JavaRef& jarray); +std::vector JavaToNativeIntArray(JNIEnv* env, + const JavaRef& jarray); + ScopedJavaLocalRef NativeToJavaBooleanArray( JNIEnv* env, const std::vector& container); diff --git a/sdk/android/native_unittests/java_types_unittest.cc b/sdk/android/native_unittests/java_types_unittest.cc index bd769e6eed..d70c4c122a 100644 --- a/sdk/android/native_unittests/java_types_unittest.cc +++ b/sdk/android/native_unittests/java_types_unittest.cc @@ -30,6 +30,47 @@ TEST(JavaTypesTest, TestJavaToNativeStringMap) { }; EXPECT_EQ(expected, output); } + +TEST(JavaTypesTest, TestNativeToJavaToNativeIntArray) { + JNIEnv* env = AttachCurrentThreadIfNeeded(); + + std::vector test_data{1, 20, 300}; + + ScopedJavaLocalRef array = NativeToJavaIntArray(env, test_data); + EXPECT_EQ(test_data, JavaToNativeIntArray(env, array)); +} + +TEST(JavaTypesTest, TestNativeToJavaToNativeByteArray) { + JNIEnv* env = AttachCurrentThreadIfNeeded(); + + std::vector test_data{1, 20, 30}; + + ScopedJavaLocalRef array = NativeToJavaByteArray(env, test_data); + EXPECT_EQ(test_data, JavaToNativeByteArray(env, array)); +} + +TEST(JavaTypesTest, TestNativeToJavaToNativeIntArrayLeakTest) { + JNIEnv* env = AttachCurrentThreadIfNeeded(); + + std::vector test_data{1, 20, 300}; + + for (int i = 0; i < 2000; i++) { + ScopedJavaLocalRef array = NativeToJavaIntArray(env, test_data); + EXPECT_EQ(test_data, JavaToNativeIntArray(env, array)); + } +} + +TEST(JavaTypesTest, TestNativeToJavaToNativeByteArrayLeakTest) { + JNIEnv* env = AttachCurrentThreadIfNeeded(); + + std::vector test_data{1, 20, 30}; + + for (int i = 0; i < 2000; i++) { + ScopedJavaLocalRef array = + NativeToJavaByteArray(env, test_data); + EXPECT_EQ(test_data, JavaToNativeByteArray(env, array)); + } +} } // namespace } // namespace test } // namespace webrtc diff --git a/sdk/android/src/jni/android_network_monitor.cc b/sdk/android/src/jni/android_network_monitor.cc index b45d17758c..31369a184e 100644 --- a/sdk/android/src/jni/android_network_monitor.cc +++ b/sdk/android/src/jni/android_network_monitor.cc @@ -99,23 +99,19 @@ static rtc::AdapterType AdapterTypeFromNetworkType(NetworkType network_type) { static rtc::IPAddress JavaToNativeIpAddress( JNIEnv* jni, const JavaRef& j_ip_address) { - ScopedJavaLocalRef j_addresses = - Java_IPAddress_getAddress(jni, j_ip_address); - size_t address_length = jni->GetArrayLength(j_addresses.obj()); - jbyte* addr_array = jni->GetByteArrayElements(j_addresses.obj(), nullptr); - CHECK_EXCEPTION(jni) << "Error during JavaToNativeIpAddress"; + std::vector address = + JavaToNativeByteArray(jni, Java_IPAddress_getAddress(jni, j_ip_address)); + size_t address_length = address.size(); if (address_length == 4) { // IP4 struct in_addr ip4_addr; - memcpy(&ip4_addr.s_addr, addr_array, 4); - jni->ReleaseByteArrayElements(j_addresses.obj(), addr_array, JNI_ABORT); + memcpy(&ip4_addr.s_addr, address.data(), 4); return rtc::IPAddress(ip4_addr); } // IP6 RTC_CHECK(address_length == 16); struct in6_addr ip6_addr; - memcpy(ip6_addr.s6_addr, addr_array, address_length); - jni->ReleaseByteArrayElements(j_addresses.obj(), addr_array, JNI_ABORT); + memcpy(ip6_addr.s6_addr, address.data(), address_length); return rtc::IPAddress(ip6_addr); } diff --git a/sdk/android/src/jni/pc/data_channel.cc b/sdk/android/src/jni/pc/data_channel.cc index 4f704e2f61..bd3b1712fa 100644 --- a/sdk/android/src/jni/pc/data_channel.cc +++ b/sdk/android/src/jni/pc/data_channel.cc @@ -146,10 +146,9 @@ static jboolean JNI_DataChannel_Send(JNIEnv* jni, const JavaParamRef& j_dc, const JavaParamRef& data, jboolean binary) { - jbyte* bytes = jni->GetByteArrayElements(data.obj(), nullptr); - bool ret = ExtractNativeDC(jni, j_dc)->Send(DataBuffer( - rtc::CopyOnWriteBuffer(bytes, jni->GetArrayLength(data.obj())), binary)); - jni->ReleaseByteArrayElements(data.obj(), bytes, JNI_ABORT); + std::vector buffer = JavaToNativeByteArray(jni, data); + bool ret = ExtractNativeDC(jni, j_dc)->Send( + DataBuffer(rtc::CopyOnWriteBuffer(buffer.data(), buffer.size()), binary)); return ret; } diff --git a/sdk/android/src/jni/video_encoder_wrapper.cc b/sdk/android/src/jni/video_encoder_wrapper.cc index 5d34cf75fe..77c1850fb0 100644 --- a/sdk/android/src/jni/video_encoder_wrapper.cc +++ b/sdk/android/src/jni/video_encoder_wrapper.cc @@ -421,17 +421,13 @@ ScopedJavaLocalRef VideoEncoderWrapper::ToJavaBitrateAllocation( jni, jni->NewObjectArray(kMaxSpatialLayers, int_array_class_.obj(), nullptr /* initial */)); for (int spatial_i = 0; spatial_i < kMaxSpatialLayers; ++spatial_i) { - ScopedJavaLocalRef j_array_spatial_layer( - jni, jni->NewIntArray(kMaxTemporalStreams)); - jint* array_spatial_layer = jni->GetIntArrayElements( - j_array_spatial_layer.obj(), nullptr /* isCopy */); + std::array spatial_layer; for (int temporal_i = 0; temporal_i < kMaxTemporalStreams; ++temporal_i) { - array_spatial_layer[temporal_i] = - allocation.GetBitrate(spatial_i, temporal_i); + spatial_layer[temporal_i] = allocation.GetBitrate(spatial_i, temporal_i); } - jni->ReleaseIntArrayElements(j_array_spatial_layer.obj(), - array_spatial_layer, JNI_COMMIT); + ScopedJavaLocalRef j_array_spatial_layer = + NativeToJavaIntArray(jni, spatial_layer); jni->SetObjectArrayElement(j_allocation_array.obj(), spatial_i, j_array_spatial_layer.obj()); }