Skip to content

Commit cac13ad

Browse files
bmehta001Copilot
bmehta001
and
Copilot
authored
Prevent Android JNI exhaustion and restore SQLite selection (#1555)
Keep local JNI references scoped to every send, including failures, and release task globals before terminal callbacks so retained native requests do not keep completed futures alive. Honor explicit AAR storage options and translate the legacy USE_ROOM flag. Add ART/CheckJNI GC regressions and AAR storage configuration checks to prevent recurrence. Files changed: .github/workflows/build-android.yml cmake/MatsdkOptions.cmake lib/android_build/maesdk/src/main/cpp/CMakeLists.txt lib/http/HttpClient_Android.cpp lib/http/HttpClient_Android.hpp tests/android-http-client/CMakeLists.txt tests/android-http-client/HttpClient.java tests/android-http-client/android-http-client-tests.cpp tests/android-http-client/run-tests.py tests/android-storage/CMakeLists.txt tests/android-storage/options-tests.cmake Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 01fc96f0-9072-4225-903a-5402ae46b7ae
1 parent bc49ccc commit cac13ad

11 files changed

Lines changed: 561 additions & 28 deletions

File tree

‎.github/workflows/build-android.yml‎

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,36 @@ concurrency:
3131
cancel-in-progress: ${{ github.event_name == 'pull_request' }}
3232

3333
jobs:
34+
jni-lifetimes:
35+
name: Android HTTP JNI lifetime regressions
36+
runs-on: ubuntu-latest
37+
steps:
38+
- name: Checkout
39+
uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0
40+
- name: Setup Java
41+
uses: actions/setup-java@b6effb05e454b25005698d916606bdc6ffcbf961 # v5.7.0
42+
with:
43+
distribution: 'temurin'
44+
java-version: '17'
45+
- name: Build production client and regression harness
46+
run: >-
47+
cmake -S tests/android-http-client -B out-http-jni -G Ninja
48+
-DCMAKE_TOOLCHAIN_FILE="$ANDROID_NDK_HOME/build/cmake/android.toolchain.cmake"
49+
-DANDROID_ABI=x86_64 -DANDROID_PLATFORM=android-30
50+
-DANDROID_STL=c++_static -DCMAKE_BUILD_TYPE=Debug
51+
- name: Compile JNI tests
52+
run: cmake --build out-http-jni --target android-http-client-tests --parallel 2
53+
- name: Enable KVM
54+
run: sudo chmod a+rw /dev/kvm
55+
- name: Run ART reference lifetime tests with CheckJNI
56+
uses: reactivecircus/android-emulator-runner@324029e2f414c084d8b15ba075288885e74aef9c # v2.34.0
57+
with:
58+
api-level: 30
59+
arch: x86_64
60+
target: google_apis
61+
disable-animations: true
62+
script: python3 tests/android-http-client/run-tests.py --sdk "$ANDROID_HOME" --library out-http-jni/libandroid-http-client-tests.so
63+
3464
no-exceptions:
3565
name: Android without C++ exceptions (${{ matrix.abi }}, Room=${{ matrix.room }})
3666
runs-on: ubuntu-latest
@@ -44,7 +74,8 @@ jobs:
4474
uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0
4575
- name: Configure full JNI SDK
4676
run: >-
47-
cmake -S . -B out-no-exceptions -G Ninja
77+
cmake -S tests/android-storage -B out-no-exceptions -G Ninja
78+
-DEXPECT_ROOM=${{ matrix.room }}
4879
-DCMAKE_TOOLCHAIN_FILE="$ANDROID_NDK_HOME/build/cmake/android.toolchain.cmake"
4980
-DANDROID_ABI=${{ matrix.abi }}
5081
-DANDROID_PLATFORM=android-24
@@ -59,6 +90,8 @@ jobs:
5990
-DMATSDK_BUILD_PACKAGE=OFF
6091
- name: Compile full SDK
6192
run: cmake --build out-no-exceptions --target mat --parallel 2
93+
- name: Verify storage option compatibility
94+
run: cmake -P tests/android-storage/options-tests.cmake
6295

6396
build:
6497
runs-on: windows-latest

‎cmake/MatsdkOptions.cmake‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,15 @@ option(MATSDK_BUILD_FUNC_TESTS
1818
"Build functional tests" "${MATSDK_PROJECT_IS_TOP_LEVEL}")
1919
option(MATSDK_BUILD_JNI_WRAPPER
2020
"Build the JNI wrapper" OFF)
21+
if(DEFINED USE_ROOM)
22+
if(NOT DEFINED MATSDK_ANDROID_USE_ROOM)
23+
set(MATSDK_ANDROID_USE_ROOM "${USE_ROOM}" CACHE BOOL
24+
"Use Android Room for offline storage")
25+
endif()
26+
message(DEPRECATION
27+
"USE_ROOM is deprecated; use MATSDK_ANDROID_USE_ROOM instead. "
28+
"MATSDK_ANDROID_USE_ROOM=${MATSDK_ANDROID_USE_ROOM} takes precedence.")
29+
endif()
2130
option(MATSDK_ANDROID_USE_ROOM
2231
"Use Android Room for offline storage" OFF)
2332
option(MATSDK_ENABLE_CAPI_HTTP_CLIENT

‎lib/android_build/maesdk/src/main/cpp/CMakeLists.txt‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,9 @@ get_filename_component(SDK_ROOT
66

77
set(BUILD_SHARED_LIBS ON CACHE BOOL "")
88
set(MATSDK_BUILD_JNI_WRAPPER ON CACHE BOOL "" FORCE)
9-
set(MATSDK_ANDROID_USE_ROOM ON CACHE BOOL "" FORCE)
9+
if(NOT DEFINED MATSDK_ANDROID_USE_ROOM AND NOT DEFINED USE_ROOM)
10+
set(MATSDK_ANDROID_USE_ROOM ON CACHE BOOL "Use Android Room for offline storage")
11+
endif()
1012
set(MATSDK_BUILD_PACKAGE OFF CACHE BOOL "" FORCE)
1113
set(MATSDK_BUILD_UNIT_TESTS OFF CACHE BOOL "" FORCE)
1214
set(MATSDK_BUILD_FUNC_TESTS OFF CACHE BOOL "" FORCE)

‎lib/http/HttpClient_Android.cpp‎

Lines changed: 90 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,35 @@
1010
#include <sstream>
1111
#include <vector>
1212

13+
namespace
14+
{
15+
class LocalFrame
16+
{
17+
public:
18+
explicit LocalFrame(JNIEnv* env) :
19+
m_env(env), m_pushed(env->PushLocalFrame(16) == JNI_OK)
20+
{
21+
}
22+
23+
~LocalFrame()
24+
{
25+
if (m_pushed)
26+
{
27+
m_env->PopLocalFrame(nullptr);
28+
}
29+
}
30+
31+
bool IsValid() const
32+
{
33+
return m_pushed;
34+
}
35+
36+
private:
37+
JNIEnv* m_env;
38+
bool m_pushed;
39+
};
40+
}
41+
1342
namespace MAT_NS_BEGIN
1443
{
1544
HttpClient_Android::HttpRequest::~HttpRequest() noexcept
@@ -22,7 +51,16 @@ namespace MAT_NS_BEGIN
2251
{
2352
return;
2453
}
54+
ReleaseJavaRequest(env);
55+
}
56+
}
57+
58+
void HttpClient_Android::HttpRequest::ReleaseJavaRequest(JNIEnv* env)
59+
{
60+
if (m_java_request)
61+
{
2562
env->DeleteGlobalRef(m_java_request);
63+
m_java_request = nullptr;
2664
}
2765
}
2866

@@ -88,7 +126,7 @@ namespace MAT_NS_BEGIN
88126
IHttpResponseCallback* callback)
89127
{
90128
JNIEnv* env = nullptr;
91-
std::string const& target_id(request->GetId());
129+
std::string const target_id(request->GetId());
92130

93131
if (s_java_vm->AttachCurrentThread(&env, nullptr) != JNI_OK)
94132
{
@@ -134,14 +172,22 @@ namespace MAT_NS_BEGIN
134172
total_size += u.first.length() + u.second.length();
135173
}
136174

137-
static constexpr int FrameSize = 16; // at most 16 local refs in the frame
138-
auto pushed = env->PushLocalFrame(FrameSize);
139-
if (CheckException(env, r))
175+
LocalFrame frame(env);
176+
if (CheckException(env, target_id))
140177
{
141178
return;
142179
}
180+
if (!frame.IsValid())
181+
{
182+
auto failed = GetAndRemoveRequest(target_id);
183+
if (failed)
184+
{
185+
CallbackForCancel(env, failed);
186+
}
187+
return;
188+
}
143189
auto header_buffer = env->NewByteArray(static_cast<jsize>(total_size));
144-
if (CheckException(env, r))
190+
if (CheckException(env, target_id))
145191
{
146192
return;
147193
}
@@ -156,7 +202,7 @@ namespace MAT_NS_BEGIN
156202
{
157203
env->SetByteArrayRegion(header_buffer, offset, key_len,
158204
reinterpret_cast<jbyte const*>(u.first.data()));
159-
if (CheckException(env, r))
205+
if (CheckException(env, target_id))
160206
{
161207
return;
162208
}
@@ -168,47 +214,47 @@ namespace MAT_NS_BEGIN
168214
{
169215
env->SetByteArrayRegion(header_buffer, offset, value_len,
170216
reinterpret_cast<jbyte const*>(u.second.data()));
171-
if (CheckException(env, r))
217+
if (CheckException(env, target_id))
172218
{
173219
return;
174220
}
175221
}
176222
offset += value_len;
177223
}
178224
auto header_lengths = env->NewIntArray(static_cast<jsize>(index_buffer.size()));
179-
if (CheckException(env, r))
225+
if (CheckException(env, target_id))
180226
{
181227
return;
182228
}
183229
env->SetIntArrayRegion(header_lengths, 0, index_buffer.size(), index_buffer.data());
184-
if (CheckException(env, r))
230+
if (CheckException(env, target_id))
185231
{
186232
return;
187233
}
188234

189235
auto body = env->NewByteArray(r->m_body.size());
190-
if (CheckException(env, r))
236+
if (CheckException(env, target_id))
191237
{
192238
return;
193239
}
194240
env->SetByteArrayRegion(body, 0, r->m_body.size(),
195241
reinterpret_cast<const jbyte*>(r->m_body.data()));
196-
if (CheckException(env, r))
242+
if (CheckException(env, target_id))
197243
{
198244
return;
199245
}
200246
auto java_id = env->NewStringUTF(request->GetId().c_str());
201-
if (CheckException(env, r))
247+
if (CheckException(env, target_id))
202248
{
203249
return;
204250
}
205251
auto url_id = env->NewStringUTF(r->m_url.c_str());
206-
if (CheckException(env, r))
252+
if (CheckException(env, target_id))
207253
{
208254
return;
209255
}
210256
auto method_id = env->NewStringUTF(r->m_method.c_str());
211-
if (CheckException(env, r))
257+
if (CheckException(env, target_id))
212258
{
213259
return;
214260
}
@@ -231,10 +277,9 @@ namespace MAT_NS_BEGIN
231277
header_lengths,
232278
header_buffer);
233279
}
234-
if (pushed == JNI_OK)
280+
if (CheckException(env, target_id))
235281
{
236-
// pass the return value "result" through the local frame pop
237-
result = env->PopLocalFrame(result);
282+
return;
238283
}
239284

