QText2DEntity: fix QTextureAtlas parenting that could lead to crashes

We rely on a DistanceFieldFont object to manage QTextureAtlas that hold
the glyphs. The DistanceFieldFont/QTextureAtlas are supposed to be parented
by the scene root to ensure that a QTextureAtlas lives as long as possible.

DistanceFieldFont/QTextureAtlas are stored in a cache global to the scene
to minimize the use of resources.

When adding text elements, we can reuse atlases since the cache is global to
the scene and only destroy an atlas (and remove it from the cache) when we
know no more glyphs are referencing it.

However we were mistakenly passing a null parenty to DistanceFieldFont instace
of the scene root. This resulted on the QTextureAtlas not being parented by
the scene root but rather by the first DistanceFieldRenderer to use the atlas.
This meants that if the DistanceFieldRenderer were to be destroyed, so would
the atlas (yet it would still be referenced by the glyph cache leading to
crashes).

Change-Id: Id84f6a651b162a4bb3c571b11388fd2429b231de
Reviewed-by: Mike Krus <mike.krus@kdab.com>
(cherry picked from commit b1a135c547f38db0b2ce6b7bc4c4cccc43ef87d3)
Reviewed-by: Paul Lemire <paul.lemire@kdab.com>

3 files changed, 18 insertions, 10 deletions

diff --git a/src/extras/text/qdistancefieldglyphcache.cpp b/src/extras/text/qdistancefieldglyphcache.cpp
index a65e7fc63..85591e680 100644
--- a/src/extras/text/qdistancefieldglyphcache.cpp
+++ b/src/extras/text/qdistancefieldglyphcache.cpp
@@ -161,6 +161,7 @@ DistanceFieldFont::DistanceFieldFont(const QRawFont &font, bool doubleRes, Qt3DC
     , m_doubleGlyphResolution(doubleRes)
     , m_parentNode(parent)
 {
+    Q_ASSERT(m_parentNode);
 }
 
 DistanceFieldFont::~DistanceFieldFont()
@@ -197,13 +198,14 @@ StoredGlyph DistanceFieldFont::refGlyph(quint32 glyph)
         // scenarios
         const int size = m_doubleGlyphResolution ? 512 : 256;
 
-        QTextureAtlas *atlas = new QTextureAtlas(m_parentNode);
+        QTextureAtlas *atlas = new QTextureAtlas();
         atlas->setWidth(size);
         atlas->setHeight(size);
         atlas->setFormat(Qt3DRender::QAbstractTexture::R8_UNorm);
         atlas->setPixelFormat(QOpenGLTexture::Red);
         atlas->setMinificationFilter(Qt3DRender::QAbstractTexture::Linear);
         atlas->setMagnificationFilter(Qt3DRender::QAbstractTexture::Linear);
+        atlas->setParent(m_parentNode);
         m_atlasses << atlas;
 
         if (!storedGlyph.addToTextureAtlas(atlas))
@@ -236,7 +238,12 @@ void DistanceFieldFont::derefGlyph(quint32 glyph)
             Q_ASSERT(m_atlasses.contains(atlas));
 
             m_atlasses.removeAll(atlas);
-            delete atlas;
+
+            // This function might have been called as a result of destroying
+            // the scene root which traverses the entire scene tree. Calling
+            // delete on the atlas here could lead to dangling pointers in the
+            // least of children being traversed for destruction.
+            atlas->deleteLater();
         }
 
         m_glyphs.erase(it);
@@ -287,7 +294,8 @@ DistanceFieldFont* QDistanceFieldGlyphCache::getOrCreateDistanceFieldFont(const
     // create new font cache
     // we set the parent node to nullptr, since the parent node of QTextureAtlasses
     // will be set when we pass them to QText2DMaterial later
-    DistanceFieldFont *dff = new DistanceFieldFont(actualFont, useDoubleRes, nullptr);
+    Q_ASSERT(m_rootNode);
+    DistanceFieldFont *dff = new DistanceFieldFont(actualFont, useDoubleRes, m_rootNode);
     m_fonts.insert(key, dff);
     return dff;
 }
@@ -324,11 +332,10 @@ QDistanceFieldGlyphCache::Glyph refAndGetGlyph(DistanceFieldFont *dff, quint32 g
     if (dff) {
         const auto entry = dff->refGlyph(glyph);
 
-        if (entry.atlas()) {
-            ret.glyphPathBoundingRect = entry.glyphPathBoundingRect();
-            ret.texCoords = entry.texCoords();
-            ret.texture = entry.atlas();
-        }
+        Q_ASSERT(entry.atlas());
+        ret.glyphPathBoundingRect = entry.glyphPathBoundingRect();
+        ret.texCoords = entry.texCoords();
+        ret.texture = entry.atlas();
     }
 
     return ret;
diff --git a/src/extras/text/qtext2dentity.cpp b/src/extras/text/qtext2dentity.cpp
index 59e8284e1..e3d3dad2e 100644
--- a/src/extras/text/qtext2dentity.cpp
+++ b/src/extras/text/qtext2dentity.cpp
@@ -304,8 +304,9 @@ void QText2DEntityPrivate::setCurrentGlyphRuns(const QVector<QGlyphRun> &runs)
         delete m_renderers.takeLast();
 
     while (m_renderers.size() < renderData.size()) {
-        DistanceFieldTextRenderer *renderer = new DistanceFieldTextRenderer(q_func());
+        DistanceFieldTextRenderer *renderer = new DistanceFieldTextRenderer();
         renderer->setColor(m_color);
+        renderer->setParent(q_func());
         m_renderers << renderer;
     }
 
diff --git a/tests/auto/extras/qtext2dentity/tst_qtext2dentity.cpp b/tests/auto/extras/qtext2dentity/tst_qtext2dentity.cpp
index 6fcc2e637..35e241839 100644
--- a/tests/auto/extras/qtext2dentity/tst_qtext2dentity.cpp
+++ b/tests/auto/extras/qtext2dentity/tst_qtext2dentity.cpp
@@ -90,7 +90,7 @@ void tst_qtext2dentity::checkChangeArbiter()
     auto atlases = lookupNodeByClassName(rootEntity.data(), "Qt3DExtras::QTextureAtlas");
     QVERIFY(atlases.length() == 1);
     auto atlas = atlases[0];
-    QTRY_VERIFY(Qt3DCore::QNodePrivate::get(atlas)->m_changeArbiter);
+    QVERIFY(Qt3DCore::QNodePrivate::get(atlas)->m_changeArbiter);
 #endif
 }