From 866832bb378d36cff1c4a0351b6b237963d75335 Mon Sep 17 00:00:00 2001 From: kdhawaniya Date: Sat, 12 Sep 2026 02:33:21 +0530 Subject: [PATCH 1/8] [Android] Expose IDataViewer as public Java API Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/CMakeLists.txt | 1 + .../maesdktest/LogManagerDDVUnitTest.java | 91 ++++++ lib/android_build/maesdk/consumer-rules.pro | 4 + .../applications/events/IDataViewer.java | 30 ++ .../applications/events/ILogManager.java | 15 + .../events/LogManagerProvider.java | 22 ++ lib/jni/JavaDataViewerProxy.cpp | 261 ++++++++++++++++++ lib/jni/JavaDataViewerProxy.hpp | 50 ++++ lib/jni/LogManager_jni.cpp | 185 ++++++++++++- 9 files changed, 650 insertions(+), 9 deletions(-) create mode 100644 lib/android_build/maesdk/src/main/java/com/microsoft/applications/events/IDataViewer.java create mode 100644 lib/jni/JavaDataViewerProxy.cpp create mode 100644 lib/jni/JavaDataViewerProxy.hpp diff --git a/lib/CMakeLists.txt b/lib/CMakeLists.txt index 29d56105e..18004224e 100644 --- a/lib/CMakeLists.txt +++ b/lib/CMakeLists.txt @@ -60,6 +60,7 @@ endif() if(MATSDK_BUILD_JNI_WRAPPER) list(APPEND SRCS + jni/JavaDataViewerProxy.cpp jni/JniConvertors.cpp jni/LogManager_jni.cpp jni/Logger_jni.cpp diff --git a/lib/android_build/app/src/androidTest/java/com/microsoft/applications/events/maesdktest/LogManagerDDVUnitTest.java b/lib/android_build/app/src/androidTest/java/com/microsoft/applications/events/maesdktest/LogManagerDDVUnitTest.java index 405749318..3eb659588 100644 --- a/lib/android_build/app/src/androidTest/java/com/microsoft/applications/events/maesdktest/LogManagerDDVUnitTest.java +++ b/lib/android_build/app/src/androidTest/java/com/microsoft/applications/events/maesdktest/LogManagerDDVUnitTest.java @@ -25,6 +25,7 @@ import com.microsoft.applications.events.DebugEventType; import com.microsoft.applications.events.DiagLevel; import com.microsoft.applications.events.HttpClient; +import com.microsoft.applications.events.IDataViewer; import com.microsoft.applications.events.ILogConfiguration; import com.microsoft.applications.events.ILogManager; import com.microsoft.applications.events.ILogger; @@ -42,7 +43,10 @@ import java.util.SortedMap; import java.util.TreeMap; import java.util.TreeSet; +import java.util.concurrent.CountDownLatch; import java.util.concurrent.FutureTask; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; import org.junit.Test; import org.junit.runner.RunWith; @@ -247,6 +251,93 @@ public void startDDVonLogManager() { LogManager.flushAndTeardown(); } + @Test + public void registerDataViewer_whenCallbackThrows_continuesDispatchAndSupportsUnregister() + throws Exception { + System.loadLibrary("maesdk"); + Context appContext = InstrumentationRegistry.getInstrumentation().getTargetContext(); + if (s_client == null) { + s_client = new MockHttpClient(appContext); + } + OfflineRoom.connectContext(appContext); + + final String token = + "0123456789abcdef9123456789abcdef-01234567-0123-0123-0123-0123456789ab-0124"; + final String factoryName = "JavaDataViewer" + System.nanoTime(); + ILogConfiguration custom = LogManager.logConfigurationFactory(); + custom.set(LogConfigurationKey.CFG_STR_PRIMARY_TOKEN, token); + custom.set(LogConfigurationKey.CFG_STR_COLLECTOR_URL, "https://viewer.contoso.com/"); + custom.set(LogConfigurationKey.CFG_STR_FACTORY_NAME, factoryName); + custom.set(LogConfigurationKey.CFG_STR_CACHE_FILE_PATH, factoryName); + + ILogManager manager = LogManagerProvider.createLogManager(custom); + CountDownLatch receivedPacket = new CountDownLatch(1); + AtomicInteger receivedByteCount = new AtomicInteger(); + IDataViewer throwingViewer = + new IDataViewer() { + @Override + public void receiveData(byte[] packetData) { + throw new IllegalStateException("Expected callback failure"); + } + + @Override + public String getName() { + return "throwing-viewer"; + } + + @Override + public boolean isTransmissionEnabled() { + return true; + } + + @Override + public String getCurrentEndpoint() { + return ""; + } + }; + IDataViewer receivingViewer = + new IDataViewer() { + @Override + public void receiveData(byte[] packetData) { + receivedByteCount.set(packetData.length); + receivedPacket.countDown(); + } + + @Override + public String getName() { + return "receiving-viewer"; + } + + @Override + public boolean isTransmissionEnabled() { + return true; + } + + @Override + public String getCurrentEndpoint() { + return "http://127.0.0.1"; + } + }; + + try { + assertThat(manager.registerDataViewer(throwingViewer), is(true)); + assertThat(manager.registerDataViewer(receivingViewer), is(true)); + assertThat(manager.registerDataViewer(receivingViewer), is(false)); + + ILogger logger = manager.getLogger(token, "java-data-viewer-test", ""); + logger.logEvent("javaDataViewerCallback"); + manager.uploadNow(); + + assertThat(receivedPacket.await(5, TimeUnit.SECONDS), is(true)); + assertThat(receivedByteCount.get(), greaterThan(0)); + assertThat(manager.unregisterDataViewer("receiving-viewer"), is(true)); + assertThat(manager.unregisterDataViewer("receiving-viewer"), is(false)); + assertThat(manager.unregisterDataViewer("throwing-viewer"), is(true)); + } finally { + manager.close(); + } + } + /* Disabling this test since it requires private modules. diff --git a/lib/android_build/maesdk/consumer-rules.pro b/lib/android_build/maesdk/consumer-rules.pro index e69de29bb..09006a474 100644 --- a/lib/android_build/maesdk/consumer-rules.pro +++ b/lib/android_build/maesdk/consumer-rules.pro @@ -0,0 +1,4 @@ +-keep interface com.microsoft.applications.events.IDataViewer { *; } +-keep class * implements com.microsoft.applications.events.IDataViewer { + public *; +} \ No newline at end of file diff --git a/lib/android_build/maesdk/src/main/java/com/microsoft/applications/events/IDataViewer.java b/lib/android_build/maesdk/src/main/java/com/microsoft/applications/events/IDataViewer.java new file mode 100644 index 000000000..d2289a748 --- /dev/null +++ b/lib/android_build/maesdk/src/main/java/com/microsoft/applications/events/IDataViewer.java @@ -0,0 +1,30 @@ +// +// Copyright (c) Microsoft Corporation. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 +// +package com.microsoft.applications.events; + +import androidx.annotation.Keep; + +/** + * Receives copies of packets uploaded by the SDK. + * + *

Implementations must return a stable, unique name for the lifetime of the registration. + * Callbacks can occur on an SDK worker thread and should return promptly. Implementations must not + * register or unregister viewers from within a callback. + */ +@Keep +public interface IDataViewer { + + /** Receives an encoded telemetry packet after it has been prepared for upload. */ + void receiveData(byte[] packetData); + + /** Returns the stable, unique name used to register this viewer. */ + String getName(); + + /** Returns whether this viewer is currently accepting packet callbacks. */ + boolean isTransmissionEnabled(); + + /** Returns the endpoint currently used by this viewer, or an empty string when disabled. */ + String getCurrentEndpoint(); +} diff --git a/lib/android_build/maesdk/src/main/java/com/microsoft/applications/events/ILogManager.java b/lib/android_build/maesdk/src/main/java/com/microsoft/applications/events/ILogManager.java index 332331cb1..9ee5d7d0b 100644 --- a/lib/android_build/maesdk/src/main/java/com/microsoft/applications/events/ILogManager.java +++ b/lib/android_build/maesdk/src/main/java/com/microsoft/applications/events/ILogManager.java @@ -59,6 +59,21 @@ public interface ILogManager extends AutoCloseable { public String getCurrentEndpoint(); + /** + * Registers a caller-provided data viewer with this LogManager. + * + * @return {@code true} when the viewer was registered, {@code false} for invalid input or a + * duplicate viewer name + */ + public boolean registerDataViewer(IDataViewer dataViewer); + + /** + * Unregisters a caller-provided data viewer by its unique name. + * + * @return {@code true} when the viewer was unregistered, {@code false} when it was not registered + */ + public boolean unregisterDataViewer(String viewerName); + public LogSessionData getLogSessionData(); public void setLevelFilter(int defaultLevel, int[] allowedLevels); diff --git a/lib/android_build/maesdk/src/main/java/com/microsoft/applications/events/LogManagerProvider.java b/lib/android_build/maesdk/src/main/java/com/microsoft/applications/events/LogManagerProvider.java index ba5d41e74..777f47d73 100644 --- a/lib/android_build/maesdk/src/main/java/com/microsoft/applications/events/LogManagerProvider.java +++ b/lib/android_build/maesdk/src/main/java/com/microsoft/applications/events/LogManagerProvider.java @@ -226,6 +226,28 @@ public String getCurrentEndpoint() { return nativeGetCurrentEndpoint(nativeLogManager); } + protected native boolean nativeRegisterDataViewer( + long nativeLogManager, IDataViewer dataViewer); + + @Override + public boolean registerDataViewer(IDataViewer dataViewer) { + if (dataViewer == null) { + return false; + } + return nativeRegisterDataViewer(nativeLogManager, dataViewer); + } + + protected native boolean nativeUnregisterDataViewer( + long nativeLogManager, String viewerName); + + @Override + public boolean unregisterDataViewer(String viewerName) { + if (viewerName == null || viewerName.isEmpty()) { + return false; + } + return nativeUnregisterDataViewer(nativeLogManager, viewerName); + } + protected static class LogSessionDataImpl implements LogSessionData { @Keep private long m_first_time; diff --git a/lib/jni/JavaDataViewerProxy.cpp b/lib/jni/JavaDataViewerProxy.cpp new file mode 100644 index 000000000..de3b4765c --- /dev/null +++ b/lib/jni/JavaDataViewerProxy.cpp @@ -0,0 +1,261 @@ +// +// Copyright (c) Microsoft Corporation. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 +// +#include "JavaDataViewerProxy.hpp" + +#include +#include +#include + +namespace MAT_NS_BEGIN +{ + namespace + { + constexpr const char* LOG_TAG = "MAE.JavaDataViewer"; + } + + std::shared_ptr JavaDataViewerProxy::Create( + JNIEnv* env, + jobject dataViewer) noexcept + { + if (env == nullptr || dataViewer == nullptr) + { + return nullptr; + } + + auto proxy = std::shared_ptr(new JavaDataViewerProxy()); + if (env->GetJavaVM(&proxy->m_javaVm) != JNI_OK) + { + return nullptr; + } + + auto dataViewerClass = env->GetObjectClass(dataViewer); + if (dataViewerClass == nullptr || env->ExceptionCheck()) + { + env->ExceptionClear(); + return nullptr; + } + + proxy->m_receiveData = env->GetMethodID(dataViewerClass, "receiveData", "([B)V"); + if (proxy->ClearPendingException(env, "receiveData lookup")) + { + env->DeleteLocalRef(dataViewerClass); + return nullptr; + } + proxy->m_getName = env->GetMethodID(dataViewerClass, "getName", "()Ljava/lang/String;"); + if (proxy->ClearPendingException(env, "getName lookup")) + { + env->DeleteLocalRef(dataViewerClass); + return nullptr; + } + proxy->m_isTransmissionEnabled = + env->GetMethodID(dataViewerClass, "isTransmissionEnabled", "()Z"); + if (proxy->ClearPendingException(env, "isTransmissionEnabled lookup")) + { + env->DeleteLocalRef(dataViewerClass); + return nullptr; + } + proxy->m_getCurrentEndpoint = + env->GetMethodID(dataViewerClass, "getCurrentEndpoint", "()Ljava/lang/String;"); + if (proxy->ClearPendingException(env, "getCurrentEndpoint lookup")) + { + env->DeleteLocalRef(dataViewerClass); + return nullptr; + } + env->DeleteLocalRef(dataViewerClass); + + if (proxy->m_receiveData == nullptr || + proxy->m_getName == nullptr || + proxy->m_isTransmissionEnabled == nullptr || + proxy->m_getCurrentEndpoint == nullptr) + { + return nullptr; + } + + proxy->m_dataViewer = env->NewGlobalRef(dataViewer); + if (proxy->m_dataViewer == nullptr || env->ExceptionCheck()) + { + env->ExceptionClear(); + return nullptr; + } + + if (!proxy->ReadString(env, proxy->m_getName, proxy->m_name) || proxy->m_name.empty()) + { + return nullptr; + } + return proxy; + } + + JavaDataViewerProxy::~JavaDataViewerProxy() noexcept + { + if (m_dataViewer == nullptr) + { + return; + } + + bool attached = false; + auto env = GetEnv(attached); + if (env != nullptr) + { + env->DeleteGlobalRef(m_dataViewer); + } + m_dataViewer = nullptr; + DetachIfNeeded(attached); + } + + void JavaDataViewerProxy::ReceiveData(const std::vector& packetData) noexcept + { + if (packetData.size() > static_cast(std::numeric_limits::max())) + { + __android_log_print(ANDROID_LOG_ERROR, LOG_TAG, "Packet is too large for a Java byte array"); + return; + } + + bool attached = false; + auto env = GetEnv(attached); + if (env == nullptr) + { + return; + } + + auto packet = env->NewByteArray(static_cast(packetData.size())); + if (packet == nullptr || ClearPendingException(env, "receiveData allocation")) + { + DetachIfNeeded(attached); + return; + } + if (!packetData.empty()) + { + env->SetByteArrayRegion( + packet, + 0, + static_cast(packetData.size()), + reinterpret_cast(packetData.data())); + } + + if (!ClearPendingException(env, "receiveData copy")) + { + env->CallVoidMethod(m_dataViewer, m_receiveData, packet); + ClearPendingException(env, "receiveData"); + } + env->DeleteLocalRef(packet); + DetachIfNeeded(attached); + } + + const char* JavaDataViewerProxy::GetName() const noexcept + { + return m_name.c_str(); + } + + bool JavaDataViewerProxy::IsTransmissionEnabled() const noexcept + { + bool attached = false; + auto env = GetEnv(attached); + if (env == nullptr) + { + return false; + } + + auto enabled = env->CallBooleanMethod(m_dataViewer, m_isTransmissionEnabled); + if (ClearPendingException(env, "isTransmissionEnabled")) + { + enabled = JNI_FALSE; + } + DetachIfNeeded(attached); + return enabled == JNI_TRUE; + } + + const std::string& JavaDataViewerProxy::GetCurrentEndpoint() const noexcept + { + std::lock_guard lock(m_endpointMutex); + bool attached = false; + auto env = GetEnv(attached); + if (env != nullptr) + { + std::string endpoint; + if (ReadString(env, m_getCurrentEndpoint, endpoint)) + { + m_currentEndpoint = std::move(endpoint); + } + else + { + m_currentEndpoint.clear(); + } + DetachIfNeeded(attached); + } + return m_currentEndpoint; + } + + JNIEnv* JavaDataViewerProxy::GetEnv(bool& attached) const noexcept + { + attached = false; + if (m_javaVm == nullptr) + { + return nullptr; + } + + JNIEnv* env = nullptr; + auto result = m_javaVm->GetEnv(reinterpret_cast(&env), JNI_VERSION_1_6); + if (result == JNI_OK) + { + return env; + } + if (result != JNI_EDETACHED || m_javaVm->AttachCurrentThread(&env, nullptr) != JNI_OK) + { + return nullptr; + } + attached = true; + return env; + } + + void JavaDataViewerProxy::DetachIfNeeded(bool attached) const noexcept + { + if (attached && m_javaVm != nullptr) + { + m_javaVm->DetachCurrentThread(); + } + } + + bool JavaDataViewerProxy::ClearPendingException( + JNIEnv* env, + const char* methodName) const noexcept + { + if (!env->ExceptionCheck()) + { + return false; + } + env->ExceptionClear(); + __android_log_print( + ANDROID_LOG_ERROR, + LOG_TAG, + "Java IDataViewer callback failed: %s", + methodName); + return true; + } + + bool JavaDataViewerProxy::ReadString( + JNIEnv* env, + jmethodID method, + std::string& value) const noexcept + { + auto javaValue = static_cast(env->CallObjectMethod(m_dataViewer, method)); + if (ClearPendingException(env, "string callback") || javaValue == nullptr) + { + return false; + } + + auto chars = env->GetStringUTFChars(javaValue, nullptr); + if (chars == nullptr) + { + ClearPendingException(env, "string conversion"); + env->DeleteLocalRef(javaValue); + return false; + } + value.assign(chars); + env->ReleaseStringUTFChars(javaValue, chars); + env->DeleteLocalRef(javaValue); + return !ClearPendingException(env, "string conversion"); + } + +} MAT_NS_END diff --git a/lib/jni/JavaDataViewerProxy.hpp b/lib/jni/JavaDataViewerProxy.hpp new file mode 100644 index 000000000..aee92d9fe --- /dev/null +++ b/lib/jni/JavaDataViewerProxy.hpp @@ -0,0 +1,50 @@ +// +// Copyright (c) Microsoft Corporation. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 +// +#ifndef JAVADATAVIEWERPROXY_HPP +#define JAVADATAVIEWERPROXY_HPP + +#include "IDataViewer.hpp" + +#include +#include +#include +#include + +namespace MAT_NS_BEGIN +{ + class JavaDataViewerProxy final : public IDataViewer + { + public: + static std::shared_ptr Create(JNIEnv* env, jobject dataViewer) noexcept; + + ~JavaDataViewerProxy() noexcept override; + + void ReceiveData(const std::vector& packetData) noexcept override; + const char* GetName() const noexcept override; + bool IsTransmissionEnabled() const noexcept override; + const std::string& GetCurrentEndpoint() const noexcept override; + + private: + JavaDataViewerProxy() = default; + + JNIEnv* GetEnv(bool& attached) const noexcept; + void DetachIfNeeded(bool attached) const noexcept; + bool ClearPendingException(JNIEnv* env, const char* methodName) const noexcept; + bool ReadString(JNIEnv* env, jmethodID method, std::string& value) const noexcept; + + JavaVM* m_javaVm = nullptr; + jobject m_dataViewer = nullptr; + jmethodID m_receiveData = nullptr; + jmethodID m_getName = nullptr; + jmethodID m_isTransmissionEnabled = nullptr; + jmethodID m_getCurrentEndpoint = nullptr; + std::string m_name; + mutable std::mutex m_endpointMutex; + mutable std::string m_currentEndpoint; + }; + +} MAT_NS_END + +#endif diff --git a/lib/jni/LogManager_jni.cpp b/lib/jni/LogManager_jni.cpp index 22b4244ee..75e50f207 100644 --- a/lib/jni/LogManager_jni.cpp +++ b/lib/jni/LogManager_jni.cpp @@ -27,6 +27,7 @@ #include #include "callbacks/DebugSourceInternal.hpp" +#include "JavaDataViewerProxy.hpp" #include "JniConvertors.hpp" #include "LogManagerBase.hpp" #include "WrapperLogManager.hpp" @@ -35,6 +36,9 @@ #endif #include "config/RuntimeConfig_Default.hpp" +#include +#include + using namespace MAT; template <> @@ -869,12 +873,16 @@ namespace ILogConfiguration config; ILogManager* manager; std::shared_ptr ddv; + std::mutex javaDataViewersMutex; + std::unordered_map> javaDataViewers; }; #else struct ManagerAndConfig { ILogConfiguration config; ILogManager* manager; + std::mutex javaDataViewersMutex; + std::unordered_map> javaDataViewers; }; #endif @@ -882,6 +890,53 @@ namespace static MCVector jniManagers; static std::mutex jniManagersMutex; + + ManagerAndConfig* getManagerAndConfig(jlong nativeLogManager) + { + std::lock_guard lock(jniManagersMutex); + if (nativeLogManager < 0 || + nativeLogManager >= static_cast(jniManagers.size())) + { + return nullptr; + } + return jniManagers[nativeLogManager].get(); + } + + void closeJavaDataViewers(ManagerAndConfig& managerAndConfig) + { + ILogManager* manager; + std::unordered_map> dataViewers; + { + std::lock_guard lock(managerAndConfig.javaDataViewersMutex); + manager = managerAndConfig.manager; + { + std::lock_guard managersLock(jniManagersMutex); + managerAndConfig.manager = nullptr; + } + dataViewers.swap(managerAndConfig.javaDataViewers); + } + + if (manager == nullptr) + { + return; + } + for (const auto& dataViewer : dataViewers) + { + try + { + manager->GetDataViewerCollection().UnregisterViewer(dataViewer.first.c_str()); + } + catch (const std::exception& exception) + { + __android_log_print( + ANDROID_LOG_WARN, + "MAE.JavaDataViewer", + "Failed to unregister Java IDataViewer '%s': %s", + dataViewer.first.c_str(), + exception.what()); + } + } + } } extern "C" JNIEXPORT jlong JNICALL @@ -979,17 +1034,14 @@ Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_na jobject /* this */, jlong nativeLogManager) { + auto managerAndConfig = getManagerAndConfig(nativeLogManager); + if (managerAndConfig == nullptr) { - std::lock_guard lock(jniManagersMutex); - if (nativeLogManager < 0 || nativeLogManager >= static_cast(jniManagers.size())) - { - return; - } - // we reset the manager member of the ManagerAndConfig, - // but the ManagerAndConfig itself will survive until - // the static jniManagers array is destroyed. - jniManagers[nativeLogManager]->manager = nullptr; + return; } + + // The ManagerAndConfig survives until the static jniManagers array is destroyed. + closeJavaDataViewers(*managerAndConfig); } extern "C" JNIEXPORT jobject JNICALL @@ -1526,6 +1578,121 @@ Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_na #endif } +extern "C" JNIEXPORT jboolean JNICALL +Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_nativeRegisterDataViewer( + JNIEnv* env, + jobject /* this */, + jlong native_log_manager, + jobject data_viewer) +{ + auto proxy = JavaDataViewerProxy::Create(env, data_viewer); + if (!proxy) + { + return false; + } + + auto manager_and_config = getManagerAndConfig(native_log_manager); + if (manager_and_config == nullptr) + { + return false; + } + + std::lock_guard lock(manager_and_config->javaDataViewersMutex); + if (manager_and_config->manager == nullptr || + manager_and_config->javaDataViewers.find(proxy->GetName()) != + manager_and_config->javaDataViewers.end()) + { + return false; + } + + bool collectionRegistered = false; + try + { + manager_and_config->manager->GetDataViewerCollection().RegisterViewer(proxy); + collectionRegistered = true; + manager_and_config->javaDataViewers.emplace(proxy->GetName(), proxy); + return true; + } + catch (const std::exception& exception) + { + if (collectionRegistered) + { + try + { + manager_and_config->manager->GetDataViewerCollection().UnregisterViewer( + proxy->GetName()); + } + catch (const std::exception& rollbackException) + { + __android_log_print( + ANDROID_LOG_ERROR, + "MAE.JavaDataViewer", + "Failed to roll back Java IDataViewer '%s': %s", + proxy->GetName(), + rollbackException.what()); + } + } + __android_log_print( + ANDROID_LOG_WARN, + "MAE.JavaDataViewer", + "Failed to register Java IDataViewer '%s': %s", + proxy->GetName(), + exception.what()); + return false; + } +} + +extern "C" JNIEXPORT jboolean JNICALL +Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_nativeUnregisterDataViewer( + JNIEnv* env, + jobject /* this */, + jlong native_log_manager, + jstring viewer_name) +{ + std::string name; + if (!TryJStringToStdString(env, viewer_name, name) || name.empty()) + { + return false; + } + + auto manager_and_config = getManagerAndConfig(native_log_manager); + if (manager_and_config == nullptr) + { + return false; + } + + ILogManager* manager; + std::shared_ptr proxy; + { + std::lock_guard lock(manager_and_config->javaDataViewersMutex); + auto viewer = manager_and_config->javaDataViewers.find(name); + if (manager_and_config->manager == nullptr || + viewer == manager_and_config->javaDataViewers.end()) + { + return false; + } + manager = manager_and_config->manager; + proxy = std::move(viewer->second); + manager_and_config->javaDataViewers.erase(viewer); + } + + try + { + manager->GetDataViewerCollection().UnregisterViewer(name.c_str()); + return true; + } + catch (const std::exception& exception) + { + __android_log_print( + ANDROID_LOG_WARN, + "MAE.JavaDataViewer", + "Failed to unregister Java IDataViewer '%s': %s", + name.c_str(), + exception.what()); + return false; + } +} + extern "C" JNIEXPORT void JNICALL Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_nativeGetLogSessionData( JNIEnv* env, From 65d01fa7ffa26a451400154cda496218e0f4768d Mon Sep 17 00:00:00 2001 From: kdhawaniya Date: Mon, 21 Sep 2026 16:58:32 +0530 Subject: [PATCH 2/8] Address review feedback on the Java IDataViewer API Preserves source compatibility, fixes reentrant dispatch, and strengthens the unregistration test. registerDataViewer and unregisterDataViewer become default methods on ILogManager returning false. Adding abstract methods to a public interface would break every consumer-owned implementation and test double on upgrade, despite the change being additive in intent. LogManagerImpl overrides both, so the native path is unaffected. DispatchDataViewerEvent now iterates a snapshot of the viewer collection rather than the member vector. m_dataViewerMapLock is recursive, so a viewer that reenters the SDK from ReceiveData - closing the owning LogManager, which unregisters every viewer - was admitted back in and erased the vector while dispatch was still walking it, invalidating the iterator. Exposing IDataViewer to arbitrary Java implementations makes that reachable from outside the SDK, so the hazard is fixed rather than only documented. Holding shared_ptr copies also keeps each viewer alive across its own callback. The IDataViewer contract now prohibits closing the owning manager from a callback, and a unit test covers a viewer that unregisters everything from ReceiveData. The instrumentation test asserted only the native return value of unregisterDataViewer, so a bridge that dropped its bookkeeping entry but left the proxy registered in DataViewerCollection would have passed. It now drives a second dispatch after unregistering, using the still registered throwing viewer as the witness that a dispatch really occurred, and asserts the unregistered viewer's callback count does not increase. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../maesdktest/LogManagerDDVUnitTest.java | 28 ++++++++- .../applications/events/IDataViewer.java | 4 +- .../applications/events/ILogManager.java | 23 +++++-- lib/api/DataViewerCollection.cpp | 9 ++- tests/unittests/DataViewerCollectionTests.cpp | 62 +++++++++++++++++++ 5 files changed, 118 insertions(+), 8 deletions(-) diff --git a/lib/android_build/app/src/androidTest/java/com/microsoft/applications/events/maesdktest/LogManagerDDVUnitTest.java b/lib/android_build/app/src/androidTest/java/com/microsoft/applications/events/maesdktest/LogManagerDDVUnitTest.java index 3eb659588..a9dd729ee 100644 --- a/lib/android_build/app/src/androidTest/java/com/microsoft/applications/events/maesdktest/LogManagerDDVUnitTest.java +++ b/lib/android_build/app/src/androidTest/java/com/microsoft/applications/events/maesdktest/LogManagerDDVUnitTest.java @@ -252,7 +252,7 @@ public void startDDVonLogManager() { } @Test - public void registerDataViewer_whenCallbackThrows_continuesDispatchAndSupportsUnregister() + public void registerDataViewer_whenCallbackThrows_continuesDispatchAndStopsAfterUnregister() throws Exception { System.loadLibrary("maesdk"); Context appContext = InstrumentationRegistry.getInstrumentation().getTargetContext(); @@ -273,10 +273,13 @@ public void registerDataViewer_whenCallbackThrows_continuesDispatchAndSupportsUn ILogManager manager = LogManagerProvider.createLogManager(custom); CountDownLatch receivedPacket = new CountDownLatch(1); AtomicInteger receivedByteCount = new AtomicInteger(); + AtomicInteger receivingViewerCalls = new AtomicInteger(); + AtomicInteger throwingViewerCalls = new AtomicInteger(); IDataViewer throwingViewer = new IDataViewer() { @Override public void receiveData(byte[] packetData) { + throwingViewerCalls.incrementAndGet(); throw new IllegalStateException("Expected callback failure"); } @@ -299,6 +302,7 @@ public String getCurrentEndpoint() { new IDataViewer() { @Override public void receiveData(byte[] packetData) { + receivingViewerCalls.incrementAndGet(); receivedByteCount.set(packetData.length); receivedPacket.countDown(); } @@ -330,8 +334,30 @@ public String getCurrentEndpoint() { assertThat(receivedPacket.await(5, TimeUnit.SECONDS), is(true)); assertThat(receivedByteCount.get(), greaterThan(0)); + assertThat(manager.unregisterDataViewer("receiving-viewer"), is(true)); assertThat(manager.unregisterDataViewer("receiving-viewer"), is(false)); + + // Unregistering must actually stop callbacks, not merely drop the bookkeeping entry: a + // bridge that left the proxy in the native DataViewerCollection would still pass the + // assertions above. Drive a second dispatch and use the still-registered throwing viewer + // as the witness that one really occurred, then assert the unregistered viewer was not + // called again. + final int receivingCallsAtUnregister = receivingViewerCalls.get(); + final int throwingCallsAtUnregister = throwingViewerCalls.get(); + + logger.logEvent("javaDataViewerCallbackAfterUnregister"); + manager.uploadNow(); + + final long deadline = System.currentTimeMillis() + 10000; + while (throwingViewerCalls.get() <= throwingCallsAtUnregister + && System.currentTimeMillis() < deadline) { + Thread.sleep(50); + } + + assertThat(throwingViewerCalls.get(), greaterThan(throwingCallsAtUnregister)); + assertThat(receivingViewerCalls.get(), is(receivingCallsAtUnregister)); + assertThat(manager.unregisterDataViewer("throwing-viewer"), is(true)); } finally { manager.close(); diff --git a/lib/android_build/maesdk/src/main/java/com/microsoft/applications/events/IDataViewer.java b/lib/android_build/maesdk/src/main/java/com/microsoft/applications/events/IDataViewer.java index d2289a748..36503859a 100644 --- a/lib/android_build/maesdk/src/main/java/com/microsoft/applications/events/IDataViewer.java +++ b/lib/android_build/maesdk/src/main/java/com/microsoft/applications/events/IDataViewer.java @@ -11,7 +11,9 @@ * *

Implementations must return a stable, unique name for the lifetime of the registration. * Callbacks can occur on an SDK worker thread and should return promptly. Implementations must not - * register or unregister viewers from within a callback. + * reenter the SDK from within a callback: do not register or unregister viewers, and do not close + * the owning {@link ILogManager}, because closing unregisters every viewer while the callback is + * still in progress. */ @Keep public interface IDataViewer { diff --git a/lib/android_build/maesdk/src/main/java/com/microsoft/applications/events/ILogManager.java b/lib/android_build/maesdk/src/main/java/com/microsoft/applications/events/ILogManager.java index 9ee5d7d0b..9a90d7eb1 100644 --- a/lib/android_build/maesdk/src/main/java/com/microsoft/applications/events/ILogManager.java +++ b/lib/android_build/maesdk/src/main/java/com/microsoft/applications/events/ILogManager.java @@ -62,17 +62,30 @@ public interface ILogManager extends AutoCloseable { /** * Registers a caller-provided data viewer with this LogManager. * - * @return {@code true} when the viewer was registered, {@code false} for invalid input or a - * duplicate viewer name + *

This is an optional capability. The default implementation returns {@code false} so that + * existing implementations of this interface remain source compatible; implementations that + * support data viewers override it. + * + * @return {@code true} when the viewer was registered, {@code false} for invalid input, a + * duplicate viewer name, or when the implementation does not support data viewers */ - public boolean registerDataViewer(IDataViewer dataViewer); + default boolean registerDataViewer(IDataViewer dataViewer) { + return false; + } /** * Unregisters a caller-provided data viewer by its unique name. * - * @return {@code true} when the viewer was unregistered, {@code false} when it was not registered + *

This is an optional capability. The default implementation returns {@code false} so that + * existing implementations of this interface remain source compatible; implementations that + * support data viewers override it. + * + * @return {@code true} when the viewer was unregistered, {@code false} when it was not + * registered, or when the implementation does not support data viewers */ - public boolean unregisterDataViewer(String viewerName); + default boolean unregisterDataViewer(String viewerName) { + return false; + } public LogSessionData getLogSessionData(); diff --git a/lib/api/DataViewerCollection.cpp b/lib/api/DataViewerCollection.cpp index 6992fee75..9c4a93205 100644 --- a/lib/api/DataViewerCollection.cpp +++ b/lib/api/DataViewerCollection.cpp @@ -16,7 +16,14 @@ namespace MAT_NS_BEGIN { return; LOCKGUARD(m_dataViewerMapLock); - for(const auto& viewer : m_dataViewerCollection) + // Dispatch over a snapshot rather than the member directly. m_dataViewerMapLock is + // recursive, so a viewer that reenters the SDK from ReceiveData - for example by + // closing the owning LogManager, which unregisters every viewer - would otherwise + // erase from the very vector being iterated here and invalidate the iterator. + // Holding shared_ptr copies additionally keeps each viewer alive for the duration of + // its own callback, even if that callback drops the last other reference to it. + const auto viewers = m_dataViewerCollection; + for(const auto& viewer : viewers) { // Task 3568800: Integrate ThreadPool to IDataViewerCollection viewer->ReceiveData(packetData); diff --git a/tests/unittests/DataViewerCollectionTests.cpp b/tests/unittests/DataViewerCollectionTests.cpp index 57b377fc3..33b51916c 100644 --- a/tests/unittests/DataViewerCollectionTests.cpp +++ b/tests/unittests/DataViewerCollectionTests.cpp @@ -264,3 +264,65 @@ TEST(DataViewerCollectionTests, IsViewerEnabledNoParam_MultipleViewersRegistered ASSERT_TRUE(dataViewerCollection.IsViewerEnabled()); } +namespace +{ + // Mirrors a viewer that reenters the SDK from its own callback - for example a Java + // viewer that closes the owning LogManager from receiveData(), which unregisters every + // viewer. m_dataViewerMapLock is recursive, so the reentrant call is admitted while + // dispatch is still walking the collection. + class ReentrantUnregisteringDataViewer : public IDataViewer + { + public: + + ReentrantUnregisteringDataViewer(const char* name, TestDataViewerCollection& collection) : + m_name(name), m_collection(collection) {} + + void ReceiveData(const std::vector&) noexcept override + { + callCount++; + m_collection.UnregisterAllViewers(); + } + + const char* GetName() const noexcept override + { + return m_name; + } + + bool IsTransmissionEnabled() const noexcept override + { + return true; + } + + const std::string& GetCurrentEndpoint() const noexcept override + { + return m_testEndpoint; + } + + int callCount { 0 }; + const char* m_name; + TestDataViewerCollection& m_collection; + const std::string m_testEndpoint { "TestEndpoint" }; + }; +} + +TEST(DataViewerCollectionTests, DispatchDataViewerEvent_ViewerUnregistersAllFromCallback_DispatchCompletesSafely) +{ + TestDataViewerCollection dataViewerCollection { }; + auto reentrantViewer = std::make_shared("ReentrantViewer", dataViewerCollection); + auto secondViewer = std::make_shared("SecondViewer", /*isTransmissionEnabled*/ true); + + dataViewerCollection.RegisterViewer(reentrantViewer); + dataViewerCollection.RegisterViewer(secondViewer); + + const std::vector packetData { 1, 2, 3 }; + + // Dispatching over the member vector directly would erase it mid-iteration here and + // invalidate the iterator; dispatching over a snapshot completes and still delivers the + // in-flight packet to viewers that were registered when dispatch began. + dataViewerCollection.DispatchDataViewerEvent(packetData); + + ASSERT_EQ(reentrantViewer->callCount, 1); + ASSERT_EQ(secondViewer->localPacketData, packetData); + ASSERT_TRUE(dataViewerCollection.GetCollection().empty()); +} + From 77b17258ae488dcd8d7ec58f41b53687ebd02bae Mon Sep 17 00:00:00 2001 From: kdhawaniya Date: Mon, 21 Sep 2026 17:22:49 +0530 Subject: [PATCH 3/8] Fix data race in JavaDataViewerProxy::GetCurrentEndpoint IDataViewer returns the endpoint by const reference, so the referent must outlive the call and must not be mutated while a caller holds it. The proxy updated a shared member under a mutex and then returned a reference to it, releasing the lock on return: two concurrent callers could read and write the same string at once, so the mutex gave no protection. Use a thread_local buffer instead, which gives each calling thread its own storage and removes the need for the lock. Behaviour is unchanged: the endpoint is still read from Java on every call, so a viewer that changes endpoints still reports the current one. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/jni/JavaDataViewerProxy.cpp | 34 ++++++++++++++++++++------------- lib/jni/JavaDataViewerProxy.hpp | 3 --- 2 files changed, 21 insertions(+), 16 deletions(-) diff --git a/lib/jni/JavaDataViewerProxy.cpp b/lib/jni/JavaDataViewerProxy.cpp index de3b4765c..2d49d4d31 100644 --- a/lib/jni/JavaDataViewerProxy.cpp +++ b/lib/jni/JavaDataViewerProxy.cpp @@ -168,23 +168,31 @@ namespace MAT_NS_BEGIN const std::string& JavaDataViewerProxy::GetCurrentEndpoint() const noexcept { - std::lock_guard lock(m_endpointMutex); + // IDataViewer returns the endpoint by reference, so the referent has to outlive the + // call and must not be mutated by a concurrent caller. A thread_local buffer gives + // each calling thread its own storage; a shared member guarded by a mutex would not, + // because the lock is released before the caller reads the reference. + static thread_local std::string currentEndpoint; + bool attached = false; auto env = GetEnv(attached); - if (env != nullptr) + if (env == nullptr) { - std::string endpoint; - if (ReadString(env, m_getCurrentEndpoint, endpoint)) - { - m_currentEndpoint = std::move(endpoint); - } - else - { - m_currentEndpoint.clear(); - } - DetachIfNeeded(attached); + currentEndpoint.clear(); + return currentEndpoint; + } + + std::string endpoint; + if (ReadString(env, m_getCurrentEndpoint, endpoint)) + { + currentEndpoint = std::move(endpoint); } - return m_currentEndpoint; + else + { + currentEndpoint.clear(); + } + DetachIfNeeded(attached); + return currentEndpoint; } JNIEnv* JavaDataViewerProxy::GetEnv(bool& attached) const noexcept diff --git a/lib/jni/JavaDataViewerProxy.hpp b/lib/jni/JavaDataViewerProxy.hpp index aee92d9fe..ed0936120 100644 --- a/lib/jni/JavaDataViewerProxy.hpp +++ b/lib/jni/JavaDataViewerProxy.hpp @@ -9,7 +9,6 @@ #include #include -#include #include namespace MAT_NS_BEGIN @@ -41,8 +40,6 @@ namespace MAT_NS_BEGIN jmethodID m_isTransmissionEnabled = nullptr; jmethodID m_getCurrentEndpoint = nullptr; std::string m_name; - mutable std::mutex m_endpointMutex; - mutable std::string m_currentEndpoint; }; } MAT_NS_END From a3604e4ae1cd3ccf2437910cdf82ae7228347a9e Mon Sep 17 00:00:00 2001 From: kdhawaniya Date: Mon, 28 Sep 2026 15:49:40 +0530 Subject: [PATCH 4/8] Release the data viewer collection lock before dispatching DispatchDataViewerEvent held m_dataViewerMapLock for the whole callback loop, so the SDK ran arbitrary viewer code while holding one of its own locks. Registration acquires the JNI viewer mutex and then this lock, while a callback that reenters registration waits for the JNI mutex with this lock already held, so the two orders could deadlock: the collection lock is recursive, but that only helps the thread already holding it, not the one blocked behind it. Reentrancy is not required for this to hurt. Because the lock spanned every callback, any registration, unregistration or LogManager close on any thread blocked until all callbacks returned. On Android the callback body is a socket write, so closing the manager could stall behind a slow or half-open viewer connection. Take the snapshot under the lock, release it, then dispatch outside it. The shared_ptr copies still keep each viewer alive for the duration of its own callback, and viewers that were registered when dispatch began still receive the in-flight packet. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/api/DataViewerCollection.cpp | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/lib/api/DataViewerCollection.cpp b/lib/api/DataViewerCollection.cpp index 9c4a93205..addd2e832 100644 --- a/lib/api/DataViewerCollection.cpp +++ b/lib/api/DataViewerCollection.cpp @@ -15,14 +15,22 @@ namespace MAT_NS_BEGIN { if (IsViewerEnabled() == false) return; - LOCKGUARD(m_dataViewerMapLock); - // Dispatch over a snapshot rather than the member directly. m_dataViewerMapLock is - // recursive, so a viewer that reenters the SDK from ReceiveData - for example by - // closing the owning LogManager, which unregisters every viewer - would otherwise - // erase from the very vector being iterated here and invalidate the iterator. - // Holding shared_ptr copies additionally keeps each viewer alive for the duration of - // its own callback, even if that callback drops the last other reference to it. - const auto viewers = m_dataViewerCollection; + // Dispatch over a snapshot taken under the lock, and release the lock before invoking any + // viewer. Iterating m_dataViewerCollection directly is unsafe because m_dataViewerMapLock + // is recursive: a viewer that reenters the SDK from ReceiveData - for example by closing + // the owning LogManager, which unregisters every viewer - would erase from the very vector + // being iterated here and invalidate the iterator. Holding the lock across a callback is + // unsafe for a second reason: registration acquires the JNI viewer mutex and then this + // lock, so a callback that reenters registration closes a lock cycle, and any slow callback + // would stall registration, unregistration and LogManager close until it returned. + // The shared_ptr copies keep each viewer alive for the duration of its own callback, even + // if it is unregistered - or loses its last other reference - while dispatch is running. + std::vector> viewers; + { + LOCKGUARD(m_dataViewerMapLock); + viewers = m_dataViewerCollection; + } + for(const auto& viewer : viewers) { // Task 3568800: Integrate ThreadPool to IDataViewerCollection From d42ddb30b84fdcb107762bd062f7e4be27c2146a Mon Sep 17 00:00:00 2001 From: kdhawaniya Date: Mon, 28 Sep 2026 16:15:47 +0530 Subject: [PATCH 5/8] Clear the pending exception when the packet allocation fails NewByteArray reports failure by returning null and leaving an OutOfMemoryError pending, but the guard tested the null first. Because || short circuits, ClearPendingException was skipped on exactly the path that needed it, so the exception stayed pending and escaped the callback. On a thread the proxy attached itself that only means detaching with an exception pending. On an already attached thread nothing clears it, and every later JNI call on that thread is undefined: the dispatch loop moves straight to the next viewer and calls NewByteArray again, which CheckJNI reports as a fatal error. With a single viewer the exception instead surfaces at an unrelated Java frame, so one viewer running out of memory is no longer isolated from the SDK or the app. Evaluate ClearPendingException first so it always runs, which also matches ReadString, and release the array when an exception was already pending but the allocation succeeded, so that branch no longer leaks a local reference. The successful path is unchanged: it already evaluated both operands. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/jni/JavaDataViewerProxy.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/jni/JavaDataViewerProxy.cpp b/lib/jni/JavaDataViewerProxy.cpp index 2d49d4d31..32045afcf 100644 --- a/lib/jni/JavaDataViewerProxy.cpp +++ b/lib/jni/JavaDataViewerProxy.cpp @@ -120,8 +120,12 @@ namespace MAT_NS_BEGIN } auto packet = env->NewByteArray(static_cast(packetData.size())); - if (packet == nullptr || ClearPendingException(env, "receiveData allocation")) + if (ClearPendingException(env, "receiveData allocation") || packet == nullptr) { + if (packet != nullptr) + { + env->DeleteLocalRef(packet); + } DetachIfNeeded(attached); return; } From dbb9728b8fec15a9e9ac1639d14290b8ef81796a Mon Sep 17 00:00:00 2001 From: kdhawaniya Date: Mon, 28 Sep 2026 23:28:06 +0530 Subject: [PATCH 6/8] Guard the new Android logging calls with HAVE_MAT_LOGGING liblog is only linked on Android when internal logging is enabled (lib/CMakeLists.txt), so the unconditional __android_log_print calls added for the Java IDataViewer support broke MATSDK_DISABLE_LOGGING=ON builds with undefined references at link time. Wrap the android/log.h includes and every new call site in #ifdef HAVE_MAT_LOGGING, matching the convention already used throughout LogManager_jni.cpp, and void the parameters that are only read by the log statements so the logging-disabled build stays warning free. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/jni/JavaDataViewerProxy.cpp | 16 ++++++++++++++++ lib/jni/LogManager_jni.cpp | 16 ++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/lib/jni/JavaDataViewerProxy.cpp b/lib/jni/JavaDataViewerProxy.cpp index 32045afcf..96413bae8 100644 --- a/lib/jni/JavaDataViewerProxy.cpp +++ b/lib/jni/JavaDataViewerProxy.cpp @@ -4,16 +4,26 @@ // #include "JavaDataViewerProxy.hpp" +#if defined __has_include +#if __has_include("mat/config.h") +#include "mat/config.h" +#endif +#endif + +#ifdef HAVE_MAT_LOGGING #include +#endif #include #include namespace MAT_NS_BEGIN { +#ifdef HAVE_MAT_LOGGING namespace { constexpr const char* LOG_TAG = "MAE.JavaDataViewer"; } +#endif std::shared_ptr JavaDataViewerProxy::Create( JNIEnv* env, @@ -108,7 +118,9 @@ namespace MAT_NS_BEGIN { if (packetData.size() > static_cast(std::numeric_limits::max())) { +#ifdef HAVE_MAT_LOGGING __android_log_print(ANDROID_LOG_ERROR, LOG_TAG, "Packet is too large for a Java byte array"); +#endif return; } @@ -238,11 +250,15 @@ namespace MAT_NS_BEGIN return false; } env->ExceptionClear(); +#ifdef HAVE_MAT_LOGGING __android_log_print( ANDROID_LOG_ERROR, LOG_TAG, "Java IDataViewer callback failed: %s", methodName); +#else + (void)methodName; +#endif return true; } diff --git a/lib/jni/LogManager_jni.cpp b/lib/jni/LogManager_jni.cpp index e5fe35351..526943430 100644 --- a/lib/jni/LogManager_jni.cpp +++ b/lib/jni/LogManager_jni.cpp @@ -928,12 +928,16 @@ namespace } catch (const std::exception& exception) { +#ifdef HAVE_MAT_LOGGING __android_log_print( ANDROID_LOG_WARN, "MAE.JavaDataViewer", "Failed to unregister Java IDataViewer '%s': %s", dataViewer.first.c_str(), exception.what()); +#else + (void)exception; +#endif } } } @@ -1624,20 +1628,28 @@ Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_na } catch (const std::exception& rollbackException) { +#ifdef HAVE_MAT_LOGGING __android_log_print( ANDROID_LOG_ERROR, "MAE.JavaDataViewer", "Failed to roll back Java IDataViewer '%s': %s", proxy->GetName(), rollbackException.what()); +#else + (void)rollbackException; +#endif } } +#ifdef HAVE_MAT_LOGGING __android_log_print( ANDROID_LOG_WARN, "MAE.JavaDataViewer", "Failed to register Java IDataViewer '%s': %s", proxy->GetName(), exception.what()); +#else + (void)exception; +#endif return false; } } @@ -1683,12 +1695,16 @@ Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_na } catch (const std::exception& exception) { +#ifdef HAVE_MAT_LOGGING __android_log_print( ANDROID_LOG_WARN, "MAE.JavaDataViewer", "Failed to unregister Java IDataViewer '%s': %s", name.c_str(), exception.what()); +#else + (void)exception; +#endif return false; } } From c7ad116f7be621d97443bfc636d868fe565d1dba Mon Sep 17 00:00:00 2001 From: kdhawaniya Date: Tue, 29 Sep 2026 00:16:53 +0530 Subject: [PATCH 7/8] Compare data viewer names by value when unregistering UnregisterViewer compared the two const char* operands with ==, which compares addresses rather than characters, while the sibling lookup in GetViewerFromCollection uses strcmp. Unregistration therefore only matched when the caller passed back the exact pointer the viewer returns from GetName(). Every pre-existing caller did exactly that, so the defect stayed latent. The Java IDataViewer bridge adds the first callers that build the name independently of the viewer object - it arrives as a Java string - so the lookup fails, UnregisterViewer throws, and the proxy is left registered and still receiving packets after unregisterDataViewer or close. RegisterViewer already rejects duplicates by value through the same strcmp lookup, so no two registered viewers can share a name and this makes unregistration symmetric with the check that guards registration. Existing callers are unaffected: identical pointers necessarily have identical contents. Also include explicitly rather than relying on a transitive include for the strcmp that this file already used, and cover the case with a unit test that unregisters through a separately allocated name. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/api/DataViewerCollection.cpp | 3 ++- tests/unittests/DataViewerCollectionTests.cpp | 15 +++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/lib/api/DataViewerCollection.cpp b/lib/api/DataViewerCollection.cpp index addd2e832..60479591a 100644 --- a/lib/api/DataViewerCollection.cpp +++ b/lib/api/DataViewerCollection.cpp @@ -4,6 +4,7 @@ // #include "DataViewerCollection.hpp" #include +#include #include namespace MAT_NS_BEGIN { @@ -67,7 +68,7 @@ namespace MAT_NS_BEGIN { LOCKGUARD(m_dataViewerMapLock); auto toErase = std::find_if(m_dataViewerCollection.begin(), m_dataViewerCollection.end(), [&viewerName](std::shared_ptr viewer) { - return viewer->GetName() == viewerName; + return strcmp(viewer->GetName(), viewerName) == 0; }); if (toErase == m_dataViewerCollection.end()) diff --git a/tests/unittests/DataViewerCollectionTests.cpp b/tests/unittests/DataViewerCollectionTests.cpp index 33b51916c..7c27b679e 100644 --- a/tests/unittests/DataViewerCollectionTests.cpp +++ b/tests/unittests/DataViewerCollectionTests.cpp @@ -134,6 +134,21 @@ TEST(DataViewerCollectionTests, UnregisterViewer_ViewerNameIsRegistered_Unregist ASSERT_TRUE(dataViewerCollection.GetCollection().empty()); } +TEST(DataViewerCollectionTests, UnregisterViewer_ViewerNameMatchesByValue_UnregistersCorrectly) +{ + std::shared_ptr viewer = std::make_shared("sharedName", /*isTransmissionEnabled*/ false); + TestDataViewerCollection dataViewerCollection { }; + dataViewerCollection.GetCollection().push_back(viewer); + + // An equal name held at a different address: the collection must match on the characters, + // not on the pointer the viewer happens to return from GetName(). + const std::string equalName { "sharedName" }; + ASSERT_NE(equalName.c_str(), viewer->GetName()); + + ASSERT_NO_THROW(dataViewerCollection.UnregisterViewer(equalName.c_str())); + ASSERT_TRUE(dataViewerCollection.GetCollection().empty()); +} + TEST(DataViewerCollectionTests, UnregisterAllViewers_NoViewersRegistered_UnregisterCallSuccessful) { TestDataViewerCollection dataViewerCollection { }; From 7682ae211d92ff987f8803522f7654b369fdb029 Mon Sep 17 00:00:00 2001 From: kdhawaniya Date: Tue, 29 Sep 2026 00:42:08 +0530 Subject: [PATCH 8/8] Evaluate viewer transmission state outside the data viewer lock IsViewerEnabled() called IsTransmissionEnabled() on every viewer while holding m_dataViewerMapLock. For a Java viewer that is a JNI call into the JVM, so the native to Java edge that the earlier dispatch fix removed from ReceiveData still existed here: registration takes the JNI viewer mutex and then this lock, so the cycle survived, and a slow Java callback still stalled registration, unregistration and LogManager close. IsViewerEnabled() is public on IDataViewerCollection, so fixing only the internal caller would have left the cycle open to external callers. Both functions now snapshot the collection under the lock and evaluate the predicate after releasing it. DispatchDataViewerEvent reuses its existing snapshot instead of calling IsViewerEnabled() first, which also removes a second lock acquisition: the set of viewers the enabled decision was made about was not necessarily the set that was dispatched to. std::any_of on an empty range returns false, matching the previous empty() plus find_if formulation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/api/DataViewerCollection.cpp | 31 +++++++++++++++++++++++++------ 1 file changed, 25 insertions(+), 6 deletions(-) diff --git a/lib/api/DataViewerCollection.cpp b/lib/api/DataViewerCollection.cpp index 60479591a..ffb8a3a19 100644 --- a/lib/api/DataViewerCollection.cpp +++ b/lib/api/DataViewerCollection.cpp @@ -13,9 +13,6 @@ namespace MAT_NS_BEGIN { void DataViewerCollection::DispatchDataViewerEvent(const std::vector& packetData) const noexcept { - if (IsViewerEnabled() == false) - return; - // Dispatch over a snapshot taken under the lock, and release the lock before invoking any // viewer. Iterating m_dataViewerCollection directly is unsafe because m_dataViewerMapLock // is recursive: a viewer that reenters the SDK from ReceiveData - for example by closing @@ -32,6 +29,17 @@ namespace MAT_NS_BEGIN { viewers = m_dataViewerCollection; } + // The enabled check runs on this same snapshot, outside the lock, for two reasons. + // IsTransmissionEnabled() is a viewer callback - a JNI call for Java viewers - and must not + // run under m_dataViewerMapLock for the reasons above. Reusing the one snapshot also means + // the set of viewers this decision is made about is the set that is dispatched to; calling + // IsViewerEnabled() first would lock a second time and could decide on a different set. + // Keep this predicate in sync with IsViewerEnabled(). + const bool anyEnabled = std::any_of(viewers.cbegin(), viewers.cend(), + [](const std::shared_ptr& viewer) { return viewer->IsTransmissionEnabled(); }); + if (!anyEnabled) + return; + for(const auto& viewer : viewers) { // Task 3568800: Integrate ThreadPool to IDataViewerCollection @@ -95,9 +103,20 @@ namespace MAT_NS_BEGIN { bool DataViewerCollection::IsViewerEnabled() const noexcept { - LOCKGUARD(m_dataViewerMapLock); - return !m_dataViewerCollection.empty() && - std::find_if(m_dataViewerCollection.begin(), m_dataViewerCollection.end(), [](std::shared_ptr viewer) { return viewer->IsTransmissionEnabled(); }) != m_dataViewerCollection.end(); + // Evaluate over a snapshot taken under the lock. IsTransmissionEnabled() is a viewer + // callback - for Java viewers it crosses into the JVM - and must not run while + // m_dataViewerMapLock is held: registration takes the JNI viewer mutex and then this lock, + // so a callback that reenters the SDK would close a lock cycle, and a slow callback would + // stall registration, unregistration and LogManager close. + // Keep this predicate in sync with DispatchDataViewerEvent(). + std::vector> viewers; + { + LOCKGUARD(m_dataViewerMapLock); + viewers = m_dataViewerCollection; + } + + return std::any_of(viewers.cbegin(), viewers.cend(), + [](const std::shared_ptr& viewer) { return viewer->IsTransmissionEnabled(); }); } bool DataViewerCollection::IsViewerRegistered(const char* viewerName) const