Skip to content

VPLAY-12337 : Crash issue is observed while switch between different … #876

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
molakalapalliharipriya wants to merge 1 commit into
base: dev_sprint_25_2
Choose a base branch
from
+47 −4
Open

VPLAY-12337 : Crash issue is observed while switch between different … #876

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 47 additions & 4 deletions priv_aamp.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8151,10 +8151,33 @@ void PrivateInstanceAAMP::Stop( bool isDestructing )
{
std::lock_guard<std::mutex> guard(mMutexPlaystart);
waitforplaystart.notify_all();
AAMPLOG_WARN("HariPriya Stop: notified PreCache thread, isDestructing=%d", isDestructing);
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Log messages contain the developer's name 'HariPriya'. This is unprofessional and makes logs harder to search and filter. Remove the name prefix from all log messages (lines 8154, 8159, 8162, 8166, 10328, 10338, 10345, 10370) and use descriptive, context-specific prefixes instead.

Copilot generated this review using guidance from repository custom instructions.
}
if(mPreCachePlaylistThreadId.joinable())
{
mPreCachePlaylistThreadId.join();
auto joinStartTime = NOW_STEADY_TS_MS;
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 'auto' keyword is used for type deduction of timing variables. For clarity and consistency with the codebase's use of explicit types for timing values (seen elsewhere as 'long long'), explicitly declare the type as 'long long' instead of using 'auto'.

Copilot generated this review using guidance from repository custom instructions.
AAMPLOG_WARN("HariPriya Stop: About to join PreCache thread, isDestructing=%d", isDestructing);

// Fix for GC deadlock: During GC finalization (isDestructing=true), the PreCache thread
// may be blocked in network I/O (curl_easy_perform). Using join() would block the main
// thread indefinitely, causing freeze/crash. Instead, use detach() to let the thread
// clean up itself. The thread will exit safely once network I/O completes.
if (isDestructing)
{
AAMPLOG_WARN("HariPriya Stop: Detaching PreCache thread (GC path) to avoid blocking, isDestructing=%d", isDestructing);
mPreCachePlaylistThreadId.detach();
}
else
{
// Normal stop path - wait for thread to complete
mPreCachePlaylistThreadId.join();
auto joinDuration = NOW_STEADY_TS_MS - joinStartTime;
AAMPLOG_WARN("HariPriya Stop: PreCache thread joined after %lld ms, isDestructing=%d", joinDuration, isDestructing);
}
}
else
{
AAMPLOG_INFO("HariPriya Stop: PreCache thread not joinable, isDestructing=%d", isDestructing);
}

if (mAampCacheHandler)
Expand Down Expand Up @@ -10271,14 +10294,17 @@ void PrivateInstanceAAMP::PreCachePlaylistDownloadTask()
{
// This is the thread function to download all the HLS Playlist in a
// differed manner
AAMPLOG_WARN("PreCachePlaylistDownloadTask: Thread started");
int maxWindowForDownload = mPreCacheDnldTimeWindow * 60; // convert to seconds
int szPlaylistCount = (int)mPreCacheDnldList.size();
if(szPlaylistCount)
{
// First wait for Tune to complete to start this functionality
{
std::unique_lock<std::mutex> lock(mMutexPlaystart);
AAMPLOG_WARN("PreCachePlaylistDownloadTask: Waiting for playstart notification");
waitforplaystart.wait(lock);
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Condition variable wait() is called without a predicate, making it vulnerable to spurious wakeups. This could lead to premature continuation if the thread wakes up before the actual playstart notification. Use wait() with a predicate or manually check the condition in a loop after waking.

Copilot generated this review using guidance from repository custom instructions.
AAMPLOG_WARN("PreCachePlaylistDownloadTask: Received playstart notification, proceeding");
}
// May be Stop is called to release all resources .
// Before download , check the state
Expand Down Expand Up @@ -10312,7 +10338,25 @@ void PrivateInstanceAAMP::PreCachePlaylistDownloadTask()
bool ret = false;
// Using StreamLock to avoid StreamAbstractionAAMP deletion when external player commands or stop call received
AcquireStreamLock();
auto getFileStartTime = NOW_STEADY_TS_MS;
AAMPLOG_WARN("HariPriya PreCachePlaylistDownloadTask: About to call GetFile (network I/O), state=%d", GetState());

// DEBUG: Simulate slow network to reproduce GC deadlock issue
// Set AAMP_SIMULATE_SLOW_PRECACHE_MS env var to inject delay (e.g., export AAMP_SIMULATE_SLOW_PRECACHE_MS=5000)
const char* delayEnv = getenv("AAMP_SIMULATE_SLOW_PRECACHE_MS");
if (delayEnv)
{
int delayMs = atoi(delayEnv);
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using atoi() without input validation could lead to undefined behavior if the environment variable contains non-numeric values. Use strtol() with proper error checking, or at minimum validate that delayEnv is not empty and contains only digits before calling atoi().

Copilot generated this review using guidance from repository custom instructions.
if (delayMs > 0)
{
AAMPLOG_WARN("HariPriya PreCachePlaylistDownloadTask: DEBUG - Simulating slow network with %d ms delay", delayMs);
std::this_thread::sleep_for(std::chrono::milliseconds(delayMs));
}
}
Comment on lines +10344 to +10355
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug/test simulation code should not be committed to production code. This artificial delay mechanism should either be removed or wrapped in a DEBUG or ENABLE_DEBUG_FEATURES preprocessor conditional. The environment variable check adds overhead to every playlist download in production builds.

Copilot generated this review using guidance from repository custom instructions.

ret = GetFile(newelem.url, newelem.type, &playlistStore, playlistEffectiveUrl, &http_code, &downloadTime, NULL, eCURLINSTANCE_PLAYLISTPRECACHE, true );
auto getFileDuration = NOW_STEADY_TS_MS - getFileStartTime;
AAMPLOG_WARN("HariPriya PreCachePlaylistDownloadTask: GetFile returned after %lld ms, ret=%d, state=%d", getFileDuration, ret, GetState());
ReleaseStreamLock();
if(ret != false)
{
Expand All @@ -10337,11 +10381,12 @@ void PrivateInstanceAAMP::PreCachePlaylistDownloadTask()
}
}
}while (idx < mPreCacheDnldList.size() && state != eSTATE_STOPPING && state != eSTATE_IDLE && state != eSTATE_ERROR);
AAMPLOG_WARN("HariPriya PreCachePlaylistDownloadTask: Exiting loop, state=%d, idx=%d, size=%zu", state, idx, mPreCacheDnldList.size());
mPreCacheDnldList.clear();
CurlTerm(eCURLINSTANCE_PLAYLISTPRECACHE);
}
}
AAMPLOG_WARN("End of PreCachePlaylistDownloadTask ");
AAMPLOG_WARN("PreCachePlaylistDownloadTask: Thread exiting, state=%d", GetState());
}

/**
Expand Down Expand Up @@ -14448,5 +14493,3 @@ void PrivateInstanceAAMP::SetStreamCaps(AampMediaType type, MediaCodecInfo&& cod
if (sink)
{
sink->SetStreamCaps(type, std::move(codecInfo));
}
}
Loading