240285
HttpRequest* cancel_me = nullptr;
@@ -263,13 +308,18 @@ namespace MAT_NS_BEGIN
263308
}
264309
}
265310
}
311+
if (CheckException(env, target_id))
312+
{
313+
return;
314+
}
266315
if (cancel_me)
267316
{
268317
CallbackForCancel(env, cancel_me);
269318
}
270319
else
271320
{
272321
env->CallVoidMethod(m_client, m_execute_id, result);
322+
CheckException(env, target_id);
273323
}
274324
}
275325

@@ -278,8 +328,18 @@ namespace MAT_NS_BEGIN
278328
if (request->m_java_request && env)
279329
{
280330
auto request_class = env->GetObjectClass(request->m_java_request);
281-
auto cancel_method = env->GetMethodID(request_class, "cancel", "(Z)Z");
282-
env->CallBooleanMethod(request->m_java_request, cancel_method, static_cast<jboolean>(JNI_TRUE));
331+
auto cancel_method = request_class ? env->GetMethodID(request_class, "cancel", "(Z)Z") : nullptr;
332+
if (cancel_method)
333+
{
334+
env->CallBooleanMethod(request->m_java_request, cancel_method, static_cast<jboolean>(JNI_TRUE));
335+
}
336+
env->DeleteLocalRef(request_class);
337+
request->ReleaseJavaRequest(env);
338+
if (env->ExceptionCheck())
339+
{
340+
env->ExceptionDescribe();
341+
env->ExceptionClear();
342+
}
283343
}
284344
if (request->m_callback)
285345
{
@@ -379,11 +439,12 @@ namespace MAT_NS_BEGIN
379439
env->DeleteGlobalRef(m_client);
380440
}
381441
m_client = env->NewGlobalRef(client_);
382-
m_client_class = env->GetObjectClass(m_client);
383-
m_create_id = env->GetMethodID(m_client_class, "createTask",
442+
auto client_class = env->GetObjectClass(m_client);
443+
m_create_id = env->GetMethodID(client_class, "createTask",
384444
"(Ljava/lang/String;Ljava/lang/String;[BLjava/lang/String;[I[B)Ljava/util/concurrent/FutureTask;");
385-
m_execute_id = env->GetMethodID(m_client_class, "executeTask",
445+
m_execute_id = env->GetMethodID(client_class, "executeTask",
386446
"(Ljava/util/concurrent/FutureTask;)V");
447+
env->DeleteLocalRef(client_class);
387448

388449
env->GetJavaVM(&s_java_vm);
389450
}
@@ -485,15 +546,19 @@ namespace MAT_NS_BEGIN
485546
return s_client;
486547
}
487548

