From 866832bb378d36cff1c4a0351b6b237963d75335 Mon Sep 17 00:00:00 2001 From: kdhawaniya Date: Sat, 12 Sep 2026 02:33:21 +0530 Subject: [PATCH 01/19] [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 02/19] 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 03/19] 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 04/19] 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 05/19] 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 06/19] 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 07/19] 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 08/19] 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 From e724319d194efafc721842d0f73e121e2d16e0ec Mon Sep 17 00:00:00 2001 From: kdhawaniya Date: Tue, 29 Sep 2026 12:39:35 +0530 Subject: [PATCH 09/19] Gate data viewer dispatch per viewer and release viewers on teardown DispatchDataViewerEvent checked whether any viewer was enabled and then called ReceiveData on every viewer. DefaultDataViewer re-checks IsTransmissionEnabled() at the top of its own ReceiveData, so the collection-wide test was only a short-circuit and a disabled C++ viewer discarded the packet itself. JavaDataViewerProxy has no equivalent guard, so a disabled Java viewer was handed encoded telemetry whenever any other viewer was enabled, contradicting the documented contract of IDataViewer.isTransmissionEnabled(). The check now runs per viewer on the existing snapshot, which replaces the collection-wide scan rather than adding to it and skips the byte array allocation and JNI call for viewers that are not accepting data. nativeFlushAndTeardown only called ILogManager::FlushAndTeardown(). That is terminal - LogManagerImpl sets m_alive to false and GetLogger() returns nullptr from then on, and nothing sets it back - but it does not unregister data viewers, so the proxies stayed in the native collection and in the javaDataViewers map. Their JNI global references pinned the application's IDataViewer objects, and everything those referenced, until close() or process exit even though no further callback could occur. Teardown now releases them too, ordered after the flush so viewers still observe the final packets. closeJavaDataViewers clears its bookkeeping under the lock and returns early once the manager pointer is null, so a later close() is a no-op. It also nulls that manager pointer, so a post-teardown flush or getLogger now returns early instead of entering a dead manager. Tests: mixed enabled/disabled and no-viewer-enabled dispatch cases, a Java gating test, and a Java test that flushAndTeardown releases a registered viewer without close(). All four fail without the corresponding fix. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../maesdktest/LogManagerDDVUnitTest.java | 146 ++++++++++++++++++ .../applications/events/IDataViewer.java | 7 +- lib/api/DataViewerCollection.cpp | 26 ++-- lib/jni/LogManager_jni.cpp | 14 ++ tests/unittests/DataViewerCollectionTests.cpp | 32 ++++ 5 files changed, 212 insertions(+), 13 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 a9dd729ee..cb8e4188c 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 @@ -364,6 +364,152 @@ public String getCurrentEndpoint() { } } + @Test + public void flushAndTeardown_withRegisteredDataViewer_releasesViewerWithoutClose() + 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 = "JavaDataViewerTeardown" + 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); + IDataViewer viewer = + new IDataViewer() { + @Override + public void receiveData(byte[] packetData) {} + + @Override + public String getName() { + return "teardown-viewer"; + } + + @Override + public boolean isTransmissionEnabled() { + return true; + } + + @Override + public String getCurrentEndpoint() { + return ""; + } + }; + + try { + assertThat(manager.registerDataViewer(viewer), is(true)); + + // flushAndTeardown is terminal - the native LogManagerImpl clears m_alive and never + // revives - so the viewer must be released here rather than held until close(). If the + // proxy and its JNI global reference were retained, the viewer would still be registered + // and this unregister would succeed. + manager.flushAndTeardown(); + + assertThat(manager.unregisterDataViewer("teardown-viewer"), is(false)); + } finally { + manager.close(); + } + } + + @Test + public void registerDataViewer_whenOneViewerDisabled_dispatchesOnlyToEnabledViewer() + 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 = "JavaDataViewerGating" + 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 enabledReceived = new CountDownLatch(1); + AtomicInteger enabledCalls = new AtomicInteger(); + AtomicInteger disabledCalls = new AtomicInteger(); + IDataViewer enabledViewer = + new IDataViewer() { + @Override + public void receiveData(byte[] packetData) { + enabledCalls.incrementAndGet(); + enabledReceived.countDown(); + } + + @Override + public String getName() { + return "enabled-viewer"; + } + + @Override + public boolean isTransmissionEnabled() { + return true; + } + + @Override + public String getCurrentEndpoint() { + return "http://127.0.0.1"; + } + }; + IDataViewer disabledViewer = + new IDataViewer() { + @Override + public void receiveData(byte[] packetData) { + disabledCalls.incrementAndGet(); + } + + @Override + public String getName() { + return "disabled-viewer"; + } + + @Override + public boolean isTransmissionEnabled() { + return false; + } + + @Override + public String getCurrentEndpoint() { + return ""; + } + }; + + try { + assertThat(manager.registerDataViewer(enabledViewer), is(true)); + assertThat(manager.registerDataViewer(disabledViewer), is(true)); + + ILogger logger = manager.getLogger(token, "java-data-viewer-gating", ""); + logger.logEvent("javaDataViewerGating"); + manager.uploadNow(); + + // The enabled viewer is the witness that a dispatch really happened; the disabled viewer + // must not be handed telemetry just because another viewer is enabled. + assertThat(enabledReceived.await(5, TimeUnit.SECONDS), is(true)); + assertThat(enabledCalls.get(), greaterThan(0)); + assertThat(disabledCalls.get(), is(0)); + + assertThat(manager.unregisterDataViewer("enabled-viewer"), is(true)); + assertThat(manager.unregisterDataViewer("disabled-viewer"), is(true)); + } finally { + manager.close(); + } + } + /* Disabling this test since it requires private modules. 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 36503859a..8a5de4308 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 @@ -24,7 +24,12 @@ public interface IDataViewer { /** Returns the stable, unique name used to register this viewer. */ String getName(); - /** Returns whether this viewer is currently accepting packet callbacks. */ + /** + * Returns whether this viewer is currently accepting packet callbacks. + * + *

