From 5b874bc3ad3233269a3282ae1f71e8c3fc80fddc Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Sat, 31 Jan 2026 00:11:42 +0800 Subject: [PATCH] Fix LVGL object deletion crash and thread safety issues in SetChatMessage (#1708) * Initial plan * Fix LVGL object deletion issue in SetChatMessage - Refresh child_count after deleting first_child to avoid using stale count - Add lv_obj_is_valid() checks before accessing deleted objects - Prevent duplicate deletion by refreshing child_count before system message deletion - Ensures last_child validation before scrolling to it Co-authored-by: 78 <4488133+78@users.noreply.github.com> * Address code review feedback on LVGL object deletion fix - Move last_child retrieval after deletion to avoid stale pointer - Add child_count > 0 check before scrolling to prevent edge case issues - Add lv_obj_is_valid() check for last_bubble for consistency Co-authored-by: 78 <4488133+78@users.noreply.github.com> * Final review: LVGL object deletion fix complete Co-authored-by: 78 <4488133+78@users.noreply.github.com> * Remove std::thread wrappers in display callbacks to fix thread safety Remove detached threads that were calling SetChatMessage from download/upgrade progress callbacks. The DisplayLockGuard mutex already provides thread safety, so we can call display methods directly from the callback thread without spawning additional threads, which were causing race conditions. Co-authored-by: 78 <4488133+78@users.noreply.github.com> * Use Application::Schedule() for display updates in callbacks Instead of calling SetChatMessage directly from download/upgrade progress callbacks, queue the updates using Application::Schedule(). This prevents blocking the network receive task and avoids potential UART FIFO overflow issues with 4G modems. The scheduled callbacks execute in the main task thread, maintaining proper thread safety. Co-authored-by: 78 <4488133+78@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: 78 <4488133+78@users.noreply.github.com> --- _codeql_detected_source_root | 1 + main/application.cc | 24 ++++++++++++------------ main/display/lcd_display.cc | 18 ++++++++++++------ 3 files changed, 25 insertions(+), 18 deletions(-) create mode 120000 _codeql_detected_source_root diff --git a/_codeql_detected_source_root b/_codeql_detected_source_root new file mode 120000 index 00000000..945c9b46 --- /dev/null +++ b/_codeql_detected_source_root @@ -0,0 +1 @@ +. \ No newline at end of file diff --git a/main/application.cc b/main/application.cc index 06ea456c..f9c95cb8 100644 --- a/main/application.cc +++ b/main/application.cc @@ -368,12 +368,12 @@ void Application::CheckAssetsVersion() { board.SetPowerSaveLevel(PowerSaveLevel::PERFORMANCE); display->SetChatMessage("system", Lang::Strings::PLEASE_WAIT); - bool success = assets.Download(download_url, [display](int progress, size_t speed) -> void { - std::thread([display, progress, speed]() { - char buffer[32]; - snprintf(buffer, sizeof(buffer), "%d%% %uKB/s", progress, speed / 1024); - display->SetChatMessage("system", buffer); - }).detach(); + bool success = assets.Download(download_url, [this, display](int progress, size_t speed) -> void { + char buffer[32]; + snprintf(buffer, sizeof(buffer), "%d%% %uKB/s", progress, speed / 1024); + Schedule([display, message = std::string(buffer)]() { + display->SetChatMessage("system", message.c_str()); + }); }); board.SetPowerSaveLevel(PowerSaveLevel::LOW_POWER); @@ -921,12 +921,12 @@ bool Application::UpgradeFirmware(const std::string& url, const std::string& ver audio_service_.Stop(); vTaskDelay(pdMS_TO_TICKS(1000)); - bool upgrade_success = Ota::Upgrade(upgrade_url, [display](int progress, size_t speed) { - std::thread([display, progress, speed]() { - char buffer[32]; - snprintf(buffer, sizeof(buffer), "%d%% %uKB/s", progress, speed / 1024); - display->SetChatMessage("system", buffer); - }).detach(); + bool upgrade_success = Ota::Upgrade(upgrade_url, [this, display](int progress, size_t speed) { + char buffer[32]; + snprintf(buffer, sizeof(buffer), "%d%% %uKB/s", progress, speed / 1024); + Schedule([display, message = std::string(buffer)]() { + display->SetChatMessage("system", message.c_str()); + }); }); if (!upgrade_success) { diff --git a/main/display/lcd_display.cc b/main/display/lcd_display.cc index c00a8a7d..cb4f96da 100644 --- a/main/display/lcd_display.cc +++ b/main/display/lcd_display.cc @@ -510,25 +510,31 @@ void LcdDisplay::SetChatMessage(const char* role, const char* content) { if (child_count >= MAX_MESSAGES) { // Delete the oldest message (first child object) lv_obj_t* first_child = lv_obj_get_child(content_, 0); - lv_obj_t* last_child = lv_obj_get_child(content_, child_count - 1); if (first_child != nullptr) { lv_obj_del(first_child); + // Refresh child count after deletion + child_count = lv_obj_get_child_cnt(content_); } - // Scroll to the last message immediately - if (last_child != nullptr) { - lv_obj_scroll_to_view_recursive(last_child, LV_ANIM_OFF); + // Scroll to the last message immediately (get last_child after deletion) + if (child_count > 0) { + lv_obj_t* last_child = lv_obj_get_child(content_, child_count - 1); + if (last_child != nullptr && lv_obj_is_valid(last_child)) { + lv_obj_scroll_to_view_recursive(last_child, LV_ANIM_OFF); + } } } // Collapse system messages (if it's a system message, check if the last message is also a system message) if (strcmp(role, "system") == 0) { + // Refresh child count to get accurate count after potential deletion above + child_count = lv_obj_get_child_cnt(content_); if (child_count > 0) { // Get the last message container lv_obj_t* last_container = lv_obj_get_child(content_, child_count - 1); - if (last_container != nullptr && lv_obj_get_child_cnt(last_container) > 0) { + if (last_container != nullptr && lv_obj_is_valid(last_container) && lv_obj_get_child_cnt(last_container) > 0) { // Get the bubble inside the container lv_obj_t* last_bubble = lv_obj_get_child(last_container, 0); - if (last_bubble != nullptr) { + if (last_bubble != nullptr && lv_obj_is_valid(last_bubble)) { // Check if bubble type is system message void* bubble_type_ptr = lv_obj_get_user_data(last_bubble); if (bubble_type_ptr != nullptr && strcmp((const char*)bubble_type_ptr, "system") == 0) {