Fix Android HTTP JNI leaks and honor SQLite storage selection

.github/workflows/build-android.yml

@@ -31,6 +31,36 @@ concurrency:
   cancel-in-progress: ${{ github.event_name == 'pull_request' }}
 
 jobs:
+  jni-lifetimes:
+    name: Android HTTP JNI lifetime regressions
+    runs-on: ubuntu-latest
+    steps:
+    - name: Checkout
+      uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0
+    - name: Setup Java
+      uses: actions/setup-java@b6effb05e454b25005698d916606bdc6ffcbf961 # v5.7.0
+      with:
+        distribution: 'temurin'
+        java-version: '17'
+    - name: Build production client and regression harness
+      run: >-
+        cmake -S tests/android-http-client -B out-http-jni -G Ninja
+        -DCMAKE_TOOLCHAIN_FILE="$ANDROID_NDK_HOME/build/cmake/android.toolchain.cmake"
+        -DANDROID_ABI=x86_64 -DANDROID_PLATFORM=android-30
+        -DANDROID_STL=c++_static -DCMAKE_BUILD_TYPE=Debug
+    - name: Compile JNI tests
+      run: cmake --build out-http-jni --target android-http-client-tests --parallel 2
+    - name: Enable KVM
+      run: sudo chmod a+rw /dev/kvm
+    - name: Run ART reference lifetime tests with CheckJNI
+      uses: reactivecircus/android-emulator-runner@324029e2f414c084d8b15ba075288885e74aef9c # v2.34.0
+      with:
+        api-level: 30
+        arch: x86_64
+        target: google_apis
+        disable-animations: true
+        script: python3 tests/android-http-client/run-tests.py --sdk "$ANDROID_HOME" --library out-http-jni/libandroid-http-client-tests.so
+
   no-exceptions:
     name: Android without C++ exceptions (${{ matrix.abi }}, Room=${{ matrix.room }})
     runs-on: ubuntu-latest
@@ -44,7 +74,8 @@ jobs:
       uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0
     - name: Configure full JNI SDK
       run: >-
-        cmake -S . -B out-no-exceptions -G Ninja
+        cmake -S tests/android-storage -B out-no-exceptions -G Ninja
+        -DEXPECT_ROOM=${{ matrix.room }}
         -DCMAKE_TOOLCHAIN_FILE="$ANDROID_NDK_HOME/build/cmake/android.toolchain.cmake"
         -DANDROID_ABI=${{ matrix.abi }}
         -DANDROID_PLATFORM=android-24
@@ -59,6 +90,8 @@ jobs:
         -DMATSDK_BUILD_PACKAGE=OFF
     - name: Compile full SDK
       run: cmake --build out-no-exceptions --target mat --parallel 2
+    - name: Verify storage option compatibility
+      run: cmake -P tests/android-storage/options-tests.cmake
 
   build:
     runs-on: windows-latest

cmake/MatsdkOptions.cmake

@@ -18,6 +18,15 @@ option(MATSDK_BUILD_FUNC_TESTS
   "Build functional tests" "${MATSDK_PROJECT_IS_TOP_LEVEL}")
 option(MATSDK_BUILD_JNI_WRAPPER
   "Build the JNI wrapper" OFF)
