1berry/qtbase
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix race condition when destroying Freetype font engines

Browse Source 
If a QFont is moved to a different thread (B) than where it belongs (A),
and the font cache is then purged on thread A, the last remaining
reference to the engine will be on thread B. When this QFontEngine is
later replaced with one created on B, we end up deleting A's QFontEngine
on thread B.

This caused QFreetypeFace::release() to be called on the wrong thread and
the face would never be removed from the thread-local Freetype cache in
A. Hence that cache would contain a dangling pointer to the freetype face
which we would later end up fetching.

To avoid this, we make sure the thread-local cache itself increases
the ref count of the face. That way, the only time it will be deleted
on a different thread is when the cache has been destroyed because the
thread has shut down.

If the last reference (except the cache reference) to a face is cleared
on a different thread, we keep it in the cache until later. It will
then be in a "deferred delete" mode and will be deleted as soon as
possible. This is done either when the thread shuts down, when a lookup
causes the "deferred delete" face to be returned, or when release()
is called on any font on the thread (at which point we will purge all
faces that only have the single cache reference.)

Fixes: QTBUG-118867
Change-Id: Ifa07a9cb6f4cd3e783e12a73d8b283e70d6fb474
Reviewed-by: Mårten Nordheim <marten.nordheim@qt.io>
(cherry picked from commit 5761dd55c824afb7a7809c7a0d3ce3050b03fe7b)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>
This commit is contained in:
Eskil Abrahamsen Blomfeldt 2023-12-15 16:16:31 +01:00 committed by Qt Cherry-pick Bot
parent 4b662e9a92
commit 97dc63861e
1 changed files with 52 additions and 18 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

70
src/gui/text/freetype/qfontengine_ft.cpp
View File

@ -115,8 +115,11 @@ public:
QtFreetypeData::~QtFreetypeData() QtFreetypeData::~QtFreetypeData()
{ {
for (QHash<QFontEngine::FaceId, QFreetypeFace *>::ConstIterator iter = faces.cbegin(); iter != faces.cend(); ++iter) for (auto iter = faces.cbegin(); iter != faces.cend(); ++iter) {
iter.value()->cleanup(); iter.value()->cleanup();
if (!iter.value()->ref.deref())
delete iter.value();
}
faces.clear(); faces.clear();
FT_Done_FreeType(library); FT_Done_FreeType(library);
library = nullptr; library = nullptr;
@ -213,10 +216,28 @@ QFreetypeFace *QFreetypeFace::getFace(const QFontEngine::FaceId &face_id,
QtFreetypeData *freetypeData = qt_getFreetypeData(); QtFreetypeData *freetypeData = qt_getFreetypeData();
QFreetypeFace *freetype = freetypeData->faces.value(face_id, nullptr); QFreetypeFace *freetype = nullptr;
if (freetype) { auto it = freetypeData->faces.find(face_id);
freetype->ref.ref(); if (it != freetypeData->faces.end()) {
} else { freetype = *it;
Q_ASSERT(freetype->ref.loadRelaxed() > 0);
if (freetype->ref.loadRelaxed() == 1) {
// If there is only one reference left to the face, it means it is only referenced by
// the cache itself, and thus it is in cleanup state (but the final outside reference
// was removed on a different thread so it could not be deleted right away). We then
// complete the cleanup and pretend we didn't find it, so that it can be re-created with
// the present state.
freetype->cleanup();
freetypeData->faces.erase(it);
delete freetype;
freetype = nullptr;
} else {
freetype->ref.ref();
}
}
if (!freetype) {
const auto deleter = [](QFreetypeFace *f) { delete f; }; const auto deleter = [](QFreetypeFace *f) { delete f; };
std::unique_ptr<QFreetypeFace, decltype(deleter)> newFreetype(new QFreetypeFace, deleter); std::unique_ptr<QFreetypeFace, decltype(deleter)> newFreetype(new QFreetypeFace, deleter);
FT_Face face; FT_Face face;
@ -322,6 +343,7 @@ QFreetypeFace *QFreetypeFace::getFace(const QFontEngine::FaceId &face_id,
QT_RETHROW; QT_RETHROW;
} }
freetype = newFreetype.release(); freetype = newFreetype.release();
freetype->ref.ref();
} }
return freetype; return freetype;
} }
@ -335,24 +357,36 @@ void QFreetypeFace::cleanup()
void QFreetypeFace::release(const QFontEngine::FaceId &face_id) void QFreetypeFace::release(const QFontEngine::FaceId &face_id)
{ {
if (!ref.deref()) { Q_UNUSED(face_id);
if (face) { bool deleteThis = !ref.deref();
QtFreetypeData *freetypeData = qt_getFreetypeData();
cleanup(); // If the only reference left over is the cache's reference, we remove it from the cache,
// granted that we are on the correct thread. If not, we leave it there to be cleaned out
auto it = freetypeData->faces.constFind(face_id); // later. While we are at it, we also purge all left over faces which are only referenced
if (it != freetypeData->faces.constEnd()) // from the cache.
freetypeData->faces.erase(it); if (face && ref.loadRelaxed() == 1) {
QtFreetypeData *freetypeData = qt_getFreetypeData();
if (freetypeData->faces.isEmpty()) { for (auto it = freetypeData->faces.constBegin(); it != freetypeData->faces.constEnd(); ) {
FT_Done_FreeType(freetypeData->library); if (it.value()->ref.loadRelaxed() == 1) {
freetypeData->library = nullptr; it.value()->cleanup();
if (it.value() == this)
deleteThis = true; // This face, delete at end of function for safety
else
delete it.value();
it = freetypeData->faces.erase(it);
} else {
++it;
} }
} }
delete this; if (freetypeData->faces.isEmpty()) {
FT_Done_FreeType(freetypeData->library);
freetypeData->library = nullptr;
}
} }
if (deleteThis)
delete this;
} }
static int computeFaceIndex(const QString &faceFileName, const QString &styleName) static int computeFaceIndex(const QString &faceFileName, const QString &styleName)