The SDK queries this before every dispatch and does not call {@link #receiveData} while it + * returns {@code false}, even when another registered viewer is enabled. + */ boolean isTransmissionEnabled(); /** Returns the endpoint currently used by this viewer, or an empty string when disabled. */ diff --git a/lib/api/DataViewerCollection.cpp b/lib/api/DataViewerCollection.cpp index ffb8a3a19..1108f14d6 100644 --- a/lib/api/DataViewerCollection.cpp +++ b/lib/api/DataViewerCollection.cpp @@ -29,19 +29,22 @@ 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; - + // Gate each viewer individually. The previous collection-wide check was only a + // short-circuit: DefaultDataViewer re-checks IsTransmissionEnabled() at the top of its own + // ReceiveData, so a disabled viewer discarded the packet itself. JavaDataViewerProxy cannot + // do that cheaply - it would have to cross into the JVM a second time - and + // IDataViewer.isTransmissionEnabled() documents per-viewer suppression, so a disabled Java + // viewer would otherwise be handed encoded telemetry whenever any other viewer was enabled. + // Checking here is also cheaper than it looks: it replaces the collection-wide scan rather + // than adding to it, and it skips the byte array allocation and JNI call for viewers that + // are not accepting data. for(const auto& viewer : viewers) { + if (!viewer->IsTransmissionEnabled()) + { + continue; + } + // Task 3568800: Integrate ThreadPool to IDataViewerCollection viewer->ReceiveData(packetData); } @@ -108,7 +111,6 @@ namespace MAT_NS_BEGIN { // 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); diff --git a/lib/jni/LogManager_jni.cpp b/lib/jni/LogManager_jni.cpp index 526943430..b08d8cd72 100644 --- a/lib/jni/LogManager_jni.cpp +++ b/lib/jni/LogManager_jni.cpp @@ -1140,6 +1140,20 @@ Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_na return; } logManager->FlushAndTeardown(); + + // FlushAndTeardown is terminal: LogManagerImpl sets m_alive to false and GetLogger() returns + // nullptr from then on, and nothing sets it back, so no further viewer callback can occur. + // It does not unregister data viewers, so without this the proxies stay in the native + // collection and in the javaDataViewers map, and their JNI global references pin the + // application's IDataViewer objects - and everything those reference - until close() or + // process exit. Release them here as well; closeJavaDataViewers clears its bookkeeping under + // the lock and returns early once the manager pointer is null, so a later close() is a safe + // no-op. Ordered after the teardown so viewers still observe packets from the final flush. + auto managerAndConfig = getManagerAndConfig(nativeLogManager); + if (managerAndConfig != nullptr) + { + closeJavaDataViewers(*managerAndConfig); + } } extern "C" JNIEXPORT jint JNICALL diff --git a/tests/unittests/DataViewerCollectionTests.cpp b/tests/unittests/DataViewerCollectionTests.cpp index 7c27b679e..3728ba827 100644 --- a/tests/unittests/DataViewerCollectionTests.cpp +++ b/tests/unittests/DataViewerCollectionTests.cpp @@ -341,3 +341,35 @@ TEST(DataViewerCollectionTests, DispatchDataViewerEvent_ViewerUnregistersAllFrom ASSERT_TRUE(dataViewerCollection.GetCollection().empty()); } +TEST(DataViewerCollectionTests, DispatchDataViewerEvent_MixedEnabledAndDisabledViewers_DispatchesOnlyToEnabled) +{ + TestDataViewerCollection dataViewerCollection { }; + auto enabledViewer = std::make_shared("EnabledViewer", /*isTransmissionEnabled*/ true); + auto disabledViewer = std::make_shared("DisabledViewer", /*isTransmissionEnabled*/ false); + + dataViewerCollection.RegisterViewer(enabledViewer); + dataViewerCollection.RegisterViewer(disabledViewer); + + const std::vector packetData { 1, 2, 3 }; + dataViewerCollection.DispatchDataViewerEvent(packetData); + + // Gating is per viewer: one enabled viewer must not cause delivery to a disabled one. + ASSERT_EQ(enabledViewer->localPacketData, packetData); + ASSERT_TRUE(disabledViewer->localPacketData.empty()); +} + +TEST(DataViewerCollectionTests, DispatchDataViewerEvent_NoViewerEnabled_DispatchesToNobody) +{ + TestDataViewerCollection dataViewerCollection { }; + auto firstViewer = std::make_shared("FirstViewer", /*isTransmissionEnabled*/ false); + auto secondViewer = std::make_shared("SecondViewer", /*isTransmissionEnabled*/ false); + + dataViewerCollection.RegisterViewer(firstViewer); + dataViewerCollection.RegisterViewer(secondViewer); + + dataViewerCollection.DispatchDataViewerEvent(std::vector { 1, 2, 3 }); + + ASSERT_TRUE(firstViewer->localPacketData.empty()); + ASSERT_TRUE(secondViewer->localPacketData.empty()); +} + From 4ce01f2ca49183c8314c3708d0f7499a21e0bf1d Mon Sep 17 00:00:00 2001 From: kdhawaniya Date: Wed, 30 Sep 2026 15:04:10 +0530 Subject: [PATCH 10/19] Document the data viewer callback contract Dispatch takes a snapshot and invokes viewers without holding the collection lock, so unregister and close return without waiting for a callback already in progress. Record that guarantee and its consequences on the public surfaces rather than coordinating removal with dispatch, which would reinstate the lock cycle the snapshot exists to break. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../applications/events/IDataViewer.java | 28 +++++++++++++------ .../applications/events/ILogManager.java | 8 ++++++ lib/api/DataViewerCollection.cpp | 7 +++++ lib/include/public/IDataViewerCollection.hpp | 13 +++++++++ 4 files changed, 48 insertions(+), 8 deletions(-) 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 8a5de4308..2978b7b16 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 @@ -9,11 +9,17 @@ /** * 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 - * 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. + *

Implementations must return a stable, unique name for the lifetime of the registration, and + * must be thread-safe: the SDK does not serialize callbacks, so {@link #receiveData} and {@link + * #isTransmissionEnabled()} may run concurrently on the same viewer. Callbacks occur on an SDK + * worker thread and should return promptly. + * + *

Unregistering and {@link ILogManager#close} do not wait for a callback already in progress, + * so a viewer may receive one final packet after removal returns. The SDK keeps the viewer alive + * for that callback, so implementations need only discard it. + * + *

Implementations must not reenter the SDK from within a callback: do not register or + * unregister viewers, and do not close the owning {@link ILogManager}. */ @Keep public interface IDataViewer { @@ -21,14 +27,20 @@ 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. */ + /** + * Returns the stable, unique name used to register this viewer. + * + *

May be called while the SDK holds an internal lock: return a precomputed value and do not + * call back into the SDK. + */ String getName(); /** * Returns whether this viewer is currently accepting packet callbacks. * - *

The SDK queries this before every dispatch and does not call {@link #receiveData} while it - * returns {@code false}, even when another registered viewer is enabled. + *

The SDK queries this before each dispatch and skips {@link #receiveData} while it returns + * {@code false}, regardless of other viewers. Because the query and the delivery are separate + * calls, a viewer disabled concurrently with a dispatch under way may still receive that packet. */ boolean isTransmissionEnabled(); 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 9a90d7eb1..ec6e655a5 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 @@ -13,6 +13,12 @@ public interface ILogManager extends AutoCloseable { public ILogConfiguration getLogConfigurationCopy(); + /** + * Flushes pending telemetry and tears down this LogManager. + * + *

Stops the telemetry pipeline before releasing registered data viewers, so no viewer + * callback is left outstanding and a separate {@link #close} is not needed to release them. + */ public void flushAndTeardown(); public Status flush(); @@ -80,6 +86,8 @@ default boolean registerDataViewer(IDataViewer dataViewer) { * existing implementations of this interface remain source compatible; implementations that * support data viewers override it. * + *

Does not wait for a callback already in progress; see {@link IDataViewer}. + * * @return {@code true} when the viewer was unregistered, {@code false} when it was not * registered, or when the implementation does not support data viewers */ diff --git a/lib/api/DataViewerCollection.cpp b/lib/api/DataViewerCollection.cpp index 1108f14d6..64e09a88a 100644 --- a/lib/api/DataViewerCollection.cpp +++ b/lib/api/DataViewerCollection.cpp @@ -23,6 +23,13 @@ namespace MAT_NS_BEGIN { // 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. + // + // Removal is deliberately not coordinated with in-flight dispatch: a viewer unregistered + // after this snapshot is taken still receives this packet. Do not "fix" that by making + // UnregisterViewer wait for outstanding callbacks - that reinstates the cycle this + // snapshot exists to break, because the unregistering thread would block on a callback + // that may in turn be waiting on a lock that thread holds. IDataViewer documents the + // resulting contract for consumers. std::vector> viewers; { LOCKGUARD(m_dataViewerMapLock); diff --git a/lib/include/public/IDataViewerCollection.hpp b/lib/include/public/IDataViewerCollection.hpp index d797230bc..005102839 100644 --- a/lib/include/public/IDataViewerCollection.hpp +++ b/lib/include/public/IDataViewerCollection.hpp @@ -23,6 +23,11 @@ namespace MAT_NS_BEGIN /// Dispatch a Data Viewer Event to all viewers in the collection. /// /// Data packet to be passed to all viewers. + /// + /// Viewers are invoked without holding the collection lock: implementations must be + /// thread-safe, and a viewer unregistered concurrently may still receive that packet. + /// The collection holds a shared_ptr for the duration of each callback. + /// virtual void DispatchDataViewerEvent(const std::vector& packetData) const noexcept = 0; ///

@@ -37,11 +42,19 @@ namespace MAT_NS_BEGIN /// /// Unique Name to identify the viewer that should be unregistered from the IDataViewerCollection. /// + /// + /// Does not wait for callbacks already in progress; a viewer may observe one more packet + /// after this returns. + /// virtual void UnregisterViewer(const char* viewerName) = 0; /// /// Unregister all registered IDataViewers. /// + /// + /// Does not wait for callbacks already in progress; a viewer may observe one more packet + /// after this returns. + /// virtual void UnregisterAllViewers() = 0; /// From afc6fe9a9e660fdca9b073e4343631ba99c02138 Mon Sep 17 00:00:00 2001 From: kdhawaniya Date: Wed, 30 Sep 2026 15:14:54 +0530 Subject: [PATCH 11/19] Do not let the viewer snapshot allocate out of a noexcept path DispatchDataViewerEvent and IsViewerEnabled are noexcept, and both copy the viewer collection to avoid invoking viewers under the lock. That copy allocates, so bad_alloc - or a system_error from the lock - would terminate the process instead of costing one packet of diagnostic data. Route both through a guarded helper that reports failure so the caller skips the dispatch. MATSDK_TRY/MATSDK_CATCH degrade to if(true)/if(false) where exceptions are disabled, so the build without C++ exceptions is unaffected. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/api/DataViewerCollection.cpp | 28 ++++++++++++++++++++++++---- lib/api/DataViewerCollection.hpp | 7 +++++++ 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/lib/api/DataViewerCollection.cpp b/lib/api/DataViewerCollection.cpp index 64e09a88a..397082828 100644 --- a/lib/api/DataViewerCollection.cpp +++ b/lib/api/DataViewerCollection.cpp @@ -11,6 +11,26 @@ namespace MAT_NS_BEGIN { MATSDK_LOG_INST_COMPONENT_CLASS(DataViewerCollection, "EventsSDK.DataViewerCollection", "Microsoft Telemetry Client - DataViewerCollection class") + bool DataViewerCollection::TrySnapshotViewers(std::vector>& viewers) const noexcept + { + // Both callers are noexcept, and copying the collection allocates. An uncaught bad_alloc + // - or a system_error from the lock - would terminate the process rather than cost one + // packet of diagnostic data, so report failure and let the caller skip instead. Nothing + // is logged from the failure path: under memory pressure the log call could throw in + // turn, which is the outcome this guard exists to prevent. + MATSDK_TRY + { + LOCKGUARD(m_dataViewerMapLock); + viewers = m_dataViewerCollection; + } + MATSDK_CATCH(...) + { + return false; + } + + return true; + } + void DataViewerCollection::DispatchDataViewerEvent(const std::vector& packetData) const noexcept { // Dispatch over a snapshot taken under the lock, and release the lock before invoking any @@ -31,9 +51,9 @@ namespace MAT_NS_BEGIN { // that may in turn be waiting on a lock that thread holds. IDataViewer documents the // resulting contract for consumers. std::vector> viewers; + if (!TrySnapshotViewers(viewers)) { - LOCKGUARD(m_dataViewerMapLock); - viewers = m_dataViewerCollection; + return; } // Gate each viewer individually. The previous collection-wide check was only a @@ -119,9 +139,9 @@ namespace MAT_NS_BEGIN { // so a callback that reenters the SDK would close a lock cycle, and a slow callback would // stall registration, unregistration and LogManager close. std::vector> viewers; + if (!TrySnapshotViewers(viewers)) { - LOCKGUARD(m_dataViewerMapLock); - viewers = m_dataViewerCollection; + return false; } return std::any_of(viewers.cbegin(), viewers.cend(), diff --git a/lib/api/DataViewerCollection.hpp b/lib/api/DataViewerCollection.hpp index 6d014af72..8676a03f1 100644 --- a/lib/api/DataViewerCollection.hpp +++ b/lib/api/DataViewerCollection.hpp @@ -37,6 +37,13 @@ namespace MAT_NS_BEGIN { mutable std::recursive_mutex m_dataViewerMapLock; + /// + /// Copy the registered viewers under the lock, for callers that must invoke viewers + /// without holding it. + /// + /// false when the snapshot could not be taken; viewers is then unusable. + bool TrySnapshotViewers(std::vector>& viewers) const noexcept; + protected: std::shared_ptr GetViewerFromCollection(const char* viewerName) const; std::vector> m_dataViewerCollection; From 574737ad811cdfcb4c5aace64d33f82612919004 Mon Sep 17 00:00:00 2001 From: kdhawaniya Date: Wed, 30 Sep 2026 15:15:05 +0530 Subject: [PATCH 12/19] Assert that unregistering does not wait for an in-flight callback The snapshot dispatch is what lets UnregisterViewer return while a viewer callback is still running, and IDataViewer now documents that guarantee. Pin it down so a later change cannot quietly reintroduce the wait, and with it the deadlock the snapshot exists to prevent. The viewer parks inside ReceiveData and is still parked when unregister returns, which distinguishes a genuinely non-blocking unregister from one that merely raced a callback that had already finished. Unregister runs on a future with a timeout so a regression fails the test rather than hanging the run. Verified by reinstating the lock across the callback loop: the test fails with the intended message instead of deadlocking. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tests/unittests/DataViewerCollectionTests.cpp | 109 ++++++++++++++++++ 1 file changed, 109 insertions(+) diff --git a/tests/unittests/DataViewerCollectionTests.cpp b/tests/unittests/DataViewerCollectionTests.cpp index 3728ba827..9840cbeb6 100644 --- a/tests/unittests/DataViewerCollectionTests.cpp +++ b/tests/unittests/DataViewerCollectionTests.cpp @@ -8,6 +8,12 @@ #include "api/DataViewerCollection.hpp" #include "CheckForExceptionOrAbort.hpp" +#include +#include +#include +#include +#include + using namespace testing; using namespace MAT; @@ -373,3 +379,106 @@ TEST(DataViewerCollectionTests, DispatchDataViewerEvent_NoViewerEnabled_Dispatch ASSERT_TRUE(secondViewer->localPacketData.empty()); } +namespace +{ + // Parks inside ReceiveData until released, so a test can observe the collection while a + // viewer callback is genuinely in progress. + class BlockingDataViewer : public IDataViewer + { + public: + + explicit BlockingDataViewer(const char* name) : m_name(name) {} + + void ReceiveData(const std::vector&) noexcept override + { + std::unique_lock lock(m_mutex); + m_inCallback = true; + m_entered.notify_all(); + m_release.wait(lock, [this] { return m_released; }); + m_inCallback = false; + } + + 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; + } + + void WaitUntilInCallback() + { + std::unique_lock lock(m_mutex); + m_entered.wait(lock, [this] { return m_inCallback; }); + } + + bool IsInCallback() + { + std::lock_guard lock(m_mutex); + return m_inCallback; + } + + void Release() + { + { + std::lock_guard lock(m_mutex); + m_released = true; + } + m_release.notify_all(); + } + + private: + std::mutex m_mutex; + std::condition_variable m_entered; + std::condition_variable m_release; + bool m_inCallback { false }; + bool m_released { false }; + const char* m_name; + const std::string m_testEndpoint { "TestEndpoint" }; + }; +} + +TEST(DataViewerCollectionTests, UnregisterViewer_CallbackInProgress_ReturnsWithoutWaitingForCallback) +{ + TestDataViewerCollection dataViewerCollection { }; + auto blockingViewer = std::make_shared("BlockingViewer"); + dataViewerCollection.RegisterViewer(blockingViewer); + + std::thread dispatcher([&dataViewerCollection]() + { + dataViewerCollection.DispatchDataViewerEvent(std::vector { 1, 2, 3 }); + }); + + blockingViewer->WaitUntilInCallback(); + + // Unregistering must not wait for a callback already in progress. Making it wait would + // deadlock any consumer whose callback cannot finish until the unregistering thread does. + auto unregistered = std::async(std::launch::async, [&dataViewerCollection]() + { + dataViewerCollection.UnregisterViewer("BlockingViewer"); + }); + + const auto unregisterStatus = unregistered.wait_for(std::chrono::seconds(30)); + + // The witness: the viewer is still parked, so unregister genuinely returned early rather + // than racing a callback that had already completed. + const bool stillInCallback = blockingViewer->IsInCallback(); + const bool collectionEmptied = dataViewerCollection.GetCollection().empty(); + + // Release before asserting so a regression fails the test instead of hanging the run. + blockingViewer->Release(); + dispatcher.join(); + unregistered.get(); + + ASSERT_EQ(unregisterStatus, std::future_status::ready) << "UnregisterViewer blocked while a viewer callback was in progress"; + ASSERT_TRUE(stillInCallback); + ASSERT_TRUE(collectionEmptied); +} + From 71d4b68b65f5de02e7b10f5706ccaf8bdafabf11 Mon Sep 17 00:00:00 2001 From: kdhawaniya Date: Wed, 30 Sep 2026 19:28:50 +0530 Subject: [PATCH 13/19] Reject a torn-down manager and keep allocations off the noexcept JNI paths closeJavaDataViewers() nulls ManagerAndConfig::manager, and flushAndTeardown() now reaches it as well as close(). nativeGetLogger() dereferenced that pointer unconditionally, so a getLogger() after either call faulted natively. Capture the manager under jniManagersMutex and return 0 instead, which surfaces as the NullPointerException LogManagerImpl.getLogger() already raises for a null handle. JavaDataViewerProxy::Create() and ReadString() are noexcept but allocated: the proxy, the shared_ptr control block and the name string could each throw bad_alloc and terminate. Use nothrow new for the object, since the macros degrade to if (true)/if (false) when exceptions are disabled and a throwing new would abort uncatchably, and guard the remaining allocations. --- lib/jni/JavaDataViewerProxy.cpp | 36 +++++++++++++++++++++++++++++++-- lib/jni/LogManager_jni.cpp | 13 ++++++++---- 2 files changed, 43 insertions(+), 6 deletions(-) diff --git a/lib/jni/JavaDataViewerProxy.cpp b/lib/jni/JavaDataViewerProxy.cpp index 96413bae8..928fc9baf 100644 --- a/lib/jni/JavaDataViewerProxy.cpp +++ b/lib/jni/JavaDataViewerProxy.cpp @@ -14,6 +14,7 @@ #include #endif #include +#include #include namespace MAT_NS_BEGIN @@ -34,7 +35,24 @@ namespace MAT_NS_BEGIN return nullptr; } - auto proxy = std::shared_ptr(new JavaDataViewerProxy()); + // Create() is noexcept because it runs on a JNI entry path, so neither the object + // allocation nor the shared_ptr control block may propagate. nothrow new covers the + // first even when exceptions are disabled; the guard covers the second. + auto raw = new (std::nothrow) JavaDataViewerProxy(); + if (raw == nullptr) + { + return nullptr; + } + std::shared_ptr proxy; + MATSDK_TRY + { + proxy.reset(raw); + } + MATSDK_CATCH(...) + { + delete raw; + return nullptr; + } if (env->GetJavaVM(&proxy->m_javaVm) != JNI_OK) { return nullptr; @@ -280,9 +298,23 @@ namespace MAT_NS_BEGIN env->DeleteLocalRef(javaValue); return false; } - value.assign(chars); + // ReadString is reached from noexcept callers, so the string allocation must not escape. + bool assigned = false; + MATSDK_TRY + { + value.assign(chars); + assigned = true; + } + MATSDK_CATCH(...) + { + assigned = false; + } env->ReleaseStringUTFChars(javaValue, chars); env->DeleteLocalRef(javaValue); + if (!assigned) + { + return false; + } return !ClearPendingException(env, "string conversion"); } diff --git a/lib/jni/LogManager_jni.cpp b/lib/jni/LogManager_jni.cpp index b08d8cd72..52d73e437 100644 --- a/lib/jni/LogManager_jni.cpp +++ b/lib/jni/LogManager_jni.cpp @@ -1091,16 +1091,21 @@ Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_na env->ExceptionDescribe(); return 0; } - ManagerAndConfig* mc; + // Capture the manager under the lock rather than dereferencing the ManagerAndConfig later: + // close() and flushAndTeardown() both null it out while the Java handle stays usable, so an + // unguarded mc->manager->GetLogger() would fault. Returning 0 here surfaces as the + // NullPointerException that LogManagerImpl.getLogger() already raises for a null handle. + ILogManager* manager = nullptr; { std::lock_guard lock(jniManagersMutex); if (nativeLogManagerIndex < 0 || nativeLogManagerIndex >= static_cast(jniManagers.size())) { return 0; } - mc = jniManagers[nativeLogManagerIndex].get(); - if (!mc) + auto mc = jniManagers[nativeLogManagerIndex].get(); + if (!mc || mc->manager == nullptr) return 0; + manager = mc->manager; } std::string token; std::string source; @@ -1111,7 +1116,7 @@ Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_na { return 0; } - return reinterpret_cast(mc->manager->GetLogger( + return reinterpret_cast(manager->GetLogger( token, source, scope)); From a7cf5d0a9c261a8462fbb83cfdb1671505847412 Mon Sep 17 00:00:00 2001 From: kdhawaniya Date: Thu, 1 Oct 2026 01:11:40 +0530 Subject: [PATCH 14/19] Fix a double free in the proxy guard and guard six more JNI entry points shared_ptr::reset(p) is specified as shared_ptr(p).swap(*this), and that constructor deletes p if the control block allocation throws. The catch handler deleted the same pointer a second time. Drop the manual delete: reset() already owns the failure path, so the guard alone is correct. The nothrow new this replaces only mattered with exceptions disabled, which this target cannot build - LogManager_jni.cpp shares the source list and has raw throws. The privacy guard, signals and sanitizer register/unregister entry points checked only their helper pointer and dereferenced logManager unguarded, so a call after close() - and now after flushAndTeardown() - faulted. Check the manager as the neighbouring entry points in this file already do. --- lib/jni/JavaDataViewerProxy.cpp | 18 +++++++----------- lib/jni/LogManager_jni.cpp | 12 ++++++------ 2 files changed, 13 insertions(+), 17 deletions(-) diff --git a/lib/jni/JavaDataViewerProxy.cpp b/lib/jni/JavaDataViewerProxy.cpp index 928fc9baf..c477dbbe2 100644 --- a/lib/jni/JavaDataViewerProxy.cpp +++ b/lib/jni/JavaDataViewerProxy.cpp @@ -14,7 +14,6 @@ #include #endif #include -#include #include namespace MAT_NS_BEGIN @@ -35,22 +34,19 @@ namespace MAT_NS_BEGIN return nullptr; } - // Create() is noexcept because it runs on a JNI entry path, so neither the object - // allocation nor the shared_ptr control block may propagate. nothrow new covers the - // first even when exceptions are disabled; the guard covers the second. - auto raw = new (std::nothrow) JavaDataViewerProxy(); - if (raw == nullptr) - { - return nullptr; - } + // Create() is noexcept, so the allocation must not propagate. reset() takes + // ownership immediately: if the control block allocation throws, it deletes the + // object itself, so there is nothing to release here - and an explicit delete + // would be a double free. Past the guard proxy is non-null, because the throwing + // form of new never returns null; a nothrow new here would need a null check + // before the dereference below. std::shared_ptr proxy; MATSDK_TRY { - proxy.reset(raw); + proxy.reset(new JavaDataViewerProxy()); } MATSDK_CATCH(...) { - delete raw; return nullptr; } if (env->GetJavaVM(&proxy->m_javaVm) != JNI_OK) diff --git a/lib/jni/LogManager_jni.cpp b/lib/jni/LogManager_jni.cpp index 52d73e437..d124adaed 100644 --- a/lib/jni/LogManager_jni.cpp +++ b/lib/jni/LogManager_jni.cpp @@ -2273,7 +2273,7 @@ Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_na #if HAS_PG auto logManager = getLogManager(native_log_manager); auto pg = PrivacyGuardHelper::GetPrivacyGuardPtr(); - if(pg != nullptr) { + if(logManager != nullptr && pg != nullptr) { logManager->SetDataInspector(pg); return true; } @@ -2290,7 +2290,7 @@ Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_na #if HAS_SS auto logManager = getLogManager(native_log_manager); auto ss = SignalsHelper::GetSignalsInspector(); - if(ss != nullptr) { + if(logManager != nullptr && ss != nullptr) { logManager->SetDataInspector(ss); return true; } @@ -2307,7 +2307,7 @@ Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_na #if HAS_SAN auto logManager = getLogManager(native_log_manager); auto sa = SanitizerHelper::GetSanitizerPtr(); - if (sa != nullptr) { + if (logManager != nullptr && sa != nullptr) { logManager->SetDataInspector(sa); return true; } @@ -2384,7 +2384,7 @@ Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_na #if HAS_PG auto logManager = getLogManager(native_log_manager); auto pg = PrivacyGuardHelper::GetPrivacyGuardPtr(); - if(pg != nullptr) { + if(logManager != nullptr && pg != nullptr) { logManager->RemoveDataInspector(pg->GetName()); return true; } @@ -2401,7 +2401,7 @@ Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_na #if HAS_SS auto logManager = getLogManager(native_log_manager); auto ss = SignalsHelper::GetSignalsInspector(); - if(ss != nullptr) { + if(logManager != nullptr && ss != nullptr) { logManager->RemoveDataInspector(ss->GetName()); return true; } @@ -2418,7 +2418,7 @@ Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_na #if HAS_SAN auto logManager = getLogManager(native_log_manager); auto sa = SanitizerHelper::GetSanitizerPtr(); - if (sa != nullptr) { + if (logManager != nullptr && sa != nullptr) { logManager->RemoveDataInspector(sa->GetName()); return true; } From 40a868afeaf1483919b333ab1bc5f31f415c5296 Mon Sep 17 00:00:00 2001 From: kdhawaniya Date: Thu, 1 Oct 2026 01:46:24 +0530 Subject: [PATCH 15/19] Add JavaDataViewerProxy.cpp to the Soong build The source was added to lib/CMakeLists.txt but not to Android.bp, and LogManager_jni.cpp calls JavaDataViewerProxy::Create, so the libmaesdk Soong target would fail to link with unresolved symbols. The other lib/jni sources missing from Android.bp are conditional on optional modules in CMake; this one is in the unconditional list. --- Android.bp | 1 + 1 file changed, 1 insertion(+) diff --git a/Android.bp b/Android.bp index 78ec0c65d..b9e55a15c 100644 --- a/Android.bp +++ b/Android.bp @@ -45,6 +45,7 @@ cc_library_shared { "lib/http/HttpClientManager.cpp", "lib/http/HttpRequestEncoder.cpp", "lib/http/HttpResponseDecoder.cpp", + "lib/jni/JavaDataViewerProxy.cpp", "lib/jni/JniConvertors.cpp", "lib/jni/LogManager_jni.cpp", "lib/jni/Logger_jni.cpp", From a552641a4c75a35a7ddb7d12a959b89153ee8ff6 Mon Sep 17 00:00:00 2001 From: kdhawaniya Date: Thu, 1 Oct 2026 11:30:36 +0530 Subject: [PATCH 16/19] Keep the LogManager handle live across flushAndTeardown closeJavaDataViewers() retired the native manager pointer, and this branch newly calls it from flushAndTeardown(), so teardown inherited close()'s handle-retiring semantics. That left the still-open Java LogManager handing out a SemanticContext wrapping pointer 0 - SemanticContext_jni dereferences it with no null check in any method - and made nativeRemoveEventListener skip RemoveEventListener while erasing its bookkeeping, dropping the last shared_ptr to a listener the ILogManager still held by raw reference. Retirement now belongs to close() alone, via a retireManager flag on closeJavaDataViewers(). It stays inside the javaDataViewersMutex critical section that hands off the viewer map: nativeRegisterDataViewer checks manager and inserts under that lock only, so retiring outside it would let a registration land between the handoff and the retirement, leaving a viewer registered with no bookkeeping entry that keeps its JNI global reference and keeps receiving callbacks after close() returned. Registration captures manager once into a local under that lock, and LogManagerImpl.getSemanticContext() now rejects a null native pointer the way getLogger() already does instead of wrapping it. Covered by flushAndTeardown_withRegisteredDataViewer_keepsManagerApisUsableUntilClose, which asserts the manager stays usable after teardown and that close() still retires it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../maesdktest/LogManagerDDVUnitTest.java | 68 +++++++++++++++++++ .../events/LogManagerProvider.java | 6 +- lib/jni/LogManager_jni.cpp | 45 ++++++++---- 3 files changed, 104 insertions(+), 15 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 cb8e4188c..676d645f8 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 @@ -420,6 +420,74 @@ public String getCurrentEndpoint() { } } + @Test + public void flushAndTeardown_withRegisteredDataViewer_keepsManagerApisUsableUntilClose() + 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 = "JavaDataViewerTeardownApis" + 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); + IDataViewer viewer = + new IDataViewer() { + @Override + public void receiveData(byte[] packetData) {} + + @Override + public String getName() { + return "teardown-apis-viewer"; + } + + @Override + public boolean isTransmissionEnabled() { + return true; + } + + @Override + public String getCurrentEndpoint() { + return ""; + } + }; + + try { + assertThat(manager.registerDataViewer(viewer), is(true)); + + manager.flushAndTeardown(); + + // Releasing the viewers must not retire the LogManager handle - only close() does that. + // getSemanticContext() wraps whatever the native layer returns, and every method on that + // wrapper dereferences the pointer without checking it, so a handle retired here would + // hand back a wrapper around 0 that faults natively on first use. + assertThat(manager.getSemanticContext(), is(notNullValue())); + + // The viewer is still released by teardown, as the sibling test asserts. + assertThat(manager.unregisterDataViewer("teardown-apis-viewer"), is(false)); + } finally { + manager.close(); + } + + // close() does retire the handle, and the Java layer surfaces that as an exception rather + // than wrapping the null native pointer. + try { + manager.getSemanticContext(); + fail("getSemanticContext() should throw once the LogManager is closed"); + } catch (NullPointerException expected) { + // expected + } + } + @Test public void registerDataViewer_whenOneViewerDisabled_dispatchesOnlyToEnabledViewer() throws Exception { 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 777f47d73..3a5c3f339 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 @@ -132,7 +132,11 @@ public String getTransmitProfileName() { @Override public ISemanticContext getSemanticContext() { - return new SemanticContext(nativeGetSemanticContext(nativeLogManager)); + long nativeSemanticContext = nativeGetSemanticContext(nativeLogManager); + if (nativeSemanticContext == 0) { + throw new NullPointerException("Null native semantic context pointer"); + } + return new SemanticContext(nativeSemanticContext); } protected native int nativeSetContextString( diff --git a/lib/jni/LogManager_jni.cpp b/lib/jni/LogManager_jni.cpp index d124adaed..c48047b1a 100644 --- a/lib/jni/LogManager_jni.cpp +++ b/lib/jni/LogManager_jni.cpp @@ -902,16 +902,26 @@ namespace return jniManagers[nativeLogManager].get(); } - void closeJavaDataViewers(ManagerAndConfig& managerAndConfig) + // retireManager distinguishes the two callers: close() retires the handle, flushAndTeardown() + // does not. Retirement must happen here rather than in a separate critical section, because + // nativeRegisterDataViewer checks manager and inserts into javaDataViewers while holding only + // javaDataViewersMutex. Nulling manager outside that lock lets a registration land between the + // map handoff and the retirement, leaving a viewer registered in the native collection with no + // bookkeeping entry - it would keep its JNI global reference and keep receiving callbacks + // after close() returned. + void closeJavaDataViewers(ManagerAndConfig& managerAndConfig, bool retireManager) { ILogManager* manager; std::unordered_map> dataViewers; { std::lock_guard lock(managerAndConfig.javaDataViewersMutex); - manager = managerAndConfig.manager; { std::lock_guard managersLock(jniManagersMutex); - managerAndConfig.manager = nullptr; + manager = managerAndConfig.manager; + if (retireManager) + { + managerAndConfig.manager = nullptr; + } } dataViewers.swap(managerAndConfig.javaDataViewers); } @@ -1044,8 +1054,9 @@ Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_na return; } - // The ManagerAndConfig survives until the static jniManagers array is destroyed. - closeJavaDataViewers(*managerAndConfig); + // The ManagerAndConfig survives until the static jniManagers array is destroyed. Retiring the + // manager is what makes every other native entry point on this LogManager fail or no-op. + closeJavaDataViewers(*managerAndConfig, /* retireManager */ true); } extern "C" JNIEXPORT jobject JNICALL @@ -1092,8 +1103,8 @@ Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_na return 0; } // Capture the manager under the lock rather than dereferencing the ManagerAndConfig later: - // close() and flushAndTeardown() both null it out while the Java handle stays usable, so an - // unguarded mc->manager->GetLogger() would fault. Returning 0 here surfaces as the + // close() retires it while the Java handle stays usable, so an unguarded + // mc->manager->GetLogger() would fault. Returning 0 here surfaces as the // NullPointerException that LogManagerImpl.getLogger() already raises for a null handle. ILogManager* manager = nullptr; { @@ -1151,13 +1162,15 @@ Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_na // It does not unregister data viewers, so without this the proxies stay in the native // collection and in the javaDataViewers map, and their JNI global references pin the // application's IDataViewer objects - and everything those reference - until close() or - // process exit. Release them here as well; closeJavaDataViewers clears its bookkeeping under - // the lock and returns early once the manager pointer is null, so a later close() is a safe - // no-op. Ordered after the teardown so viewers still observe packets from the final flush. + // process exit. Release them here as well; closeJavaDataViewers swaps out its bookkeeping + // under the lock, so a later close() finds nothing left to do. retireManager is false because + // teardown must not retire the handle - doing so would make the still-open Java LogManager + // hand out a SemanticContext wrapping pointer 0 and would silently skip RemoveEventListener. + // Ordered after the teardown so viewers still observe packets from the final flush. auto managerAndConfig = getManagerAndConfig(nativeLogManager); if (managerAndConfig != nullptr) { - closeJavaDataViewers(*managerAndConfig); + closeJavaDataViewers(*managerAndConfig, /* retireManager */ false); } } @@ -1620,8 +1633,12 @@ Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_na return false; } + // Capture the manager once under javaDataViewersMutex. closeJavaDataViewers retires it and + // swaps the map while holding that same lock, so this check and the registration below cannot + // interleave with a close(). std::lock_guard lock(manager_and_config->javaDataViewersMutex); - if (manager_and_config->manager == nullptr || + ILogManager* manager = manager_and_config->manager; + if (manager == nullptr || manager_and_config->javaDataViewers.find(proxy->GetName()) != manager_and_config->javaDataViewers.end()) { @@ -1631,7 +1648,7 @@ Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_na bool collectionRegistered = false; try { - manager_and_config->manager->GetDataViewerCollection().RegisterViewer(proxy); + manager->GetDataViewerCollection().RegisterViewer(proxy); collectionRegistered = true; manager_and_config->javaDataViewers.emplace(proxy->GetName(), proxy); return true; @@ -1642,7 +1659,7 @@ Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_na { try { - manager_and_config->manager->GetDataViewerCollection().UnregisterViewer( + manager->GetDataViewerCollection().UnregisterViewer( proxy->GetName()); } catch (const std::exception& rollbackException) From e71f6c8bd8d58ce477fa059f3b379f07ab22d430 Mon Sep 17 00:00:00 2001 From: kdhawaniya Date: Thu, 1 Oct 2026 16:27:33 +0530 Subject: [PATCH 17/19] Close viewer registration once the viewers are released flushAndTeardown() releases the registered viewers but deliberately leaves manager non-null, so that the still-open Java LogManager keeps working. nativeRegisterDataViewer was using that pointer as its liveness check, so after the teardown cleanup pass a registration could still succeed. The viewer would then never be released unless close() was called, retaining its JNI global reference and the Java object graph behind it until close() or process exit - which is exactly what releasing viewers at teardown is meant to avoid - and contradicting the contract that close() is not needed to release viewers. Track the terminal state explicitly instead of inferring it from the manager pointer: ManagerAndConfig gains viewersClosed, closeJavaDataViewers sets it for both callers, and registration refuses once it is set. It is set inside the javaDataViewersMutex critical section that hands off the viewer map, because nativeRegisterDataViewer checks it and inserts under that lock only, so updating it outside would let a registration land between the handoff and the update. Covered by registerDataViewer_afterFlushAndTeardown_isRejected. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../maesdktest/LogManagerDDVUnitTest.java | 54 +++++++++++++++++++ lib/jni/LogManager_jni.cpp | 39 +++++++++----- 2 files changed, 80 insertions(+), 13 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 676d645f8..345facb2e 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 @@ -488,6 +488,60 @@ public String getCurrentEndpoint() { } } + @Test + public void registerDataViewer_afterFlushAndTeardown_isRejected() 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 = "JavaDataViewerPostTeardown" + 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); + IDataViewer viewer = + new IDataViewer() { + @Override + public void receiveData(byte[] packetData) {} + + @Override + public String getName() { + return "post-teardown-viewer"; + } + + @Override + public boolean isTransmissionEnabled() { + return true; + } + + @Override + public String getCurrentEndpoint() { + return ""; + } + }; + + try { + manager.flushAndTeardown(); + + // flushAndTeardown releases the registered viewers and is terminal, so registration has to + // stay closed afterwards. The manager pointer deliberately stays non-null across teardown, + // so it cannot serve as the liveness check: a viewer accepted here would hold its JNI + // global reference - and the Java object graph behind it - until close() or process exit, + // which is exactly what releasing viewers at teardown is meant to avoid. + assertThat(manager.registerDataViewer(viewer), is(false)); + } finally { + manager.close(); + } + } + @Test public void registerDataViewer_whenOneViewerDisabled_dispatchesOnlyToEnabledViewer() throws Exception { diff --git a/lib/jni/LogManager_jni.cpp b/lib/jni/LogManager_jni.cpp index c48047b1a..5d465c72c 100644 --- a/lib/jni/LogManager_jni.cpp +++ b/lib/jni/LogManager_jni.cpp @@ -875,6 +875,10 @@ namespace std::shared_ptr ddv; std::mutex javaDataViewersMutex; std::unordered_map> javaDataViewers; + // Set once the viewers have been released, by either close() or the terminal + // flushAndTeardown(). Registration is refused from then on: teardown deliberately leaves + // manager non-null, so the manager pointer alone is no longer a liveness check. + bool viewersClosed = false; }; #else struct ManagerAndConfig @@ -883,6 +887,8 @@ namespace ILogManager* manager; std::mutex javaDataViewersMutex; std::unordered_map> javaDataViewers; + // See the HAS_DDV definition above. + bool viewersClosed = false; }; #endif @@ -903,12 +909,15 @@ namespace } // retireManager distinguishes the two callers: close() retires the handle, flushAndTeardown() - // does not. Retirement must happen here rather than in a separate critical section, because - // nativeRegisterDataViewer checks manager and inserts into javaDataViewers while holding only - // javaDataViewersMutex. Nulling manager outside that lock lets a registration land between the - // map handoff and the retirement, leaving a viewer registered in the native collection with no - // bookkeeping entry - it would keep its JNI global reference and keep receiving callbacks - // after close() returned. + // does not. Both close registration permanently via viewersClosed - neither call has a + // counterpart that revives the manager, and leaving registration open after teardown would + // let a viewer be added after this final cleanup pass and leak its JNI global reference, and + // the Java object graph behind it, until close() or process exit. + // + // Both flags are set inside the javaDataViewersMutex critical section that hands off the + // viewer map, because nativeRegisterDataViewer checks them and inserts while holding only + // that lock. Updating them outside it would let a registration land between the handoff and + // the update, leaving a viewer registered in the native collection with no bookkeeping entry. void closeJavaDataViewers(ManagerAndConfig& managerAndConfig, bool retireManager) { ILogManager* manager; @@ -923,6 +932,7 @@ namespace managerAndConfig.manager = nullptr; } } + managerAndConfig.viewersClosed = true; dataViewers.swap(managerAndConfig.javaDataViewers); } @@ -1163,10 +1173,11 @@ Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_na // collection and in the javaDataViewers map, and their JNI global references pin the // application's IDataViewer objects - and everything those reference - until close() or // process exit. Release them here as well; closeJavaDataViewers swaps out its bookkeeping - // under the lock, so a later close() finds nothing left to do. retireManager is false because - // teardown must not retire the handle - doing so would make the still-open Java LogManager - // hand out a SemanticContext wrapping pointer 0 and would silently skip RemoveEventListener. - // Ordered after the teardown so viewers still observe packets from the final flush. + // under the lock and closes registration permanently, so no viewer can be added after this + // pass and a later close() finds nothing left to do. retireManager is false because teardown + // must not retire the handle - doing so would make the still-open Java LogManager hand out a + // SemanticContext wrapping pointer 0 and would silently skip RemoveEventListener. Ordered + // after the teardown so viewers still observe packets from the final flush. auto managerAndConfig = getManagerAndConfig(nativeLogManager); if (managerAndConfig != nullptr) { @@ -1633,12 +1644,14 @@ Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_na return false; } - // Capture the manager once under javaDataViewersMutex. closeJavaDataViewers retires it and - // swaps the map while holding that same lock, so this check and the registration below cannot - // interleave with a close(). + // Capture the manager once under javaDataViewersMutex. closeJavaDataViewers sets viewersClosed + // and swaps the map while holding that same lock, so this check and the registration below + // cannot interleave with a close() or a flushAndTeardown(). viewersClosed, not the manager + // pointer, is the liveness check: teardown leaves the manager non-null on purpose. std::lock_guard lock(manager_and_config->javaDataViewersMutex); ILogManager* manager = manager_and_config->manager; if (manager == nullptr || + manager_and_config->viewersClosed || manager_and_config->javaDataViewers.find(proxy->GetName()) != manager_and_config->javaDataViewers.end()) { From 860c4ca16c0eafdef78e8f12d45846368edb2dbc Mon Sep 17 00:00:00 2001 From: kdhawaniya Date: Fri, 2 Oct 2026 01:26:47 +0530 Subject: [PATCH 18/19] [Android] Export consumer ProGuard rules from the Soong build The Gradle build publishes maesdk/consumer-rules.pro via consumerProguardFiles, but the Soong android_library had no equivalent, so Soong consumers got none of the keep rules. IDataViewer implementations are reached only through JNI GetMethodID, so R8 in a consuming app was free to rename or strip receiveData and getName and break viewer registration at runtime. Add an optimize block with proguard_flags_files and export_proguard_flags_files. The export flag is what propagates the rules to reverse dependencies; the flags files alone would apply only to this module and be a no-op for consumers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- Android.bp | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/Android.bp b/Android.bp index b9e55a15c..0fd9be530 100644 --- a/Android.bp +++ b/Android.bp @@ -116,5 +116,14 @@ android_library { "-Aroom.schemaLocation=lib/android_build/maesdk/schemas", "-Aroom.incremental=true", "-Aroom.expandProjection=true" - ] + ], + // Soong equivalent of the consumerProguardFiles entry in maesdk/build.gradle. IDataViewer + // implementations are reached only through JNI GetMethodID, so R8 in a consuming app would + // otherwise be free to rename or strip receiveData/getName and break viewer registration. + // export_proguard_flags_files is what propagates the rules to consumers; without it they + // would apply only to this module. + optimize: { + proguard_flags_files: ["lib/android_build/maesdk/consumer-rules.pro"], + export_proguard_flags_files: true + } } From db835d0f18dde77efb40cf941189d11e4d83c896 Mon Sep 17 00:00:00 2001 From: kdhawaniya Date: Fri, 2 Oct 2026 14:08:55 +0530 Subject: [PATCH 19/19] ci: re-trigger CI to clear flaky iOS simulator job BasicFuncTests.storageFileSizeDoesntExceedConfiguredSize is flaky on the macOS/iOS simulator: it fails intermittently on main's own commit 88defca7 (failed 2026-09-30, passed 2026-10-01 and 2026-10-02 on identical sources). No source changes; empty commit only to re-run the matrix. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>