488-
bool HttpClient_Android::CheckException(JNIEnv* env, HttpRequest* request)
549+
bool HttpClient_Android::CheckException(JNIEnv* env, std::string const& id)
489550
{
490551
if (env->ExceptionCheck() == JNI_FALSE)
491552
{
492553
return false;
493554
}
494555
env->ExceptionDescribe();
495556
env->ExceptionClear();
496-
CallbackForCancel(env, request);
557+
auto request = GetAndRemoveRequest(id);
558+
if (request)
559+
{
560+
CallbackForCancel(env, request);
561+
}
497562
return true;
498563
}
499564

@@ -559,6 +624,7 @@ extern "C" JNIEXPORT void
559624
{
560625
return;
561626
}
627+
request->ReleaseJavaRequest(env);
562628
auto callback = request->GetCallback();
563629
auto response = new Microsoft::Applications::Events::HttpClient_Android::HttpResponse(request->GetId());
564630
response->SetResponse(statusCode);

‎lib/http/HttpClient_Android.hpp‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -159,6 +159,8 @@ namespace MAT_NS_BEGIN
159159
m_parent.EraseRequest(this);
160160
}
161161

162+
void ReleaseJavaRequest(JNIEnv* env);
163+
162164
protected:
163165
HttpClient_Android& m_parent;
164166
HttpHeaders m_headers;
@@ -199,7 +201,6 @@ namespace MAT_NS_BEGIN
199201
std::vector<HttpRequest*> m_requests;
200202

