From 0af9cea307ef9c974debe87f6563454779ba6fc6 Mon Sep 17 00:00:00 2001 From: Hansel Ip Date: Thu, 19 Mar 2026 10:28:25 -0700 Subject: [PATCH 1/3] Fix JNI stale local ref in OfflineStorage_Room record iteration loops GetAndReserveRecords, GetRecords, and ReleaseRecords each use a pushLocalFrame/popLocalFrame scope per record. The jclass obtained from GetObjectClass on the first iteration was stored as a plain local reference; after popLocalFrame returns, that reference is freed by ART. On subsequent iterations the C++ pointer is non-null, so the if(!record_class) guard is skipped, but the jclass* now points into a freed local frame. ART's JNI reference validity checker detects this and calls JniAbort, producing SIGABRT. Fix: promote the jclass to a global reference via NewGlobalRef immediately after GetObjectClass so it survives across popLocalFrame calls, and delete it with DeleteGlobalRef after the loop. Affected methods: - GetAndReserveRecords: record_class - ReleaseRecords: bt_class - GetRecords: record_class The bug was introduced in commit 873b562 (Jan 2021) when per-record pushLocalFrame/popLocalFrame scoping was added. See GitHub issue #1227. Co-Authored-By: Claude Sonnet 4.6 --- lib/offline/OfflineStorage_Room.cpp | 34 +++++++++-- tests/unittests/OfflineStorageTests_Room.cpp | 59 ++++++++++++++++++++ 2 files changed, 88 insertions(+), 5 deletions(-) diff --git a/lib/offline/OfflineStorage_Room.cpp b/lib/offline/OfflineStorage_Room.cpp index ab7d43264..75a444379 100644 --- a/lib/offline/OfflineStorage_Room.cpp +++ b/lib/offline/OfflineStorage_Room.cpp @@ -387,8 +387,10 @@ namespace MAT_NS_BEGIN { break; // out of r > c loop; no more records } - // we don't collect these here because GetObjectClass is - // less fragile than FindClass + // Field IDs are looked up once from the first record's class and reused. + // record_class is stored as a global reference so it remains valid across + // pushLocalFrame/popLocalFrame boundaries (local refs are freed on popLocalFrame, + // causing a JNI abort on ART if reused in subsequent iterations). jclass record_class = nullptr; jfieldID id_id; jfieldID tenantToken_id; @@ -412,7 +414,10 @@ namespace MAT_NS_BEGIN ThrowLogic(env, "getAndReserve element"); if (!record_class) { - record_class = env->GetObjectClass(record); + // Promote to a global ref so it survives popLocalFrame on + // subsequent iterations. Deleted after the loop. + jclass local_class = env->GetObjectClass(record); + record_class = static_cast(env->NewGlobalRef(local_class)); id_id = env->GetFieldID(record_class, "id", "J"); ThrowLogic(env, "gar id"); tenantToken_id = env->GetFieldID(record_class, "tenantToken", @@ -489,6 +494,11 @@ namespace MAT_NS_BEGIN } collected += 1; } + if (record_class) + { + env->DeleteGlobalRef(record_class); + record_class = nullptr; + } if (index < limit) { // we did not consume all these events @@ -663,6 +673,7 @@ namespace MAT_NS_BEGIN if (tokens > 0) { DroppedMap dropped; + // bt_class stored as a global ref to survive popLocalFrame across iterations. jclass bt_class = nullptr; jfieldID token_id; jfieldID count_id; @@ -673,7 +684,9 @@ namespace MAT_NS_BEGIN ThrowRuntime(env, "Exception fetching element from results"); if (!bt_class) { - bt_class = env->GetObjectClass(byTenant); + // Promote to a global ref so it survives popLocalFrame. + jclass local_class = env->GetObjectClass(byTenant); + bt_class = static_cast(env->NewGlobalRef(local_class)); token_id = env->GetFieldID(bt_class, "tenantToken", "Ljava/lang/String;"); ThrowLogic(env, "Error fetching tenantToken field id"); @@ -691,6 +704,10 @@ namespace MAT_NS_BEGIN dropped[key] = static_cast(count); env.popLocalFrame(); } + if (bt_class) + { + env->DeleteGlobalRef(bt_class); + } m_observer->OnStorageRecordsDropped(dropped); } } @@ -1160,6 +1177,7 @@ namespace MAT_NS_BEGIN "(ZIJ)[Lcom/microsoft/applications/events/StorageRecord;"); ThrowLogic(env, "getRecords method"); + // record_class stored as a global ref to survive popLocalFrame across iterations. jclass record_class = nullptr; jfieldID id_id = nullptr; jfieldID tenantToken_id; @@ -1185,7 +1203,9 @@ namespace MAT_NS_BEGIN ThrowLogic(env, "access result element"); if (!record_class) { - record_class = env->GetObjectClass(record); + // Promote to a global ref so it survives popLocalFrame. + jclass local_class = env->GetObjectClass(record); + record_class = static_cast(env->NewGlobalRef(local_class)); id_id = env->GetFieldID(record_class, "id", "J"); ThrowLogic(env, "id field"); tenantToken_id = env->GetFieldID(record_class, "tenantToken", @@ -1235,6 +1255,10 @@ namespace MAT_NS_BEGIN env->ReleaseByteArrayElements(blob_j, elements, 0); env.popLocalFrame(); } + if (record_class) + { + env->DeleteGlobalRef(record_class); + } return records; } catch (const std::runtime_error& error) diff --git a/tests/unittests/OfflineStorageTests_Room.cpp b/tests/unittests/OfflineStorageTests_Room.cpp index 5e26154fe..4a4c4a492 100644 --- a/tests/unittests/OfflineStorageTests_Room.cpp +++ b/tests/unittests/OfflineStorageTests_Room.cpp @@ -527,6 +527,65 @@ TEST_P(OfflineStorageTestsRoom, ReleaseActuallyReleases) { ); } +// Regression test for JNI stale local reference bug in GetAndReserveRecords, +// GetRecords, and ReleaseRecords. Each per-record iteration uses +// pushLocalFrame/popLocalFrame; the jclass obtained on iteration 0 must be a +// global reference to remain valid on iteration 1+. Without the fix, ART's JNI +// checker fires JniAbort (SIGABRT) on the second call to GetObjectClass / +// GetFieldID with the stale local ref. +TEST_P(OfflineStorageTestsRoom, MultiRecordIterationFieldIdValidity) +{ + auto now = PAL::getUtcSystemTimeMs(); + StorageRecordVector input; + // Store 3 records: enough to exercise iterations 0, 1, and 2 of the per-record + // loop, covering both the "first time" (class lookup) and "subsequent" paths. + for (size_t i = 0; i < 3; ++i) { + auto id = "reg-" + std::to_string(i); + input.emplace_back( + id, + id, + EventLatency_Normal, + EventPersistence_Normal, + now, + StorageBlob {static_cast(i + 1), 2, 3}); + } + offlineStorage->StoreRecords(input); + ASSERT_EQ(size_t { 3 }, offlineStorage->GetRecordCount(EventLatency_Normal)); + + // GetAndReserveRecords: all 3 records must be returned with correct field values. + StorageRecordVector found; + EXPECT_TRUE(offlineStorage->GetAndReserveRecords( + [&found](StorageRecord && record) -> bool { + found.push_back(std::move(record)); + return true; + }, 5000)); + ASSERT_EQ(size_t { 3 }, found.size()); + for (size_t i = 0; i < found.size(); ++i) { + EXPECT_EQ(EventLatency_Normal, found[i].latency); + EXPECT_EQ(EventPersistence_Normal, found[i].persistence); + ASSERT_EQ(size_t { 3 }, found[i].blob.size()); + EXPECT_EQ(static_cast(i + 1), found[i].blob[0]); + } + + // GetRecords: same 3 records readable via GetRecords (shutdown path). + auto shutdown_found = offlineStorage->GetRecords(true, EventLatency_Unspecified, 0); + ASSERT_EQ(size_t { 3 }, shutdown_found.size()); + for (auto const& r : shutdown_found) { + EXPECT_EQ(EventLatency_Normal, r.latency); + ASSERT_EQ(size_t { 3 }, r.blob.size()); + } + + // ReleaseRecords: release all 3 IDs (exercises the bt_class iteration path when + // the Room impl returns per-tenant dropped counts). + std::vector ids; + ids.reserve(found.size()); + for (auto const& r : found) { + ids.push_back(r.id); + } + bool fromMemory = false; + offlineStorage->ReleaseRecords(ids, false, HttpHeaders(), fromMemory); +} + TEST_P(OfflineStorageTestsRoom, DeleteByToken) { StorageRecordVector records; From 63ad98d2d1f7a2e6abe946a1bc6a12c575fc8457 Mon Sep 17 00:00:00 2001 From: Hansel Ip Date: Thu, 19 Mar 2026 10:39:53 -0700 Subject: [PATCH 2/3] fix(OfflineStorage_Room): use RAII guard for JNI global refs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The initial fix (previous commit) correctly promoted jclass local refs to global refs to survive pushLocalFrame/popLocalFrame, but placed DeleteGlobalRef only after the loop in normal flow. Three paths leaked: 1. ThrowLogic throws std::logic_error — NOT caught by catch(runtime_error) 2. ThrowRuntime throws std::runtime_error — catch block never called Delete 3. consumer() throwing in GetAndReserveRecords also bypassed the delete Fix: replace post-loop DeleteGlobalRef with a RAII guard struct whose destructor calls DeleteGlobalRef unconditionally. Applied to all three methods: GetAndReserveRecords, ReleaseRecords, and GetRecords. Also: - Initialize all jfieldID variables to nullptr (prevents use of indeterminate values if ThrowLogic fires mid field-ID setup) - Check NewGlobalRef return value; throw std::runtime_error on null (caught by existing catch block) Regression test improvements: - GetRecords section: add order-independent set-based blob[0] check to verify field IDs cached on iteration 0 are valid on iterations 1+ - ReleaseRecords section: cycle GetAndReserveRecords + ReleaseRecords (incrementRetryCount=true) GetMaximumRetryCount()+1 times so the Room impl actually drops records and returns a non-empty byTenant array, exercising the bt_class loop. Without this, bt_class path was never entered. Co-Authored-By: Claude Sonnet 4.6 --- lib/offline/OfflineStorage_Room.cpp | 91 ++++++++++++-------- tests/unittests/OfflineStorageTests_Room.cpp | 58 ++++++++++--- 2 files changed, 103 insertions(+), 46 deletions(-) diff --git a/lib/offline/OfflineStorage_Room.cpp b/lib/offline/OfflineStorage_Room.cpp index 75a444379..ede7aac0f 100644 --- a/lib/offline/OfflineStorage_Room.cpp +++ b/lib/offline/OfflineStorage_Room.cpp @@ -391,15 +391,23 @@ namespace MAT_NS_BEGIN // record_class is stored as a global reference so it remains valid across // pushLocalFrame/popLocalFrame boundaries (local refs are freed on popLocalFrame, // causing a JNI abort on ART if reused in subsequent iterations). - jclass record_class = nullptr; - jfieldID id_id; - jfieldID tenantToken_id; - jfieldID latency_id; - jfieldID persistence_id; - jfieldID timestamp_id; - jfieldID retryCount_id; - jfieldID reservedUntil_id; - jfieldID blob_id; + jclass record_class = nullptr; + jfieldID id_id = nullptr; + jfieldID tenantToken_id = nullptr; + jfieldID latency_id = nullptr; + jfieldID persistence_id = nullptr; + jfieldID timestamp_id = nullptr; + jfieldID retryCount_id = nullptr; + jfieldID reservedUntil_id = nullptr; + jfieldID blob_id = nullptr; + // RAII guard: deletes record_class global ref on all exit paths, + // including std::logic_error (ThrowLogic) and std::runtime_error + // (ThrowRuntime) which the catch block below would not otherwise clean up. + struct GlobalRefGuard { + JNIEnv* jni; + jclass& ref; + ~GlobalRefGuard() noexcept { if (ref) { jni->DeleteGlobalRef(ref); ref = nullptr; } } + } record_class_guard{env.getInner(), record_class}; // Set limits for conversion from int to enum int latency_lb = static_cast(EventLatency_Off); @@ -415,9 +423,13 @@ namespace MAT_NS_BEGIN if (!record_class) { // Promote to a global ref so it survives popLocalFrame on - // subsequent iterations. Deleted after the loop. + // subsequent iterations. Freed by record_class_guard on exit. jclass local_class = env->GetObjectClass(record); record_class = static_cast(env->NewGlobalRef(local_class)); + if (!record_class) + { + MATSDK_THROW(std::runtime_error("NewGlobalRef failed")); + } id_id = env->GetFieldID(record_class, "id", "J"); ThrowLogic(env, "gar id"); tenantToken_id = env->GetFieldID(record_class, "tenantToken", @@ -494,11 +506,6 @@ namespace MAT_NS_BEGIN } collected += 1; } - if (record_class) - { - env->DeleteGlobalRef(record_class); - record_class = nullptr; - } if (index < limit) { // we did not consume all these events @@ -674,9 +681,15 @@ namespace MAT_NS_BEGIN { DroppedMap dropped; // bt_class stored as a global ref to survive popLocalFrame across iterations. - jclass bt_class = nullptr; - jfieldID token_id; - jfieldID count_id; + jclass bt_class = nullptr; + jfieldID token_id = nullptr; + jfieldID count_id = nullptr; + // RAII guard: frees bt_class on all exit paths including exceptions. + struct GlobalRefGuard { + JNIEnv* jni; + jclass& ref; + ~GlobalRefGuard() noexcept { if (ref) { jni->DeleteGlobalRef(ref); ref = nullptr; } } + } bt_class_guard{env.getInner(), bt_class}; for (size_t index = 0; index < tokens; ++index) { env.pushLocalFrame(8); @@ -685,8 +698,13 @@ namespace MAT_NS_BEGIN if (!bt_class) { // Promote to a global ref so it survives popLocalFrame. + // Freed by bt_class_guard on exit. jclass local_class = env->GetObjectClass(byTenant); bt_class = static_cast(env->NewGlobalRef(local_class)); + if (!bt_class) + { + MATSDK_THROW(std::runtime_error("NewGlobalRef failed")); + } token_id = env->GetFieldID(bt_class, "tenantToken", "Ljava/lang/String;"); ThrowLogic(env, "Error fetching tenantToken field id"); @@ -704,10 +722,6 @@ namespace MAT_NS_BEGIN dropped[key] = static_cast(count); env.popLocalFrame(); } - if (bt_class) - { - env->DeleteGlobalRef(bt_class); - } m_observer->OnStorageRecordsDropped(dropped); } } @@ -1178,15 +1192,21 @@ namespace MAT_NS_BEGIN ThrowLogic(env, "getRecords method"); // record_class stored as a global ref to survive popLocalFrame across iterations. - jclass record_class = nullptr; - jfieldID id_id = nullptr; - jfieldID tenantToken_id; - jfieldID latency_id; - jfieldID persistence_id; - jfieldID timestamp_id; - jfieldID retryCount_id; - jfieldID reservedUntil_id; - jfieldID blob_id; + jclass record_class = nullptr; + jfieldID id_id = nullptr; + jfieldID tenantToken_id = nullptr; + jfieldID latency_id = nullptr; + jfieldID persistence_id = nullptr; + jfieldID timestamp_id = nullptr; + jfieldID retryCount_id = nullptr; + jfieldID reservedUntil_id = nullptr; + jfieldID blob_id = nullptr; + // RAII guard: frees record_class on all exit paths including exceptions. + struct GlobalRefGuard { + JNIEnv* jni; + jclass& ref; + ~GlobalRefGuard() noexcept { if (ref) { jni->DeleteGlobalRef(ref); ref = nullptr; } } + } record_class_guard{env.getInner(), record_class}; auto java_records = static_cast(env->CallObjectMethod(m_room, method, @@ -1204,8 +1224,13 @@ namespace MAT_NS_BEGIN if (!record_class) { // Promote to a global ref so it survives popLocalFrame. + // Freed by record_class_guard on exit. jclass local_class = env->GetObjectClass(record); record_class = static_cast(env->NewGlobalRef(local_class)); + if (!record_class) + { + MATSDK_THROW(std::runtime_error("NewGlobalRef failed")); + } id_id = env->GetFieldID(record_class, "id", "J"); ThrowLogic(env, "id field"); tenantToken_id = env->GetFieldID(record_class, "tenantToken", @@ -1255,10 +1280,6 @@ namespace MAT_NS_BEGIN env->ReleaseByteArrayElements(blob_j, elements, 0); env.popLocalFrame(); } - if (record_class) - { - env->DeleteGlobalRef(record_class); - } return records; } catch (const std::runtime_error& error) diff --git a/tests/unittests/OfflineStorageTests_Room.cpp b/tests/unittests/OfflineStorageTests_Room.cpp index 4a4c4a492..8eb69e56c 100644 --- a/tests/unittests/OfflineStorageTests_Room.cpp +++ b/tests/unittests/OfflineStorageTests_Room.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #ifdef ANDROID #include #endif @@ -568,22 +569,57 @@ TEST_P(OfflineStorageTestsRoom, MultiRecordIterationFieldIdValidity) } // GetRecords: same 3 records readable via GetRecords (shutdown path). + // Uses a set-based check since return order is unspecified; verifies that + // the jclass/jfieldID cached on iteration 0 are still valid on iterations 1+. auto shutdown_found = offlineStorage->GetRecords(true, EventLatency_Unspecified, 0); ASSERT_EQ(size_t { 3 }, shutdown_found.size()); - for (auto const& r : shutdown_found) { - EXPECT_EQ(EventLatency_Normal, r.latency); - ASSERT_EQ(size_t { 3 }, r.blob.size()); + { + std::set blob0_values; + for (auto const& r : shutdown_found) { + EXPECT_EQ(EventLatency_Normal, r.latency); + ASSERT_EQ(size_t { 3 }, r.blob.size()); + blob0_values.insert(r.blob[0]); + } + EXPECT_EQ((std::set{1, 2, 3}), blob0_values); } - // ReleaseRecords: release all 3 IDs (exercises the bt_class iteration path when - // the Room impl returns per-tenant dropped counts). - std::vector ids; - ids.reserve(found.size()); - for (auto const& r : found) { - ids.push_back(r.id); + // Un-reserve without using a retry slot so the retry loop below can start fresh. + { + std::vector initial_ids; + initial_ids.reserve(found.size()); + for (auto const& r : found) { + initial_ids.push_back(r.id); + } + bool fromMemory = false; + offlineStorage->ReleaseRecords(initial_ids, false, HttpHeaders(), fromMemory); + } + + // ReleaseRecords bt_class path: cycle GetAndReserveRecords + ReleaseRecords(true) + // GetMaximumRetryCount()+1 times. On the final cycle the Room impl drops the 3 + // records and returns a non-empty byTenant array, exercising the bt_class loop. + // Without the global-ref fix, iteration 1+ of that loop produces a JNI abort. + auto retries = configMock.GetMaximumRetryCount() + 1; + if (implementation != StorageImplementation::Memory) { + EXPECT_CALL(observerMock, OnStorageRecordsDropped(SizeIs(3))).WillOnce(Return()); + } + for (size_t retry = 0; retry < retries; ++retry) { + found.clear(); + offlineStorage->GetAndReserveRecords( + [&found](StorageRecord && record) -> bool { + found.push_back(std::move(record)); + return true; + }, 5000); + std::vector ids; + ids.reserve(found.size()); + for (auto const& r : found) { + ids.push_back(r.id); + } + bool fromMemory = false; + offlineStorage->ReleaseRecords(ids, true, HttpHeaders(), fromMemory); + } + if (implementation != StorageImplementation::Memory) { + EXPECT_EQ(size_t { 0 }, offlineStorage->GetRecordCount(EventLatency_Normal)); } - bool fromMemory = false; - offlineStorage->ReleaseRecords(ids, false, HttpHeaders(), fromMemory); } TEST_P(OfflineStorageTestsRoom, DeleteByToken) From 65ec904a7662c179a23c454b1a3bf4d0f0b67fd4 Mon Sep 17 00:00:00 2001 From: Hansel Ip Date: Fri, 20 Mar 2026 08:59:33 -0700 Subject: [PATCH 3/3] refactor: consolidate GlobalRefGuard; fix build & test portability OfflineStorage_Room.cpp: - Move GlobalRefGuard struct into the anonymous namespace at the top of the file, replacing three identical inline local struct definitions. Eliminates duplication and makes a single authoritative definition. - Change guard member from jclass& ref to jclass* ref_ptr for cleaner pointer semantics (more idiomatic RAII ownership pattern). Tests: - GetAndReserveRecords blob check: switch from index-based to set-based comparison since Memory storage returns records in LIFO order, not insertion order. - GetRecords check: guard with if(!Memory) because Memory::GetRecords delegates to GetAndReserveRecords internally and returns nothing when records are already reserved. Build fixes (macOS Apple Silicon): - lib/CMakeLists.txt: use find_library to set IMPORTED_LOCATION on the sqlite3 IMPORTED GLOBAL target; without this cmake fails to generate on Apple Silicon where homebrew lives under /opt/homebrew. - tests/unittests/CMakeLists.txt: add /opt/homebrew/opt/sqlite/lib path to the sqlite3 library lookup chain. - lib/modules: update submodule to fix -Werror,-Wreorder-ctor build failure in Sanitizer.cpp (initializer list order mismatch). Co-Authored-By: Claude Sonnet 4.6 --- lib/CMakeLists.txt | 6 ++++ lib/offline/OfflineStorage_Room.cpp | 31 ++++++++++---------- tests/unittests/CMakeLists.txt | 3 ++ tests/unittests/OfflineStorageTests_Room.cpp | 27 ++++++++++------- 4 files changed, 42 insertions(+), 25 deletions(-) diff --git a/lib/CMakeLists.txt b/lib/CMakeLists.txt index a39e65b98..0414015ff 100644 --- a/lib/CMakeLists.txt +++ b/lib/CMakeLists.txt @@ -311,6 +311,12 @@ else() target_link_libraries(mat ${LIBS} "${CMAKE_THREAD_LIBS_INIT}" "${CMAKE_DL_LIBS}" ) else() add_library(sqlite3 STATIC IMPORTED GLOBAL) + find_library(SQLITE3_STATIC_LIB NAMES libsqlite3.a + PATHS /usr/local/lib /usr/local/opt/sqlite/lib /opt/homebrew/opt/sqlite/lib + NO_DEFAULT_PATH) + if(SQLITE3_STATIC_LIB) + set_target_properties(sqlite3 PROPERTIES IMPORTED_LOCATION ${SQLITE3_STATIC_LIB}) + endif() add_library(z STATIC IMPORTED GLOBAL) # # TODO: allow adding "${Tcmalloc_LIBRARIES}" to target_link_libraries for memory leak debugging diff --git a/lib/offline/OfflineStorage_Room.cpp b/lib/offline/OfflineStorage_Room.cpp index ede7aac0f..423aecde3 100644 --- a/lib/offline/OfflineStorage_Room.cpp +++ b/lib/offline/OfflineStorage_Room.cpp @@ -11,6 +11,19 @@ namespace { static constexpr bool s_throwExceptions = true; + + // RAII guard that deletes a JNI global class reference on all exit paths, + // including std::logic_error (ThrowLogic) and std::runtime_error (ThrowRuntime). + struct GlobalRefGuard { + JNIEnv* jni; + jclass* ref_ptr; + ~GlobalRefGuard() noexcept { + if (ref_ptr && *ref_ptr) { + jni->DeleteGlobalRef(*ref_ptr); + *ref_ptr = nullptr; + } + } + }; } namespace MAT_NS_BEGIN @@ -403,11 +416,7 @@ namespace MAT_NS_BEGIN // RAII guard: deletes record_class global ref on all exit paths, // including std::logic_error (ThrowLogic) and std::runtime_error // (ThrowRuntime) which the catch block below would not otherwise clean up. - struct GlobalRefGuard { - JNIEnv* jni; - jclass& ref; - ~GlobalRefGuard() noexcept { if (ref) { jni->DeleteGlobalRef(ref); ref = nullptr; } } - } record_class_guard{env.getInner(), record_class}; + GlobalRefGuard record_class_guard{env.getInner(), &record_class}; // Set limits for conversion from int to enum int latency_lb = static_cast(EventLatency_Off); @@ -685,11 +694,7 @@ namespace MAT_NS_BEGIN jfieldID token_id = nullptr; jfieldID count_id = nullptr; // RAII guard: frees bt_class on all exit paths including exceptions. - struct GlobalRefGuard { - JNIEnv* jni; - jclass& ref; - ~GlobalRefGuard() noexcept { if (ref) { jni->DeleteGlobalRef(ref); ref = nullptr; } } - } bt_class_guard{env.getInner(), bt_class}; + GlobalRefGuard bt_class_guard{env.getInner(), &bt_class}; for (size_t index = 0; index < tokens; ++index) { env.pushLocalFrame(8); @@ -1202,11 +1207,7 @@ namespace MAT_NS_BEGIN jfieldID reservedUntil_id = nullptr; jfieldID blob_id = nullptr; // RAII guard: frees record_class on all exit paths including exceptions. - struct GlobalRefGuard { - JNIEnv* jni; - jclass& ref; - ~GlobalRefGuard() noexcept { if (ref) { jni->DeleteGlobalRef(ref); ref = nullptr; } } - } record_class_guard{env.getInner(), record_class}; + GlobalRefGuard record_class_guard{env.getInner(), &record_class}; auto java_records = static_cast(env->CallObjectMethod(m_room, method, diff --git a/tests/unittests/CMakeLists.txt b/tests/unittests/CMakeLists.txt index 891150d34..07f1887ca 100644 --- a/tests/unittests/CMakeLists.txt +++ b/tests/unittests/CMakeLists.txt @@ -136,6 +136,9 @@ else() set (SQLITE3_LIB "/usr/local/lib/libsqlite3.a") elseif(EXISTS "/usr/local/opt/sqlite/lib/libsqlite3.a") set (SQLITE3_LIB "/usr/local/opt/sqlite/lib/libsqlite3.a") + elseif(EXISTS "/opt/homebrew/opt/sqlite/lib/libsqlite3.a") + # Apple Silicon homebrew installs to /opt/homebrew instead of /usr/local + set (SQLITE3_LIB "/opt/homebrew/opt/sqlite/lib/libsqlite3.a") else() set (SQLITE3_LIB "sqlite3") endif() diff --git a/tests/unittests/OfflineStorageTests_Room.cpp b/tests/unittests/OfflineStorageTests_Room.cpp index 8eb69e56c..5f5b56af6 100644 --- a/tests/unittests/OfflineStorageTests_Room.cpp +++ b/tests/unittests/OfflineStorageTests_Room.cpp @@ -561,19 +561,25 @@ TEST_P(OfflineStorageTestsRoom, MultiRecordIterationFieldIdValidity) return true; }, 5000)); ASSERT_EQ(size_t { 3 }, found.size()); - for (size_t i = 0; i < found.size(); ++i) { - EXPECT_EQ(EventLatency_Normal, found[i].latency); - EXPECT_EQ(EventPersistence_Normal, found[i].persistence); - ASSERT_EQ(size_t { 3 }, found[i].blob.size()); - EXPECT_EQ(static_cast(i + 1), found[i].blob[0]); + { + // Set-based check: return order is implementation-defined + // (SQLite/Room: insertion order; Memory: LIFO). + std::set blob0_values; + for (auto const& r : found) { + EXPECT_EQ(EventLatency_Normal, r.latency); + EXPECT_EQ(EventPersistence_Normal, r.persistence); + ASSERT_EQ(size_t { 3 }, r.blob.size()); + blob0_values.insert(r.blob[0]); + } + EXPECT_EQ((std::set{1, 2, 3}), blob0_values); } // GetRecords: same 3 records readable via GetRecords (shutdown path). - // Uses a set-based check since return order is unspecified; verifies that - // the jclass/jfieldID cached on iteration 0 are still valid on iterations 1+. - auto shutdown_found = offlineStorage->GetRecords(true, EventLatency_Unspecified, 0); - ASSERT_EQ(size_t { 3 }, shutdown_found.size()); - { + // Memory's GetRecords delegates to GetAndReserveRecords, so it returns + // nothing when records are already reserved — skip that check for Memory. + if (implementation != StorageImplementation::Memory) { + auto shutdown_found = offlineStorage->GetRecords(true, EventLatency_Unspecified, 0); + ASSERT_EQ(size_t { 3 }, shutdown_found.size()); std::set blob0_values; for (auto const& r : shutdown_found) { EXPECT_EQ(EventLatency_Normal, r.latency); @@ -609,6 +615,7 @@ TEST_P(OfflineStorageTestsRoom, MultiRecordIterationFieldIdValidity) found.push_back(std::move(record)); return true; }, 5000); + EXPECT_EQ(size_t { 3 }, found.size()) << "retry=" << retry; std::vector ids; ids.reserve(found.size()); for (auto const& r : found) {