+if(DEFINED USE_ROOM)
+  if(NOT DEFINED MATSDK_ANDROID_USE_ROOM)
+    set(MATSDK_ANDROID_USE_ROOM "${USE_ROOM}" CACHE BOOL
+      "Use Android Room for offline storage")
+  endif()
+  message(DEPRECATION
+    "USE_ROOM is deprecated; use MATSDK_ANDROID_USE_ROOM instead. "
+    "MATSDK_ANDROID_USE_ROOM=${MATSDK_ANDROID_USE_ROOM} takes precedence.")
+endif()
 option(MATSDK_ANDROID_USE_ROOM
   "Use Android Room for offline storage" OFF)
 option(MATSDK_ENABLE_CAPI_HTTP_CLIENT

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

@@ -6,7 +6,9 @@ get_filename_component(SDK_ROOT
 
 set(BUILD_SHARED_LIBS ON CACHE BOOL "")
 set(MATSDK_BUILD_JNI_WRAPPER ON CACHE BOOL "" FORCE)
-set(MATSDK_ANDROID_USE_ROOM ON CACHE BOOL "" FORCE)
+if(NOT DEFINED MATSDK_ANDROID_USE_ROOM AND NOT DEFINED USE_ROOM)
+  set(MATSDK_ANDROID_USE_ROOM ON CACHE BOOL "Use Android Room for offline storage")
+endif()
 set(MATSDK_BUILD_PACKAGE OFF CACHE BOOL "" FORCE)
 set(MATSDK_BUILD_UNIT_TESTS OFF CACHE BOOL "" FORCE)
 set(MATSDK_BUILD_FUNC_TESTS OFF CACHE BOOL "" FORCE)

lib/http/HttpClient_Android.cpp

@@ -10,6 +10,35 @@
 #include <sstream>
 #include <vector>
 
+namespace
+{
+    class LocalFrame
+    {
+       public:
+        explicit LocalFrame(JNIEnv* env) :
+            m_env(env), m_pushed(env->PushLocalFrame(16) == JNI_OK)
+        {
+        }
+
+        ~LocalFrame()
+        {
+            if (m_pushed)
+            {
+                m_env->PopLocalFrame(nullptr);
+            }
+        }
+
+        bool IsValid() const
+        {
+            return m_pushed;
+        }
+
+       private:
+        JNIEnv* m_env;
+        bool m_pushed;
+    };
+}
+
 namespace MAT_NS_BEGIN
 {
     HttpClient_Android::HttpRequest::~HttpRequest() noexcept
@@ -22,7 +51,16 @@ namespace MAT_NS_BEGIN
             {
                 return;
             }
+            ReleaseJavaRequest(env);
+        }
+    }
+
+    void HttpClient_Android::HttpRequest::ReleaseJavaRequest(JNIEnv* env)
+    {
+        if (m_java_request)
+        {
             env->DeleteGlobalRef(m_java_request);
+            m_java_request = nullptr;
         }
     }
 
@@ -88,7 +126,7 @@ namespace MAT_NS_BEGIN
                                               IHttpResponseCallback* callback)
     {
         JNIEnv* env = nullptr;
-        std::string const& target_id(request->GetId());
+        std::string const target_id(request->GetId());
 
         if (s_java_vm->AttachCurrentThread(&env, nullptr) != JNI_OK)
         {
@@ -134,14 +172,22 @@ namespace MAT_NS_BEGIN
             total_size += u.first.length() + u.second.length();
         }
 
-        static constexpr int FrameSize = 16;  // at most 16 local refs in the frame
-        auto pushed = env->PushLocalFrame(FrameSize);
-        if (CheckException(env, r))
+        LocalFrame frame(env);
+        if (CheckException(env, target_id))
         {
             return;
         }
+        if (!frame.IsValid())
+        {
+            auto failed = GetAndRemoveRequest(target_id);
+            if (failed)
+            {
+                CallbackForCancel(env, failed);
+            }
+            return;
+        }
         auto header_buffer = env->NewByteArray(static_cast<jsize>(total_size));
-        if (CheckException(env, r))
+        if (CheckException(env, target_id))
         {
             return;
         }
@@ -156,7 +202,7 @@ namespace MAT_NS_BEGIN
             {
                 env->SetByteArrayRegion(header_buffer, offset, key_len,
                                         reinterpret_cast<jbyte const*>(u.first.data()));
-                if (CheckException(env, r))
+                if (CheckException(env, target_id))
                 {
                     return;
                 }
@@ -168,47 +214,47 @@ namespace MAT_NS_BEGIN
             {
                 env->SetByteArrayRegion(header_buffer, offset, value_len,
                                         reinterpret_cast<jbyte const*>(u.second.data()));
-                if (CheckException(env, r))
+                if (CheckException(env, target_id))
                 {
                     return;
                 }
             }
             offset += value_len;
         }
         auto header_lengths = env->NewIntArray(static_cast<jsize>(index_buffer.size()));
-        if (CheckException(env, r))
+        if (CheckException(env, target_id))
         {
             return;
         }
         env->SetIntArrayRegion(header_lengths, 0, index_buffer.size(), index_buffer.data());
-        if (CheckException(env, r))
+        if (CheckException(env, target_id))
         {
             return;
         }
 
         auto body = env->NewByteArray(r->m_body.size());
-        if (CheckException(env, r))
+        if (CheckException(env, target_id))
         {
             return;
         }
         env->SetByteArrayRegion(body, 0, r->m_body.size(),
                                 reinterpret_cast<const jbyte*>(r->m_body.data()));
-        if (CheckException(env, r))
+        if (CheckException(env, target_id))
         {
             return;
         }
         auto java_id = env->NewStringUTF(request->GetId().c_str());
-        if (CheckException(env, r))
+        if (CheckException(env, target_id))
         {
             return;
         }
         auto url_id = env->NewStringUTF(r->m_url.c_str());
-        if (CheckException(env, r))
+        if (CheckException(env, target_id))
         {
             return;
         }
         auto method_id = env->NewStringUTF(r->m_method.c_str());
-        if (CheckException(env, r))
+        if (CheckException(env, target_id))
         {
             return;
         }
@@ -231,10 +277,9 @@ namespace MAT_NS_BEGIN
                 header_lengths,
                 header_buffer);
         }