201203
jobject m_client = nullptr;
202-
jclass m_client_class = nullptr;
203204
jmethodID m_send_id = nullptr;
204205
jmethodID m_create_id = nullptr;
205206
jmethodID m_execute_id = nullptr;
@@ -209,7 +210,7 @@ namespace MAT_NS_BEGIN
209210
static std::shared_ptr<HttpClient_Android> s_client;
210211
static std::string s_cache_file_path;
211212

212-
bool CheckException(JNIEnv* env, HttpRequest* request);
213+
bool CheckException(JNIEnv* env, std::string const& id);
213214
};
214215

215216
}

‎tests/android-http-client/CMakeLists.txt‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
cmake_minimum_required(VERSION 3.19...3.31)
2+
project(AndroidHttpClientTests LANGUAGES C CXX)
3+
4+
if(NOT ANDROID)
5+
message(FATAL_ERROR "These JNI lifetime tests must run on Android ART.")
6+
endif()
7+
8+
set(BUILD_SHARED_LIBS OFF CACHE BOOL "" FORCE)
9+
set(MATSDK_BUILD_JNI_WRAPPER OFF CACHE BOOL "" FORCE)
10+
set(MATSDK_BUILD_UNIT_TESTS OFF CACHE BOOL "" FORCE)
11+
set(MATSDK_BUILD_FUNC_TESTS OFF CACHE BOOL "" FORCE)
12+
set(MATSDK_BUILD_TEST_TOOL OFF CACHE BOOL "" FORCE)
13+
set(MATSDK_BUILD_PACKAGE OFF CACHE BOOL "" FORCE)
14+
add_subdirectory(../.. sdk)
15+
16+
add_library(android-http-client-tests SHARED android-http-client-tests.cpp)
17+
target_compile_features(android-http-client-tests PRIVATE cxx_std_14)
18+
target_include_directories(android-http-client-tests PRIVATE
19+
../../lib
20+
../../lib/include
21+
../../lib/include/mat
22+
../../lib/pal
23+
../../lib/utils)
24+
target_link_libraries(android-http-client-tests PRIVATE
25+
mat matsdk_internal_config matsdk_build_options)

0 commit comments

Comments
 (0)