Skip to content

Fix Android PAGView release race. #3561

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

Merged
shlzxjp merged 3 commits into from
Jul 28, 2026
Merged

Fix Android PAGView release race. #3561

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
26df866
Fix Android PAGView release race.
leiyue123 Jul 7, 2026
1553506
Fix Android code format.
leiyue123 Jul 7, 2026
49e4037
Merge remote-tracking branch 'origin/main' into bugfix/thunderllei_an…
leiyue123 Jul 24, 2026
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
37 changes: 24 additions & 13 deletions android/libpag/src/main/java/org/libpag/PAGView.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -555,20 +555,24 @@ public void onSurfaceTextureUpdated(SurfaceTexture surface) {

@Override
protected void onAttachedToWindow() {
isAttachedToWindow = true;
synchronized (PAGView.this) {
isAttachedToWindow = true;
}
super.onAttachedToWindow();
checkVisible();
}

@Override
protected void onDetachedFromWindow() {
isAttachedToWindow = false;
synchronized (PAGView.this) {
isAttachedToWindow = false;
}
checkVisible();
super.onDetachedFromWindow();
if (pagSurface != null) {
pagSurface.release();
pagSurface = null;
}
checkVisible();
}


Expand All @@ -588,12 +592,18 @@ public void onVisibilityAggregated(boolean isVisible) {
private boolean isVisible = false;

private void checkVisible() {
boolean visible = isAttachedToWindow && isShown();
if (isVisible == visible) {
return;
boolean attached = false;
synchronized (PAGView.this) {
attached = isAttachedToWindow;
}
isVisible = visible;
if (isVisible) {
boolean visible = attached && isShown();

@shlzxjp shlzxjp Jul 15, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

这里把 isShown() 特意放在前后两个 synchronized(PAGView.this) 块之间、而非合并为一个锁块，应该是刻意设计——isShown() 是 Android View 框架方法，持锁调用可能触发重入/回调导致死锁，所以必须在锁外调用。但代码无注释说明，读者容易把这两个锁误当作可合并的冗余，从而把 isShown() 挪进锁内引入死锁。建议加一行注释说明 isShown() 必须锁外调用的原因。

@leiyue123 leiyue123 Jul 24, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

done

synchronized (PAGView.this) {
if (isVisible == visible) {
return;
}
isVisible = visible;
}
if (visible) {
animator.setDuration(pagPlayer.duration());
animator.update();
} else {
Expand Down Expand Up @@ -661,16 +671,17 @@ public void onAnimationRepeat(PAGAnimator animator) {
}

public void onAnimationUpdate(PAGAnimator animator) {
pagPlayer.setProgress(animator.progress());
boolean changed = false;
synchronized (PAGView.this) {
if (!isAttachedToWindow) {
return;
}
pagPlayer.setProgress(animator.progress());
if (isVisible) {
animator.setDuration(pagPlayer.duration());
}
changed = flush();

@shlzxjp shlzxjp Jul 24, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

renderLock 把 surface 生命周期与异步渲染串行化，正确根治了 surface release 与 flush 并发的问题，锁交互本身安全（无重入死锁、无跨层锁顺序环）。但这里在 worker 线程持有 renderLock 的整个期间执行了重量级的 flush()（完整一帧渲染）。而 UI 线程的 onAttachedToWindow / onDetachedFromWindow / onSurfaceTextureAvailable/Destroyed 都要抢同一把 renderLock：当某帧 flush 较慢时，UI 线程的这些生命周期回调会被阻塞至该帧结束，极端情况下存在卡顿甚至 ANR 风险。建议评估是否可缩小锁粒度（例如渲染阶段与 surface 增删用更细的临界区），或确认 flush 时长在可接受范围内。非阻塞项，供参考。

@leiyue123 leiyue123 Jul 27, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

这个锁难拆分。核心矛盾在于：flush() 渲染到 pagSurface 期间，必须保证 pagSurface 不被并发释放。
如果把 flush() 移到锁外，锁释放后到 flush() 执行前这段窗口里，onDetachedFromWindow 可以释放 pagSurface，而 flush() 随后会操作已释放的对象。虽然 JNI 层有 std::mutex 保护不会 crash，但 Java 侧的 PAGSurface 已经被释放，行为不可预期。
要把 flush() 安全地移出锁外，需要在 Java 层引入类似引用计数的机制（flush 开始前持有引用，结束后释放），改动较大且逻辑分散。当前直接持锁的方式虽然会把 UI 生命周期回调短暂挡在外面，但逻辑集中在一点，正确性容易保证。
建议先保持当前方案，如果后续有 trace 证明这里确实导致 UI 卡顿，再针对性地拆分。

}
if (isVisible) {
animator.setDuration(pagPlayer.duration());
}
boolean changed = flush();
if (changed) {
updateTextureView();
}
Expand Down
29 changes: 20 additions & 9 deletions src/platform/android/JPAGAnimator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ static jmethodID PAGAnimator_onAnimationEnd;
static jmethodID PAGAnimator_onAnimationCancel;
static jmethodID PAGAnimator_onAnimationRepeat;
static jmethodID PAGAnimator_onAnimationUpdate;
static std::mutex PAGAnimator_contextLocker = {};

class AnimatorListener : public pag::PAGAnimator::Listener {
public:
Expand Down Expand Up @@ -131,7 +132,6 @@ class JPAGAnimator {
tempAnimator = std::move(animator);
tempListener = std::move(listener);
}
// Call cancel outside the lock to avoid deadlock

@shlzxjp shlzxjp Jul 15, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

不建议删除这行 “Call cancel outside the lock to avoid deadlock” 注释。下方代码刻意采用“锁内 std::move 出局部变量、出锁后再 cancel()”的写法，正是为避免 cancel() 同步回调 listener 时重入 locker 造成死锁。删掉注释后，这段非直觉的写法失去了唯一的解释，后来者很可能误以为可简化为锁内直接 cancel 而重新引入死锁。项目规范也要求 workaround 需加注释说明原因，建议恢复。

@leiyue123 leiyue123 Jul 24, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

done

if (tempAnimator) {
tempAnimator->cancel();
}
Expand All @@ -147,6 +147,7 @@ class JPAGAnimator {
using namespace pag;

std::shared_ptr<PAGAnimator> getPAGAnimator(JNIEnv* env, jobject thiz) {
std::lock_guard<std::mutex> autoLock(PAGAnimator_contextLocker);
auto animator =
reinterpret_cast<JPAGAnimator*>(env->GetLongField(thiz, PAGAnimator_nativeContext));
if (animator == nullptr) {
Expand All @@ -156,9 +157,23 @@ std::shared_ptr<PAGAnimator> getPAGAnimator(JNIEnv* env, jobject thiz) {
}

void setPAGAnimator(JNIEnv* env, jobject thiz, JPAGAnimator* animator) {
auto old = reinterpret_cast<JPAGAnimator*>(env->GetLongField(thiz, PAGAnimator_nativeContext));
JPAGAnimator* old = nullptr;
{
std::lock_guard<std::mutex> autoLock(PAGAnimator_contextLocker);
old = reinterpret_cast<JPAGAnimator*>(env->GetLongField(thiz, PAGAnimator_nativeContext));
env->SetLongField(thiz, PAGAnimator_nativeContext, (jlong)animator);
}
delete old;
}

void clearPAGAnimator(JNIEnv* env, jobject thiz) {
JPAGAnimator* old = nullptr;
{
std::lock_guard<std::mutex> autoLock(PAGAnimator_contextLocker);
old = reinterpret_cast<JPAGAnimator*>(env->GetLongField(thiz, PAGAnimator_nativeContext));
env->SetLongField(thiz, PAGAnimator_nativeContext, 0);
}
delete old;
env->SetLongField(thiz, PAGAnimator_nativeContext, (jlong)animator);
}

extern "C" {
Expand All @@ -177,15 +192,11 @@ PAG_API void Java_org_libpag_PAGAnimator_nativeSetup(JNIEnv* env, jobject thiz)
}

PAG_API void Java_org_libpag_PAGAnimator_nativeRelease(JNIEnv* env, jobject thiz) {
auto jPlayer =
reinterpret_cast<JPAGAnimator*>(env->GetLongField(thiz, PAGAnimator_nativeContext));
if (jPlayer != nullptr) {
jPlayer->clear();
}
clearPAGAnimator(env, thiz);
}

PAG_API void Java_org_libpag_PAGAnimator_nativeFinalize(JNIEnv* env, jobject thiz) {
setPAGAnimator(env, thiz, nullptr);
clearPAGAnimator(env, thiz);
}

PAG_API jboolean Java_org_libpag_PAGAnimator_isSync(JNIEnv* env, jobject thiz) {
Expand Down
29 changes: 20 additions & 9 deletions src/platform/android/JPAGPlayer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,24 +28,38 @@

namespace pag {
static jfieldID PAGPlayer_nativeContext;
}
static std::mutex PAGPlayer_contextLocker = {};

@shlzxjp shlzxjp Jul 15, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

这里（以及 JPAGAnimator.cpp、JPAGSurface.cpp）新增使用了 std::mutex，但均未显式 #include ，目前依赖 pag.h 间接包含才能编译。按 IWYU 原则，建议在这三个 .cpp 中显式添加 #include ，避免未来上游头文件调整包含链时被动编译失败。非阻塞项。

@leiyue123 leiyue123 Jul 24, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

done

} // namespace pag

using namespace pag;

std::shared_ptr<PAGPlayer> getPAGPlayer(JNIEnv* env, jobject thiz) {
std::lock_guard<std::mutex> autoLock(PAGPlayer_contextLocker);
auto jPlayer = reinterpret_cast<JPAGPlayer*>(env->GetLongField(thiz, PAGPlayer_nativeContext));
if (jPlayer == nullptr) {
return nullptr;
}
return jPlayer->get();
}

void clearPAGPlayer(JNIEnv* env, jobject thiz) {
JPAGPlayer* old = nullptr;
{
std::lock_guard<std::mutex> autoLock(PAGPlayer_contextLocker);
old = reinterpret_cast<JPAGPlayer*>(env->GetLongField(thiz, PAGPlayer_nativeContext));
env->SetLongField(thiz, PAGPlayer_nativeContext, 0);
}
delete old;
}

void setPAGPlayer(JNIEnv* env, jobject thiz, JPAGPlayer* player) {
auto old = reinterpret_cast<JPAGPlayer*>(env->GetLongField(thiz, PAGPlayer_nativeContext));
if (old != nullptr) {
delete old;
JPAGPlayer* old = nullptr;
{
std::lock_guard<std::mutex> autoLock(PAGPlayer_contextLocker);
old = reinterpret_cast<JPAGPlayer*>(env->GetLongField(thiz, PAGPlayer_nativeContext));
env->SetLongField(thiz, PAGPlayer_nativeContext, (jlong)player);
}
env->SetLongField(thiz, PAGPlayer_nativeContext, (jlong)player);
delete old;
}

extern "C" {
Expand All @@ -60,10 +74,7 @@ PAG_API void Java_org_libpag_PAGPlayer_nativeSetup(JNIEnv* env, jobject thiz) {
}

PAG_API void Java_org_libpag_PAGPlayer_nativeRelease(JNIEnv* env, jobject thiz) {
auto jPlayer = reinterpret_cast<JPAGPlayer*>(env->GetLongField(thiz, PAGPlayer_nativeContext));
if (jPlayer != nullptr) {
jPlayer->clear();
}
clearPAGPlayer(env, thiz);

@shlzxjp shlzxjp Jul 15, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nativeRelease 改用了 clearPAGPlayer，但下方 nativeFinalize 仍是 setPAGPlayer(env, thiz, nullptr)。而 Animator/Surface 的 finalize 都统一走了 clear 逻辑，三个文件处理方式不一致。虽然 setPAGPlayer(nullptr) 与 clearPAGPlayer 行为等价、无 bug，但建议把 nativeFinalize 也改为 clearPAGPlayer(env, thiz)，让 setPAGPlayer 只保留 nativeSetup 的“设新值”语义，统一风格、便于维护。

@leiyue123 leiyue123 Jul 24, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

done

}

PAG_API void Java_org_libpag_PAGPlayer_nativeFinalize(JNIEnv* env, jobject thiz) {
Expand Down
26 changes: 19 additions & 7 deletions src/platform/android/JPAGSurface.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,13 @@

namespace pag {
static jfieldID PAGSurface_nativeSurface;
static std::mutex PAGSurface_contextLocker = {};
} // namespace pag

using namespace pag;

std::shared_ptr<PAGSurface> getPAGSurface(JNIEnv* env, jobject thiz) {
std::lock_guard<std::mutex> autoLock(PAGSurface_contextLocker);
auto pagSurface =
reinterpret_cast<JPAGSurface*>(env->GetLongField(thiz, PAGSurface_nativeSurface));
if (pagSurface == nullptr) {
Expand All @@ -43,24 +45,34 @@ std::shared_ptr<PAGSurface> getPAGSurface(JNIEnv* env, jobject thiz) {
return pagSurface->get();
}

void clearPAGSurface(JNIEnv* env, jobject thiz) {
JPAGSurface* old = nullptr;
{
std::lock_guard<std::mutex> autoLock(PAGSurface_contextLocker);
old = reinterpret_cast<JPAGSurface*>(env->GetLongField(thiz, PAGSurface_nativeSurface));
env->SetLongField(thiz, PAGSurface_nativeSurface, 0);
}
delete old;
}

extern "C" {

PAG_API void Java_org_libpag_PAGSurface_nativeInit(JNIEnv* env, jclass clazz) {
PAGSurface_nativeSurface = env->GetFieldID(clazz, "nativeSurface", "J");
}

PAG_API void Java_org_libpag_PAGSurface_nativeRelease(JNIEnv* env, jobject thiz) {
auto jPAGSurface =
reinterpret_cast<JPAGSurface*>(env->GetLongField(thiz, PAGSurface_nativeSurface));
if (jPAGSurface != nullptr) {
jPAGSurface->clear();
}
clearPAGSurface(env, thiz);
}

PAG_API void Java_org_libpag_PAGSurface_nativeFinalize(JNIEnv* env, jobject thiz) {
auto old = reinterpret_cast<JPAGSurface*>(env->GetLongField(thiz, PAGSurface_nativeSurface));
JPAGSurface* old = nullptr;
{
std::lock_guard<std::mutex> autoLock(PAGSurface_contextLocker);
old = reinterpret_cast<JPAGSurface*>(env->GetLongField(thiz, PAGSurface_nativeSurface));
env->SetLongField(thiz, PAGSurface_nativeSurface, 0);

@shlzxjp shlzxjp Jul 15, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nativeFinalize 这里把加锁+置0+delete 的逻辑内联了一遍，与上方刚新增的 clearPAGSurface 函数体逐行完全相同。建议直接调用 clearPAGSurface(env, thiz)（与 nativeRelease 一致），消除同文件内的重复逻辑，后续调整锁策略时也不易漏改一处。

@leiyue123 leiyue123 Jul 24, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

done

}
delete old;
env->SetLongField(thiz, PAGSurface_nativeSurface, (jlong)thiz);
}

PAG_API jint Java_org_libpag_PAGSurface_width(JNIEnv* env, jobject thiz) {
Expand Down
Loading