Merged
修复callback ref可能被提前delete,导致的crash问题 #233
修复callback ref可能被提前delete,导致的crash问题 #233
Merged
wu-guangzhaoAdd on Jun 22
Total: 4 files changed+148-40
@@ -254,7 +254,6 @@ All parameters must be set before calling `prepareAsync()`.
254| `enable-accurate-seek` | integer | `0` | Accurate seek: `1`=seek to exact timestamp, `0`=seek to nearest keyframe (faster). |254| `enable-accurate-seek` | integer | `0` | Accurate seek: `1`=seek to exact timestamp, `0`=seek to nearest keyframe (faster). |
255| `soundtouch` | integer | `0` | Enable soundtouch pitch correction for variable-speed playback. Must be set before `setSpeed()`. |255| `soundtouch` | integer | `0` | Enable soundtouch pitch correction for variable-speed playback. Must be set before `setSpeed()`. |
256| `mediacodec-all-videos` | integer | `0` | Enable hardware decoding (H.264/H.265, etc.). |256| `mediacodec-all-videos` | integer | `0` | Enable hardware decoding (H.264/H.265, etc.). |
257-| `mediacodec-hevc` | integer | `0` | Enable hardware decoding for H.265 (HEVC) only. |
258| `overlay-format` | integer | `808596553` | Output pixel format: `842094158`=NV12, `808596553`=I420. |257| `overlay-format` | integer | `808596553` | Output pixel format: `842094158`=NV12, `808596553`=I420. |
cpf-manager

【AI-Review】【建议】【软件设计】【其他软件设计问题】README中mediacodec-hevc选项移除与crash修复无关

● 问题:README.md和README_zh.md中移除 mediacodec-hevc 选项文档的变更与本PR的核心目标(修复callback ref被异步delete导致的use-after-free crash)无关。混合不相关变更会增加代码审查难度,也不利于后续git bisect定位问题。

● 影响:建议。不影响功能,但降低了变更的原子性和可追溯性。

● 建议:将 mediacodec-hevc 文档移除拆分为独立提交,并在commit message中说明移除原因(如该选项已被 mediacodec-all-videos 覆盖、不再独立支持等)。

