Fix Android PAGView release race.#3561
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3561 +/- ##
========================================
Coverage 81.32% 81.33%
========================================
Files 652 652
Lines 77024 77024
Branches 21747 21873 +126
========================================
+ Hits 62640 62645 +5
+ Misses 9953 9947 -6
- Partials 4431 4432 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| if (jPlayer != nullptr) { | ||
| jPlayer->clear(); | ||
| } | ||
| clearPAGPlayer(env, thiz); |
There was a problem hiding this comment.
nativeRelease 改用了 clearPAGPlayer，但下方 nativeFinalize 仍是 setPAGPlayer(env, thiz, nullptr)。而 Animator/Surface 的 finalize 都统一走了 clear 逻辑，三个文件处理方式不一致。虽然 setPAGPlayer(nullptr) 与 clearPAGPlayer 行为等价、无 bug，但建议把 nativeFinalize 也改为 clearPAGPlayer(env, thiz)，让 setPAGPlayer 只保留 nativeSetup 的“设新值”语义，统一风格、便于维护。
| namespace pag { | ||
| static jfieldID PAGPlayer_nativeContext; | ||
| } | ||
| static std::mutex PAGPlayer_contextLocker = {}; |
There was a problem hiding this comment.
这里（以及 JPAGAnimator.cpp、JPAGSurface.cpp）新增使用了 std::mutex，但均未显式 #include ，目前依赖 pag.h 间接包含才能编译。按 IWYU 原则，建议在这三个 .cpp 中显式添加 #include ，避免未来上游头文件调整包含链时被动编译失败。非阻塞项。
| { | ||
| std::lock_guard<std::mutex> autoLock(PAGSurface_contextLocker); | ||
| old = reinterpret_cast<JPAGSurface*>(env->GetLongField(thiz, PAGSurface_nativeSurface)); | ||
| env->SetLongField(thiz, PAGSurface_nativeSurface, 0); |
There was a problem hiding this comment.
nativeFinalize 这里把加锁+置0+delete 的逻辑内联了一遍，与上方刚新增的 clearPAGSurface 函数体逐行完全相同。建议直接调用 clearPAGSurface(env, thiz)（与 nativeRelease 一致），消除同文件内的重复逻辑，后续调整锁策略时也不易漏改一处。
| tempAnimator = std::move(animator); | ||
| tempListener = std::move(listener); | ||
| } | ||
| // Call cancel outside the lock to avoid deadlock |
There was a problem hiding this comment.
不建议删除这行 “Call cancel outside the lock to avoid deadlock” 注释。下方代码刻意采用“锁内 std::move 出局部变量、出锁后再 cancel()”的写法，正是为避免 cancel() 同步回调 listener 时重入 locker 造成死锁。删掉注释后，这段非直觉的写法失去了唯一的解释，后来者很可能误以为可简化为锁内直接 cancel 而重新引入死锁。项目规范也要求 workaround 需加注释说明原因，建议恢复。
| } | ||
| isVisible = visible; | ||
| if (isVisible) { | ||
| boolean visible = attached && isShown(); |
There was a problem hiding this comment.
这里把 isShown() 特意放在前后两个 synchronized(PAGView.this) 块之间、而非合并为一个锁块，应该是刻意设计——isShown() 是 Android View 框架方法，持锁调用可能触发重入/回调导致死锁，所以必须在锁外调用。但代码无注释说明，读者容易把这两个锁误当作可合并的冗余，从而把 isShown() 挪进锁内引入死锁。建议加一行注释说明 isShown() 必须锁外调用的原因。
3 participants
变更说明
关联 Issue
验证