diff --git a/android/app/src/test/cpp/CMakeLists.txt b/android/app/src/test/cpp/CMakeLists.txt index adf181e40..748227f61 100644 --- a/android/app/src/test/cpp/CMakeLists.txt +++ b/android/app/src/test/cpp/CMakeLists.txt @@ -18,3 +18,12 @@ add_executable(mpv_utf8_convert_test mpv_utf8_convert_test.cpp) target_compile_features(mpv_utf8_convert_test PRIVATE cxx_std_17) add_test(NAME mpv_utf8_convert_test COMMAND mpv_utf8_convert_test) + +find_package(Threads REQUIRED) +add_executable(mpv_lifecycle_test mpv_lifecycle_test.cpp) +target_compile_features(mpv_lifecycle_test PRIVATE cxx_std_17) +target_include_directories(mpv_lifecycle_test PRIVATE fakes ../../../../libmpv/src/main/cpp/include) +target_link_libraries(mpv_lifecycle_test PRIVATE Threads::Threads) + +add_test(NAME mpv_lifecycle_test COMMAND mpv_lifecycle_test) +set_tests_properties(mpv_lifecycle_test PROPERTIES TIMEOUT 30) diff --git a/android/app/src/test/cpp/fakes/android/log.h b/android/app/src/test/cpp/fakes/android/log.h index 523a7c228..d53a0bcf0 100644 --- a/android/app/src/test/cpp/fakes/android/log.h +++ b/android/app/src/test/cpp/fakes/android/log.h @@ -2,6 +2,7 @@ #define ANDROID_LOG_INFO 4 #define ANDROID_LOG_WARN 5 +#define ANDROID_LOG_ERROR 6 #ifdef __cplusplus extern "C" { diff --git a/android/app/src/test/cpp/fakes/jni.h b/android/app/src/test/cpp/fakes/jni.h index e63d3dd50..b3a41bb0a 100644 --- a/android/app/src/test/cpp/fakes/jni.h +++ b/android/app/src/test/cpp/fakes/jni.h @@ -1,5 +1,6 @@ #pragma once +#include #include #include #include @@ -14,6 +15,10 @@ using jboolean = uint8_t; using jbyte = int8_t; using jint = int32_t; using jsize = jint; +using jlong = int64_t; +using jdouble = double; +using jobject = void*; +using jmethodID = void*; struct _jclass {}; using jclass = _jclass*; @@ -28,11 +33,53 @@ struct _jstring { }; using jstring = _jstring*; +struct _jobjectArray { + std::vector objects; +}; +using jobjectArray = _jobjectArray*; + +class JavaVM { + public: + void (*on_detach)() = nullptr; + + jint DetachCurrentThread() { + if (on_detach) on_detach(); + return 0; + } +}; + class JNIEnv { public: bool exception_pending = false; bool fail_next_write = false; + JavaVM* vm = nullptr; + jobject (*on_new_global_ref)(jobject) = nullptr; + void (*on_delete_global_ref)(jobject) = nullptr; + void (*on_static_void_method)(jmethodID, va_list) = nullptr; + + jint GetJavaVM(JavaVM** result) { + *result = vm; + return 0; + } + + jobject NewGlobalRef(jobject object) { return on_new_global_ref ? on_new_global_ref(object) : object; } + + void DeleteGlobalRef(jobject object) { + if (on_delete_global_ref) on_delete_global_ref(object); + } + + void DeleteLocalRef(jobject) {} + + void CallStaticVoidMethod(jclass, jmethodID method, ...) { + va_list args; + va_start(args, method); + if (on_static_void_method) on_static_void_method(method, args); + va_end(args); + } + + jsize GetArrayLength(jobjectArray array) { return static_cast(array->objects.size()); } + jobject GetObjectArrayElement(jobjectArray array, jsize index) { return array->objects.at(index); } jsize GetArrayLength(jbyteArray array) { return static_cast(array->bytes.size()); } void GetByteArrayRegion(jbyteArray array, jsize offset, jsize length, jbyte* destination) { diff --git a/android/app/src/test/cpp/mpv_lifecycle_test.cpp b/android/app/src/test/cpp/mpv_lifecycle_test.cpp new file mode 100644 index 000000000..96d4482ba --- /dev/null +++ b/android/app/src/test/cpp/mpv_lifecycle_test.cpp @@ -0,0 +1,521 @@ +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +static int tracked_mutex_lock(pthread_mutex_t* mutex); +static int tracked_write_lock(pthread_rwlock_t* lock); +static int controlled_thread_create(pthread_t* thread, const pthread_attr_t* attr, void* (*entry)(void*), void* arg); + +// Compile the real JNI entries, admission guards, event loop and surface +// cleanup. Only external dependencies are controlled; no copy of the lock +// algorithm or direct assignment to production lifecycle globals is used. +#define UTIL_EXTERN +#define pthread_mutex_lock tracked_mutex_lock +#define pthread_rwlock_wrlock tracked_write_lock +#define pthread_create controlled_thread_create +// Android's two-argument thread naming API is not available on macOS. +#define pthread_setname_np(thread, name) ((void)0) +#include "../../../../libmpv/src/main/cpp/main.cpp" +#undef pthread_setname_np +#undef pthread_create +#undef pthread_rwlock_wrlock +#undef pthread_mutex_lock +#include "../../../../libmpv/src/main/cpp/event.cpp" +#include "../../../../libmpv/src/main/cpp/render.cpp" + +struct mpv_handle { + bool initialized = false; + bool terminated = false; + bool event_started = false; + bool event_exited = false; + bool woken = false; + bool hook_pending = false; + bool reject_hook_during_destroy = false; + bool callback_entered = false; + bool callback_returned = false; + bool command_active = false; + bool termination_entered = false; + int initialize_result = 0; + int hook_continuations = 0; + int commands = 0; + jlong callback_session = 0; + jobject video = nullptr; + jobject osd = nullptr; + mpv_event_hook hook{"on_preloaded", 17}; + mpv_event event{}; +}; + +namespace { + +std::mutex gate; +std::condition_variable changed; +JavaVM vm; +JNIEnv jni; +int app_context; +mpv_handle* active_handle = nullptr; +// Retain fake allocations after termination so any erroneous late MPV access +// fails explicitly rather than depending on allocator reuse or undefined UAF. +std::vector> handles; +std::vector> callback_strings; +std::map global_refs; +mpv_handle* held_termination = nullptr; +bool allow_termination = false; +bool allow_command = false; +bool successor_waiting = false; +bool reader_draining = false; +bool fail_thread_create = false; +thread_local mpv_handle* event_handle = nullptr; +enum class Operation { ordinary, successor, replacing_reader }; +thread_local Operation operation = Operation::ordinary; + +void require(bool condition, const char* message) { + if (condition) return; + std::fprintf(stderr, "%s\n", message); + std::abort(); +} + +template +void await(std::unique_lock& lock, Predicate predicate, const char* message) { + require(changed.wait_for(lock, std::chrono::seconds(5), predicate), message); +} + +void require_live(mpv_handle* handle) { + require(handle && handle == active_handle && !handle->terminated, "MPV accessed outside its native lifetime"); +} + +void require_surfaces(mpv_handle* handle) { + if (handle->video) require(global_refs[handle->video] == 1, "video Surface released before termination completed"); + if (handle->osd) require(global_refs[handle->osd] == 1, "OSD Surface released before termination completed"); +} + +jobject new_global_ref(jobject object) { + std::lock_guard lock(gate); + if (object) ++global_refs[object]; + return object; +} + +void delete_global_ref(jobject object) { + if (!object) return; + std::lock_guard lock(gate); + require(global_refs[object] > 0, "JNI global reference released more than once"); + if (active_handle && (active_handle->video == object || active_handle->osd == object)) { + require(active_handle->terminated, "Surface cleanup preceded native termination"); + } + --global_refs[object]; +} + +void detach_event_thread() { + std::lock_guard lock(gate); + require_live(event_handle); + event_handle->event_exited = true; + changed.notify_all(); +} + +void on_static_void_method(jmethodID method, va_list args) { + require(method == mpv_MpvPlayer_onHook, "unexpected callback in lifecycle scenario"); + const jlong session = va_arg(args, jlong); + const jstring name = va_arg(args, jstring); + const jlong hook = va_arg(args, jlong); + require(name->value == "on_preloaded", "wrong hook callback delivered"); + { + std::unique_lock lock(gate); + require_live(event_handle); + event_handle->callback_session = session; + event_handle->callback_entered = true; + changed.notify_all(); + if (event_handle->reject_hook_during_destroy) { + // The callback is already inside Java when teardown starts. Kotlin's + // rejected-channel path continues synchronously on this event thread. + await(lock, [] { return event_handle->woken; }, "destroy never woke the overlapping hook callback"); + } + } + jni_func_name(nativeHookContinue)(&jni, nullptr, session, hook); + { + std::lock_guard lock(gate); + event_handle->callback_returned = true; + changed.notify_all(); + } +} + +jlong create_player() { + const jlong session = jni_func_name(nativeCreate)(&jni, nullptr, &app_context); + require(session > 0, "nativeCreate failed"); + return session; +} + +jlong command(jlong session, const char* name = "play") { + _jstring argument{name}; + _jobjectArray arguments{{&argument}}; + return jni_func_name(nativeCommand)(&jni, nullptr, session, &arguments); +} + +void initialize_player(jlong session) { + require(jni_func_name(nativeInit)(&jni, nullptr, session) == 0, "nativeInit failed"); +} + +void destroy_player(jlong session) { jni_func_name(nativeDestroy)(&jni, nullptr, session); } + +void reset_dependencies() { + std::lock_guard lock(gate); + require(active_handle == nullptr, "previous test left a live MPV instance"); + for (const auto& entry : global_refs) { + require(entry.first == &app_context || entry.second == 0, "terminal Surface cleanup leaked a global reference"); + } + handles.clear(); + callback_strings.clear(); + global_refs.clear(); + held_termination = nullptr; + allow_termination = false; + allow_command = false; + successor_waiting = false; + reader_draining = false; + fail_thread_create = false; + jni.exception_pending = false; +} + +void rejected_hook_and_successor_retirement() { + reset_dependencies(); + const jlong old_session = create_player(); + mpv_handle* old = active_handle; + int video, osd; + jni_func_name(nativeAttachSurface)(&jni, nullptr, old_session, &video); + jni_func_name(nativeAttachOsdSurface)(&jni, nullptr, old_session, &osd); + { + std::lock_guard lock(gate); + old->hook_pending = true; + old->reject_hook_during_destroy = true; + held_termination = old; + } + initialize_player(old_session); + { + std::unique_lock lock(gate); + await(lock, [&] { return old->callback_entered; }, "event loop did not deliver the hook"); + } + std::thread retiring([&] { destroy_player(old_session); }); + { + std::unique_lock lock(gate); + await(lock, [&] { return old->termination_entered; }, "retirement deadlocked with the synchronous hook callback"); + require(old->callback_returned && old->event_exited, "termination preceded event-thread completion"); + require(old->callback_session == old_session, "retiring callback lost its bound session"); + require(old->hook_continuations == 0, "revoked hook reached the retiring MPV handle"); + require_surfaces(old); + } + + jlong successor_session = 0; + std::thread creating([&] { + operation = Operation::successor; + successor_session = create_player(); + }); + { + std::unique_lock lock(gate); + await(lock, [] { return successor_waiting; }, "successor did not wait for terminal retirement"); + } + // S must be available while termination holds L. These calls must reject + // without touching the retiring core, surfaces, or a future successor. + require(command(old_session) == MPV_ERROR_UNINITIALIZED, "revoked command was admitted during termination"); + jni_func_name(nativeHookContinue)(&jni, nullptr, old_session, old->hook.id); + jni_func_name(nativeDetachSurface)(&jni, nullptr, old_session); + jni_func_name(nativeDetachOsdSurface)(&jni, nullptr, old_session); + { + std::lock_guard lock(gate); + require_surfaces(old); + allow_termination = true; + changed.notify_all(); + } + retiring.join(); + creating.join(); + require(successor_session > old_session, "successor did not receive a new session"); + mpv_handle* successor = active_handle; + { + std::lock_guard lock(gate); + successor->hook_pending = true; + } + jni_func_name(nativeAttachSurface)(&jni, nullptr, successor_session, &video); + jni_func_name(nativeAttachOsdSurface)(&jni, nullptr, successor_session, &osd); + initialize_player(successor_session); + { + std::unique_lock lock(gate); + await(lock, [&] { return successor->callback_returned; }, "successor event loop did not serve its own hook"); + require(successor->callback_session == successor_session, "successor inherited the old event binding"); + require(successor->hook_continuations == 1, "live hook was not continued exactly once"); + } + destroy_player(old_session); + require( + jni_func_name(nativeInit)(&jni, nullptr, old_session) == MPV_ERROR_UNINITIALIZED, "stale init reached successor"); + require(command(old_session) == MPV_ERROR_UNINITIALIZED, "stale command reached successor"); + jni_func_name(nativeHookContinue)(&jni, nullptr, old_session, old->hook.id); + jni_func_name(nativeDetachSurface)(&jni, nullptr, old_session); + jni_func_name(nativeDetachOsdSurface)(&jni, nullptr, old_session); + require(command(successor_session) == 0, "stale teardown retired the successor"); + { + std::lock_guard lock(gate); + require_surfaces(successor); + require(successor->hook_continuations == 1, "old hook id was forwarded to successor"); + } + destroy_player(successor_session); +} + +void admitted_command_survives_replacement() { + reset_dependencies(); + const jlong old_session = create_player(); + mpv_handle* old = active_handle; + initialize_player(old_session); + jlong result = MPV_ERROR_GENERIC; + std::thread reader([&] { result = command(old_session, "hold"); }); + { + std::unique_lock lock(gate); + await(lock, [&] { return old->command_active; }, "command was not admitted"); + } + jlong successor_session = 0; + std::thread replacing([&] { + operation = Operation::replacing_reader; + successor_session = create_player(); + }); + { + std::unique_lock lock(gate); + await(lock, [] { return reader_draining; }, "replacement did not drain the admitted command"); + require(!old->termination_entered && !old->woken, "retirement overtook an admitted command"); + allow_command = true; + changed.notify_all(); + } + reader.join(); + replacing.join(); + require(result == 0 && old->commands == 1, "admitted command lost its handle before returning"); + require(command(old_session) == MPV_ERROR_UNINITIALIZED, "late old-session command was admitted"); + destroy_player(old_session); + initialize_player(successor_session); + require(command(successor_session) == 0, "replacement was affected by stale owner traffic"); + destroy_player(successor_session); +} + +void partial_initialization_can_retire() { + enum class Failure { configuration, initialization, thread_start }; + for (Failure failure : {Failure::configuration, Failure::initialization, Failure::thread_start}) { + reset_dependencies(); + const jlong session = create_player(); + mpv_handle* failed = active_handle; + if (failure == Failure::configuration) { + _jstring invalid{"not-a-level"}; + require( + jni_func_name(nativeSetLogLevel)(&jni, nullptr, session, &invalid) < 0, "invalid configuration succeeded"); + } else { + failed->initialize_result = failure == Failure::initialization ? MPV_ERROR_GENERIC : 0; + fail_thread_create = failure == Failure::thread_start; + require(jni_func_name(nativeInit)(&jni, nullptr, session) < 0, "injected initialization failure was lost"); + if (failure == Failure::thread_start) + require(jni.ExceptionCheck(), "thread-start failure lost its JNI exception"); + } + destroy_player(session); + require( + failed->terminated && !failed->event_started && !failed->woken, "partial init used an unstarted event thread"); + require(command(session) == MPV_ERROR_UNINITIALIZED, "failed session retained public admission"); + jni.exception_pending = false; + const jlong successor = create_player(); + initialize_player(successor); + require(command(successor) == 0, "partial init prevented the next player from working"); + destroy_player(successor); + } +} + +} // namespace + +// Observe real contention to arrange overlap without sleeps or a guessed +// scheduling delay. The production guards still acquire the real pthread +// locks; the notification only lets the test release its external MPV gates. +static int tracked_mutex_lock(pthread_mutex_t* mutex) { + if (operation != Operation::successor) return pthread_mutex_lock(mutex); + const int result = pthread_mutex_trylock(mutex); + if (result != EBUSY) return result; + { + std::lock_guard lock(gate); + successor_waiting = true; + changed.notify_all(); + } + return pthread_mutex_lock(mutex); +} + +static int tracked_write_lock(pthread_rwlock_t* lock) { + if (operation != Operation::replacing_reader) return pthread_rwlock_wrlock(lock); + const int result = pthread_rwlock_trywrlock(lock); + if (result != EBUSY) return result; + { + std::lock_guard guard(gate); + reader_draining = true; + changed.notify_all(); + } + return pthread_rwlock_wrlock(lock); +} + +static int controlled_thread_create(pthread_t* thread, const pthread_attr_t* attr, void* (*entry)(void*), void* arg) { + std::lock_guard lock(gate); + if (fail_thread_create) { + fail_thread_create = false; + return EAGAIN; + } + const int result = pthread_create(thread, attr, entry, arg); + if (result == 0) active_handle->event_started = true; + return result; +} + +extern "C" mpv_handle* mpv_create() { + std::lock_guard lock(gate); + require(active_handle == nullptr, "successor MPV creation overlapped predecessor termination"); + for (const auto& entry : global_refs) { + require(entry.first == &app_context || entry.second == 0, "successor creation preceded retiring Surface cleanup"); + } + handles.push_back(std::make_unique()); + active_handle = handles.back().get(); + return active_handle; +} + +extern "C" int mpv_initialize(mpv_handle* handle) { + std::lock_guard lock(gate); + require_live(handle); + if (handle->initialize_result < 0) return handle->initialize_result; + handle->initialized = true; + return 0; +} + +extern "C" void mpv_wakeup(mpv_handle* handle) { + std::lock_guard lock(gate); + require_live(handle); + require(!handle->command_active, "wake overtook an admitted command"); + require(handle->event_started, "wake targeted an unstarted event thread"); + handle->woken = true; + changed.notify_all(); +} + +extern "C" void mpv_terminate_destroy(mpv_handle* handle) { + std::unique_lock lock(gate); + require_live(handle); + require(!handle->command_active, "termination overtook an admitted command"); + require(!handle->event_started || handle->event_exited, "termination overtook the bound event thread"); + require_surfaces(handle); + handle->termination_entered = true; + changed.notify_all(); + if (handle == held_termination) + await(lock, [] { return allow_termination; }, "test did not release native termination"); + require_surfaces(handle); + handle->terminated = true; + active_handle = nullptr; +} + +extern "C" mpv_event* mpv_wait_event(mpv_handle* handle, double) { + std::unique_lock lock(gate); + event_handle = handle; + require_live(handle); + await(lock, [&] { return handle->hook_pending || handle->woken; }, "event loop was not woken for teardown"); + handle->event = {}; + if (handle->hook_pending) { + handle->hook_pending = false; + handle->event.event_id = MPV_EVENT_HOOK; + handle->event.data = &handle->hook; + } + return &handle->event; +} + +extern "C" int mpv_hook_add(mpv_handle* handle, uint64_t, const char*, int) { + std::lock_guard lock(gate); + require_live(handle); + return 0; +} + +extern "C" int mpv_hook_continue(mpv_handle* handle, uint64_t id) { + std::lock_guard lock(gate); + require_live(handle); + require(id == handle->hook.id && handle->hook_continuations == 0, "hook continued twice or on the wrong handle"); + ++handle->hook_continuations; + return 0; +} + +extern "C" int mpv_command_ret(mpv_handle* handle, const char** args, mpv_node* result) { + std::unique_lock lock(gate); + require_live(handle); + require(handle->initialized, "command reached an uninitialized core"); + handle->command_active = true; + changed.notify_all(); + if (std::strcmp(args[0], "hold") == 0) + await(lock, [] { return allow_command; }, "test did not release admitted command"); + require_live(handle); + handle->command_active = false; + ++handle->commands; + *result = {}; + return 0; +} + +extern "C" int mpv_request_log_messages(mpv_handle* handle, const char* level) { + std::lock_guard lock(gate); + require_live(handle); + return std::strcmp(level, "not-a-level") == 0 ? MPV_ERROR_INVALID_PARAMETER : 0; +} + +extern "C" int mpv_set_option(mpv_handle* handle, const char* name, mpv_format format, void* data) { + std::lock_guard lock(gate); + require_live(handle); + require(format == MPV_FORMAT_INT64, "unexpected Surface option format"); + jobject object = reinterpret_cast(static_cast(*static_cast(data))); + if (std::strcmp(name, "wid") == 0) { + handle->video = object; + } else { + require(std::strcmp(name, "vo-mediacodec-osd-surface") == 0, "unexpected Surface option"); + handle->osd = object; + } + return 0; +} + +extern "C" int mpv_get_property(mpv_handle*, const char*, mpv_format, void*) { + require(false, "unexpected property read in lifecycle scenario"); + return MPV_ERROR_GENERIC; +} + +extern "C" const char* mpv_error_string(int) { return "controlled MPV failure"; } +extern "C" void mpv_free_node_contents(mpv_node*) {} +extern "C" int av_jni_set_java_vm(void*, void*) { return 0; } +extern "C" int av_jni_set_android_app_ctx(void*, void*) { return 0; } +extern "C" int __android_log_print(int, const char*, const char*, ...) { return 0; } + +bool acquire_jni_env(JavaVM* supplied_vm, JNIEnv** env) { + require(supplied_vm == &vm, "event thread acquired the wrong Java VM"); + *env = &jni; + return true; +} + +void init_methods_cache(JNIEnv*) { + std::lock_guard lock(gate); + require(active_handle == nullptr, "JNI environment rewritten before predecessor retirement"); + mpv_MpvPlayer_onHook = reinterpret_cast(1); +} + +jstring new_java_string(JNIEnv*, const char* value) { + std::lock_guard lock(gate); + callback_strings.push_back(std::make_unique<_jstring>(_jstring{value ? value : ""})); + return callback_strings.back().get(); +} + +std::string java_string_to_utf8(JNIEnv*, jstring value) { return value ? value->value : ""; } +void die(const char*) { jni.exception_pending = true; } + +int main() { + jni.vm = &vm; + jni.on_new_global_ref = new_global_ref; + jni.on_delete_global_ref = delete_global_ref; + jni.on_static_void_method = on_static_void_method; + vm.on_detach = detach_event_thread; + rejected_hook_and_successor_retirement(); + admitted_command_survives_replacement(); + partial_initialization_can_retire(); + reset_dependencies(); + std::puts("MPV lifecycle: callback overlap, reader lifetime, successor isolation and partial init passed"); + return 0; +} diff --git a/android/libmpv/src/main/cpp/event.cpp b/android/libmpv/src/main/cpp/event.cpp index f90bcf381..adf14292d 100644 --- a/android/libmpv/src/main/cpp/event.cpp +++ b/android/libmpv/src/main/cpp/event.cpp @@ -11,7 +11,8 @@ // starts (event_thread_bind). Kotlin drops a callback whose session is not // the wrapper it published for, so a retiring core's tail — end-file, // property changes, a hook raised just before teardown — can never be read -// as the successor's. +// as the successor's. L keeps this binding immutable through join, including +// the interval after admission is revoked; callbacks never acquire L. static mpv_handle* thread_mpv; static uint64_t thread_session; diff --git a/android/libmpv/src/main/cpp/event.h b/android/libmpv/src/main/cpp/event.h index 54e568a96..b67ffe555 100644 --- a/android/libmpv/src/main/cpp/event.h +++ b/android/libmpv/src/main/cpp/event.h @@ -4,10 +4,10 @@ #include -// Names the session the next event_thread serves. Called under the lifecycle -// write lock before pthread_create, which publishes the values to the thread; -// the thread then never reads the globals, so a successor session replacing -// them cannot redirect it. +// Names the session the next event_thread serves. Called under L and S before +// pthread_create publishes the binding. L prevents rebinding until that thread +// is joined; the thread never reads the live admission slots and can safely +// finish a callback after revocation. Callbacks must never acquire L. void event_thread_bind(mpv_handle* mpv, uint64_t session); void* event_thread(void* arg); diff --git a/android/libmpv/src/main/cpp/globals.h b/android/libmpv/src/main/cpp/globals.h index f97cb870c..9fd7aa5bc 100644 --- a/android/libmpv/src/main/cpp/globals.h +++ b/android/libmpv/src/main/cpp/globals.h @@ -15,10 +15,14 @@ extern JavaVM* g_vm; // session it originated from, so a retired wrapper can neither touch nor be // fed by its successor. // -// g_session_lock: write-held across create/init/destroy, read-held for every -// other entry. A reader admitted under a matching session keeps the handle -// alive until it returns; retirement waits for it, then flips g_session so -// later callers with the old id are refused before they reach mpv. +// g_session_lock (S) guards admission: write-held for publication, init and +// revocation, read-held by every other JNI entry through its last handle use. +// Retirement drains admitted readers and clears these slots under S, then +// releases S BEFORE wake/join/terminate so callbacks can reenter and reject. +// A separate lifecycle mutex (L, private to main.cpp) serializes create/init/ +// destroy through termination AND surface cleanup. Lock order is L -> S; +// readers and event callbacks never acquire L. The event thread borrows its +// immutable bound handle until joined, even after the admission slots clear. extern mpv_handle* g_mpv; extern uint64_t g_session; extern pthread_rwlock_t g_session_lock; diff --git a/android/libmpv/src/main/cpp/main.cpp b/android/libmpv/src/main/cpp/main.cpp index 82a560b6d..fc4e00e30 100644 --- a/android/libmpv/src/main/cpp/main.cpp +++ b/android/libmpv/src/main/cpp/main.cpp @@ -41,20 +41,33 @@ uint64_t g_session; pthread_rwlock_t g_session_lock = PTHREAD_RWLOCK_INITIALIZER; std::atomic g_event_thread_request_exit(false); +// Lifecycle serialization (L) is separate from session admission (S). +// Only nativeCreate/nativeInit/nativeDestroy acquire L, always before S. +static pthread_mutex_t lifecycle_lock = PTHREAD_MUTEX_INITIALIZER; static uint64_t next_session = 0; static pthread_t event_thread_id; static bool event_thread_started = false; -// Write-locked scope for the lifecycle entries; waits for every admitted -// reader to return before the handle can change underneath it. +// Held through retirement, termination and surface cleanup, so a successor +// cannot overlap or rebind the retiring event thread. Callbacks never take L. class LifecycleGuard { public: - LifecycleGuard() { pthread_rwlock_wrlock(&g_session_lock); } - ~LifecycleGuard() { pthread_rwlock_unlock(&g_session_lock); } + LifecycleGuard() { pthread_mutex_lock(&lifecycle_lock); } + ~LifecycleGuard() { pthread_mutex_unlock(&lifecycle_lock); } LifecycleGuard(const LifecycleGuard&) = delete; LifecycleGuard& operator=(const LifecycleGuard&) = delete; }; +// Bounded admission-write scope (S), always nested inside L. Never hold this +// while joining or terminating: an event callback can reenter SessionGuard. +class SessionWriteGuard { + public: + SessionWriteGuard() { pthread_rwlock_wrlock(&g_session_lock); } + ~SessionWriteGuard() { pthread_rwlock_unlock(&g_session_lock); } + SessionWriteGuard(const SessionWriteGuard&) = delete; + SessionWriteGuard& operator=(const SessionWriteGuard&) = delete; +}; + static void prepare_environment(JNIEnv* env, jobject appctx) { setlocale(LC_NUMERIC, "C"); @@ -66,25 +79,27 @@ static void prepare_environment(JNIEnv* env, jobject appctx) { init_methods_cache(env); } -// Caller holds the write lock. Stops the event thread, retires the session -// id (so a late nativeHookContinue / property call for it is refused, not -// delivered to whatever comes next), and tears the core down. mpv itself -// continues any hook the destroyed handle was still holding. -static void destroy_locked(JNIEnv* env) { +// Caller holds L and S(write). Acquiring S drained every admitted JNI reader; +// revoke further admission before letting an in-flight callback finish. +// The lifecycle owner retains the handle and immutable event-thread binding. +static mpv_handle* revoke_locked() { mpv_handle* local_mpv = g_mpv; + g_mpv = NULL; + g_session = 0; + if (event_thread_started) g_event_thread_request_exit = true; + return local_mpv; +} +// Caller holds L, but NOT S. The event thread is the revoked handle's only +// remaining borrower; a rejected hook can take S and return during this join. +static void destroy_locked(JNIEnv* env, mpv_handle* local_mpv) { // Configuration (including an invalid initial log level) can fail before // nativeInit starts the event thread. if (event_thread_started) { - g_event_thread_request_exit = true; mpv_wakeup(local_mpv); pthread_join(event_thread_id, NULL); event_thread_started = false; } - - g_mpv = NULL; - g_session = 0; - // The MediaCodec VO can retain the Surface until final decoder teardown. // Keep its JNI refs alive for the entire blocking termination. mpv_terminate_destroy(local_mpv); @@ -93,13 +108,20 @@ static void destroy_locked(JNIEnv* env) { jni_func(jlong, nativeCreate, jobject appctx) { LifecycleGuard lock; - prepare_environment(env, appctx); - - if (g_mpv) { + mpv_handle* predecessor; + { + SessionWriteGuard admission; + predecessor = revoke_locked(); + } + if (predecessor) { ALOGE("destroying leaked mpv instance"); - destroy_locked(env); + destroy_locked(env, predecessor); } + // Do not rewrite process-wide JNI state while the predecessor can callback. + prepare_environment(env, appctx); + + SessionWriteGuard admission; g_mpv = mpv_create(); if (!g_mpv) { die("context init failed"); @@ -113,6 +135,7 @@ jni_func(jlong, nativeCreate, jobject appctx) { jni_func(jint, nativeInit, jlong session) { LifecycleGuard lock; + SessionWriteGuard admission; if (!g_mpv || g_session != (uint64_t)session) return MPV_ERROR_UNINITIALIZED; const int result = mpv_initialize(g_mpv); @@ -140,10 +163,15 @@ jni_func(jint, nativeInit, jlong session) { jni_func(void, nativeDestroy, jlong session) { LifecycleGuard lock; - // A wrapper whose session a later nativeCreate already retired has nothing - // left to destroy; the successor is not its to touch. - if (!g_mpv || g_session != (uint64_t)session) return; - destroy_locked(env); + mpv_handle* local_mpv; + { + SessionWriteGuard admission; + // A wrapper whose session a later nativeCreate already retired has nothing + // left to destroy; the successor is not its to touch. + if (!g_mpv || g_session != (uint64_t)session) return; + local_mpv = revoke_locked(); + } + destroy_locked(env, local_mpv); } jni_func(jint, nativeSetLogLevel, jlong session, jstring jlevel) { @@ -202,9 +230,9 @@ jni_func(jlong, nativeCommand, jlong session, jobjectArray jarray) { return playlist_entry_id; } -// A continuation for a retired session is dropped: the hook id belongs to a -// handle that no longer exists, and destroying that handle already continued -// it. The successor must never receive it. +// A continuation for a revoked session is dropped, even while its handle is +// still retiring. Destruction releases any outstanding hooks; the successor +// must never receive an old hook id. jni_func(void, nativeHookContinue, jlong session, jlong id) { SessionGuard guard(session); if (!guard.mpv) return; diff --git a/android/libmpv/src/main/cpp/render.cpp b/android/libmpv/src/main/cpp/render.cpp index 2e1c6aebc..77bc7ced3 100644 --- a/android/libmpv/src/main/cpp/render.cpp +++ b/android/libmpv/src/main/cpp/render.cpp @@ -70,8 +70,9 @@ jni_func(void, nativeDetachOsdSurface, jlong session) { osd_surface = NULL; } -// Caller holds the lifecycle write lock; the surfaces belong to the session -// being retired and are released with it. +// Caller holds L after revoking admission, draining JNI readers, joining the +// event thread and terminating mpv. S is not held. L prevents a successor from +// publishing new surfaces until these retiring references have been released. void render_cleanup(JNIEnv* env) { if (surface) { env->DeleteGlobalRef(surface);