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 5f784a563..405749318 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 @@ -511,6 +511,10 @@ public void onDebugEvent(DebugEvent evt) { ListenForFilter listener = new ListenForFilter(); manager.addEventListener(DebugEventType.EVT_FILTERED, listener); + long listenerIdentity = listener.nativeIdentity; + assertThat(listenerIdentity, is(not(-1L))); + manager.addEventListener(DebugEventType.EVT_REJECTED, listener); + assertThat(listener.nativeIdentity, is(listenerIdentity)); logger.logEvent("noprops"); manager.uploadNow(); try { @@ -531,6 +535,9 @@ public void onDebugEvent(DebugEvent evt) { assertThat(listener.filteredCount, is(1L)); } manager.removeEventListener(DebugEventType.EVT_FILTERED, listener); + assertThat(listener.nativeIdentity, is(listenerIdentity)); + manager.removeEventListener(DebugEventType.EVT_REJECTED, listener); + assertThat(listener.nativeIdentity, is(-1L)); int[] everything = { DiagLevel.DIAG_LEVEL_REQUIRED.value(), DiagLevel.DIAG_LEVEL_OPTIONAL.value() }; manager.setLevelFilter(DiagLevel.DIAG_LEVEL_OPTIONAL.value(), everything); } 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 edd0d1771..ba5d41e74 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 @@ -272,11 +272,13 @@ public void addEventListener(DebugEventType eventType, DebugEventListener listen listener.nativeIdentity = nativeAddEventListener(nativeLogManager, eventType.value(), listener, listener.nativeIdentity); } - public native void nativeRemoveEventListener(long nativeLogManager, long eventType, long identity); + public native long nativeRemoveEventListener( + long nativeLogManager, long eventType, long identity, DebugEventListener listener); @Override public void removeEventListener(DebugEventType eventType, DebugEventListener listener) { - nativeRemoveEventListener(nativeLogManager, eventType.value(), listener.nativeIdentity); + listener.nativeIdentity = nativeRemoveEventListener( + nativeLogManager, eventType.value(), listener.nativeIdentity, listener); } private native boolean nativeRegisterPrivacyGuard(long nativeLogManager); diff --git a/lib/callbacks/DebugSource.cpp b/lib/callbacks/DebugSource.cpp index 8662b9088..cc70009d3 100644 --- a/lib/callbacks/DebugSource.cpp +++ b/lib/callbacks/DebugSource.cpp @@ -3,14 +3,88 @@ // SPDX-License-Identifier: Apache-2.0 // #include "mat/config.h" +#include "callbacks/DebugSourceInternal.hpp" #include "DebugEvents.hpp" #include "utils/Utils.hpp" #include "pal/PAL.hpp" #include +#include namespace MAT_NS_BEGIN { + namespace + { + thread_local std::vector pendingListeners; + std::atomic + pendingReleaseCallback{nullptr}; + + class PendingListenersScope + { + public: + explicit PendingListenersScope(const std::vector& listeners) : + remaining(listeners) + { + pendingListeners.insert( + pendingListeners.end(), + listeners.begin(), + listeners.end()); + } + + ~PendingListenersScope() + { + for (auto listener : remaining) + { + RemovePending(listener); + auto callback = pendingReleaseCallback.load(); + if (callback != nullptr) + { + callback(listener); + } + } + } + + void BeginCallback(DebugEventListener* listener) + { + auto current = std::find(remaining.begin(), remaining.end(), listener); + if (current != remaining.end()) + { + remaining.erase(current); + } + RemovePending(listener); + } + + private: + static void RemovePending(DebugEventListener* listener) + { + auto pending = std::find( + pendingListeners.rbegin(), + pendingListeners.rend(), + listener); + if (pending != pendingListeners.rend()) + { + pendingListeners.erase(std::next(pending).base()); + } + } + + std::vector remaining; + }; + } + + bool IsDebugEventListenerPending(const DebugEventListener* listener) noexcept + { + return std::find( + pendingListeners.begin(), + pendingListeners.end(), + listener) != pendingListeners.end(); + } + + void SetDebugEventListenerPendingReleaseCallback( + DebugEventListenerPendingReleaseCallback callback) noexcept + { + pendingReleaseCallback.store(callback); + } + /// Add event listener for specific debug event type. void DebugEventSource::AddEventListener(DebugEventType type, DebugEventListener &listener) { @@ -45,8 +119,10 @@ namespace MAT_NS_BEGIN { if (listeners.size()) { // Events filter handlers list - auto &v = listeners[evt.type]; - for (auto listener : v) { + auto eventListeners = listeners[evt.type]; + PendingListenersScope pendingScope(eventListeners); + for (auto listener : eventListeners) { + pendingScope.BeginCallback(listener); listener->OnDebugEvent(evt); dispatched = true; } @@ -85,4 +161,3 @@ namespace MAT_NS_BEGIN { } } MAT_NS_END - diff --git a/lib/callbacks/DebugSourceInternal.hpp b/lib/callbacks/DebugSourceInternal.hpp new file mode 100644 index 000000000..5adfe75bd --- /dev/null +++ b/lib/callbacks/DebugSourceInternal.hpp @@ -0,0 +1,18 @@ +// +// Copyright (c) Microsoft Corporation. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 +// +#pragma once + +#include "DebugEvents.hpp" + +namespace MAT_NS_BEGIN +{ + using DebugEventListenerPendingReleaseCallback = + void (*)(DebugEventListener*); + + bool IsDebugEventListenerPending(const DebugEventListener* listener) noexcept; + void SetDebugEventListenerPendingReleaseCallback( + DebugEventListenerPendingReleaseCallback callback) noexcept; +} +MAT_NS_END diff --git a/lib/jni/JniConvertors.cpp b/lib/jni/JniConvertors.cpp index dc61a4304..c8931cae6 100644 --- a/lib/jni/JniConvertors.cpp +++ b/lib/jni/JniConvertors.cpp @@ -196,12 +196,27 @@ EventProperties GetEventProperties(JNIEnv* env, const jstring& jstrEventName, co std::vector ConvertJObjectArrayToStdStringVector(JNIEnv* env, const jobjectArray& jArrayToConvert) { std::vector stringVector; - stringVector.reserve(env->GetArrayLength(jArrayToConvert)); + auto length = env->GetArrayLength(jArrayToConvert); + if (env->ExceptionCheck()) + { + return stringVector; + } + stringVector.reserve(length); - for(int i = 0; i < env->GetArrayLength(jArrayToConvert); i++) + for(int i = 0; i < length; i++) { auto jStringValue = static_cast(env->GetObjectArrayElement(jArrayToConvert, i)); + if (env->ExceptionCheck()) + { + env->DeleteLocalRef(jStringValue); + return stringVector; + } auto stringValue = JStringToStdString(env, jStringValue); + if (env->ExceptionCheck()) + { + env->DeleteLocalRef(jStringValue); + return stringVector; + } if(!stringValue.empty()) { stringVector.emplace_back(std::move(stringValue)); diff --git a/lib/jni/LogManager_jni.cpp b/lib/jni/LogManager_jni.cpp index ed1cfcef7..c6a0b6213 100644 --- a/lib/jni/LogManager_jni.cpp +++ b/lib/jni/LogManager_jni.cpp @@ -26,6 +26,7 @@ #endif #include +#include "callbacks/DebugSourceInternal.hpp" #include "JniConvertors.hpp" #include "LogManagerBase.hpp" #include "WrapperLogManager.hpp" @@ -399,6 +400,17 @@ namespace } } + bool TryJStringToStdString(JNIEnv* env, jstring value, std::string& result) + { + if (value == nullptr) + { + return false; + } + + result = JStringToStdString(env, value); + return !env->ExceptionCheck(); + } + /** * Smart object to manage PushLocalFrame/PopLocalFrame */ @@ -578,10 +590,12 @@ namespace continue; } auto key = static_cast(k); - auto cstringKey = env->GetStringUTFChars(key, nullptr); - rethrow(env); - std::string stringKey(cstringKey); - env->ReleaseStringUTFChars(key, cstringKey); + std::string stringKey; + if (!TryJStringToStdString(env, key, stringKey)) + { + rethrow(env); + throw std::runtime_error("Unable to convert configuration key"); + } auto value = env->CallObjectMethod(configuration, getMethod, key); rethrow(env); if (!value) @@ -634,12 +648,12 @@ namespace case ValueTypes::STRING: { auto s = static_cast(value); - auto cString = env->GetStringUTFChars( - s, - nullptr); - rethrow(env); - std::string cppString(cString); - env->ReleaseStringUTFChars(s, cString); + std::string cppString; + if (!TryJStringToStdString(env, s, cppString)) + { + rethrow(env); + throw std::runtime_error("Unable to convert string value"); + } return Variant(std::move(cppString)); } case ValueTypes::VARIANT_MAP: @@ -671,9 +685,12 @@ namespace auto jName = static_cast(env->CallObjectMethod(actual, gnMethod)); - auto cName = env->GetStringUTFChars(jName, nullptr); - std::string className(cName); - env->ReleaseStringUTFChars(jName, cName); + std::string className; + if (!TryJStringToStdString(env, jName, className)) + { + rethrow(env); + throw std::runtime_error("Unable to convert class name"); + } __android_log_print(ANDROID_LOG_ERROR, "MAE", "Unsupported class %s", @@ -869,10 +886,11 @@ Java_com_microsoft_applications_events_LogManager_nativeInitializeConfig(JNIEnv* __android_log_print(ANDROID_LOG_INFO, "MAE", "Translated map: %s", cereal.c_str()); - auto tokenUTF = env->GetStringUTFChars(tenant_token, nullptr); - rethrow(env); - std::string token(tokenUTF); - env->ReleaseStringUTFChars(tenant_token, tokenUTF); + std::string token; + if (!TryJStringToStdString(env, tenant_token, token)) + { + return 0; + } auto logger = WrapperLogManager::Initialize(token, logConfiguration); return reinterpret_cast(logger); } @@ -1012,15 +1030,15 @@ Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_na if (!mc) return 0; } - auto tokenUtf = env->GetStringUTFChars(jToken, nullptr); - std::string token{tokenUtf}; - env->ReleaseStringUTFChars(jToken, tokenUtf); - auto sourceUtf = env->GetStringUTFChars(jSource, nullptr); - std::string source{sourceUtf}; - env->ReleaseStringUTFChars(jSource, sourceUtf); - auto scopeUtf = env->GetStringUTFChars(jScope, nullptr); - std::string scope{scopeUtf}; - env->ReleaseStringUTFChars(jScope, scopeUtf); + std::string token; + std::string source; + std::string scope; + if (!TryJStringToStdString(env, jToken, token) || + !TryJStringToStdString(env, jSource, source) || + !TryJStringToStdString(env, jScope, scope)) + { + return 0; + } return reinterpret_cast(mc->manager->GetLogger( token, source, @@ -1134,9 +1152,11 @@ Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_na { return STATUS_EFAIL; } - auto profile_string = env->GetStringUTFChars(profile, nullptr); - std::string stringyProfile(profile_string); - env->ReleaseStringUTFChars(profile, profile_string); + std::string stringyProfile; + if (!TryJStringToStdString(env, profile, stringyProfile)) + { + return STATUS_EFAIL; + } return logManager->SetTransmitProfile(stringyProfile); } @@ -1152,9 +1172,11 @@ Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_na { return STATUS_EFAIL; } - auto chars = env->GetStringUTFChars(json, nullptr); - std::string cppJson(chars); - env->ReleaseStringUTFChars(json, chars); + std::string cppJson; + if (!TryJStringToStdString(env, json, cppJson)) + { + return STATUS_EFAIL; + } return logManager->LoadTransmitProfiles(cppJson); } @@ -1215,12 +1237,13 @@ Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_na { return STATUS_EFAIL; } - auto chars = env->GetStringUTFChars(name, nullptr); - std::string cppName(chars); - env->ReleaseStringUTFChars(name, chars); - chars = env->GetStringUTFChars(value, nullptr); - std::string cppValue(chars); - env->ReleaseStringUTFChars(value, chars); + std::string cppName; + std::string cppValue; + if (!TryJStringToStdString(env, name, cppName) || + !TryJStringToStdString(env, value, cppValue)) + { + return STATUS_EFAIL; + } return logManager->SetContext(cppName, cppValue, static_cast(pii_kind)); @@ -1240,9 +1263,11 @@ Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_na { return STATUS_EFAIL; } - auto chars = env->GetStringUTFChars(name, nullptr); - std::string cppName(chars); - env->ReleaseStringUTFChars(name, chars); + std::string cppName; + if (!TryJStringToStdString(env, name, cppName)) + { + return STATUS_EFAIL; + } return logManager->SetContext(cppName, value, static_cast(pii_kind)); } @@ -1261,9 +1286,11 @@ Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_na { return STATUS_EFAIL; } - auto chars = env->GetStringUTFChars(name, nullptr); - std::string cppName(chars); - env->ReleaseStringUTFChars(name, chars); + std::string cppName; + if (!TryJStringToStdString(env, name, cppName)) + { + return STATUS_EFAIL; + } return logManager->SetContext(cppName, value, static_cast(pii_kind)); } @@ -1282,9 +1309,11 @@ Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_na { return STATUS_EFAIL; } - auto chars = env->GetStringUTFChars(name, nullptr); - std::string cppName(chars); - env->ReleaseStringUTFChars(name, chars); + std::string cppName; + if (!TryJStringToStdString(env, name, cppName)) + { + return STATUS_EFAIL; + } return logManager->SetContext(cppName, value, static_cast(pii_kind)); } @@ -1303,9 +1332,11 @@ Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_na { return STATUS_EFAIL; } - auto chars = env->GetStringUTFChars(name, nullptr); - std::string cppName(chars); - env->ReleaseStringUTFChars(name, chars); + std::string cppName; + if (!TryJStringToStdString(env, name, cppName)) + { + return STATUS_EFAIL; + } return logManager->SetContext(cppName, value, static_cast(pii_kind)); } @@ -1324,9 +1355,11 @@ Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_na { return STATUS_EFAIL; } - auto chars = env->GetStringUTFChars(name, nullptr); - std::string cppName(chars); - env->ReleaseStringUTFChars(name, chars); + std::string cppName; + if (!TryJStringToStdString(env, name, cppName)) + { + return STATUS_EFAIL; + } auto dateClass = env->GetObjectClass(value); auto getTimeID = env->GetMethodID(dateClass, "getTime", "()J"); auto javaMilliseconds = env->CallLongMethod(value, getTimeID); @@ -1351,14 +1384,15 @@ Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_na { return STATUS_EFAIL; } - auto chars = env->GetStringUTFChars(name, nullptr); - std::string cppName(chars); - env->ReleaseStringUTFChars(name, chars); - chars = env->GetStringUTFChars(value, nullptr); - auto result = logManager->SetContext(cppName, value, - static_cast(pii_kind)); - env->ReleaseStringUTFChars(value, chars); - return result; + std::string cppName; + std::string cppValue; + if (!TryJStringToStdString(env, name, cppName) || + !TryJStringToStdString(env, value, cppValue)) + { + return STATUS_EFAIL; + } + return logManager->SetContext(cppName, GUID_t(cppValue.c_str()), + static_cast(pii_kind)); } extern "C" JNIEXPORT jboolean JNICALL @@ -1537,56 +1571,230 @@ Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_na namespace { - struct JniDebugEventListener : DebugEventListener + struct JniDebugEventListener; + void ReleaseUnregisteredListener( + const std::shared_ptr& listener); + + struct JniDebugEventListener : + DebugEventListener, + std::enable_shared_from_this { + struct Registration + { + enum class State + { + Adding, + Active, + Cancelled, + Removing + }; + + jlong logManager; + DebugEventType eventType; + State state; + }; + JavaVM* javaVm; - jobject javaListener; + jobject javaListener = nullptr; + jclass eventClass = nullptr; + jmethodID eventConstructor = nullptr; + jmethodID listenerMethod = nullptr; + std::vector registrations; JniDebugEventListener() = delete; - JniDebugEventListener(JavaVM* _javaVm, jobject _javaListener) : - javaVm(_javaVm) + JniDebugEventListener(JNIEnv* env, JavaVM* vm, jobject listener) : + javaVm(vm) { - JNIEnv* env = nullptr; - _javaVm->AttachCurrentThread(&env, nullptr); - javaListener = env->NewGlobalRef(_javaListener); + auto localEventClass = + env->FindClass("com/microsoft/applications/events/DebugEvent"); + rethrow(env); + eventConstructor = env->GetMethodID( + localEventClass, "", "(JJJJJLjava/lang/Object;J)V"); + rethrow(env); + + auto localListenerClass = env->GetObjectClass(listener); + rethrow(env); + listenerMethod = env->GetMethodID( + localListenerClass, + "onDebugEvent", + "(Lcom/microsoft/applications/events/DebugEvent;)V"); + rethrow(env); + + eventClass = static_cast(env->NewGlobalRef(localEventClass)); + rethrow(env); + javaListener = env->NewGlobalRef(listener); + if (javaListener == nullptr) + { + env->DeleteGlobalRef(eventClass); + eventClass = nullptr; + rethrow(env); + throw std::runtime_error("Unable to retain debug event listener"); + } + + env->DeleteLocalRef(localListenerClass); + env->DeleteLocalRef(localEventClass); } - ~JniDebugEventListener() + ~JniDebugEventListener() override { - JNIEnv* env = nullptr; - javaVm->AttachCurrentThread(&env, nullptr); + bool detach = false; + auto env = GetEnv(detach); + if (env == nullptr) + { + return; + } + env->DeleteGlobalRef(javaListener); + env->DeleteGlobalRef(eventClass); + if (detach) + { + javaVm->DetachCurrentThread(); + } } void OnDebugEvent(DebugEvent& evt) override { + auto keepAlive = shared_from_this(); + ReleaseUnregisteredListener(keepAlive); + bool detach = false; + auto env = GetEnv(detach); + if (env == nullptr) + { + return; + } + + if (env->PushLocalFrame(1) == JNI_OK) + { + auto eventLocal = env->NewObject( + eventClass, + eventConstructor, + static_cast(evt.seq), + static_cast(evt.ts), + static_cast(evt.type), + static_cast(evt.param1), + static_cast(evt.param2), + static_cast(nullptr), + static_cast(evt.size)); + if (eventLocal != nullptr && !env->ExceptionCheck()) + { + env->CallVoidMethod(javaListener, listenerMethod, eventLocal); + } + env->PopLocalFrame(nullptr); + } + + if (env->ExceptionCheck()) + { + env->ExceptionDescribe(); + env->ExceptionClear(); + } + if (detach) + { + keepAlive->javaVm->DetachCurrentThread(); + } + } + + bool IsSameListener(JNIEnv* env, jobject listener) const + { + return env->IsSameObject(javaListener, listener) == JNI_TRUE; + } + + std::vector::iterator FindRegistration( + jlong logManager, + DebugEventType eventType) + { + return std::find_if( + registrations.begin(), + registrations.end(), + [logManager, eventType](const Registration& registration) { + return registration.logManager == logManager && + registration.eventType == eventType; + }); + } + + bool HasLiveRegistrations() const + { + return std::any_of( + registrations.begin(), + registrations.end(), + [](const Registration& registration) { + return registration.state == Registration::State::Adding || + registration.state == Registration::State::Active; + }); + } + + private: + JNIEnv* GetEnv(bool& detach) const + { + detach = false; JNIEnv* env = nullptr; - javaVm->AttachCurrentThread(&env, nullptr); - auto eventClassId = - env->FindClass("com/microsoft/applications/events/DebugEvent"); - auto constructorId = env->GetMethodID(eventClassId, "", - "(JJJJJLjava/lang/Object;J)V"); - jobject eventLocal; - eventLocal = env->NewObject(eventClassId, - constructorId, - static_cast(evt.seq), - static_cast(evt.ts), - static_cast(evt.type), - static_cast(evt.param1), - static_cast(evt.param2), - static_cast(nullptr), - static_cast(evt.size)); - auto classId = env->GetObjectClass(javaListener); - auto methodId = env->GetMethodID(classId, - "onDebugEvent", - "(Lcom/microsoft/applications/events/DebugEvent;)V"); - env->CallVoidMethod(javaListener, methodId, eventLocal); + auto status = javaVm->GetEnv(reinterpret_cast(&env), JNI_VERSION_1_6); + if (status == JNI_EDETACHED) + { + if (javaVm->AttachCurrentThread(&env, nullptr) != JNI_OK) + { + return nullptr; + } + detach = true; + } + else if (status != JNI_OK) + { + return nullptr; + } + + return env; } }; - static std::vector> listeners; + static std::vector> listeners; static std::mutex listeners_mutex; + + void ReleaseUnregisteredListener( + const std::shared_ptr& listener) + { + std::lock_guard lock(listeners_mutex); + if (!listener->registrations.empty() || + IsDebugEventListenerPending(listener.get())) + { + return; + } + + auto existing = std::find(listeners.begin(), listeners.end(), listener); + if (existing != listeners.end()) + { + existing->reset(); + } + } + + void ReleasePendingJniDebugEventListener( + DebugEventListener* listener) noexcept + { + std::shared_ptr callback; + { + std::lock_guard lock(listeners_mutex); + auto existing = std::find_if( + listeners.begin(), + listeners.end(), + [listener](const std::shared_ptr& value) { + return value.get() == listener; + }); + if (existing == listeners.end() || (*existing)->HasLiveRegistrations()) + { + return; + } + callback = *existing; + } + ReleaseUnregisteredListener(callback); + } + + void EnsurePendingReleaseCallbackRegistered() + { + static std::once_flag registerCallback; + std::call_once(registerCallback, [] { + SetDebugEventListenerPendingReleaseCallback( + ReleasePendingJniDebugEventListener); + }); + } } // anonymous namespace extern "C" JNIEXPORT jlong JNICALL @@ -1598,34 +1806,233 @@ Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_na jobject listener, jlong current_identity) { - JavaVM* vm; - env->GetJavaVM(&vm); - std::unique_ptr callback = std::make_unique(vm, listener); auto logManager = getLogManager(native_log_manager); - logManager->AddEventListener(static_cast(event_type), *callback); - if (current_identity >= 0) { - return current_identity; + if (logManager == nullptr || listener == nullptr) + { + return -1; + } + + JavaVM* vm = nullptr; + if (env->GetJavaVM(&vm) != JNI_OK) + { + return -1; + } + + try + { + EnsurePendingReleaseCallbackRegistered(); + + auto eventType = static_cast(event_type); + std::shared_ptr callback; + jlong identity = -1; + { + std::lock_guard lock(listeners_mutex); + if (current_identity >= 0 && + current_identity < static_cast(listeners.size()) && + listeners[current_identity] && + listeners[current_identity]->IsSameListener(env, listener)) + { + callback = listeners[current_identity]; + identity = current_identity; + } + + if (!callback) + { + auto existing = std::find_if( + listeners.begin(), + listeners.end(), + [env, listener](const std::shared_ptr& value) { + return value && value->IsSameListener(env, listener); + }); + if (existing != listeners.end()) + { + callback = *existing; + identity = static_cast( + std::distance(listeners.begin(), existing)); + } + } + + if (!callback) + { + callback = std::make_shared(env, vm, listener); + callback->registrations.push_back( + {native_log_manager, eventType, JniDebugEventListener::Registration::State::Adding}); + + auto available = std::find(listeners.begin(), listeners.end(), nullptr); + if (available == listeners.end()) + { + identity = static_cast(listeners.size()); + listeners.emplace_back(callback); + } + else + { + identity = static_cast(std::distance(listeners.begin(), available)); + *available = callback; + } + } + else if (callback->FindRegistration(native_log_manager, eventType) != + callback->registrations.end()) + { + return identity; + } + else + { + callback->registrations.push_back( + {native_log_manager, eventType, JniDebugEventListener::Registration::State::Adding}); + } + } + + try + { + logManager->AddEventListener(eventType, *callback); + } + catch (...) + { + std::lock_guard lock(listeners_mutex); + auto registration = + callback->FindRegistration(native_log_manager, eventType); + if (registration != callback->registrations.end() && + (registration->state == + JniDebugEventListener::Registration::State::Adding || + registration->state == + JniDebugEventListener::Registration::State::Cancelled)) + { + callback->registrations.erase(registration); + } + if (callback->registrations.empty() && + !IsDebugEventListenerPending(callback.get())) + { + auto existing = std::find(listeners.begin(), listeners.end(), callback); + if (existing != listeners.end()) + { + existing->reset(); + } + } + throw; + } + + bool removeCancelledRegistration = false; + { + std::lock_guard lock(listeners_mutex); + auto registration = + callback->FindRegistration(native_log_manager, eventType); + if (registration == callback->registrations.end()) + { + removeCancelledRegistration = true; + } + else if (registration->state == + JniDebugEventListener::Registration::State::Cancelled) + { + removeCancelledRegistration = true; + } + else + { + registration->state = + JniDebugEventListener::Registration::State::Active; + } + } + + if (removeCancelledRegistration) + { + logManager->RemoveEventListener(eventType, *callback); + std::lock_guard lock(listeners_mutex); + auto registration = + callback->FindRegistration(native_log_manager, eventType); + if (registration != callback->registrations.end() && + registration->state == + JniDebugEventListener::Registration::State::Cancelled) + { + callback->registrations.erase(registration); + } + if (callback->registrations.empty() && + !IsDebugEventListenerPending(callback.get())) + { + listeners[identity].reset(); + } + } + std::lock_guard lock(listeners_mutex); + return callback->HasLiveRegistrations() ? identity : -1; + } + catch (const std::exception& e) + { + if (!env->ExceptionCheck()) + { + auto exceptionClass = env->FindClass("java/lang/RuntimeException"); + if (exceptionClass != nullptr) + { + env->ThrowNew(exceptionClass, e.what()); + env->DeleteLocalRef(exceptionClass); + } + } + return -1; } - std::lock_guard l(listeners_mutex); - listeners.emplace_back(std::move(callback)); - return listeners.size(); } extern "C" -JNIEXPORT void JNICALL +JNIEXPORT jlong JNICALL Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_nativeRemoveEventListener( JNIEnv *env, jobject thiz, jlong native_log_manager, jlong eventType, - jlong identity) { - std::lock_guard l(listeners_mutex); - if (identity < 0 || identity >= static_cast(jniManagers.size()) || !listeners[identity]) + jlong identity, + jobject listener) { + auto logManager = getLogManager(native_log_manager); + std::shared_ptr callback; + auto event = static_cast(eventType); + { + std::lock_guard lock(listeners_mutex); + if (identity < 0 || + identity >= static_cast(listeners.size()) || + !listeners[identity] || + !listeners[identity]->IsSameListener(env, listener)) + { + return identity; + } + + callback = listeners[identity]; + auto registration = + callback->FindRegistration(native_log_manager, event); + if (registration == callback->registrations.end()) + { + return callback->HasLiveRegistrations() ? identity : -1; + } + if (registration->state == + JniDebugEventListener::Registration::State::Adding) + { + registration->state = + JniDebugEventListener::Registration::State::Cancelled; + return callback->HasLiveRegistrations() ? identity : -1; + } + if (registration->state != + JniDebugEventListener::Registration::State::Active) + { + return callback->HasLiveRegistrations() ? identity : -1; + } + registration->state = + JniDebugEventListener::Registration::State::Removing; + } + + if (logManager != nullptr) { - return; + logManager->RemoveEventListener(event, *callback); } - auto logManager = getLogManager(native_log_manager); - logManager->RemoveEventListener(static_cast(eventType), *listeners[identity]); + + std::lock_guard lock(listeners_mutex); + auto registration = callback->FindRegistration(native_log_manager, event); + if (registration != callback->registrations.end() && + registration->state == + JniDebugEventListener::Registration::State::Removing) + { + callback->registrations.erase(registration); + } + if (listeners[identity] == callback && + callback->registrations.empty() && + !IsDebugEventListenerPending(callback.get())) + { + listeners[identity].reset(); + } + return callback->HasLiveRegistrations() ? identity : -1; } extern "C" diff --git a/lib/jni/PrivacyGuard_jni.cpp b/lib/jni/PrivacyGuard_jni.cpp index 8fd23867a..1cbe9e96c 100644 --- a/lib/jni/PrivacyGuard_jni.cpp +++ b/lib/jni/PrivacyGuard_jni.cpp @@ -7,6 +7,10 @@ #include "modules/privacyguard/PrivacyGuard.hpp" #include "PrivacyGuardHelper.hpp" +#include +#include +#include + using namespace MAT; CommonDataContext GenerateCommonDataContextObject(JNIEnv* env, @@ -22,25 +26,59 @@ CommonDataContext GenerateCommonDataContextObject(JNIEnv* env, CommonDataContext cdc; if(domainName != nullptr) { cdc.DomainName = JStringToStdString(env, domainName); + if (env->ExceptionCheck()) { + return cdc; + } } if(machineName != nullptr) { cdc.MachineName = JStringToStdString(env, machineName); + if (env->ExceptionCheck()) { + return cdc; + } } cdc.UserNames = ConvertJObjectArrayToStdStringVector(env, userNames); + if (env->ExceptionCheck()) { + return cdc; + } cdc.UserAliases = ConvertJObjectArrayToStdStringVector(env, userAliases); + if (env->ExceptionCheck()) { + return cdc; + } cdc.IpAddresses = ConvertJObjectArrayToStdStringVector(env, ipAddresses); + if (env->ExceptionCheck()) { + return cdc; + } cdc.LanguageIdentifiers = ConvertJObjectArrayToStdStringVector(env, languageIdentifiers); + if (env->ExceptionCheck()) { + return cdc; + } cdc.MachineIds = ConvertJObjectArrayToStdStringVector(env, machineIds); + if (env->ExceptionCheck()) { + return cdc; + } cdc.OutOfScopeIdentifiers = ConvertJObjectArrayToStdStringVector(env, outOfScopeIdentifiers); return cdc; } -std::shared_ptr spPrivacyGuard; +namespace +{ + // PrivacyGuard borrows its configured event names, so keep their storage with the guard. + struct PrivacyGuardState + { + std::string notificationEventName; + std::string semanticContextEventName; + std::string summaryEventName; + std::unique_ptr privacyGuard; + }; + + std::shared_ptr spPrivacyGuard; + std::mutex privacyGuardMutex; +} std::shared_ptr PrivacyGuardHelper::GetPrivacyGuardPtr() noexcept { - return spPrivacyGuard; + return std::atomic_load(&spPrivacyGuard); } extern "C" @@ -55,23 +93,37 @@ Java_com_microsoft_applications_events_PrivacyGuard_nativeInitializePrivacyGuard jboolean ScanForUrls, jboolean DisableAdvancedScans, jboolean StampEventIKeyForConcerns) { - if (spPrivacyGuard != nullptr) { + std::lock_guard lock(privacyGuardMutex); + if (std::atomic_load(&spPrivacyGuard) != nullptr) { return false; } InitializationConfiguration config( reinterpret_cast(iLoggerNativePtr), CommonDataContext{}); + auto state = std::make_shared(); if (NotificationEventName != nullptr) { - config.NotificationEventName = JStringToStdString(env, NotificationEventName).c_str(); + state->notificationEventName = JStringToStdString(env, NotificationEventName); + if (env->ExceptionCheck()) { + return false; + } + config.NotificationEventName = state->notificationEventName.c_str(); } if (SemanticContextEventName != nullptr) { - config.SemanticContextNotificationEventName = JStringToStdString(env, SemanticContextEventName).c_str(); + state->semanticContextEventName = JStringToStdString(env, SemanticContextEventName); + if (env->ExceptionCheck()) { + return false; + } + config.SemanticContextNotificationEventName = state->semanticContextEventName.c_str(); } if (SummaryEventName != nullptr) { - config.SummaryEventName = JStringToStdString(env, SummaryEventName).c_str(); + state->summaryEventName = JStringToStdString(env, SummaryEventName); + if (env->ExceptionCheck()) { + return false; + } + config.SummaryEventName = state->summaryEventName.c_str(); } config.UseEventFieldPrefix = static_cast(UseEventFieldPrefix); @@ -79,7 +131,10 @@ Java_com_microsoft_applications_events_PrivacyGuard_nativeInitializePrivacyGuard config.DisableAdvancedScans = static_cast(DisableAdvancedScans); config.StampEventIKeyForConcerns = static_cast(StampEventIKeyForConcerns); - spPrivacyGuard = std::make_shared(config); + state->privacyGuard = std::make_unique(config); + std::atomic_store( + &spPrivacyGuard, + std::shared_ptr(state, state->privacyGuard.get())); return true; } @@ -103,32 +158,50 @@ Java_com_microsoft_applications_events_PrivacyGuard_nativeInitializePrivacyGuard jobjectArray languageIdentifiers, jobjectArray machineIds, jobjectArray outOfScopeIdentifiers) { - if (spPrivacyGuard != nullptr) { + std::lock_guard lock(privacyGuardMutex); + if (std::atomic_load(&spPrivacyGuard) != nullptr) { return false; } + auto commonDataContext = GenerateCommonDataContextObject(env, + domainName, + machineName, + userNames, + userAliases, + ipAddresses, + languageIdentifiers, + machineIds, + outOfScopeIdentifiers); + if (env->ExceptionCheck()) { + return false; + } InitializationConfiguration config( reinterpret_cast(iLoggerNativePtr), - GenerateCommonDataContextObject(env, - domainName, - machineName, - userNames, - userAliases, - ipAddresses, - languageIdentifiers, - machineIds, - outOfScopeIdentifiers)); + commonDataContext); + auto state = std::make_shared(); if (NotificationEventName != NULL) { - config.NotificationEventName = JStringToStdString(env, NotificationEventName).c_str(); + state->notificationEventName = JStringToStdString(env, NotificationEventName); + if (env->ExceptionCheck()) { + return false; + } + config.NotificationEventName = state->notificationEventName.c_str(); } if (SemanticContextEventName != NULL) { - config.SemanticContextNotificationEventName = JStringToStdString(env, SemanticContextEventName).c_str(); + state->semanticContextEventName = JStringToStdString(env, SemanticContextEventName); + if (env->ExceptionCheck()) { + return false; + } + config.SemanticContextNotificationEventName = state->semanticContextEventName.c_str(); } if (SummaryEventName != NULL) { - config.SummaryEventName = JStringToStdString(env, SummaryEventName).c_str(); + state->summaryEventName = JStringToStdString(env, SummaryEventName); + if (env->ExceptionCheck()) { + return false; + } + config.SummaryEventName = state->summaryEventName.c_str(); } config.UseEventFieldPrefix = static_cast(UseEventFieldPrefix); @@ -136,7 +209,10 @@ Java_com_microsoft_applications_events_PrivacyGuard_nativeInitializePrivacyGuard config.DisableAdvancedScans = static_cast(DisableAdvancedScans); config.StampEventIKeyForConcerns = static_cast(StampEventIKeyForConcerns); - spPrivacyGuard = std::make_shared(config); + state->privacyGuard = std::make_unique(config); + std::atomic_store( + &spPrivacyGuard, + std::shared_ptr(state, state->privacyGuard.get())); return true; } @@ -144,12 +220,13 @@ extern "C" JNIEXPORT jboolean JNICALL Java_com_microsoft_applications_events_PrivacyGuard_uninitialize(const JNIEnv *env, jclass /*this*/) { - if(spPrivacyGuard == nullptr) + std::lock_guard lock(privacyGuardMutex); + if (std::atomic_load(&spPrivacyGuard) == nullptr) { return false; } - spPrivacyGuard.reset(); + std::atomic_store(&spPrivacyGuard, std::shared_ptr{}); return true; } @@ -157,17 +234,19 @@ Java_com_microsoft_applications_events_PrivacyGuard_uninitialize(const JNIEnv *e extern "C" JNIEXPORT jboolean JNICALL Java_com_microsoft_applications_events_PrivacyGuard_setEnabled(const JNIEnv *env, jclass /*this*/, jboolean isEnabled) { - if (spPrivacyGuard == nullptr) { + auto privacyGuard = PrivacyGuardHelper::GetPrivacyGuardPtr(); + if (privacyGuard == nullptr) { return false; } - spPrivacyGuard->SetEnabled(static_cast(isEnabled)); + privacyGuard->SetEnabled(static_cast(isEnabled)); return true; } extern "C" JNIEXPORT jboolean JNICALL Java_com_microsoft_applications_events_PrivacyGuard_isEnabled(const JNIEnv *env, jclass /*this*/) { - return spPrivacyGuard != nullptr && spPrivacyGuard->IsEnabled(); + auto privacyGuard = PrivacyGuardHelper::GetPrivacyGuardPtr(); + return privacyGuard != nullptr && privacyGuard->IsEnabled(); } extern "C" @@ -182,20 +261,25 @@ Java_com_microsoft_applications_events_PrivacyGuard_nativeAppendCommonDataContex jobjectArray languageIdentifiers, jobjectArray machineIds, jobjectArray outOfScopeIdentifiers) { - if (spPrivacyGuard == nullptr) { + auto privacyGuard = PrivacyGuardHelper::GetPrivacyGuardPtr(); + if (privacyGuard == nullptr) { return false; } - spPrivacyGuard->AppendCommonDataContext(GenerateCommonDataContextObject(env, - domainName, - machineName, - userNames, - userAliases, - ipAddresses, - languageIdentifiers, - machineIds, - outOfScopeIdentifiers)); + auto commonDataContext = GenerateCommonDataContextObject(env, + domainName, + machineName, + userNames, + userAliases, + ipAddresses, + languageIdentifiers, + machineIds, + outOfScopeIdentifiers); + if (env->ExceptionCheck()) { + return false; + } + privacyGuard->AppendCommonDataContext(commonDataContext); return true; } @@ -206,19 +290,28 @@ Java_com_microsoft_applications_events_PrivacyGuard_nativeAddIgnoredConcern(JNIE jstring eventName, jstring fieldName, jint dataConcern) { - if (spPrivacyGuard == nullptr) { + auto privacyGuard = PrivacyGuardHelper::GetPrivacyGuardPtr(); + if (privacyGuard == nullptr) { return; } auto eventNameStr = JStringToStdString(env, eventName); + if (env->ExceptionCheck()) { + return; + } auto fieldNameStr = JStringToStdString(env, fieldName); + if (env->ExceptionCheck()) { + return; + } auto dataConcernInt = static_cast(dataConcern); - spPrivacyGuard->AddIgnoredConcern(eventNameStr, fieldNameStr, static_cast(dataConcernInt)); + privacyGuard->AddIgnoredConcern( + eventNameStr, + fieldNameStr, + static_cast(dataConcernInt)); } extern "C" JNIEXPORT jboolean JNICALL Java_com_microsoft_applications_events_PrivacyGuard_isInitialized(const JNIEnv *env, jclass/* this */){ - return spPrivacyGuard != nullptr; + return PrivacyGuardHelper::GetPrivacyGuardPtr() != nullptr; } - diff --git a/tests/unittests/DebugEventSourceTests.cpp b/tests/unittests/DebugEventSourceTests.cpp index 5797cd83e..cf725d6db 100644 --- a/tests/unittests/DebugEventSourceTests.cpp +++ b/tests/unittests/DebugEventSourceTests.cpp @@ -188,6 +188,27 @@ TEST(DebugEventSourceTests, DispatchEvent_TwoEventsOneSameAsListenerType_Listene ASSERT_EQ(countOfEventsSeen, uint64_t { 1 }); } +TEST(DebugEventSourceTests, DispatchEvent_ListenerRemovesLaterListener_SnapshotRemainsValid) +{ + TestDebugEventSource source; + TestDebugEventListener removingListener; + TestDebugEventListener removedListener; + uint64_t removedListenerCalls {}; + removingListener.OnDebugEventOverride = [&](DebugEvent&) noexcept { + source.RemoveEventListener(EVT_LOG_EVENT, removedListener); + }; + removedListener.OnDebugEventOverride = [&](DebugEvent&) noexcept { + removedListenerCalls++; + }; + source.AddEventListener(EVT_LOG_EVENT, removingListener); + source.AddEventListener(EVT_LOG_EVENT, removedListener); + + source.DispatchEvent(DebugEvent { EVT_LOG_EVENT }); + source.DispatchEvent(DebugEvent { EVT_LOG_EVENT }); + + ASSERT_EQ(removedListenerCalls, uint64_t { 1 }); +} + TEST(DebugEventSourceTests, DispatchEvent_OneEventToCascaded_ListenerSeesOneEvent) { TestDebugEventSource source; @@ -217,4 +238,3 @@ TEST(DebugEventSourceTests, DispatchEvent_OneEventToCascadedAndToSource_Listener ASSERT_EQ(sequenceNumberToCountMap[1], uint64_t { 2 }); } - diff --git a/tests/unittests/TransmissionPolicyManagerTests.cpp b/tests/unittests/TransmissionPolicyManagerTests.cpp index 6cbdb99f5..c2ce2c3ae 100644 --- a/tests/unittests/TransmissionPolicyManagerTests.cpp +++ b/tests/unittests/TransmissionPolicyManagerTests.cpp @@ -274,7 +274,7 @@ TEST_F(TransmissionPolicyManagerTests, UploadPostponedWithInsufficientAvailableB .WillOnce(Return(999999)); EXPECT_CALL(tpm, scheduleUpload(1000, EventLatency_Normal, false)) .WillOnce(Return()); - tpm.uploadAsync(EventLatency_Normal); + tpm.uploadAsyncParent(EventLatency_Normal); EXPECT_THAT(tpm.uploadScheduled(), false); } @@ -288,7 +288,7 @@ TEST_F(TransmissionPolicyManagerTests, UploadInitiatesUpload) EventsUploadContextPtr upload; EXPECT_CALL(*this, resultInitiateUpload(_)) .WillOnce(SaveArg<0>(&upload)); - tpm.uploadAsync(EventLatency_Normal); + tpm.uploadAsyncParent(EventLatency_Normal); EXPECT_THAT(tpm.uploadScheduled(), false); EXPECT_THAT(upload, NotNull());