fix(UI): Tear down theme styles/fonts before lv_deinit to avoid dangling pointers - #104
Open
jaysuk wants to merge 2 commits into
Open
fix(UI): Tear down theme styles/fonts before lv_deinit to avoid dangling pointers#104jaysuk wants to merge 2 commits into
jaysuk wants to merge 2 commits into
Conversation
…inters Theme::setThemeActive() lazily builds each theme's LvglStyles/ComponentStyles exactly once (guarded by `if (!m_lvgl || !m_components)`), which is correct for production where lv_init() runs exactly once per process. The test harness calls lv_init()/lv_deinit() around every single test, but nothing ever reset m_lvgl/m_components back to null in between - so every test after the first reused the first test's styles, whose internal LVGL-heap-backed buffers pointed into memory lv_deinit() had already invalidated. Reading a style property from one of these on the second test's UI tree segfaults (reproduced via gdb: SIGSEGV in lv_style_get_prop_inlined, called while building the second test's TabView, on a style living in LVGL's static work_mem arena that a previous lv_init() cycle had already overwritten). Add Theme::deinit()/Themes::deinit() as the mirror image of init(), and call it in UiTestSuiteInner's destructor before lv_deinit() - while the heap those styles were allocated from is still valid, so freeing them is safe. The existing lazy-init check in setThemeActive() then rebuilds everything fresh next test, with no code path needing to change there. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Theme::deinit() reset m_lvgl/m_components but missed m_fonts. Font's destructor/move-assign delete against whatever FontManager::s_fontManager is current *at the time of the call*, not the manager a given Font was actually created from. FontManager::init() unconditionally replaces s_fontManager every test (no lazy-init guard), so a Theme's fonts from test N were being "deleted" against test N+1's font manager when Theme::setTypeface() overwrote them - freeing against the wrong manager/generation, corrupting state that then crashed later reading glyph data out of UI::Themes::s_fonts. Confirmed via gdb: after the first deinit() fix, the styles-related segfault was gone but the same test pair still crashed, now in lv_font_get_glyph_dsc(font_p=&UI::Themes::s_fonts, ...). Resetting m_fonts alongside m_lvgl/m_components - while the owning FontManager generation is still valid - resolves it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Running more than one gtest case from the same fixture in a single
DuetScreen.testsprocess invocation segfaults on the second test. Reproduces on stockdev— e.g.:Root cause
UiTestSuiteInnercallslv_init()/lv_deinit()around every single test, giving each test a fresh LVGL heap. That's correct for test isolation, but two pieces of long-lived app state don't get torn down to match:Theme::m_lvgl/m_components—Theme::setThemeActive()only callsinit()(which allocates freshLvglStyles/ComponentStyles) if!m_lvgl || !m_components. That guard is correct for production, wherelv_init()runs exactly once per process — but nothing ever resets it back to null between tests, so every test after the first reapplies the first test's styles, whose internal LVGL-heap-backed buffers point into memory the interveninglv_deinit()has already invalidated.Theme::m_fonts—FontManager::init()unconditionally replaces the statics_fontManagerevery test (no lazy-init guard).Font's destructor/move-assignment delete the font they wrap against whatevers_fontManageris current at the time of the call, not the manager that actually created it. SinceTheme::m_fontssurvives across the test boundary,Theme::setTypeface()on test N+1 ends up "deleting" test N's fonts against test N+1's (different) font manager.Confirmed via gdb — first crash was in
lv_style_get_prop_inlined, reading a style living in LVGL's staticwork_memarena that a previouslv_init()generation had already overwritten. After fixing (1) alone, the same two-test scenario still crashed, now inlv_font_get_glyph_dscreadingUI::Themes::s_fonts— confirming (2) as a second, independent instance of the same underlying pattern.Fix
Add
Theme::deinit()/Themes::deinit()as the mirror image ofinit(): releasem_lvgl,m_components, andm_fonts(plus the free-standings_lvglStyles/s_componentStyles) for every theme. Call it fromUiTestSuiteInner's destructor beforelv_deinit()— while the heap those objects were allocated from (and the font manager that created their fonts) is still valid, so releasing them is well-defined. The existing lazy-init checks insetThemeActive()then correctly rebuild everything fresh on the next test; no other call site needed to change.This only affects the test harness. Production calls
lv_init()exactly once and neverlv_deinit()s, sodeinit()is simply never called there.Test plan
TestTabview.*(29 tests) run as a single process, no--gtest_filterbatching workaround: all pass, no crash (previously segfaulted on the 2nd test every time)TestTabview.Empty:TestTabview.AddSingleTab) passes cleanly, confirmed via clean exit code (not just "didn't hang")Note on scope
While verifying this, I found
TestHomeView.*still crashes under the same "run more than one test in a process" scenario, via a different mechanism (staleLvObj*inUI::Navigation's screen-stack tracking, also not reset between tests). Same underlying category of bug, different subsystem — filed as a separate issue rather than folded into this fix, since this PR is scoped to the theme/style/font lifecycle specifically.🤖 Generated with Claude Code