-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix fonts leaks #536
fix fonts leaks #536
Conversation
Regarding this commit:
Again just asking: why ? For consistency, or real technical need by your following commits? |
I think in theory it's supposed to make it a touch harder to accidentally make a certain class of mistakes. I have no (sufficiently informed) opinion on whether it succeeds at that. |
Yes, the whole point of using references is to simplify the instance lifecycle: share access to the fonts while ensuring it's kept alive as long as needed and only destroy the instance when the last reference is destroyed. So it's better to avoid "breaking out" of that by returning a direct pointer. |
Clear all font references before calling `UnregisterDocumentFonts` (so each font reference count can be checked for leaks).
Over using direct pointers.
Ensure each font internal font references are cleared to avoid circular reference loops that would prevent garbage collecting some fonts and/or leaking others.
88ccc12
to
455791f
Compare
if (droppedCount) { | ||
// We need more than 1 gc() for a complete cleanup: to drop font | ||
// instances that were only referenced by dropped fonts (fallbacks, | ||
// bullet, decimal, …). The performance impact of reinstantiating |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This reads asbullet, decimal, â~@¦).
for people from the previous century still using latin1 terminals.)
- koreader/crengine#527 - koreader/crengine#528 - koreader/crengine#530 - koreader/crengine#531 - koreader/crengine#532 - koreader/crengine#533 - koreader/crengine#534 - koreader/crengine#535 - koreader/crengine#536 - koreader/crengine#538 - koreader/crengine#539 - koreader/crengine#540 - koreader/crengine#541 - koreader/crengine#542 - koreader/crengine#544
- koreader/crengine#527 - koreader/crengine#528 - koreader/crengine#530 - koreader/crengine#531 - koreader/crengine#532 - koreader/crengine#533 - koreader/crengine#534 - koreader/crengine#535 - koreader/crengine#536 - koreader/crengine#538 - koreader/crengine#539 - koreader/crengine#540 - koreader/crengine#541 - koreader/crengine#542 - koreader/crengine#544
@benoit-pierre:
Probably related to this PR (it does not crash with v2023.06.1). |
It does not crash on my version thanks to this commit. I haven't investigated further. |
Feels like PR'ing some of your fixes ? |
This commit was not meant to fix that assert, but to prevent hundreds of unnecessary font registrations on some books (FWIR, resulting in 7x slower loads). After reverting that commit, those books still load fine, unlike your EPUB. So there's still a need to investigate why. |
Not really familiar with all this embeddedfont stuff, I hope you can follow up. <link rel="stylesheet" type="text/css" href="../Styles/style.css"/>
[...]
<style>
@font-face {
font-family: "Adobe Garamond Pro (embedded)";
font-weight: normal;
font-style: normal;
src: url(../Fonts/AdobeGaramondPro-FoE-Regular.otf);
}
</style>
</head> It's a duplicated font, also present in style.css - but I don't think the duplication of a same font is the reason. crengine/crengine/src/epubfmt.cpp Lines 2039 to 2044 in 55429c3
and calling unregisterEmbeddedFonts() have your assert met all the fonts with a refcount of 2.(If I skip the assert, it pass through that and render the document, and the normal unregisterEmbeddedFonts() called later on met all the fonts with a refcount of 1.
As it is a codepath rarely taken, it's possible that you could reproduce the crash with your build even if you just add to some fragment: <style>
@font-face {
font-family: "crap";
font-weight: normal;
font-style: normal;
src: url(../Fonts/non-existant-font.otf);
}
</style> |
Or.... may be the fix is logical and simple :) : --- a/crengine/src/epubfmt.cpp
+++ b/crengine/src/epubfmt.cpp
@@ -2038,14 +2038,15 @@ bool ImportEpubDocument( LVStreamRef stream, ldomDocument * m_doc, LVDocViewCall
if ( fontList.length() != fontList_nb_before_head_parsing ) {
// New fonts met when parsing <head><style> of some DocFragments
+ // Drop styles (before unregistering fonts, as they may reference them)
+ m_doc->forceReinitStyles();
+ // todo: we could avoid forceReinitStyles() when embedded fonts are disabled
+ // (but being here is quite rare - and having embedded font disabled even more)
m_doc->unregisterEmbeddedFonts();
// set document font list, and register fonts
m_doc->getEmbeddedFontList().set(fontList);
m_doc->registerEmbeddedFonts();
printf("CRE: document loaded, but styles re-init needed (cause: embedded fonts)\n");
- m_doc->forceReinitStyles();
- // todo: we could avoid forceReinitStyles() when embedded fonts are disabled
- // (but being here is quite rare - and having embedded font disabled even more)
}
// fragmentCount is not fool proof, best to check if we really made |
LGTM. |
Main 2 changes:
This change is