fix fonts leaks

[benoit-pierre]

Main 2 changes:

• ensure a self-referencing font can still possibly be garbage collected.
• check and ensure we don't remove the last reference (from the font manager internal list of instances / registrations) to a font with a reference count >1 (leak).

---

This change is 

[poire-z]

Regarding this commit:

lvfntman: prefer font references
Over using direct pointers.

Again just asking: why ? For consistency, or real technical need by your following commits?

[Frenzie]

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.

[benoit-pierre]

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.

[poire-z]

@benoit-pierre: I was scripting the opening of all the books from my history, to check for some crengine log. I got a crash with some old book I found once on mobilread I think - probably free, the latest version at https://books.djazz.se/fallout-equestria/ doesn't crash - but this one does: Fallout Equestria Official Ebook190405.zip

03/16/24-22:28:39 DEBUG CreDocument: loading document...
luajit: crengine/src/lvfntman.cpp:7489: void LVFontCache::_removeDocumentFonts(int, LVPtrVector<LVFontCacheItem>&): Assertion `!item->_fnt || item->_fnt.getRefCount() == 1' failed.
#0  0x00007ffff7d62e2c in ?? () from /lib/x86_64-linux-gnu/libc.so.6
#1  0x00007ffff7d13fb2 in raise () from /lib/x86_64-linux-gnu/libc.so.6
#2  0x00007ffff7cfe472 in abort () from /lib/x86_64-linux-gnu/libc.so.6
#3  0x00007ffff7cfe395 in ?? () from /lib/x86_64-linux-gnu/libc.so.6
#4  0x00007ffff7d0ceb2 in __assert_fail () from /lib/x86_64-linux-gnu/libc.so.6
#5  0x00007fffe050b633 in LVFontCache::_removeDocumentFonts (this=this@entry=0x555556e41fc0, documentId=documentId@entry=4, list=...)
    at crengine/src/lvfntman.cpp:7489
#6  0x00007fffe050b6e0 in LVFontCache::removeDocumentFonts (this=0x555556e41fc0, documentId=4) at crengine/src/lvfntman.cpp:7501
#7  0x00007fffe05161e1 in LVFreeTypeFontManager::UnregisterDocumentFonts (this=<optimized out>, documentId=<optimized out>)
    at crengine/src/lvfntman.cpp:6383
#8  0x00007fffe049babd in ldomDocument::unregisterEmbeddedFonts (this=this@entry=0x55555714b0b0) at crengine/src/lvtinydom.cpp:20294
#9  0x00007fffe05854d3 in ImportEpubDocument (stream=..., m_doc=0x55555714b0b0, progressCallback=0x555556f85cc0, formatCallback=formatCallback@entry=0x5555574917e0,
    metadataOnly=<optimized out>) at crengine/src/epubfmt.cpp:1852

Probably related to this PR (it does not crash with v2023.06.1). Interested in having a look?

[benoit-pierre]

It does not crash on my version thanks to this commit. I haven't investigated further.

[poire-z]

Feels like PR'ing some of your fixes ?

[benoit-pierre]

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.

[poire-z]

Not really familiar with all this embeddedfont stuff, I hope you can follow up. Anyway, I pinpoint what makes it crash: One of the xhtml fragment of this EPUB (chapter_45.xhtml) has in it:

  <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. But the presence of a @font-face in <style> causes this to trigger: https://github.com/koreader/crengine/blob/55429c37868322408df15cfb0d26bc839b90cccf/crengine/src/epubfmt.cpp#L2039-L2044 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>

[poire-z]

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

[benoit-pierre]

LGTM.