QQmlDataBlob: Use RefPointer when interacting with QQmlTypeLoader

QQmlTypeloader and QQmlTypeLoaderThread so far used to manually call ref
and deref on QQmlDataBlob pointers. Changing the refcount was necessary,
as the blobs were stored in various data structures which should keep
the blobs alive; namely the Message struct subclasses use in QQmlThread
and the NetworkReplies hash-map.
However, the whole setup was rather brittle; especially when it comes to
QQmlThread, where methods adding Message structs would increment the
refcount, and the actual callback stored in the Message would decrement
it. This would obviously cause leaks if the Message::call would not be
invoked (e.g. due to the QQmlThread shutting down).
By replacing the raw pointers with QQmlDataBlob::Ptr (which is a new
typedef for QQmlRefPointer<QQmlDataBlob>), we can avoid the whole
issue[1], and also simplify the code. Note that Message stores a
QQmlDataBlob::Ptr by virtue of inferring the type from its stored
method.

[1]: At least in theory; QQmlThread does not clean up its message list
on shut-down so far.

Change-Id: I6aaf5a9dcddfd38795059518ca2d3650072bcda3
Reviewed-by: Ulf Hermann <ulf.hermann@qt.io>
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>

5 files changed, 49 insertions, 65 deletions

diff --git a/src/qml/qml/qqmldatablob_p.h b/src/qml/qml/qqmldatablob_p.h
index e975043871..522d02e97a 100644
--- a/src/qml/qml/qqmldatablob_p.h
+++ b/src/qml/qml/qqmldatablob_p.h
@@ -38,6 +38,8 @@ class QQmlTypeLoader;
 class Q_QML_PRIVATE_EXPORT QQmlDataBlob : public QQmlRefCount
 {
 public:
+    using Ptr = QQmlRefPointer<QQmlDataBlob>;
+
     enum Status {
         Null,                    // Prior to QQmlTypeLoader::load()
         Loading,                 // Prior to data being received and dataReceived() being called
diff --git a/src/qml/qml/qqmltypeloader.cpp b/src/qml/qml/qqmltypeloader.cpp
index 24340d8ecf..70b21f667c 100644
--- a/src/qml/qml/qqmltypeloader.cpp
+++ b/src/qml/qml/qqmltypeloader.cpp
@@ -88,8 +88,6 @@ void QQmlTypeLoader::invalidate()
     // Need to delete the network replies after
     // the loader thread is shutdown as it could be
     // getting new replies while we clear them
-    for (NetworkReplies::Iterator iter = m_networkReplies.begin(); iter != m_networkReplies.end(); ++iter)
-        (*iter)->release();
     m_networkReplies.clear();
 #endif // qml_network
 }
@@ -214,21 +212,21 @@ void QQmlTypeLoader::loadWithCachedUnit(QQmlDataBlob *blob, const QQmlPrivate::C
     doLoad(CachedLoader(unit), blob, mode);
 }
 
-void QQmlTypeLoader::loadWithStaticDataThread(QQmlDataBlob *blob, const QByteArray &data)
+void QQmlTypeLoader::loadWithStaticDataThread(const QQmlDataBlob::Ptr &blob, const QByteArray &data)
 {
     ASSERT_LOADTHREAD();
 
     setData(blob, data);
 }
 
-void QQmlTypeLoader::loadWithCachedUnitThread(QQmlDataBlob *blob, const QQmlPrivate::CachedQmlUnit *unit)
+void QQmlTypeLoader::loadWithCachedUnitThread(const QQmlDataBlob::Ptr &blob, const QQmlPrivate::CachedQmlUnit *unit)
 {
     ASSERT_LOADTHREAD();
 
     setCachedUnit(blob, unit);
 }
 
-void QQmlTypeLoader::loadThread(QQmlDataBlob *blob)
+void QQmlTypeLoader::loadThread(const QQmlDataBlob::Ptr &blob)
 {
     ASSERT_LOADTHREAD();
 
@@ -264,7 +262,6 @@ void QQmlTypeLoader::loadThread(QQmlDataBlob *blob)
 #if QT_CONFIG(qml_network)
         QNetworkReply *reply = m_thread->networkAccessManager()->get(QNetworkRequest(blob->m_url));
         QQmlTypeLoaderNetworkReplyProxy *nrp = m_thread->networkReplyProxy();
-        blob->addref();
         m_networkReplies.insert(reply, blob);
 
         if (reply->isFinished()) {
@@ -296,7 +293,7 @@ void QQmlTypeLoader::networkReplyFinished(QNetworkReply *reply)
 
     reply->deleteLater();
 
-    QQmlDataBlob *blob = m_networkReplies.take(reply);
+    QQmlRefPointer<QQmlDataBlob> blob = m_networkReplies.take(reply);
 
     Q_ASSERT(blob);
 
@@ -312,7 +309,7 @@ void QQmlTypeLoader::networkReplyFinished(QNetworkReply *reply)
             QNetworkReply *reply = m_thread->networkAccessManager()->get(QNetworkRequest(url));
             QObject *nrp = m_thread->networkReplyProxy();
             QObject::connect(reply, SIGNAL(finished()), nrp, SLOT(finished()));
-            m_networkReplies.insert(reply, blob);
+            m_networkReplies.insert(reply, std::move(blob));
 #ifdef DATABLOB_DEBUG
             qWarning("QQmlDataBlob: redirected to %s", qPrintable(blob->finalUrlString()));
 #endif
@@ -326,8 +323,6 @@ void QQmlTypeLoader::networkReplyFinished(QNetworkReply *reply)
         QByteArray data = reply->readAll();
         setData(blob, data);
     }
-
-    blob->release();
 }
 
 void QQmlTypeLoader::networkReplyProgress(QNetworkReply *reply,
@@ -335,7 +330,7 @@ void QQmlTypeLoader::networkReplyProgress(QNetworkReply *reply,
 {
     Q_ASSERT(m_thread->isThisThread());
 
-    QQmlDataBlob *blob = m_networkReplies.value(reply);
+    const QQmlRefPointer<QQmlDataBlob> blob = m_networkReplies.value(reply);
 
     Q_ASSERT(blob);
 
@@ -384,7 +379,7 @@ void QQmlTypeLoader::initializeEngine(QQmlExtensionInterface *iface, const char
     doInitializeEngine(iface, m_thread, engine(), uri);
 }
 
-void QQmlTypeLoader::setData(QQmlDataBlob *blob, const QByteArray &data)
+void QQmlTypeLoader::setData(const QQmlDataBlob::Ptr &blob, const QByteArray &data)
 {
     QQmlDataBlob::SourceCodeData d;
     d.inlineSourceCode = QString::fromUtf8(data);
@@ -392,17 +387,17 @@ void QQmlTypeLoader::setData(QQmlDataBlob *blob, const QByteArray &data)
     setData(blob, d);
 }
 
-void QQmlTypeLoader::setData(QQmlDataBlob *blob, const QString &fileName)
+void QQmlTypeLoader::setData(const QQmlDataBlob::Ptr &blob, const QString &fileName)
 {
     QQmlDataBlob::SourceCodeData d;
     d.fileInfo = QFileInfo(fileName);
     setData(blob, d);
 }
 
-void QQmlTypeLoader::setData(QQmlDataBlob *blob, const QQmlDataBlob::SourceCodeData &d)
+void QQmlTypeLoader::setData(const QQmlDataBlob::Ptr &blob, const QQmlDataBlob::SourceCodeData &d)
 {
     Q_TRACE_SCOPE(QQmlCompiling, blob->url());
-    QQmlCompilingProfiler prof(profiler(), blob);
+    QQmlCompilingProfiler prof(profiler(), blob.data());
 
     blob->m_inCallback = true;
 
@@ -419,10 +414,10 @@ void QQmlTypeLoader::setData(QQmlDataBlob *blob, const QQmlDataBlob::SourceCodeD
     blob->tryDone();
 }
 
-void QQmlTypeLoader::setCachedUnit(QQmlDataBlob *blob, const QQmlPrivate::CachedQmlUnit *unit)
+void QQmlTypeLoader::setCachedUnit(const QQmlDataBlob::Ptr &blob, const QQmlPrivate::CachedQmlUnit *unit)
 {
     Q_TRACE_SCOPE(QQmlCompiling, blob->url());
-    QQmlCompilingProfiler prof(profiler(), blob);
+    QQmlCompilingProfiler prof(profiler(), blob.data());
 
     blob->m_inCallback = true;
 
diff --git a/src/qml/qml/qqmltypeloader_p.h b/src/qml/qml/qqmltypeloader_p.h
index 641a15494c..98edf3241b 100644
--- a/src/qml/qml/qqmltypeloader_p.h
+++ b/src/qml/qml/qqmltypeloader_p.h
@@ -171,20 +171,20 @@ private:
 
     void shutdownThread();
 
-    void loadThread(QQmlDataBlob *);
-    void loadWithStaticDataThread(QQmlDataBlob *, const QByteArray &);
-    void loadWithCachedUnitThread(QQmlDataBlob *blob, const QQmlPrivate::CachedQmlUnit *unit);
+    void loadThread(const QQmlDataBlob::Ptr &);
+    void loadWithStaticDataThread(const QQmlDataBlob::Ptr &, const QByteArray &);
+    void loadWithCachedUnitThread(const QQmlDataBlob::Ptr &blob, const QQmlPrivate::CachedQmlUnit *unit);
 #if QT_CONFIG(qml_network)
     void networkReplyFinished(QNetworkReply *);
     void networkReplyProgress(QNetworkReply *, qint64, qint64);
 
-    typedef QHash<QNetworkReply *, QQmlDataBlob *> NetworkReplies;
+    typedef QHash<QNetworkReply *, QQmlDataBlob::Ptr> NetworkReplies;
 #endif
 
-    void setData(QQmlDataBlob *, const QByteArray &);
-    void setData(QQmlDataBlob *, const QString &fileName);
-    void setData(QQmlDataBlob *, const QQmlDataBlob::SourceCodeData &);
-    void setCachedUnit(QQmlDataBlob *blob, const QQmlPrivate::CachedQmlUnit *unit);
+    void setData(const QQmlDataBlob::Ptr &, const QByteArray &);
+    void setData(const QQmlDataBlob::Ptr &, const QString &fileName);
+    void setData(const QQmlDataBlob::Ptr &, const QQmlDataBlob::SourceCodeData &);
+    void setCachedUnit(const QQmlDataBlob::Ptr &blob, const QQmlPrivate::CachedQmlUnit *unit);
 
     typedef QHash<QUrl, QQmlTypeData *> TypeCache;
     typedef QHash<QUrl, QQmlScriptBlob *> ScriptCache;
diff --git a/src/qml/qml/qqmltypeloaderthread.cpp b/src/qml/qml/qqmltypeloaderthread.cpp
index c24a3ce02b..3d35962c08 100644
--- a/src/qml/qml/qqmltypeloaderthread.cpp
+++ b/src/qml/qml/qqmltypeloaderthread.cpp
@@ -40,45 +40,38 @@ QQmlTypeLoaderNetworkReplyProxy *QQmlTypeLoaderThread::networkReplyProxy() const
 }
 #endif // qml_network
 
-void QQmlTypeLoaderThread::load(QQmlDataBlob *b)
+void QQmlTypeLoaderThread::load(const QQmlDataBlob::Ptr &b)
 {
-    b->addref();
     callMethodInThread(&This::loadThread, b);
 }
 
-void QQmlTypeLoaderThread::loadAsync(QQmlDataBlob *b)
+void QQmlTypeLoaderThread::loadAsync(const QQmlDataBlob::Ptr &b)
 {
-    b->addref();
     postMethodToThread(&This::loadThread, b);
 }
 
-void QQmlTypeLoaderThread::loadWithStaticData(QQmlDataBlob *b, const QByteArray &d)
+void QQmlTypeLoaderThread::loadWithStaticData(const QQmlDataBlob::Ptr &b, const QByteArray &d)
 {
-    b->addref();
     callMethodInThread(&This::loadWithStaticDataThread, b, d);
 }
 
-void QQmlTypeLoaderThread::loadWithStaticDataAsync(QQmlDataBlob *b, const QByteArray &d)
+void QQmlTypeLoaderThread::loadWithStaticDataAsync(const QQmlDataBlob::Ptr &b, const QByteArray &d)
 {
-    b->addref();
     postMethodToThread(&This::loadWithStaticDataThread, b, d);
 }
 
-void QQmlTypeLoaderThread::loadWithCachedUnit(QQmlDataBlob *b, const QQmlPrivate::CachedQmlUnit *unit)
+void QQmlTypeLoaderThread::loadWithCachedUnit(const QQmlDataBlob::Ptr &b, const QQmlPrivate::CachedQmlUnit *unit)
 {
-    b->addref();
     callMethodInThread(&This::loadWithCachedUnitThread, b, unit);
 }
 
-void QQmlTypeLoaderThread::loadWithCachedUnitAsync(QQmlDataBlob *b, const QQmlPrivate::CachedQmlUnit *unit)
+void QQmlTypeLoaderThread::loadWithCachedUnitAsync(const QQmlDataBlob::Ptr &b, const QQmlPrivate::CachedQmlUnit *unit)
 {
-    b->addref();
     postMethodToThread(&This::loadWithCachedUnitThread, b, unit);
 }
 
-void QQmlTypeLoaderThread::callCompleted(QQmlDataBlob *b)
+void QQmlTypeLoaderThread::callCompleted(const QQmlDataBlob::Ptr &b)
 {
-    b->addref();
 #if !QT_CONFIG(thread)
     if (!isThisThread())
         postMethodToThread(&This::callCompletedMain, b);
@@ -87,9 +80,8 @@ void QQmlTypeLoaderThread::callCompleted(QQmlDataBlob *b)
 #endif
 }
 
-void QQmlTypeLoaderThread::callDownloadProgressChanged(QQmlDataBlob *b, qreal p)
+void QQmlTypeLoaderThread::callDownloadProgressChanged(const QQmlDataBlob::Ptr &b, qreal p)
 {
-    b->addref();
 #if !QT_CONFIG(thread)
     if (!isThisThread())
         postMethodToThread(&This::callDownloadProgressChangedMain, b, p);
@@ -110,41 +102,36 @@ void QQmlTypeLoaderThread::initializeEngine(QQmlEngineExtensionInterface *iface,
     callMethodInMain(&This::initializeEngineExtensionMain, iface, uri);
 }
 
-void QQmlTypeLoaderThread::loadThread(QQmlDataBlob *b)
+void QQmlTypeLoaderThread::loadThread(const QQmlDataBlob::Ptr &b)
 {
     m_loader->loadThread(b);
-    b->release();
 }
 
-void QQmlTypeLoaderThread::loadWithStaticDataThread(QQmlDataBlob *b, const QByteArray &d)
+void QQmlTypeLoaderThread::loadWithStaticDataThread(const QQmlDataBlob::Ptr &b, const QByteArray &d)
 {
     m_loader->loadWithStaticDataThread(b, d);
-    b->release();
 }
 
-void QQmlTypeLoaderThread::loadWithCachedUnitThread(QQmlDataBlob *b, const QQmlPrivate::CachedQmlUnit *unit)
+void QQmlTypeLoaderThread::loadWithCachedUnitThread(const QQmlDataBlob::Ptr &b, const QQmlPrivate::CachedQmlUnit *unit)
 {
     m_loader->loadWithCachedUnitThread(b, unit);
-    b->release();
 }
 
-void QQmlTypeLoaderThread::callCompletedMain(QQmlDataBlob *b)
+void QQmlTypeLoaderThread::callCompletedMain(const QQmlDataBlob::Ptr &b)
 {
 #ifdef DATABLOB_DEBUG
     qWarning("QQmlTypeLoaderThread: %s completed() callback", qPrintable(b->urlString()));
 #endif
     b->completed();
-    b->release();
 }
 
-void QQmlTypeLoaderThread::callDownloadProgressChangedMain(QQmlDataBlob *b, qreal p)
+void QQmlTypeLoaderThread::callDownloadProgressChangedMain(const QQmlDataBlob::Ptr &b, qreal p)
 {
 #ifdef DATABLOB_DEBUG
     qWarning("QQmlTypeLoaderThread: %s downloadProgressChanged(%f) callback",
              qPrintable(b->urlString()), p);
 #endif
     b->downloadProgressChanged(p);
-    b->release();
 }
 
 void QQmlTypeLoaderThread::initializeExtensionMain(QQmlExtensionInterface *iface,
diff --git a/src/qml/qml/qqmltypeloaderthread_p.h b/src/qml/qml/qqmltypeloaderthread_p.h
index c155b59165..4f65fc0cbd 100644
--- a/src/qml/qml/qqmltypeloaderthread_p.h
+++ b/src/qml/qml/qqmltypeloaderthread_p.h
@@ -17,6 +17,7 @@
 
 #include <private/qqmlthread_p.h>
 #include <private/qv4compileddata_p.h>
+#include <private/qqmldatablob_p.h>
 
 #include <QtQml/qtqmlglobal.h>
 
@@ -27,7 +28,6 @@
 
 QT_BEGIN_NAMESPACE
 
-class QQmlDataBlob;
 class QQmlTypeLoader;
 class QQmlEngineExtensionInterface;
 class QQmlExtensionInterface;
@@ -46,23 +46,23 @@ public:
     QNetworkAccessManager *networkAccessManager() const;
     QQmlTypeLoaderNetworkReplyProxy *networkReplyProxy() const;
 #endif // qml_network
-    void load(QQmlDataBlob *b);
-    void loadAsync(QQmlDataBlob *b);
-    void loadWithStaticData(QQmlDataBlob *b, const QByteArray &);
-    void loadWithStaticDataAsync(QQmlDataBlob *b, const QByteArray &);
-    void loadWithCachedUnit(QQmlDataBlob *b, const QQmlPrivate::CachedQmlUnit *unit);
-    void loadWithCachedUnitAsync(QQmlDataBlob *b, const QQmlPrivate::CachedQmlUnit *unit);
-    void callCompleted(QQmlDataBlob *b);
-    void callDownloadProgressChanged(QQmlDataBlob *b, qreal p);
+    void load(const QQmlDataBlob::Ptr &b);
+    void loadAsync(const QQmlDataBlob::Ptr &b);
+    void loadWithStaticData(const QQmlDataBlob::Ptr &b, const QByteArray &);
+    void loadWithStaticDataAsync(const QQmlDataBlob::Ptr &b, const QByteArray &);
+    void loadWithCachedUnit(const QQmlDataBlob::Ptr &b, const QQmlPrivate::CachedQmlUnit *unit);
+    void loadWithCachedUnitAsync(const QQmlDataBlob::Ptr &b, const QQmlPrivate::CachedQmlUnit *unit);
+    void callCompleted(const QQmlDataBlob::Ptr &b);
+    void callDownloadProgressChanged(const QQmlDataBlob::Ptr &b, qreal p);
     void initializeEngine(QQmlExtensionInterface *, const char *);
     void initializeEngine(QQmlEngineExtensionInterface *, const char *);
 
 private:
-    void loadThread(QQmlDataBlob *b);
-    void loadWithStaticDataThread(QQmlDataBlob *b, const QByteArray &);
-    void loadWithCachedUnitThread(QQmlDataBlob *b, const QQmlPrivate::CachedQmlUnit *unit);
-    void callCompletedMain(QQmlDataBlob *b);
-    void callDownloadProgressChangedMain(QQmlDataBlob *b, qreal p);
+    void loadThread(const QQmlDataBlob::Ptr &b);
+    void loadWithStaticDataThread(const QQmlDataBlob::Ptr &b, const QByteArray &);
+    void loadWithCachedUnitThread(const QQmlDataBlob::Ptr &b, const QQmlPrivate::CachedQmlUnit *unit);
+    void callCompletedMain(const QQmlDataBlob::Ptr &b);
+    void callDownloadProgressChangedMain(const QQmlDataBlob::Ptr &b, qreal p);
     void initializeExtensionMain(QQmlExtensionInterface *iface, const char *uri);
     void initializeEngineExtensionMain(QQmlEngineExtensionInterface *iface, const char *uri);