likedislike
wu-guangzhao
wu-guangzhao
Jun 23 Comments:
259| `max-buffer-size` | integer | `15728640` | Max read-ahead buffer size in bytes. Recommended for VOD: `102400`. |258| `max-buffer-size` | integer | `15728640` | Max read-ahead buffer size in bytes. Recommended for VOD: `102400`. |
260| `min-frames` | integer | `50000` | Minimum buffered frames before playback starts. Lower reduces startup latency; higher reduces stuttering. |259| `min-frames` | integer | `50000` | Minimum buffered frames before playback starts. Lower reduces startup latency; higher reduces stuttering. |
@@ -335,8 +334,6 @@ mIjkMediaPlayer.off('deviceChange');
335```typescript334```typescript
336// Enable hardware decoding for H.264 and H.265335// Enable hardware decoding for H.264 and H.265
337mIjkMediaPlayer.setOption(IjkMediaPlayer.OPT_CATEGORY_PLAYER, "mediacodec-all-videos", "1");336mIjkMediaPlayer.setOption(IjkMediaPlayer.OPT_CATEGORY_PLAYER, "mediacodec-all-videos", "1");
338-// Enable hardware decoding for H.265 only
339-mIjkMediaPlayer.setOption(IjkMediaPlayer.OPT_CATEGORY_PLAYER, "mediacodec-hevc", "1");
340// Set output pixel format: NV12(842094158) or I420(808596553)337// Set output pixel format: NV12(842094158) or I420(808596553)
341mIjkMediaPlayer.setOption(IjkMediaPlayer.OPT_CATEGORY_PLAYER, "overlay-format", "842094158");338mIjkMediaPlayer.setOption(IjkMediaPlayer.OPT_CATEGORY_PLAYER, "overlay-format", "842094158");
342```339```
@@ -255,7 +255,6 @@ mIjkMediaPlayer.setOption(IjkMediaPlayer.OPT_CATEGORY_FORMAT, "connect_timeout",
255| `enable-accurate-seek` | 整数 | `0` | 精确寻帧;`1`=跳转到精确时间点,`0`=跳转到最近关键帧(更快) |255| `enable-accurate-seek` | 整数 | `0` | 精确寻帧;`1`=跳转到精确时间点,`0`=跳转到最近关键帧(更快) |
256| `soundtouch` | 整数 | `0` | 启用 soundtouch 音调处理,倍速播放时保持音调;需先于 `setSpeed()` 设置 |256| `soundtouch` | 整数 | `0` | 启用 soundtouch 音调处理,倍速播放时保持音调;需先于 `setSpeed()` 设置 |
257| `mediacodec-all-videos` | 整数 | `0` | 开启所有视频格式(H.264/H.265 等)的硬件解码 |257| `mediacodec-all-videos` | 整数 | `0` | 开启所有视频格式(H.264/H.265 等)的硬件解码 |
258-| `mediacodec-hevc` | 整数 | `0` | 仅开启 H.265(HEVC)硬件解码 |
259| `overlay-format` | 整数 | `808596553` | 视频输出像素格式:`842094158`=NV12,`808596553`=I420 |258| `overlay-format` | 整数 | `808596553` | 视频输出像素格式:`842094158`=NV12,`808596553`=I420 |
260| `max-buffer-size` | 整数 | `15728640` | 预读缓冲区最大字节数(byte),超出后暂停预读;常用点播建议値:`102400` |259| `max-buffer-size` | 整数 | `15728640` | 预读缓冲区最大字节数(byte),超出后暂停预读;常用点播建议値:`102400` |
261| `min-frames` | 整数 | `50000` | 开始播放前的最少缓冲帧数;降低可减少起播延迟,升高可减少卡顿 |260| `min-frames` | 整数 | `50000` | 开始播放前的最少缓冲帧数;降低可减少起播延迟,升高可减少卡顿 |
@@ -337,8 +336,6 @@ mIjkMediaPlayer.off('deviceChange');
337```typescript336```typescript
338// 开启 H.264 和 H.265 硬解码337// 开启 H.264 和 H.265 硬解码
339mIjkMediaPlayer.setOption(IjkMediaPlayer.OPT_CATEGORY_PLAYER, "mediacodec-all-videos", "1");338mIjkMediaPlayer.setOption(IjkMediaPlayer.OPT_CATEGORY_PLAYER, "mediacodec-all-videos", "1");
340-// 仅开启 H.265 硬解码
341-mIjkMediaPlayer.setOption(IjkMediaPlayer.OPT_CATEGORY_PLAYER, "mediacodec-hevc", "1");
342// 设置输出像素格式:NV12(842094158) 或 I420(808596553)339// 设置输出像素格式:NV12(842094158) 或 I420(808596553)
343mIjkMediaPlayer.setOption(IjkMediaPlayer.OPT_CATEGORY_PLAYER, "overlay-format", "842094158");340mIjkMediaPlayer.setOption(IjkMediaPlayer.OPT_CATEGORY_PLAYER, "overlay-format", "842094158");
344```341```
@@ -15,6 +15,8 @@
15 15 
16#include "ijkplayer_napi.h"16#include "ijkplayer_napi.h"
17#include "native_window/external_window.h"17#include "native_window/external_window.h"
18+#include <cstring>
19+#include <cstdlib>
18 20 
19const int32_t STR_DEFAULT_SIZE = 2048;21const int32_t STR_DEFAULT_SIZE = 2048;
20const int32_t INDEX_0 = 0;22const int32_t INDEX_0 = 0;
@@ -28,6 +30,7 @@ const int32_t PARAM_COUNT_4 = 4;
28 30 
29OH_NativeXComponent_Callback IJKPlayerNapi::callback_;31OH_NativeXComponent_Callback IJKPlayerNapi::callback_;
30std::unordered_map<std::string, IJKPlayerNapi *> IJKPlayerNapi::ijkPlayerNapi_;32std::unordered_map<std::string, IJKPlayerNapi *> IJKPlayerNapi::ijkPlayerNapi_;
33+std::mutex IJKPlayerNapi::mapMutex_;
31std::string xcomponentId_;34std::string xcomponentId_;
32bool destroyResource;35bool destroyResource;
33bool IJKPlayerNapi::gIsVideo = true;36bool IJKPlayerNapi::gIsVideo = true;
@@ -38,7 +41,7 @@ struct CallbackContext {
38 int what = 0;41 int what = 0;
39 int arg1 = 0;42 int arg1 = 0;
40 int arg2 = 0;43 int arg2 = 0;
41- char *obj;44+ char *obj = nullptr;
42};45};
43 46 
44struct RecordCallBackInfo {47struct RecordCallBackInfo {
@@ -68,12 +71,98 @@ struct CommonCallBackInfo {
68 std::string xcomponentId;71 std::string xcomponentId;
69};72};
70 73 
74+struct CleanupContext {
75+ napi_env env;
76+ napi_ref callbackRef;
77+};
78+ 
79+void cleanup_message_ref(uv_work_t *work, int status)
80+{
81+ CleanupContext *cleanupCtx = static_cast<CleanupContext *>(work->data);
82+ if (cleanupCtx != nullptr) {
83+ napi_delete_reference(cleanupCtx->env, cleanupCtx->callbackRef);
84+ delete cleanupCtx;
85+ }
86+ if (work != nullptr) {
87+ delete work;
88+ }
89+}
90+ 
91+static void queueRefCleanup(napi_env env, napi_ref ref)
92+{
93+ uv_loop_s *loop = nullptr;
94+ napi_get_uv_event_loop(env, &loop);
95+ if (loop == nullptr) {
96+ LOGE("napi-->queueRefCleanup: loop null, delete ref immediately");
97+ napi_delete_reference(env, ref);
cpf-manager

【AI-Review】【严重】【基础代码问题】【稳定性问题】queueRefCleanup降级路径在非JS线程调用napi_delete_reference,可能导致crash

● 问题:queueRefCleanup函数(第91-109行)在napi_get_uv_event_loop返回null(第95行)或work分配失败(第101行)时,直接在当前线程调用napi_delete_reference(env, ref)作为降级处理。然而queueRefCleanup是从onSurfaceDestroyedCB调用的,XComponent回调可能在非JS线程执行。NAPI规范要求napi_delete_reference必须在JS线程(拥有napi_env的线程)上调用,在非JS线程调用可能导致内存损坏或crash。

● 影响:严重。虽然在正常情况下napi_get_uv_event_loop不会返回null、内存分配通常也会成功,但在极端场景(如JS引擎正在关闭、内存不足)下降级路径会被触发,而在非JS线程调用NAPI API可能导致不可预测的crash,且此类问题极难复现和定位。

● 建议:降级路径不应直接调用napi_delete_reference,而应记录警告并接受ref泄漏作为安全降级(ref泄漏比在错误线程调用NAPI API安全得多)。或者,可以在CleanupContext中存储足够的信息,由JS线程的下一个回调周期来清理:

if (loop == nullptr) {
    LOGE("napi-->queueRefCleanup: loop null, ref leaked to avoid unsafe NAPI call");
    // napi_delete_reference(env, ref);  // 移除:在非JS线程调用不安全
    return;
}
likedislike
wu-guangzhao
wu-guangzhao
Jun 23 Comments:
98+ return;
cpf-manager

【AI-Review】【一般】【基础代码问题】【代码逻辑错误】queueRefCleanup使用了未前向声明的cleanup_message_ref

● 问题: queueRefCleanup函数(第80行)在uv_queue_work调用中使用了cleanup_message_ref作为after_work_cb参数,但cleanup_message_ref的定义在第129行,位于queueRefCleanup之后。虽然某些编译器可能允许这种隐式声明(C语言风格),但在C++严格模式下,调用未声明的函数是未定义行为。当前代码能够编译通过可能是因为编译器的扩展行为,但这不可靠且不符合C++标准。

● 影响: 一般。在严格的C++编译器或开启严格编译选项时可能导致编译失败。当前依赖编译器隐式函数声明,存在可移植性和维护风险。

● 建议: 在queueRefCleanup之前添加cleanup_message_ref的前向声明:

void cleanup_message_ref(uv_work_t *work, int status);

或者调整函数定义顺序,将cleanup_message_ref移到queueRefCleanup之前。

likedislike
99+ }
100+ uv_work_t *work = new (std::nothrow) uv_work_t;
101+ if (work == nullptr) {
102+ LOGE("napi-->queueRefCleanup: work alloc failed, delete ref immediately");
103+ napi_delete_reference(env, ref);
104+ return;
105+ }
106+ CleanupContext *cleanupCtx = new CleanupContext{env, ref};
107+ work->data = (void *)cleanupCtx;
108+ uv_queue_work(loop, work, [](uv_work_t *work) {}, cleanup_message_ref);
109+}
110+ 
111+static void freeCallbackContext(CallbackContext *context)
112+{
113+ if (context == nullptr) {
114+ return;
115+ }
116+ if (context->obj != nullptr) {
117+ free(context->obj);
118+ }
119+ delete context;
120+}
121+ 
122+static bool getInstanceLocked(std::string &id, napi_env &env, napi_ref &ref)
123+{
124+ std::lock_guard<std::mutex> lock(IJKPlayerNapi::mapMutex_);
125+ auto it = IJKPlayerNapi::ijkPlayerNapi_.find(id);
126+ IJKPlayerNapi *instance = nullptr;
127+ if (it != IJKPlayerNapi::ijkPlayerNapi_.end()) {
128+ instance = it->second;
129+ if (instance == nullptr) {
130+ return false;
131+ }
132+ } else {
133+ instance = new (std::nothrow) IJKPlayerNapi(id);
134+ if (instance == nullptr) {
135+ return false;
136+ }
137+ IJKPlayerNapi::ijkPlayerNapi_[id] = instance;
138+ }
139+ env = instance->envMessage_;
140+ ref = instance->callBackRefMessage_;
141+ return true;
142+}
143+ 
71void after_message(uv_work_t *work, int status)144void after_message(uv_work_t *work, int status)
72{145{
73 LOGI("napi-->uv_queue_work");146 LOGI("napi-->uv_queue_work");
74 CallbackContext *context = static_cast<CallbackContext *>(work->data);147 CallbackContext *context = static_cast<CallbackContext *>(work->data);
148+ if (context == nullptr || context->env == nullptr || context->callbackRef == nullptr) {
149+ LOGW("napi-->after_message: env or callbackRef is null");
150+ freeCallbackContext(context);
151+ if (work != nullptr) {
152+ delete work;
153+ }
154+ return;
155+ }
75 napi_value callback = nullptr;156 napi_value callback = nullptr;
76- napi_get_reference_value(context->env, context->callbackRef, &callback);157+ napi_status refStatus = napi_get_reference_value(context->env, context->callbackRef, &callback);
cpf-managercpf-manager

【AI-Review】【一般】【基础代码问题】【稳定性问题】after_message错误路径中callbackRef未被清理

● 问题:在 after_message 第112-122行,当 napi_get_reference_value 返回失败或 callback 为nullptr时,函数直接返回并释放了 context->obj、context 和 work,但没有删除 callbackRef。此时 IJKPlayerNapi::callBackRefMessage_ 仍指向该引用。后续 messageCallBack 会继续使用同一个失效的引用,导致 after_message 在每次回调时重复进入错误路径,消息永远无法送达JS层,且无任何日志提示需要重新设置监听器。

● 影响:一般。如果JS回调被GC或引用失效,消息回调将静默失败,用户无法收到播放器事件通知(如播放完成、错误等),且难以排查原因。

● 建议:在 after_message 的错误路径中,当检测到 callback == nullptr(引用有效但底层JS对象已被GC)时,设置一个标志或输出更明确的告警日志,提示应用层需要重新调用 setMessageListener。例如:

if (refStatus != napi_ok || callback == nullptr) {
    LOGE("napi-->after_message: callback reference invalid, setMessageListener should be called again");
    // ...
}
likedislike
wu-guangzhao
wu-guangzhao
Jun 23 Comments:

【AI-Review】【严重】【基础代码问题】【多线程问题】after_message持有的napi_ref可能已被异步删除导致use-after-free

● 问题:messageCallBack(第186行)通过getInstanceLocked获取env和ref的拷贝,存入CallbackContext后通过uv_queue_work调度到after_message。但在调度期间,onSurfaceDestroyedCB可能执行了queueRefCleanup(savedEnv, savedRef),该函数也是通过uv_queue_work在同一event loop上调度cleanup_message_ref来删除ref。由于after_message和cleanup_message_ref都在主线程event loop上执行,它们的执行顺序由libuv调度决定。如果cleanup_message_ref先执行删除了ref,之后after_message执行napi_get_reference_value时操作的是已删除的ref,将导致crash或未定义行为。

关键时序:messageCallBack中获取ref拷贝 → uv_queue_work(after_message) → onSurfaceDestroyedCB中queueRefCleanup → uv_queue_work(cleanup_message_ref) → libuv按调度顺序执行callback → 若cleanup_message_ref先执行,ref被删除 → after_message对已删除ref调用napi_get_reference_value。

● 影响:严重。在Surface销毁与消息回调并发场景下,可能导致对已释放napi_ref的操作,引发native crash。

● 建议:考虑以下方案之一:

  1. 在onSurfaceDestroyedCB中,将ref从instance移出保存到局部变量后,不要立即queueRefCleanup,而是先检查是否有pending的after_message任务。可以使用引用计数机制:instance维护一个pendingCallbackCount_,messageCallBack递增,after_message递减,onSurfaceDestroyedCB等待计数归零后再清理ref。
  2. 使用napi_reference_ref/napi_reference_unref管理ref的生命周期。在messageCallBack中napi_reference_ref增加引用计数,在after_message中napi_reference_unref减少引用计数,当引用计数归零时才真正删除。
likedislike
wu-guangzhao
wu-guangzhao
Jun 23 Comments:
158+ if (refStatus != napi_ok || callback == nullptr) {
159+ LOGE("napi-->after_message: napi_get_reference_value failed");
160+ freeCallbackContext(context);
161+ if (work != nullptr) {
162+ delete work;
163+ }
164+ return;
165+ }
77 napi_value what_;166 napi_value what_;
78 napi_value arg1_;167 napi_value arg1_;
79 napi_value arg2_;168 napi_value arg2_;
@@ -90,42 +179,56 @@ void after_message(uv_work_t *work, int status)
90 napi_value argv_3[] = {what_, arg1_, arg2_};179 napi_value argv_3[] = {what_, arg1_, arg2_};
91 napi_call_function(context->env, nullptr, callback, PARAM_COUNT_3, argv_3, &ret);180 napi_call_function(context->env, nullptr, callback, PARAM_COUNT_3, argv_3, &ret);
92 }181 }
182+ freeCallbackContext(context);
93 if (work != nullptr) {183 if (work != nullptr) {
94 delete work;184 delete work;
95 }185 }
96- delete context;
97 LOGI("napi-->uv_queue_work end");186 LOGI("napi-->uv_queue_work end");
98}187}
99 188 
100void messageCallBack(int what, int arg1, int arg2, char *obj, std::string id)189void messageCallBack(int what, int arg1, int arg2, char *obj, std::string id)
101{190{
102 LOGI("napi-->messageCallBack");191 LOGI("napi-->messageCallBack");
103- struct CallbackContext *context = new CallbackContext();192+ CallbackContext *context = new CallbackContext();
104- IJKPlayerNapi *instance = IJKPlayerNapi::getInstance(id);193+ context->what = what;
105- if (instance == nullptr) {194+ context->arg1 = arg1;
195+ context->arg2 = arg2;
196+ context->obj = nullptr;
197+ if (obj != nullptr) {
198+ size_t objLen = strlen(obj) + 1;
199+ context->obj = (char *)malloc(objLen);
200+ if (context->obj != nullptr) {
201+ memcpy(context->obj, obj, objLen);
202+ }
203+ }
204+ napi_env env = nullptr;
205+ napi_ref ref = nullptr;
206+ bool found = getInstanceLocked(id, env, ref);
207+ if (!found) {
106 LOGI("napi-->instance null");208 LOGI("napi-->instance null");
107- delete context;209+ freeCallbackContext(context);
108 return;210 return;
109 }211 }
110- context->env = instance->envMessage_;212+ if (env == nullptr || ref == nullptr) {
213+ LOGW("napi-->envMessage_ or callBackRefMessage_ null, skip message");
214+ freeCallbackContext(context);
215+ return;
216+ }
217+ context->env = env;
218+ context->callbackRef = ref;
111 uv_loop_s *loopMessage = nullptr;219 uv_loop_s *loopMessage = nullptr;
112- napi_get_uv_event_loop(context->env, &loopMessage);220+ napi_get_uv_event_loop(env, &loopMessage);
113 if (loopMessage == nullptr) {221 if (loopMessage == nullptr) {
114 LOGI("napi-->loopMessage null");222 LOGI("napi-->loopMessage null");
115- delete context;223+ freeCallbackContext(context);
116 return;224 return;
117 }225 }
118 uv_work_t *work = new (std::nothrow) uv_work_t;226 uv_work_t *work = new (std::nothrow) uv_work_t;
119 if (work == nullptr) {227 if (work == nullptr) {
120 LOGI("napi-->work null");228 LOGI("napi-->work null");
121- delete context;229+ freeCallbackContext(context);
122 return;230 return;
123 }231 }
124- context->what = what;
125- context->arg1 = arg1;
126- context->arg2 = arg2;
127- context->obj = obj;
128- context->callbackRef = instance->callBackRefMessage_;
129 work->data = (void *)context;232 work->data = (void *)context;
130 uv_queue_work(loopMessage, work, [](uv_work_t *work) {}, after_message);233 uv_queue_work(loopMessage, work, [](uv_work_t *work) {}, after_message);
131}234}
@@ -1087,25 +1190,33 @@ void onSurfaceDestroyedCB(OH_NativeXComponent *component, void *window)
1087 return;1190 return;
1088 }1191 }
1089 std::string id(idStr);1192 std::string id(idStr);
1090- auto ijkplayerNapi = IJKPlayerNapi::getInstance(id);1193+ IJKPlayerNapi *instance = nullptr;
1091- ijkplayerNapi->onSurfaceDestroyed(component, window);1194+ napi_env savedEnv = nullptr;
1092- 1195+ napi_ref savedRef = nullptr;
1093- // If a JS callback reference was created, delete it to avoid leaking JS objects1196+ {
1094- if (ijkplayerNapi != nullptr) {1197+ std::lock_guard<std::mutex> lock(IJKPlayerNapi::mapMutex_);
1095- if (ijkplayerNapi->callBackRefMessage_ != nullptr && ijkplayerNapi->envMessage_ != nullptr) {1198+ auto it = IJKPlayerNapi::ijkPlayerNapi_.find(id);
1096- napi_status status = napi_delete_reference(ijkplayerNapi->envMessage_, ijkplayerNapi->callBackRefMessage_);1199+ if (it == IJKPlayerNapi::ijkPlayerNapi_.end()) {
1097- if (status != napi_ok) {1200+ LOGE("napi-->onSurfaceDestroyedCB instance not found");
1098- LOGE("napi-->onSurfaceDestroyedCB napi_delete_reference failed");1201+ return;
1099- }
1100- ijkplayerNapi->callBackRefMessage_ = nullptr;
1101 }1202 }
1102- 1203+ instance = it->second;
1103- IJKPlayerNapi::ijkPlayerNapi_.erase(id);1204+ instance->onSurfaceDestroyed(component, window);
cpf-manager

【AI-Review】【严重】【基础代码问题】【多线程问题】onSurfaceDestroyed()在持有mapMutex_锁时被调用,存在死锁风险

● 问题:onSurfaceDestroyedCB中第1204行instance->onSurfaceDestroyed(component, window)在持有mapMutex_锁的情况下被调用。而getInstance()(第1280行)也会获取mapMutex_锁。当前onSurfaceDestroyed()实现为空(仅打日志),不会触发死锁。但如果未来onSurfaceDestroyed()的实现中需要调用getInstance()或任何其他获取mapMutex_的函数,将导致同一线程重复获取非递归互斥锁,造成死锁。

● 影响:严重。当前不会触发,但这是一个脆弱的设计——后续对onSurfaceDestroyed()的任何扩展都可能无意中引入死锁。由于mapMutex_是std::mutex(非递归锁),同一线程重复加锁是未定义行为,通常直接死锁。

● 建议:将instance->onSurfaceDestroyed(component, window)移到锁作用域之外执行。onSurfaceDestroyed()不依赖map的状态,仅需component和window参数,因此可以安全地在释放锁后调用:

{
    std::lock_guard<std::mutex> lock(IJKPlayerNapi::mapMutex_);
    auto it = IJKPlayerNapi::ijkPlayerNapi_.find(id);
    if (it == IJKPlayerNapi::ijkPlayerNapi_.end()) {
        LOGE("napi-->onSurfaceDestroyedCB instance not found");
        return;
    }
    instance = it->second;
    if (instance->callBackRefMessage_ != nullptr && instance->envMessage_ != nullptr) {
        savedEnv = instance->envMessage_;
        savedRef = instance->callBackRefMessage_;
        instance->callBackRefMessage_ = nullptr;
        instance->envMessage_ = nullptr;
    }
    IJKPlayerNapi::ijkPlayerNapi_.erase(it);
}
instance->onSurfaceDestroyed(component, window);
likedislike
wu-guangzhao
wu-guangzhao
Jun 23 Comments:
1104- 1205+ if (instance->callBackRefMessage_ != nullptr && instance->envMessage_ != nullptr) {
1105- delete ijkplayerNapi->ijkPlayerNapiProxy_;1206+ savedEnv = instance->envMessage_;
1106- ijkplayerNapi->ijkPlayerNapiProxy_ = nullptr;1207+ savedRef = instance->callBackRefMessage_;
1107- delete ijkplayerNapi;1208+ instance->callBackRefMessage_ = nullptr;
1108- ijkplayerNapi = nullptr;1209+ instance->envMessage_ = nullptr;
1210+ }
1211+ IJKPlayerNapi::ijkPlayerNapi_.erase(it);
1212+ }
1213+ if (savedRef != nullptr && savedEnv != nullptr) {
1214+ queueRefCleanup(savedEnv, savedRef);
1215+ }
1216+ if (instance != nullptr) {
cpf-manager

【AI-Review】【一般】【基础代码问题】【多线程问题】onSurfaceDestroyedCB中delete instance后ijkPlayerNapiProxy_可能仍被其他线程引用

● 问题:onSurfaceDestroyedCB(第1213-1217行)在锁外执行delete instance->ijkPlayerNapiProxy_和delete instance。但getInstance被45处调用(包括NAPI回调函数如stop、pause、reset、release等),这些调用虽然通过getInstance的锁保护能获取到instance指针,但使用instance指针(如调用ijkPlayerNapiProxy_->IjkMediaPlayer_stop())时锁已释放。如果此时onSurfaceDestroyedCB执行了delete instance,这些NAPI回调将访问已释放内存。

虽然onSurfaceDestroyedCB先从map中erase了instance,新调用getInstance不会获取到该instance,但已在锁内获取到instance指针的旧调用仍会继续使用它,与onSurfaceDestroyedCB中的delete存在竞态。

● 影响:一般。在Surface销毁与NAPI操作并发场景下,可能导致use-after-free。但由于XComponent回调通常在主线程执行,而NAPI调用也在主线程,实际并发概率取决于是否有异步操作在work线程中引用instance(如stopAsync和releaseAsync中的async_work回调)。

● 建议:考虑使用std::shared_ptr/std::weak_ptr管理IJKPlayerNapi实例的生命周期。map存储std::shared_ptr,使用方持有std::weak_ptr或std::shared_ptr,确保实例在所有使用者完成操作后才被销毁。或者引入引用计数机制,在onSurfaceDestroyedCB中仅标记待销毁,在引用计数归零时才真正delete。

likedislike
wu-guangzhao
wu-guangzhao
Jun 23 Comments:
1217+ delete instance->ijkPlayerNapiProxy_;
1218+ instance->ijkPlayerNapiProxy_ = nullptr;
1219+ delete instance;
1109 }1220 }
1110}1221}
1111 1222 
@@ -1167,6 +1278,7 @@ IJKPlayerNapi::IJKPlayerNapi(std::string &id) : id_(id), component_(nullptr)
1167 1278 
1168IJKPlayerNapi *IJKPlayerNapi::getInstance(std::string &id)1279IJKPlayerNapi *IJKPlayerNapi::getInstance(std::string &id)
1169{1280{
cpf-manager

【AI-Review】【严重】【基础代码问题】【多线程问题】getInstance加锁后释放锁到使用instance之间存在use-after-free窗口

● 问题: getInstance(第1278行)虽然已加锁保护map的查找和插入操作,但锁在函数返回时就释放了。调用者拿到IJKPlayerNapi*裸指针后,在锁外使用该指针访问成员(如ijkPlayerNapiProxy_)。而onSurfaceDestroyedCB在锁内执行erase和锁外执行delete instance,存在以下竞态:

  1. 线程A调用getInstance(id)获取instance指针,锁释放
  2. 线程B执行onSurfaceDestroyedCB,加锁 → erase → 释放锁 → delete instance
  3. 线程A使用instance指针访问ijkPlayerNapiProxy_->xxx(),此时instance已被delete

这个问题在所有45个调用getInstance的NAPI函数中都存在(如setDataSource、prepareAsync、start、stop等),因为它们都在锁外持有并使用instance指针。

● 影响: 严重。在Surface销毁与任何NAPI调用并发时,都可能导致use-after-free崩溃。这是此PR未完全解决的核心问题——锁只保护了map本身的一致性,但没有保护instance对象的生命周期。

● 建议: 考虑使用std::shared_ptr<IJKPlayerNapi>管理instance生命周期,map中存储shared_ptr,调用者获取shared_ptr副本。这样onSurfaceDestroyedCB从map中erase后,只要还有调用者持有shared_ptr,instance就不会被delete。或者引入引用计数机制,在instance上标记"正在销毁"状态,让其他调用者检查后提前返回。

likedislike
wu-guangzhao
wu-guangzhao
Jun 23 Comments:
1281+ std::lock_guard<std::mutex> lock(mapMutex_);
1170 LOGI("napi-->IJKPlayerNapi::getInstance");1282 LOGI("napi-->IJKPlayerNapi::getInstance");
1171 if (ijkPlayerNapi_.find(id) == ijkPlayerNapi_.end()) {1283 if (ijkPlayerNapi_.find(id) == ijkPlayerNapi_.end()) {
1172 LOGI("napi-->IJKPlayerNapi::getInstance create object");1284 LOGI("napi-->IJKPlayerNapi::getInstance create object");
@@ -17,6 +17,7 @@
17#define ijkplayer_ijkplayer_napi_H17#define ijkplayer_ijkplayer_napi_H
18#include <string>18#include <string>
19#include <unordered_map>19#include <unordered_map>
20+#include <mutex>
20#include <ace/xcomponent/native_interface_xcomponent.h>21#include <ace/xcomponent/native_interface_xcomponent.h>
21#include <napi/native_api.h>22#include <napi/native_api.h>
22#include "../proxy/ijkplayer_napi_proxy.h"23#include "../proxy/ijkplayer_napi_proxy.h"
@@ -81,6 +82,7 @@ public:
81public:82public:
82 IJKPlayerNapiProxy *ijkPlayerNapiProxy_;83 IJKPlayerNapiProxy *ijkPlayerNapiProxy_;
83 static std::unordered_map<std::string, IJKPlayerNapi *> ijkPlayerNapi_;84 static std::unordered_map<std::string, IJKPlayerNapi *> ijkPlayerNapi_;
85+ static std::mutex mapMutex_;
84 std::unordered_map<std::string, OH_NativeXComponent *> nativeXComponentMap_;86 std::unordered_map<std::string, OH_NativeXComponent *> nativeXComponentMap_;
85 std::unordered_map<std::string, void *> nativeWindowMap_;87 std::unordered_map<std::string, void *> nativeWindowMap_;
86 static OH_NativeXComponent_Callback callback_;88 static OH_NativeXComponent_Callback callback_;