VPAAMP-1495 uveExecuter quit unexpectedly

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

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<std::mutex> 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<std::mutex> 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<std::mutex> 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<std::mutex> 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");
 	}

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

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;