diff --git a/jsbindings/jsbindings.cpp b/jsbindings/jsbindings.cpp index 62a6c07120..d841708d2c 100644 --- a/jsbindings/jsbindings.cpp +++ b/jsbindings/jsbindings.cpp @@ -4926,8 +4926,13 @@ void __attribute__ ((destructor(101))) _aamp_term() LOG_WARN_EX("stopped aamp"); delete _allocated_aamp; _allocated_aamp = NULL; + //Clear any active js mediaplayer instances on term. + // Guarded here: when aamp_UnloadJSController() has already been called from + // main() (the expected path), _allocated_aamp is NULL and we skip this call + // entirely. That matters because destructor(101) fires after C++ static + // object destructors, so AAMPMediaPlayer_JS::_jsMediaPlayerInstances may + // already be destroyed — accessing it then would be undefined behaviour. + ClearAAMPPlayerInstances(); } - //Clear any active js mediaplayer instances on term - ClearAAMPPlayerInstances(); } #endif diff --git a/priv_aamp.cpp b/priv_aamp.cpp index 929562b13a..eb75a64c5c 100644 --- a/priv_aamp.cpp +++ b/priv_aamp.cpp @@ -1990,10 +1990,37 @@ PrivateInstanceAAMP::PrivateInstanceAAMP(AampConfig *config) : mReportProgressPo /** * @brief PrivateInstanceAAMP Destructor */ +/** + * @brief Reports an exception escaping teardown of the pause position monitor. + * Has external linkage and a stable name so a debugger breakpoint can be set on it; the marker + * line is written straight to stderr so test harnesses see it regardless of AAMP log level. + */ +void AampTeardownExceptionHook(int playerId, const char *what, bool stillJoinable) +{ + fprintf(stderr, "AAMP_TEARDOWN_EXCEPTION: PLAYER[%d] StopPausePositionMonitoring threw during destruction: %s%s\n", + playerId, what, stillJoinable ? " (monitor thread still joinable, std::terminate will follow)" : ""); + fflush(stderr); + AAMPLOG_ERR("AAMP_TEARDOWN_EXCEPTION: PLAYER[%d] %s", playerId, what); +} + PrivateInstanceAAMP::~PrivateInstanceAAMP() { mAampTrackWorkerManager.reset(); - StopPausePositionMonitoring("AAMP destroyed"); + // Last resort: StopPausePositionMonitoring() should not throw, but an intermittent + // teardown abort was still observed. Report it loudly instead of letting it escape this noexcept + // destructor. If the thread is still joinable, its member destructor will terminate anyway. + try + { + StopPausePositionMonitoring("AAMP destroyed"); + } + catch (const std::exception &e) + { + AampTeardownExceptionHook(mPlayerId, e.what(), mPausePositionMonitoringThreadID.joinable()); + } + catch (...) + { + AampTeardownExceptionHook(mPlayerId, "unknown exception", mPausePositionMonitoringThreadID.joinable()); + } PlayerCCManager::GetInstance()->Release(mCCId); mCCId = 0; { @@ -2255,7 +2282,10 @@ void PrivateInstanceAAMP::RunPausePositionMonitoring(void) */ void PrivateInstanceAAMP::StartPausePositionMonitoring(long long pausePositionMilliseconds) { - StopPausePositionMonitoring("Start new pos monitor"); + // Serialize with StopPausePositionMonitoring() so a monitor cannot be (re)started while another + // thread is stopping/joining it, which would leave a joinable thread behind at teardown. + std::lock_guard lifecycleLock(mPausePositionLifecycleMutex); + StopPausePositionMonitoringLocked("Start new pos monitor"); if (pausePositionMilliseconds < 0) { @@ -2291,20 +2321,32 @@ void PrivateInstanceAAMP::StartPausePositionMonitoring(long long pausePositionMi * @brief stop the PausePositionMonitoring thread used for PauseAt functionality */ void PrivateInstanceAAMP::StopPausePositionMonitoring(std::string reason) +{ + std::lock_guard lifecycleLock(mPausePositionLifecycleMutex); + StopPausePositionMonitoringLocked(reason); +} + +/** + * @brief cancel and always join the PausePositionMonitoring thread; caller holds mPausePositionLifecycleMutex + */ +void PrivateInstanceAAMP::StopPausePositionMonitoringLocked(const std::string &reason) { if (mPausePositionMonitoringThreadID.joinable()) { - std::unique_lock lock(mPausePositionMonitorMutex); - - if (mPausePositionMilliseconds != AAMP_PAUSE_POSITION_INVALID_POSITION) { - long long positionMs = GetPositionMilliseconds(); - AAMPLOG_WARN("PLAYER[%d] Stop position monitoring, reason: '%s', current position %lldms, requested position %lldms, rate %f", - mPlayerId, reason.c_str(), positionMs, mPausePositionMilliseconds.load(), rate); - mPausePositionMilliseconds = AAMP_PAUSE_POSITION_INVALID_POSITION; - mPausePositionMonitorCV.notify_one(); + // Cancel under the monitor mutex so the worker cannot miss the wake-up between its + // predicate check and wait_until(). The mutex is a member and valid for the object's lifetime. + std::lock_guard lock(mPausePositionMonitorMutex); + if (mPausePositionMilliseconds != AAMP_PAUSE_POSITION_INVALID_POSITION) + { + long long positionMs = GetPositionMilliseconds(); + AAMPLOG_WARN("PLAYER[%d] Stop position monitoring, reason: '%s', current position %lldms, requested position %lldms, rate %f", + mPlayerId, reason.c_str(), positionMs, mPausePositionMilliseconds.load(), rate); + mPausePositionMilliseconds = AAMP_PAUSE_POSITION_INVALID_POSITION; + } + mPausePositionMonitorCV.notify_all(); } - lock.unlock(); + // Never detach: the worker uses this object, so it must be joined before the owner can be destroyed. mPausePositionMonitoringThreadID.join(); AAMPLOG_TRACE("joined PositionMonitor"); } diff --git a/priv_aamp.h b/priv_aamp.h index 1d85a2034c..ffc99550e8 100644 --- a/priv_aamp.h +++ b/priv_aamp.h @@ -3858,6 +3858,7 @@ class PrivateInstanceAAMP : public DrmCallbacks, public std::enable_shared_from_ * @param[in] reason - Reason why the pause position monitoring is stopped */ void StopPausePositionMonitoring(std::string reason); + void StopPausePositionMonitoringLocked(const std::string &reason); /** * @fn WaitForDiscontinuityProcessToComplete @@ -4467,6 +4468,7 @@ class PrivateInstanceAAMP : public DrmCallbacks, public std::enable_shared_from_ */ void StopLatencyMonitor(); + std::mutex mPausePositionLifecycleMutex; // Serializes start/stop/join of the PausePositionMonitoring thread std::mutex mPausePositionMonitorMutex; // Mutex lock for PausePosition condition variable std::condition_variable mPausePositionMonitorCV; // Condition Variable to signal to stop PausePosition monitoring std::thread mPausePositionMonitoringThreadID; // Thread Id of the PausePositionMonitoring thread diff --git a/test/jsBindingTest/UVEBinding.cpp b/test/jsBindingTest/UVEBinding.cpp index 7f080c1b2d..27f0d36c3b 100644 --- a/test/jsBindingTest/UVEBinding.cpp +++ b/test/jsBindingTest/UVEBinding.cpp @@ -38,6 +38,7 @@ // AAMP JS registration extern "C" void aamp_LoadJSController(JSGlobalContextRef context); +extern "C" void aamp_UnloadJSController(JSGlobalContextRef context); static GMainLoop* gMainLoop = nullptr; @@ -924,6 +925,7 @@ static int main_func(int argc, char** argv) if (!evaluateGlobalScript(ctx, preloads[i - 1], argv[i])) { interval_shutdown(); http_shutdown(); + aamp_UnloadJSController(ctx); JSGlobalContextRelease(ctx); return 1; } @@ -954,6 +956,12 @@ static int main_func(int argc, char** argv) g_main_loop_unref(gMainLoop); interval_shutdown(); http_shutdown(); + // Explicitly tear down the AAMP player while C++ statics (gMutex, gActivePrivAAMPs, + // PlayerCCManager, AampStreamSinkManager, ...) are still alive. _aamp_term() is a + // __attribute__((destructor(101))) that fires very late — after those statics are + // destroyed — so it must find _allocated_aamp already NULL or it will crash trying + // to lock a destroyed mutex (SIGABRT / std::terminate). + aamp_UnloadJSController(ctx); JSGlobalContextRelease(ctx); return 0;