-        if (pushed == JNI_OK)
+        if (CheckException(env, target_id))
         {
-            // pass the return value "result" through the local frame pop
-            result = env->PopLocalFrame(result);
+            return;
         }
 
         HttpRequest* cancel_me = nullptr;
@@ -263,13 +308,18 @@ namespace MAT_NS_BEGIN
                 }
             }
         }
+        if (CheckException(env, target_id))
+        {
+            return;
+        }
         if (cancel_me)
         {
             CallbackForCancel(env, cancel_me);
         }
         else
         {
             env->CallVoidMethod(m_client, m_execute_id, result);
+            CheckException(env, target_id);
         }
     }
 
@@ -278,8 +328,18 @@ namespace MAT_NS_BEGIN
         if (request->m_java_request && env)
         {
             auto request_class = env->GetObjectClass(request->m_java_request);
-            auto cancel_method = env->GetMethodID(request_class, "cancel", "(Z)Z");
-            env->CallBooleanMethod(request->m_java_request, cancel_method, static_cast<jboolean>(JNI_TRUE));
+            auto cancel_method = request_class ? env->GetMethodID(request_class, "cancel", "(Z)Z") : nullptr;
+            if (cancel_method)
+            {
+                env->CallBooleanMethod(request->m_java_request, cancel_method, static_cast<jboolean>(JNI_TRUE));
+            }
+            env->DeleteLocalRef(request_class);
+            request->ReleaseJavaRequest(env);
+            if (env->ExceptionCheck())
+            {
+                env->ExceptionDescribe();
+                env->ExceptionClear();
+            }
         }
         if (request->m_callback)
         {
@@ -379,11 +439,12 @@ namespace MAT_NS_BEGIN
             env->DeleteGlobalRef(m_client);
         }
         m_client = env->NewGlobalRef(client_);
