Fix a bug in video_encoder_wrapper where int array was not freed properly.
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 <magjed@webrtc.org>
Commit-Queue: Sami Kalliomäki <sakal@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#26529}diff --git a/sdk/android/BUILD.gn b/sdk/android/BUILD.gn
index 2974e1a..f666c3d 100644
--- a/sdk/android/BUILD.gn
+++ b/sdk/android/BUILD.gn
@@ -860,6 +860,7 @@
":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 df0a229..5617b18 100644
--- a/sdk/android/native_api/jni/java_types.cc
+++ b/sdk/android/native_api/jni/java_types.cc
@@ -206,6 +206,49 @@
return str ? NativeToJavaString(jni, *str) : nullptr;
}
+ScopedJavaLocalRef<jbyteArray> NativeToJavaByteArray(
+ JNIEnv* env,
+ rtc::ArrayView<int8_t> container) {
+ ScopedJavaLocalRef<jbyteArray> 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<jintArray> NativeToJavaIntArray(
+ JNIEnv* env,
+ rtc::ArrayView<int32_t> container) {
+ ScopedJavaLocalRef<jintArray> 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<int8_t> JavaToNativeByteArray(JNIEnv* env,
+ const JavaRef<jbyteArray>& jarray) {
+ int8_t* array_ptr =
+ env->GetByteArrayElements(jarray.obj(), /*isCopy=*/nullptr);
+ size_t array_length = env->GetArrayLength(jarray.obj());
+ std::vector<int8_t> container(array_ptr, array_ptr + array_length);
+ env->ReleaseByteArrayElements(jarray.obj(), array_ptr, /*mode=*/JNI_ABORT);
+ return container;
+}
+
+std::vector<int32_t> JavaToNativeIntArray(JNIEnv* env,
+ const JavaRef<jintArray>& jarray) {
+ int32_t* array_ptr =
+ env->GetIntArrayElements(jarray.obj(), /*isCopy=*/nullptr);
+ size_t array_length = env->GetArrayLength(jarray.obj());
+ std::vector<int32_t> container(array_ptr, array_ptr + array_length);
+ env->ReleaseIntArrayElements(jarray.obj(), array_ptr, /*mode=*/JNI_ABORT);
+ return container;
+}
+
ScopedJavaLocalRef<jobjectArray> NativeToJavaBooleanArray(
JNIEnv* env,
const std::vector<bool>& container) {
diff --git a/sdk/android/native_api/jni/java_types.h b/sdk/android/native_api/jni/java_types.h
index 3b85704..c8f0c60 100644
--- a/sdk/android/native_api/jni/java_types.h
+++ b/sdk/android/native_api/jni/java_types.h
@@ -23,6 +23,7 @@
#include <vector>
#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 @@
return j_container;
}
+ScopedJavaLocalRef<jbyteArray> NativeToJavaByteArray(
+ JNIEnv* env,
+ rtc::ArrayView<int8_t> container);
+ScopedJavaLocalRef<jintArray> NativeToJavaIntArray(
+ JNIEnv* env,
+ rtc::ArrayView<int32_t> container);
+
+std::vector<int8_t> JavaToNativeByteArray(JNIEnv* env,
+ const JavaRef<jbyteArray>& jarray);
+std::vector<int32_t> JavaToNativeIntArray(JNIEnv* env,
+ const JavaRef<jintArray>& jarray);
+
ScopedJavaLocalRef<jobjectArray> NativeToJavaBooleanArray(
JNIEnv* env,
const std::vector<bool>& container);
diff --git a/sdk/android/native_unittests/java_types_unittest.cc b/sdk/android/native_unittests/java_types_unittest.cc
index bd769e6..d70c4c1 100644
--- a/sdk/android/native_unittests/java_types_unittest.cc
+++ b/sdk/android/native_unittests/java_types_unittest.cc
@@ -30,6 +30,47 @@
};
EXPECT_EQ(expected, output);
}
+
+TEST(JavaTypesTest, TestNativeToJavaToNativeIntArray) {
+ JNIEnv* env = AttachCurrentThreadIfNeeded();
+
+ std::vector<int32_t> test_data{1, 20, 300};
+
+ ScopedJavaLocalRef<jintArray> array = NativeToJavaIntArray(env, test_data);
+ EXPECT_EQ(test_data, JavaToNativeIntArray(env, array));
+}
+
+TEST(JavaTypesTest, TestNativeToJavaToNativeByteArray) {
+ JNIEnv* env = AttachCurrentThreadIfNeeded();
+
+ std::vector<int8_t> test_data{1, 20, 30};
+
+ ScopedJavaLocalRef<jbyteArray> array = NativeToJavaByteArray(env, test_data);
+ EXPECT_EQ(test_data, JavaToNativeByteArray(env, array));
+}
+
+TEST(JavaTypesTest, TestNativeToJavaToNativeIntArrayLeakTest) {
+ JNIEnv* env = AttachCurrentThreadIfNeeded();
+
+ std::vector<int32_t> test_data{1, 20, 300};
+
+ for (int i = 0; i < 2000; i++) {
+ ScopedJavaLocalRef<jintArray> array = NativeToJavaIntArray(env, test_data);
+ EXPECT_EQ(test_data, JavaToNativeIntArray(env, array));
+ }
+}
+
+TEST(JavaTypesTest, TestNativeToJavaToNativeByteArrayLeakTest) {
+ JNIEnv* env = AttachCurrentThreadIfNeeded();
+
+ std::vector<int8_t> test_data{1, 20, 30};
+
+ for (int i = 0; i < 2000; i++) {
+ ScopedJavaLocalRef<jbyteArray> 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 b45d177..31369a1 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::IPAddress JavaToNativeIpAddress(
JNIEnv* jni,
const JavaRef<jobject>& j_ip_address) {
- ScopedJavaLocalRef<jbyteArray> 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<int8_t> 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 4f704e2..bd3b171 100644
--- a/sdk/android/src/jni/pc/data_channel.cc
+++ b/sdk/android/src/jni/pc/data_channel.cc
@@ -146,10 +146,9 @@
const JavaParamRef<jobject>& j_dc,
const JavaParamRef<jbyteArray>& 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<int8_t> 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 5d34cf7..77c1850 100644
--- a/sdk/android/src/jni/video_encoder_wrapper.cc
+++ b/sdk/android/src/jni/video_encoder_wrapper.cc
@@ -421,17 +421,13 @@
jni, jni->NewObjectArray(kMaxSpatialLayers, int_array_class_.obj(),
nullptr /* initial */));
for (int spatial_i = 0; spatial_i < kMaxSpatialLayers; ++spatial_i) {
- ScopedJavaLocalRef<jintArray> j_array_spatial_layer(
- jni, jni->NewIntArray(kMaxTemporalStreams));
- jint* array_spatial_layer = jni->GetIntArrayElements(
- j_array_spatial_layer.obj(), nullptr /* isCopy */);
+ std::array<int32_t, kMaxTemporalStreams> 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<jintArray> j_array_spatial_layer =
+ NativeToJavaIntArray(jni, spatial_layer);
jni->SetObjectArrayElement(j_allocation_array.obj(), spatial_i,
j_array_spatial_layer.obj());
}
