From 8bb895588ab6d7d0d931d5b7fed16bcc21022373 Mon Sep 17 00:00:00 2001 From: Jose Fagundez Date: Thu, 14 May 2026 09:28:20 +0200 Subject: [PATCH 1/5] SERXIONE-8617 Fix crash in AAMPGstPlayer::Pause when receiving EAS alert Reason for Change: Fix crash in AAMPGstPlayer::Pause Summary of Changes: - Add protection against null playerInstance to AAMPGstPlayer::Pause - Lock sync mutex in AAMPGstPlayer::Stop - Test changes in L1 tests - Add mocks for SyncBegin and SyncEnd Test Procedure: Send EAS alerts and confirm there is no crash Priority: P0 Risks: Low --- aampgstplayer.cpp | 24 +++++++--- test/utests/fakes/FakePrivateInstanceAAMP.cpp | 8 ++++ test/utests/mocks/MockPrivateInstanceAAMP.h | 2 + .../tests/AampGstPlayer/FunctionalTests.cpp | 46 +++++++++++++++++++ 4 files changed, 73 insertions(+), 7 deletions(-) diff --git a/aampgstplayer.cpp b/aampgstplayer.cpp index 9512db260b..5eacf371c7 100644 --- a/aampgstplayer.cpp +++ b/aampgstplayer.cpp @@ -881,12 +881,14 @@ void AAMPGstPlayer::EndOfStreamReached(AampMediaType type) */ void AAMPGstPlayer::Stop(bool keepLastFrame) { + aamp->SyncBegin(); AAMPLOG_MIL("entering AAMPGstPlayer_Stop keepLastFrame %d", keepLastFrame); StopMonitorAvTimer(); playerInstance->Stop(keepLastFrame); aamp->seiTimecode.assign(""); AAMPLOG_MIL("exiting AAMPGstPlayer_Stop"); + aamp->SyncEnd(); } @@ -983,21 +985,29 @@ long long AAMPGstPlayer::GetPositionMilliseconds(void) */ bool AAMPGstPlayer::Pause( bool pause, bool forceStopGstreamerPreBuffering ) { - aamp->SyncBegin(); /* Obtains a mutex lock */ + bool res = false; - AAMPLOG_MIL("entering AAMPGstPlayer_Pause - pause(%d) stop-pre-buffering(%d)", pause, forceStopGstreamerPreBuffering); + aamp->SyncBegin(); /* Obtains a mutex lock */ - bool res = this->playerInstance->Pause(pause, forceStopGstreamerPreBuffering); - if(res) + if (!playerInstance) + { + AAMPLOG_WARN("AAMPGstPlayer_Pause called but playerInstance is null"); + } + else { - if(!aamp->IsGstreamerSubsEnabled()) - aamp->PauseSubtitleParser(pause); + AAMPLOG_MIL("entering AAMPGstPlayer_Pause - pause(%d) stop-pre-buffering(%d)", pause, forceStopGstreamerPreBuffering); + + res = this->playerInstance->Pause(pause, forceStopGstreamerPreBuffering); + if(res) + { + if(!aamp->IsGstreamerSubsEnabled()) + aamp->PauseSubtitleParser(pause); + } } aamp->SyncEnd(); /* Releases the mutex */ return res; - //return retValue; } /** diff --git a/test/utests/fakes/FakePrivateInstanceAAMP.cpp b/test/utests/fakes/FakePrivateInstanceAAMP.cpp index 1d60204833..e12d431e5e 100644 --- a/test/utests/fakes/FakePrivateInstanceAAMP.cpp +++ b/test/utests/fakes/FakePrivateInstanceAAMP.cpp @@ -1032,10 +1032,18 @@ void PrivateInstanceAAMP::StopTrackInjection(AampMediaType type) void PrivateInstanceAAMP::SyncBegin(void) { + if (g_mockPrivateInstanceAAMP != nullptr) + { + g_mockPrivateInstanceAAMP->SyncBegin(); + } } void PrivateInstanceAAMP::SyncEnd(void) { + if (g_mockPrivateInstanceAAMP != nullptr) + { + g_mockPrivateInstanceAAMP->SyncEnd(); + } } void PrivateInstanceAAMP::UpdateCullingState(double culledSecs) diff --git a/test/utests/mocks/MockPrivateInstanceAAMP.h b/test/utests/mocks/MockPrivateInstanceAAMP.h index 4b86c57ef7..4457057b0b 100644 --- a/test/utests/mocks/MockPrivateInstanceAAMP.h +++ b/test/utests/mocks/MockPrivateInstanceAAMP.h @@ -105,6 +105,8 @@ class MockPrivateInstanceAAMP MOCK_METHOD(void, NotifyReservationComplete, (const std::string& reservationId)); MOCK_METHOD(void, LoadIDX, (ProfilerBucketType bucketType, std::string fragmentUrl, std::string& effectiveUrl, std::vector& fragment, unsigned int curlInstance, const char *range, int& http_code, double *downloadTime, AampMediaType mediaType, int *fogError)); MOCK_METHOD(void, UpdateUseSinglePipeline, ()); + MOCK_METHOD(void, SyncBegin, ()); + MOCK_METHOD(void, SyncEnd, ()); }; extern MockPrivateInstanceAAMP *g_mockPrivateInstanceAAMP; diff --git a/test/utests/tests/AampGstPlayer/FunctionalTests.cpp b/test/utests/tests/AampGstPlayer/FunctionalTests.cpp index 3f6ec96b46..23b0bb06da 100644 --- a/test/utests/tests/AampGstPlayer/FunctionalTests.cpp +++ b/test/utests/tests/AampGstPlayer/FunctionalTests.cpp @@ -629,4 +629,50 @@ TEST_F(AAMPGstPlayerTests, MonitorAV ) DestroyAMPGstPlayer(); } +TEST_F(AAMPGstPlayerTests, Pause_NullPlayerInstance) +{ + // Setup + ConstructAMPGstPlayer(); + + // Simulate playerInstance being null (as could happen during a race with Stop) + InterfacePlayerRDK *savedPlayerInstance = mAAMPGstPlayer->playerInstance; + mAAMPGstPlayer->playerInstance = nullptr; + + // Expect SyncBegin/SyncEnd to be called even when playerInstance is null + EXPECT_CALL(*g_mockPrivateInstanceAAMP, SyncBegin()).Times(2); + EXPECT_CALL(*g_mockPrivateInstanceAAMP, SyncEnd()).Times(2); + + // Code under test - should return false without crashing + bool result = mAAMPGstPlayer->Pause(true, false); + EXPECT_FALSE(result); + + result = mAAMPGstPlayer->Pause(false, false); + EXPECT_FALSE(result); + + // Restore playerInstance for proper cleanup + mAAMPGstPlayer->playerInstance = savedPlayerInstance; + + // Tidy Up + DestroyAMPGstPlayer(); +} +TEST_F(AAMPGstPlayerTests, Stop_WithPipeline) +{ + // Setup + ConstructAMPGstPlayer(); + SetupPipeline(&tbl[0]); + + // Expect SyncBegin/SyncEnd to be called during Stop + EXPECT_CALL(*g_mockPrivateInstanceAAMP, SyncBegin()).Times(1); + EXPECT_CALL(*g_mockPrivateInstanceAAMP, SyncEnd()).Times(1); + + // Code under test - Stop should execute with SyncBegin/SyncEnd without issues + mAAMPGstPlayer->Stop(false); + + // After Stop, playerInstance->Stop() should have been called (pipeline torn down) + // The DestroyAMPGstPlayer expectations for pipeline teardown are no longer needed + isPipelineSetup = false; + + // Tidy Up + DestroyAMPGstPlayer(); +} From 05e623a677b4e8d74e880ee3ae8a6ffb273be19f Mon Sep 17 00:00:00 2001 From: Jose Fagundez Date: Fri, 15 May 2026 11:48:47 +0200 Subject: [PATCH 2/5] Fix L1 tests failures by using NiceMock instead of StrictMock --- test/utests/tests/FragmentCollectorAdTests/AdSelectionTests.cpp | 2 +- test/utests/tests/StreamAbstractionAAMP_MPD/AudioOnlyTests.cpp | 2 +- .../StreamAbstractionAAMP_MPD/AudioTrackSelectionTests.cpp | 2 +- .../tests/StreamAbstractionAAMP_MPD/AudioTrackSwitchTests.cpp | 2 +- .../utests/tests/StreamAbstractionAAMP_MPD/FetcherLoopTests.cpp | 2 +- test/utests/tests/StreamAbstractionAAMP_MPD/FunctionalTests.cpp | 2 +- test/utests/tests/StreamAbstractionAAMP_MPD/LinearFOGTests.cpp | 2 +- .../tests/StreamAbstractionAAMP_MPD/StreamSelectionTest.cpp | 2 +- test/utests/tests/StreamAbstractionAAMP_MPD/subtitleTests.cpp | 2 +- .../tests/fragmentcollector_mpd/FindTimedMetadataTests.cpp | 2 +- .../tests/fragmentcollector_mpd/fragmentcollector_mpd1.cpp | 2 +- 11 files changed, 11 insertions(+), 11 deletions(-) diff --git a/test/utests/tests/FragmentCollectorAdTests/AdSelectionTests.cpp b/test/utests/tests/FragmentCollectorAdTests/AdSelectionTests.cpp index 61e1e46cd5..a6c0111ca3 100644 --- a/test/utests/tests/FragmentCollectorAdTests/AdSelectionTests.cpp +++ b/test/utests/tests/FragmentCollectorAdTests/AdSelectionTests.cpp @@ -368,7 +368,7 @@ class AdSelectionTests : public ::testing::Test mPrivateInstanceAAMP->mIsDefaultOffset = true; - g_mockPrivateInstanceAAMP = new StrictMock(); + g_mockPrivateInstanceAAMP = new NiceMock(); g_mockMediaStreamContext = new StrictMock(); diff --git a/test/utests/tests/StreamAbstractionAAMP_MPD/AudioOnlyTests.cpp b/test/utests/tests/StreamAbstractionAAMP_MPD/AudioOnlyTests.cpp index f62eefb4b1..823b2f00ea 100644 --- a/test/utests/tests/StreamAbstractionAAMP_MPD/AudioOnlyTests.cpp +++ b/test/utests/tests/StreamAbstractionAAMP_MPD/AudioOnlyTests.cpp @@ -131,7 +131,7 @@ class AudioOnlyTests : public ::testing::Test g_mockAampGstPlayer = new MockAAMPGstPlayer(mPrivateInstanceAAMP); - g_mockPrivateInstanceAAMP = new StrictMock(); + g_mockPrivateInstanceAAMP = new NiceMock(); g_mockMediaStreamContext = new StrictMock(); diff --git a/test/utests/tests/StreamAbstractionAAMP_MPD/AudioTrackSelectionTests.cpp b/test/utests/tests/StreamAbstractionAAMP_MPD/AudioTrackSelectionTests.cpp index 63e36835ce..943966c94e 100644 --- a/test/utests/tests/StreamAbstractionAAMP_MPD/AudioTrackSelectionTests.cpp +++ b/test/utests/tests/StreamAbstractionAAMP_MPD/AudioTrackSelectionTests.cpp @@ -110,7 +110,7 @@ class SelectAudioTrackTests : public ::testing::Test mPrivateInstanceAAMP->mIsDefaultOffset = true; - g_mockPrivateInstanceAAMP = new StrictMock(); + g_mockPrivateInstanceAAMP = new NiceMock(); g_mockMediaStreamContext = new StrictMock(); diff --git a/test/utests/tests/StreamAbstractionAAMP_MPD/AudioTrackSwitchTests.cpp b/test/utests/tests/StreamAbstractionAAMP_MPD/AudioTrackSwitchTests.cpp index a133cfea3b..cc0732869f 100644 --- a/test/utests/tests/StreamAbstractionAAMP_MPD/AudioTrackSwitchTests.cpp +++ b/test/utests/tests/StreamAbstractionAAMP_MPD/AudioTrackSwitchTests.cpp @@ -111,7 +111,7 @@ class SwitchAudioTrackTests : public ::testing::Test mPrivateInstanceAAMP->mIsDefaultOffset = true; - g_mockPrivateInstanceAAMP = new StrictMock(); + g_mockPrivateInstanceAAMP = new NiceMock(); g_mockMediaStreamContext = new StrictMock(); diff --git a/test/utests/tests/StreamAbstractionAAMP_MPD/FetcherLoopTests.cpp b/test/utests/tests/StreamAbstractionAAMP_MPD/FetcherLoopTests.cpp index a242cfdf24..19916c7eed 100644 --- a/test/utests/tests/StreamAbstractionAAMP_MPD/FetcherLoopTests.cpp +++ b/test/utests/tests/StreamAbstractionAAMP_MPD/FetcherLoopTests.cpp @@ -309,7 +309,7 @@ class FetcherLoopTests : public ::testing::Test assert( g_mockAampUtils == nullptr ); g_mockAampGstPlayer = new MockAAMPGstPlayer(mPrivateInstanceAAMP); mPrivateInstanceAAMP->mIsDefaultOffset = true; - g_mockPrivateInstanceAAMP = new StrictMock(); + g_mockPrivateInstanceAAMP = new NiceMock(); g_mockMediaStreamContext = new StrictMock(); g_mockAampMPDDownloader = new StrictMock(); g_mockAampStreamSinkManager = new NiceMock(); diff --git a/test/utests/tests/StreamAbstractionAAMP_MPD/FunctionalTests.cpp b/test/utests/tests/StreamAbstractionAAMP_MPD/FunctionalTests.cpp index f69220ab7d..6f592e668a 100644 --- a/test/utests/tests/StreamAbstractionAAMP_MPD/FunctionalTests.cpp +++ b/test/utests/tests/StreamAbstractionAAMP_MPD/FunctionalTests.cpp @@ -164,7 +164,7 @@ class FunctionalTestsBase mPrivateInstanceAAMP->mIsDefaultOffset = true; - g_mockPrivateInstanceAAMP = new StrictMock(); + g_mockPrivateInstanceAAMP = new NiceMock(); g_mockMediaStreamContext = new StrictMock(); diff --git a/test/utests/tests/StreamAbstractionAAMP_MPD/LinearFOGTests.cpp b/test/utests/tests/StreamAbstractionAAMP_MPD/LinearFOGTests.cpp index f078405c4b..22c73826be 100644 --- a/test/utests/tests/StreamAbstractionAAMP_MPD/LinearFOGTests.cpp +++ b/test/utests/tests/StreamAbstractionAAMP_MPD/LinearFOGTests.cpp @@ -157,7 +157,7 @@ class LinearFOGTests : public testing::TestWithParam mPrivateInstanceAAMP->mIsDefaultOffset = true; - g_mockPrivateInstanceAAMP = new StrictMock(); + g_mockPrivateInstanceAAMP = new NiceMock(); g_mockMediaStreamContext = new StrictMock(); diff --git a/test/utests/tests/StreamAbstractionAAMP_MPD/StreamSelectionTest.cpp b/test/utests/tests/StreamAbstractionAAMP_MPD/StreamSelectionTest.cpp index 3e07728c13..d4bb3e47f0 100644 --- a/test/utests/tests/StreamAbstractionAAMP_MPD/StreamSelectionTest.cpp +++ b/test/utests/tests/StreamAbstractionAAMP_MPD/StreamSelectionTest.cpp @@ -413,7 +413,7 @@ class StreamSelectionTests : public testing::TestWithParammIsDefaultOffset = true; g_mockAampConfig = new NiceMock(); mPrivateInstanceAAMP->mIsDefaultOffset = true; - g_mockPrivateInstanceAAMP = new StrictMock(); + g_mockPrivateInstanceAAMP = new NiceMock(); g_mockMediaStreamContext = new StrictMock(); g_mockAampMPDDownloader = new StrictMock(); mStreamAbstractionAAMP_MPD = nullptr; diff --git a/test/utests/tests/StreamAbstractionAAMP_MPD/subtitleTests.cpp b/test/utests/tests/StreamAbstractionAAMP_MPD/subtitleTests.cpp index 73a759d5d6..daf857af40 100644 --- a/test/utests/tests/StreamAbstractionAAMP_MPD/subtitleTests.cpp +++ b/test/utests/tests/StreamAbstractionAAMP_MPD/subtitleTests.cpp @@ -144,7 +144,7 @@ class SubtitleTrackTests : public ::testing::Test mPrivateInstanceAAMP = new PrivateInstanceAAMP(gpGlobalConfig); g_mockAampConfig = new NiceMock(); mPrivateInstanceAAMP->mIsDefaultOffset = true; - g_mockPrivateInstanceAAMP = new StrictMock(); + g_mockPrivateInstanceAAMP = new NiceMock(); g_mockMediaStreamContext = new StrictMock(); g_mockAampMPDDownloader = new StrictMock(); diff --git a/test/utests/tests/fragmentcollector_mpd/FindTimedMetadataTests.cpp b/test/utests/tests/fragmentcollector_mpd/FindTimedMetadataTests.cpp index a60ae447db..0094cae844 100644 --- a/test/utests/tests/fragmentcollector_mpd/FindTimedMetadataTests.cpp +++ b/test/utests/tests/fragmentcollector_mpd/FindTimedMetadataTests.cpp @@ -95,7 +95,7 @@ class FindTimedMetadataTests : public ::testing::Test mPrivateInstanceAAMP = new PrivateInstanceAAMP(gpGlobalConfig); - g_mockPrivateInstanceAAMP = new StrictMock(); + g_mockPrivateInstanceAAMP = new NiceMock(); g_mockAampUtils = new NiceMock(); diff --git a/test/utests/tests/fragmentcollector_mpd/fragmentcollector_mpd1.cpp b/test/utests/tests/fragmentcollector_mpd/fragmentcollector_mpd1.cpp index 2b56286776..c34c0d7741 100644 --- a/test/utests/tests/fragmentcollector_mpd/fragmentcollector_mpd1.cpp +++ b/test/utests/tests/fragmentcollector_mpd/fragmentcollector_mpd1.cpp @@ -129,7 +129,7 @@ class MpdTests : public ::testing::Test mPrivateInstanceAAMP = new PrivateInstanceAAMP(gpGlobalConfig); - g_mockPrivateInstanceAAMP = new StrictMock(); + g_mockPrivateInstanceAAMP = new NiceMock(); mStreamAbstractionAAMP_MPD = nullptr; From f9d4e7c4d5971d76a327c950f1ce59fb82394139 Mon Sep 17 00:00:00 2001 From: Jose Fagundez Date: Mon, 18 May 2026 19:03:29 +0200 Subject: [PATCH 3/5] Print log lines when enter and exit Pause() --- aampgstplayer.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/aampgstplayer.cpp b/aampgstplayer.cpp index 5eacf371c7..485cfcb27a 100644 --- a/aampgstplayer.cpp +++ b/aampgstplayer.cpp @@ -989,22 +989,25 @@ bool AAMPGstPlayer::Pause( bool pause, bool forceStopGstreamerPreBuffering ) aamp->SyncBegin(); /* Obtains a mutex lock */ + AAMPLOG_MIL("entering AAMPGstPlayer_Pause - pause(%d) stop-pre-buffering(%d)", pause, forceStopGstreamerPreBuffering); + if (!playerInstance) { AAMPLOG_WARN("AAMPGstPlayer_Pause called but playerInstance is null"); } else { - AAMPLOG_MIL("entering AAMPGstPlayer_Pause - pause(%d) stop-pre-buffering(%d)", pause, forceStopGstreamerPreBuffering); - res = this->playerInstance->Pause(pause, forceStopGstreamerPreBuffering); if(res) { if(!aamp->IsGstreamerSubsEnabled()) + { aamp->PauseSubtitleParser(pause); + } } } + AAMPLOG_TRACE("exit AAMPGstPlayer_Pause"); aamp->SyncEnd(); /* Releases the mutex */ return res; From b5fdfc9ec946bbccc43da7359cf6b7fc1d443f7d Mon Sep 17 00:00:00 2001 From: Jose Fagundez Date: Tue, 19 May 2026 19:35:13 +0200 Subject: [PATCH 4/5] Delete the playerInstance check from Pause() --- aampgstplayer.cpp | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/aampgstplayer.cpp b/aampgstplayer.cpp index 6e44bb67f3..789d65d450 100644 --- a/aampgstplayer.cpp +++ b/aampgstplayer.cpp @@ -1059,23 +1059,14 @@ bool AAMPGstPlayer::Pause( bool pause, bool forceStopGstreamerPreBuffering ) AAMPLOG_MIL("entering AAMPGstPlayer_Pause - pause(%d) stop-pre-buffering(%d)", pause, forceStopGstreamerPreBuffering); - if (!playerInstance) + res = this->playerInstance->Pause(pause, forceStopGstreamerPreBuffering); + if (res && !aamp->IsGstreamerSubsEnabled()) { - AAMPLOG_WARN("AAMPGstPlayer_Pause called but playerInstance is null"); - } - else - { - res = this->playerInstance->Pause(pause, forceStopGstreamerPreBuffering); - if(res) - { - if(!aamp->IsGstreamerSubsEnabled()) - { - aamp->PauseSubtitleParser(pause); - } - } + aamp->PauseSubtitleParser(pause); } - AAMPLOG_TRACE("exit AAMPGstPlayer_Pause"); + AAMPLOG_TRACE("exit AAMPGstPlayer_Pause returns %d", res); + aamp->SyncEnd(); /* Releases the mutex */ return res; From a50686766f4a4009acce874cfe16be226d9c60a3 Mon Sep 17 00:00:00 2001 From: Jose Fagundez Date: Wed, 20 May 2026 11:06:12 +0200 Subject: [PATCH 5/5] Revert unnecessary change to Pause() --- aampgstplayer.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/aampgstplayer.cpp b/aampgstplayer.cpp index 789d65d450..dba61663ac 100644 --- a/aampgstplayer.cpp +++ b/aampgstplayer.cpp @@ -1053,13 +1053,11 @@ long long AAMPGstPlayer::GetPositionMilliseconds(void) */ bool AAMPGstPlayer::Pause( bool pause, bool forceStopGstreamerPreBuffering ) { - bool res = false; - aamp->SyncBegin(); /* Obtains a mutex lock */ AAMPLOG_MIL("entering AAMPGstPlayer_Pause - pause(%d) stop-pre-buffering(%d)", pause, forceStopGstreamerPreBuffering); - res = this->playerInstance->Pause(pause, forceStopGstreamerPreBuffering); + bool res = this->playerInstance->Pause(pause, forceStopGstreamerPreBuffering); if (res && !aamp->IsGstreamerSubsEnabled()) { aamp->PauseSubtitleParser(pause);