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

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

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

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

This is an optional capability. The default implementation returns {@code false} so that + * existing implementations of this interface remain source compatible; implementations that + * support data viewers override it. + * + * @return {@code true} when the viewer was unregistered, {@code false} when it was not + * registered, or when the implementation does not support data viewers */ - public boolean unregisterDataViewer(String viewerName); + default boolean unregisterDataViewer(String viewerName) { + return false; + } public LogSessionData getLogSessionData(); diff --git a/lib/api/DataViewerCollection.cpp b/lib/api/DataViewerCollection.cpp index 6992fee75..9c4a93205 100644 --- a/lib/api/DataViewerCollection.cpp +++ b/lib/api/DataViewerCollection.cpp @@ -16,7 +16,14 @@ namespace MAT_NS_BEGIN { return; LOCKGUARD(m_dataViewerMapLock); - for(const auto& viewer : m_dataViewerCollection) + // Dispatch over a snapshot rather than the member directly. m_dataViewerMapLock is + // recursive, so a viewer that reenters the SDK from ReceiveData - for example by + // closing the owning LogManager, which unregisters every viewer - would otherwise + // erase from the very vector being iterated here and invalidate the iterator. + // Holding shared_ptr copies additionally keeps each viewer alive for the duration of + // its own callback, even if that callback drops the last other reference to it. + const auto viewers = m_dataViewerCollection; + for(const auto& viewer : viewers) { // Task 3568800: Integrate ThreadPool to IDataViewerCollection viewer->ReceiveData(packetData); diff --git a/tests/unittests/DataViewerCollectionTests.cpp b/tests/unittests/DataViewerCollectionTests.cpp index 57b377fc3..33b51916c 100644 --- a/tests/unittests/DataViewerCollectionTests.cpp +++ b/tests/unittests/DataViewerCollectionTests.cpp @@ -264,3 +264,65 @@ TEST(DataViewerCollectionTests, IsViewerEnabledNoParam_MultipleViewersRegistered ASSERT_TRUE(dataViewerCollection.IsViewerEnabled()); } +namespace +{ + // Mirrors a viewer that reenters the SDK from its own callback - for example a Java + // viewer that closes the owning LogManager from receiveData(), which unregisters every + // viewer. m_dataViewerMapLock is recursive, so the reentrant call is admitted while + // dispatch is still walking the collection. + class ReentrantUnregisteringDataViewer : public IDataViewer + { + public: + + ReentrantUnregisteringDataViewer(const char* name, TestDataViewerCollection& collection) : + m_name(name), m_collection(collection) {} + + void ReceiveData(const std::vector&) noexcept override + { + callCount++; + m_collection.UnregisterAllViewers(); + } + + const char* GetName() const noexcept override + { + return m_name; + } + + bool IsTransmissionEnabled() const noexcept override + { + return true; + } + + const std::string& GetCurrentEndpoint() const noexcept override + { + return m_testEndpoint; + } + + int callCount { 0 }; + const char* m_name; + TestDataViewerCollection& m_collection; + const std::string m_testEndpoint { "TestEndpoint" }; + }; +} + +TEST(DataViewerCollectionTests, DispatchDataViewerEvent_ViewerUnregistersAllFromCallback_DispatchCompletesSafely) +{ + TestDataViewerCollection dataViewerCollection { }; + auto reentrantViewer = std::make_shared("ReentrantViewer", dataViewerCollection); + auto secondViewer = std::make_shared("SecondViewer", /*isTransmissionEnabled*/ true); + + dataViewerCollection.RegisterViewer(reentrantViewer); + dataViewerCollection.RegisterViewer(secondViewer); + + const std::vector packetData { 1, 2, 3 }; + + // Dispatching over the member vector directly would erase it mid-iteration here and + // invalidate the iterator; dispatching over a snapshot completes and still delivers the + // in-flight packet to viewers that were registered when dispatch began. + dataViewerCollection.DispatchDataViewerEvent(packetData); + + ASSERT_EQ(reentrantViewer->callCount, 1); + ASSERT_EQ(secondViewer->localPacketData, packetData); + ASSERT_TRUE(dataViewerCollection.GetCollection().empty()); +} + From 77b17258ae488dcd8d7ec58f41b53687ebd02bae Mon Sep 17 00:00:00 2001 From: kdhawaniya Date: Mon, 21 Sep 2026 17:22:49 +0530 Subject: [PATCH 3/3] 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