diff --git a/Android.bp b/Android.bp index 78ec0c65d..0fd9be530 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", @@ -115,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 + } } diff --git a/lib/CMakeLists.txt b/lib/CMakeLists.txt index e775caf08..d311be424 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..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 @@ -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,387 @@ public void startDDVonLogManager() { LogManager.flushAndTeardown(); } + @Test + public void registerDataViewer_whenCallbackThrows_continuesDispatchAndStopsAfterUnregister() + 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(); + 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"); + } + + @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) { + receivingViewerCalls.incrementAndGet(); + 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)); + + // 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(); + } + } + + @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 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_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 { + 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/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..2978b7b16 --- /dev/null +++ b/lib/android_build/maesdk/src/main/java/com/microsoft/applications/events/IDataViewer.java @@ -0,0 +1,49 @@ +// +// 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, 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 { + + /** 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. + * + *

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 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(); + + /** 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..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(); @@ -59,6 +65,36 @@ public interface ILogManager extends AutoCloseable { public String getCurrentEndpoint(); + /** + * Registers a caller-provided data viewer with this LogManager. + * + *

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 + */ + default boolean registerDataViewer(IDataViewer dataViewer) { + return false; + } + + /** + * Unregisters a caller-provided data viewer by its unique 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. + * + *

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 + */ + default boolean unregisterDataViewer(String viewerName) { + return false; + } + 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..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( @@ -226,6 +230,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/api/DataViewerCollection.cpp b/lib/api/DataViewerCollection.cpp index 6992fee75..397082828 100644 --- a/lib/api/DataViewerCollection.cpp +++ b/lib/api/DataViewerCollection.cpp @@ -4,20 +4,74 @@ // #include "DataViewerCollection.hpp" #include +#include #include 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 { - if (IsViewerEnabled() == false) + // 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. + // + // 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; + if (!TrySnapshotViewers(viewers)) + { return; + } - LOCKGUARD(m_dataViewerMapLock); - for(const auto& viewer : m_dataViewerCollection) + // 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); } @@ -52,7 +106,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()) @@ -79,9 +133,19 @@ 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. + std::vector> viewers; + if (!TrySnapshotViewers(viewers)) + { + return false; + } + + return std::any_of(viewers.cbegin(), viewers.cend(), + [](const std::shared_ptr& viewer) { return viewer->IsTransmissionEnabled(); }); } bool DataViewerCollection::IsViewerRegistered(const char* viewerName) const 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; 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; /// diff --git a/lib/jni/JavaDataViewerProxy.cpp b/lib/jni/JavaDataViewerProxy.cpp new file mode 100644 index 000000000..c477dbbe2 --- /dev/null +++ b/lib/jni/JavaDataViewerProxy.cpp @@ -0,0 +1,317 @@ +// +// Copyright (c) Microsoft Corporation. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 +// +#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, + jobject dataViewer) noexcept + { + if (env == nullptr || dataViewer == 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(new JavaDataViewerProxy()); + } + MATSDK_CATCH(...) + { + return nullptr; + } + 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())) + { +#ifdef HAVE_MAT_LOGGING + __android_log_print(ANDROID_LOG_ERROR, LOG_TAG, "Packet is too large for a Java byte array"); +#endif + return; + } + + bool attached = false; + auto env = GetEnv(attached); + if (env == nullptr) + { + return; + } + + auto packet = env->NewByteArray(static_cast(packetData.size())); + if (ClearPendingException(env, "receiveData allocation") || packet == nullptr) + { + if (packet != nullptr) + { + env->DeleteLocalRef(packet); + } + 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 + { + // 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) + { + currentEndpoint.clear(); + return currentEndpoint; + } + + std::string endpoint; + if (ReadString(env, m_getCurrentEndpoint, endpoint)) + { + currentEndpoint = std::move(endpoint); + } + else + { + currentEndpoint.clear(); + } + DetachIfNeeded(attached); + return 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(); +#ifdef HAVE_MAT_LOGGING + __android_log_print( + ANDROID_LOG_ERROR, + LOG_TAG, + "Java IDataViewer callback failed: %s", + methodName); +#else + (void)methodName; +#endif + 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; + } + // 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"); + } + +} MAT_NS_END diff --git a/lib/jni/JavaDataViewerProxy.hpp b/lib/jni/JavaDataViewerProxy.hpp new file mode 100644 index 000000000..ed0936120 --- /dev/null +++ b/lib/jni/JavaDataViewerProxy.hpp @@ -0,0 +1,47 @@ +// +// 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 + +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; + }; + +} MAT_NS_END + +#endif diff --git a/lib/jni/LogManager_jni.cpp b/lib/jni/LogManager_jni.cpp index 70cb5b1ec..5d465c72c 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,22 @@ namespace ILogConfiguration config; ILogManager* manager; 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 { ILogConfiguration config; ILogManager* manager; + std::mutex javaDataViewersMutex; + std::unordered_map> javaDataViewers; + // See the HAS_DDV definition above. + bool viewersClosed = false; }; #endif @@ -882,6 +896,71 @@ 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(); + } + + // retireManager distinguishes the two callers: close() retires the handle, flushAndTeardown() + // 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; + std::unordered_map> dataViewers; + { + std::lock_guard lock(managerAndConfig.javaDataViewersMutex); + { + std::lock_guard managersLock(jniManagersMutex); + manager = managerAndConfig.manager; + if (retireManager) + { + managerAndConfig.manager = nullptr; + } + } + managerAndConfig.viewersClosed = true; + 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) + { +#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 + } + } + } } extern "C" JNIEXPORT jlong JNICALL @@ -979,17 +1058,15 @@ 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. 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 @@ -1035,16 +1112,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() 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; { 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; @@ -1055,7 +1137,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)); @@ -1084,6 +1166,23 @@ 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 swaps out its bookkeeping + // 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) + { + closeJavaDataViewers(*managerAndConfig, /* retireManager */ false); + } } extern "C" JNIEXPORT jint JNICALL @@ -1526,6 +1625,139 @@ 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; + } + + // 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()) + { + return false; + } + + bool collectionRegistered = false; + try + { + 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->GetDataViewerCollection().UnregisterViewer( + proxy->GetName()); + } + 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; + } +} + +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) + { +#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; + } +} + extern "C" JNIEXPORT void JNICALL Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_nativeGetLogSessionData( JNIEnv* env, @@ -2071,7 +2303,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; } @@ -2088,7 +2320,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; } @@ -2105,7 +2337,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; } @@ -2182,7 +2414,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; } @@ -2199,7 +2431,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; } @@ -2216,7 +2448,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; } diff --git a/tests/unittests/DataViewerCollectionTests.cpp b/tests/unittests/DataViewerCollectionTests.cpp index 57b377fc3..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; @@ -134,6 +140,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 { }; @@ -264,3 +285,200 @@ 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()); +} + +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()); +} + +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); +} +