Fix font test segfault on SDL3 #3549
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughUpdated font deallocation in src_c/font.c to conditionally call TTF_CloseFont based on SDL version and a generation check, preventing closure after SDL_ttf teardown. For SDL3+, close only when generations match; for older SDL_ttf, preserve prior nulling-then-close behavior. Public interfaces unchanged. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant FontObj as Font object
participant SDLTTF as SDL_ttf
Caller->>FontObj: deallocate()
alt SDL >= 3.0.0
FontObj->>FontObj: check generations (self vs current)
alt generations match
FontObj->>SDLTTF: TTF_CloseFont(font)
FontObj->>FontObj: self->font = NULL
else generations mismatch
FontObj->>FontObj: skip close (no-op)
end
else SDL < 3.0.0
FontObj->>FontObj: null underlying face (private trick)
FontObj->>SDLTTF: TTF_CloseFont(font)
FontObj->>FontObj: self->font = NULL
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src_c/font.c (1)
1213-1232: Wrong version macro: use SDL_TTF_VERSION_ATLEAST for TTF-specific behaviorThis guard should key off the SDL_ttf version, not the SDL core version. The rest of the file consistently uses SDL_TTF_VERSION_ATLEAST for TTF API differences. Using SDL_VERSION_ATLEAST here risks selecting the wrong branch on mixed-version builds and is inconsistent.
Apply this diff:
-#if SDL_VERSION_ATLEAST(3, 0, 0) +#if SDL_TTF_VERSION_ATLEAST(3, 0, 0)
🧹 Nitpick comments (2)
src_c/font.c (2)
1213-1218: Clarify comment to reflect SDL_ttf 3.x semanticsMinor wording nit: This is about SDL_ttf 3.x teardown behavior, not SDL3 core. Tighten the comment for accuracy and consistency with the rest of the file.
Apply this diff:
- // In SDL3_ttf, it seems that closing a font after its library was - // destroyed segfaults. So only close if same generation. - // TODO SDL3: - // TTF docs say "A well-written program should call TTF_CloseFont() - // on any open fonts before calling this function!" + // In SDL_ttf 3.x, closing a font after TTF_Quit() may segfault. + // Only close when the font was created in the current generation. + // TODO(SDL_ttf 3): Docs recommend calling TTF_CloseFont() on all open + // fonts before TTF_Quit(); our generation guard intentionally violates + // that to avoid crashes when teardown order is not guaranteed.
1213-1232: Optionally clear self->font even if font_initialized is falseIf TTF was already quit (font_initialized == 0), this block is skipped and self->font is left non-NULL until the object memory is freed. Harmless, but setting it to NULL defensively avoids any chance of stale handle reuse in future maintenance or re-entrant finalization paths.
You can add this just before calling tp_free (outside the conditional), e.g.:
/* Ensure member cleared even if we skipped TTF_CloseFont because SDL_ttf was quit. */ self->font = NULL;
|
For reviewers, this PR is still ready for review. I have looked over the coderabbit comments and none of them are valid or need to be done. |
There was a problem hiding this comment.
For now I think this is good enough, fixing a segfault should be a priority and this PR does that. Once our SDL3 support is more mature we could investigate into the edge case (quit not in the same generationl) and see if there's a memory leak and then plan a fix (probably keeping track of all open fonts on our side and closing stuff manually on quit might be needed?)
Tested in combination with #3428 and #3544
EDIT: removed extremely misleading AI summary