-        m_client_class = env->GetObjectClass(m_client);
-        m_create_id = env->GetMethodID(m_client_class, "createTask",
+        auto client_class = env->GetObjectClass(m_client);
+        m_create_id = env->GetMethodID(client_class, "createTask",
                                        "(Ljava/lang/String;Ljava/lang/String;[BLjava/lang/String;[I[B)Ljava/util/concurrent/FutureTask;");
-        m_execute_id = env->GetMethodID(m_client_class, "executeTask",
+        m_execute_id = env->GetMethodID(client_class, "executeTask",
                                         "(Ljava/util/concurrent/FutureTask;)V");
+        env->DeleteLocalRef(client_class);
 
         env->GetJavaVM(&s_java_vm);
     }
@@ -485,15 +546,19 @@ namespace MAT_NS_BEGIN
         return s_client;
     }
 
-    bool HttpClient_Android::CheckException(JNIEnv* env, HttpRequest* request)
+    bool HttpClient_Android::CheckException(JNIEnv* env, std::string const& id)
     {
         if (env->ExceptionCheck() == JNI_FALSE)
         {
             return false;
         }
         env->ExceptionDescribe();
         env->ExceptionClear();
-        CallbackForCancel(env, request);
+        auto request = GetAndRemoveRequest(id);
+        if (request)
+        {
+            CallbackForCancel(env, request);
+        }
         return true;
     }
 
@@ -559,6 +624,7 @@ extern "C" JNIEXPORT void
     {
         return;
     }
+    request->ReleaseJavaRequest(env);
     auto callback = request->GetCallback();
     auto response = new Microsoft::Applications::Events::HttpClient_Android::HttpResponse(request->GetId());
     response->SetResponse(statusCode);

lib/http/HttpClient_Android.hpp

@@ -159,6 +159,8 @@ namespace MAT_NS_BEGIN
                 m_parent.EraseRequest(this);
             }
 
+            void ReleaseJavaRequest(JNIEnv* env);
+
            protected:
             HttpClient_Android& m_parent;
             HttpHeaders m_headers;
@@ -199,7 +201,6 @@ namespace MAT_NS_BEGIN
         std::vector<HttpRequest*> m_requests;
 
         jobject m_client = nullptr;
-        jclass m_client_class = nullptr;
         jmethodID m_send_id = nullptr;
         jmethodID m_create_id = nullptr;
         jmethodID m_execute_id = nullptr;
@@ -209,7 +210,7 @@ namespace MAT_NS_BEGIN
         static std::shared_ptr<HttpClient_Android> s_client;
         static std::string s_cache_file_path;
 
-        bool CheckException(JNIEnv* env, HttpRequest* request);
+        bool CheckException(JNIEnv* env, std::string const& id);
     };
 
 }

tests/android-http-client/CMakeLists.txt

@@ -0,0 +1,25 @@
+cmake_minimum_required(VERSION 3.19...3.31)
+project(AndroidHttpClientTests LANGUAGES C CXX)
+
+if(NOT ANDROID)
+  message(FATAL_ERROR "These JNI lifetime tests must run on Android ART.")
+endif()
+
+set(BUILD_SHARED_LIBS OFF CACHE BOOL "" FORCE)
+set(MATSDK_BUILD_JNI_WRAPPER OFF CACHE BOOL "" FORCE)
+set(MATSDK_BUILD_UNIT_TESTS OFF CACHE BOOL "" FORCE)
+set(MATSDK_BUILD_FUNC_TESTS OFF CACHE BOOL "" FORCE)
+set(MATSDK_BUILD_TEST_TOOL OFF CACHE BOOL "" FORCE)
+set(MATSDK_BUILD_PACKAGE OFF CACHE BOOL "" FORCE)
+add_subdirectory(../.. sdk)
+
+add_library(android-http-client-tests SHARED android-http-client-tests.cpp)
+target_compile_features(android-http-client-tests PRIVATE cxx_std_14)
+target_include_directories(android-http-client-tests PRIVATE
+  ../../lib
+  ../../lib/include
+  ../../lib/include/mat
+  ../../lib/pal
+  ../../lib/utils)
+target_link_libraries(android-http-client-tests PRIVATE
+  mat matsdk_internal_config matsdk_build_options)