Tighten the interface of QQmlTypeModule

No other classes really need direct access to QQmlTypeModulePrivate.

Adding or removing types, as well as lookup of types needs to be
protected by a mutex, as that can happen from multiple threads. However,
we don't need to acquire the global type registration mutex in order to
change the internals of some QQmlTypeModule. Rather, each type module
gets its own mutex.

The minimum and maximum versions as well as the "locked" property can be
handled with atomic integers as they only ever move in one direction.

The module and majorVersion properties are constant over the life time
of the object. Therefore they don't need any locking.

Task-number: QTBUG-73271
Change-Id: I23fe7dcaf521ccecaaaf19c1fb0436e109c42f03
Reviewed-by: Lars Knoll <lars.knoll@qt.io>

4 files changed, 123 insertions, 108 deletions

diff --git a/src/qml/qml/qqmlmetatype.cpp b/src/qml/qml/qqmlmetatype.cpp
index 34412a5fcb..8c3717844c 100644
--- a/src/qml/qml/qqmlmetatype.cpp
+++ b/src/qml/qml/qqmlmetatype.cpp
@@ -253,7 +253,7 @@ bool checkRegistration(QQmlType::RegistrationType typeType, QQmlMetaTypeData *da
             versionedUri.uri = nameSpace;
             versionedUri.majorVersion = majorVersion;
             if (QQmlTypeModule* qqtm = data->uriToModule.value(versionedUri, 0)){
-                if (QQmlTypeModulePrivate::get(qqtm)->locked){
+                if (qqtm->isLocked()){
                     QString failure(QCoreApplication::translate("qmlRegisterType",
                                                                 "Cannot install %1 '%2' into protected module '%3' version '%4'"));
                     data->recordTypeRegFailure(failure.arg(registrationTypeString(typeType)).arg(typeName).arg(nameSpace).arg(majorVersion));
@@ -272,8 +272,7 @@ QQmlTypeModule *getTypeModule(const QHashedString &uri, int majorVersion, QQmlMe
     QQmlMetaTypeData::VersionedUri versionedUri(uri, majorVersion);
     QQmlTypeModule *module = data->uriToModule.value(versionedUri);
     if (!module) {
-        module = new QQmlTypeModule;
-        module->d->uri = versionedUri;
+        module = new QQmlTypeModule(versionedUri.uri, versionedUri.majorVersion);
         data->uriToModule.insert(versionedUri, module);
     }
     return module;
@@ -309,7 +308,7 @@ void addTypeToData(QQmlTypePrivate *type, QQmlMetaTypeData *data)
 
         QQmlTypeModule *module = getTypeModule(mod, type->version_maj, data);
         Q_ASSERT(module);
-        module->d->add(type);
+        module->add(type);
     }
 }
 
@@ -447,7 +446,7 @@ bool QQmlMetaType::protectModule(const char *uri, int majVersion)
     versionedUri.majorVersion = majVersion;
 
     if (QQmlTypeModule* qqtm = data->uriToModule.value(versionedUri, 0)) {
-        QQmlTypeModulePrivate::get(qqtm)->locked = true;
+        qqtm->lock();
         return true;
     }
     return false;
@@ -460,9 +459,7 @@ void QQmlMetaType::registerModule(const char *uri, int versionMajor, int version
     QQmlTypeModule *module = getTypeModule(QString::fromUtf8(uri), versionMajor, data);
     Q_ASSERT(module);
 
-    QQmlTypeModulePrivate *p = QQmlTypeModulePrivate::get(module);
-    p->minMinorVersion = qMin(p->minMinorVersion, versionMinor);
-    p->maxMinorVersion = qMax(p->maxMinorVersion, versionMinor);
+    module->addMinorVersion(versionMinor);
 }
 
 int QQmlMetaType::typeId(const char *uri, int versionMajor, int versionMinor, const char *qmlName)
@@ -543,7 +540,7 @@ bool QQmlMetaType::isLockedModule(const QString &uri, int majVersion)
     versionedUri.uri = uri;
     versionedUri.majorVersion = majVersion;
     if (QQmlTypeModule* qqtm = data->uriToModule.value(versionedUri, 0))
-        return QQmlTypeModulePrivate::get(qqtm)->locked;
+        return qqtm->isLocked();
     return false;
 }
 
@@ -900,20 +897,15 @@ QQmlPropertyCache *QQmlMetaType::propertyCache(const QQmlType &type, int minorVe
 void QQmlMetaType::unregisterType(int typeIndex)
 {
     QQmlMetaTypeDataPtr data;
-    {
-        const QQmlTypePrivate *d = data->types.value(typeIndex).priv();
-        if (d) {
-            removeQQmlTypePrivate(data->idToType, d);
-            removeQQmlTypePrivate(data->nameToType, d);
-            removeQQmlTypePrivate(data->urlToType, d);
-            removeQQmlTypePrivate(data->urlToNonFileImportType, d);
-            removeQQmlTypePrivate(data->metaObjectToType, d);
-            for (QQmlMetaTypeData::TypeModules::Iterator module = data->uriToModule.begin(); module != data->uriToModule.end(); ++module) {
-                 QQmlTypeModulePrivate *modulePrivate = (*module)->priv();
-                 modulePrivate->remove(d);
-            }
-            data->types[typeIndex] = QQmlType();
-        }
+    if (const QQmlTypePrivate *d = data->types.value(typeIndex).priv()) {
+        removeQQmlTypePrivate(data->idToType, d);
+        removeQQmlTypePrivate(data->nameToType, d);
+        removeQQmlTypePrivate(data->urlToType, d);
+        removeQQmlTypePrivate(data->urlToNonFileImportType, d);
+        removeQQmlTypePrivate(data->metaObjectToType, d);
+        for (auto & module : data->uriToModule)
+            module->remove(d);
+        data->types[typeIndex] = QQmlType();
     }
 }
 
@@ -921,54 +913,48 @@ void QQmlMetaType::freeUnusedTypesAndCaches()
 {
     QQmlMetaTypeDataPtr data;
 
-    {
-        bool deletedAtLeastOneType;
-        do {
-            deletedAtLeastOneType = false;
-            QList<QQmlType>::Iterator it = data->types.begin();
-            while (it != data->types.end()) {
-                const QQmlTypePrivate *d = (*it).priv();
-                if (d && d->refCount == 1) {
-                    deletedAtLeastOneType = true;
-
-                    removeQQmlTypePrivate(data->idToType, d);
-                    removeQQmlTypePrivate(data->nameToType, d);
-                    removeQQmlTypePrivate(data->urlToType, d);
-                    removeQQmlTypePrivate(data->urlToNonFileImportType, d);
-                    removeQQmlTypePrivate(data->metaObjectToType, d);
-
-                    for (QQmlMetaTypeData::TypeModules::Iterator module = data->uriToModule.begin(); module != data->uriToModule.end(); ++module) {
-                        QQmlTypeModulePrivate *modulePrivate = (*module)->priv();
-                        modulePrivate->remove(d);
-                    }
-
-                    *it = QQmlType();
-                } else {
-                    ++it;
-                }
+    bool deletedAtLeastOneType;
+    do {
+        deletedAtLeastOneType = false;
+        QList<QQmlType>::Iterator it = data->types.begin();
+        while (it != data->types.end()) {
+            const QQmlTypePrivate *d = (*it).priv();
+            if (d && d->refCount == 1) {
+                deletedAtLeastOneType = true;
+
+                removeQQmlTypePrivate(data->idToType, d);
+                removeQQmlTypePrivate(data->nameToType, d);
+                removeQQmlTypePrivate(data->urlToType, d);
+                removeQQmlTypePrivate(data->urlToNonFileImportType, d);
+                removeQQmlTypePrivate(data->metaObjectToType, d);
+
+                for (auto &module : data->uriToModule)
+                    module->remove(d);
+
+                *it = QQmlType();
+            } else {
+                ++it;
             }
-        } while (deletedAtLeastOneType);
-    }
-
-    {
-        bool deletedAtLeastOneCache;
-        do {
-            deletedAtLeastOneCache = false;
-            QHash<const QMetaObject *, QQmlPropertyCache *>::Iterator it = data->propertyCaches.begin();
-            while (it != data->propertyCaches.end()) {
-
-                if ((*it)->count() == 1) {
-                    QQmlPropertyCache *pc = nullptr;
-                    qSwap(pc, *it);
-                    it = data->propertyCaches.erase(it);
-                    pc->release();
-                    deletedAtLeastOneCache = true;
-                } else {
-                    ++it;
-                }
+        }
+    } while (deletedAtLeastOneType);
+
+    bool deletedAtLeastOneCache;
+    do {
+        deletedAtLeastOneCache = false;
+        QHash<const QMetaObject *, QQmlPropertyCache *>::Iterator it = data->propertyCaches.begin();
+        while (it != data->propertyCaches.end()) {
+
+            if ((*it)->count() == 1) {
+                QQmlPropertyCache *pc = nullptr;
+                qSwap(pc, *it);
+                it = data->propertyCaches.erase(it);
+                pc->release();
+                deletedAtLeastOneCache = true;
+            } else {
+                ++it;
             }
-        } while (deletedAtLeastOneCache);
-    }
+        }
+    } while (deletedAtLeastOneCache);
 }
 
 /*!
diff --git a/src/qml/qml/qqmltypemodule.cpp b/src/qml/qml/qqmltypemodule.cpp
index 05264abb1d..4d7553fbab 100644
--- a/src/qml/qml/qqmltypemodule.cpp
+++ b/src/qml/qml/qqmltypemodule.cpp
@@ -45,46 +45,60 @@
 
 QT_BEGIN_NAMESPACE
 
-QQmlTypeModule::QQmlTypeModule()
-    : d(new QQmlTypeModulePrivate)
+QQmlTypeModule::QQmlTypeModule(const QString &module, int majorVersion)
+    : d(new QQmlTypeModulePrivate(module, majorVersion))
 {
 }
 
 QQmlTypeModule::~QQmlTypeModule()
 {
-    delete d; d = nullptr;
+    delete d;
 }
 
 QString QQmlTypeModule::module() const
 {
-    return d->uri.uri;
+    // No need to lock. d->module is const
+    return d->module;
 }
 
 int QQmlTypeModule::majorVersion() const
 {
-    return d->uri.majorVersion;
+    // No need to lock. d->majorVersion is const
+    return d->majorVersion;
 }
 
 int QQmlTypeModule::minimumMinorVersion() const
 {
-    return d->minMinorVersion;
+    return d->minMinorVersion.load();
 }
 
 int QQmlTypeModule::maximumMinorVersion() const
 {
-    return d->maxMinorVersion;
+    return d->maxMinorVersion.load();
 }
 
-void QQmlTypeModulePrivate::add(QQmlTypePrivate *type)
+void QQmlTypeModule::addMinorVersion(int version)
 {
-    int minVersion = type->version_min;
-    minMinorVersion = qMin(minMinorVersion, minVersion);
-    maxMinorVersion = qMax(maxMinorVersion, minVersion);
+    for (int oldVersion = d->minMinorVersion.load();
+         oldVersion > version && !d->minMinorVersion.testAndSetOrdered(oldVersion, version);
+         oldVersion = d->minMinorVersion.load()) {
+    }
+
+    for (int oldVersion = d->maxMinorVersion.load();
+         oldVersion < version && !d->maxMinorVersion.testAndSetOrdered(oldVersion, version);
+         oldVersion = d->maxMinorVersion.load()) {
+    }
+}
 
-    QList<QQmlTypePrivate *> &list = typeHash[type->elementName];
+void QQmlTypeModule::add(QQmlTypePrivate *type)
+{
+    QMutexLocker lock(&d->mutex);
+    addMinorVersion(type->version_min);
+
+    QList<QQmlTypePrivate *> &list = d->typeHash[type->elementName];
     for (int ii = 0; ii < list.count(); ++ii) {
         Q_ASSERT(list.at(ii));
-        if (list.at(ii)->version_min < minVersion) {
+        if (list.at(ii)->version_min < type->version_min) {
             list.insert(ii, type);
             return;
         }
@@ -92,9 +106,10 @@ void QQmlTypeModulePrivate::add(QQmlTypePrivate *type)
     list.append(type);
 }
 
-void QQmlTypeModulePrivate::remove(const QQmlTypePrivate *type)
+void QQmlTypeModule::remove(const QQmlTypePrivate *type)
 {
-    for (TypeHash::MutableIterator elementIt = typeHash.begin(); elementIt != typeHash.end();) {
+    QMutexLocker lock(&d->mutex);
+    for (auto elementIt = d->typeHash.begin(); elementIt != d->typeHash.end();) {
         QQmlMetaType::removeQQmlTypePrivate(elementIt.value(), type);
 
 #if 0
@@ -108,10 +123,19 @@ void QQmlTypeModulePrivate::remove(const QQmlTypePrivate *type)
     }
 }
 
-QQmlType QQmlTypeModule::type(const QHashedStringRef &name, int minor) const
+bool QQmlTypeModule::isLocked() const
 {
-    QMutexLocker lock(QQmlMetaType::typeRegistrationLock());
+    return d->locked.load() != 0;
+}
+
+void QQmlTypeModule::lock()
+{
+    d->locked.store(1);
+}
 
+QQmlType QQmlTypeModule::type(const QHashedStringRef &name, int minor) const
+{
+    QMutexLocker lock(&d->mutex);
     QList<QQmlTypePrivate *> *types = d->typeHash.value(name);
     if (types) {
         for (int ii = 0; ii < types->count(); ++ii)
@@ -124,8 +148,7 @@ QQmlType QQmlTypeModule::type(const QHashedStringRef &name, int minor) const
 
 QQmlType QQmlTypeModule::type(const QV4::String *name, int minor) const
 {
-    QMutexLocker lock(QQmlMetaType::typeRegistrationLock());
-
+    QMutexLocker lock(&d->mutex);
     QList<QQmlTypePrivate *> *types = d->typeHash.value(name);
     if (types) {
         for (int ii = 0; ii < types->count(); ++ii)
@@ -138,7 +161,7 @@ QQmlType QQmlTypeModule::type(const QV4::String *name, int minor) const
 
 void QQmlTypeModule::walkCompositeSingletons(const std::function<void(const QQmlType &)> &callback) const
 {
-    QMutexLocker lock(QQmlMetaType::typeRegistrationLock());
+    QMutexLocker lock(&d->mutex);
     for (auto typeCandidates = d->typeHash.begin(), end = d->typeHash.end();
          typeCandidates != end; ++typeCandidates) {
         for (auto type: typeCandidates.value()) {
diff --git a/src/qml/qml/qqmltypemodule_p.h b/src/qml/qml/qqmltypemodule_p.h
index 6107988a70..b84a91b5db 100644
--- a/src/qml/qml/qqmltypemodule_p.h
+++ b/src/qml/qml/qqmltypemodule_p.h
@@ -72,9 +72,19 @@ class QQmlTypeModulePrivate;
 class QQmlTypeModule
 {
 public:
+    QQmlTypeModule(const QString &uri = QString(), int majorVersion = 0);
+    ~QQmlTypeModule();
+
+    void add(QQmlTypePrivate *);
+    void remove(const QQmlTypePrivate *type);
+
+    bool isLocked() const;
+    void lock();
+
     QString module() const;
     int majorVersion() const;
 
+    void addMinorVersion(int minorVersion);
     int minimumMinorVersion() const;
     int maximumMinorVersion() const;
 
@@ -83,17 +93,7 @@ public:
 
     void walkCompositeSingletons(const std::function<void(const QQmlType &)> &callback) const;
 
-    QQmlTypeModulePrivate *priv() { return d; }
 private:
-    //Used by register functions and creates the QQmlTypeModule for them
-    friend QQmlTypeModule *getTypeModule(const QHashedString &uri, int majorVersion, QQmlMetaTypeData *data);
-    friend void addTypeToData(QQmlTypePrivate *type, QQmlMetaTypeData *data);
-    friend struct QQmlMetaTypeData;
-    friend class QQmlMetaType;
-    friend class QQmlTypeModulePrivate;
-
-    QQmlTypeModule();
-    ~QQmlTypeModule();
     QQmlTypeModulePrivate *d;
 };
 
diff --git a/src/qml/qml/qqmltypemodule_p_p.h b/src/qml/qml/qqmltypemodule_p_p.h
index c045d3ed9b..b1dab1c4a0 100644
--- a/src/qml/qml/qqmltypemodule_p_p.h
+++ b/src/qml/qml/qqmltypemodule_p_p.h
@@ -55,27 +55,33 @@
 #include <private/qstringhash_p.h>
 #include <private/qqmlmetatypedata_p.h>
 
+#include <QtCore/qmutex.h>
+
 QT_BEGIN_NAMESPACE
 
 class QQmlTypeModulePrivate
 {
 public:
-    QQmlTypeModulePrivate()
-        : minMinorVersion(INT_MAX), maxMinorVersion(0), locked(false) {}
+    QQmlTypeModulePrivate(QString module, int majorVersion) :
+        module(std::move(module)), majorVersion(majorVersion)
+    {}
 
-    static QQmlTypeModulePrivate* get(QQmlTypeModule* q) { return q->d; }
+    const QString module;
+    const int majorVersion = 0;
 
-    QQmlMetaTypeData::VersionedUri uri;
+    // Can only ever decrease
+    QAtomicInt minMinorVersion = std::numeric_limits<int>::max();
 
-    int minMinorVersion;
-    int maxMinorVersion;
-    bool locked;
+    // Can only ever increase
+    QAtomicInt maxMinorVersion = 0;
 
-    void add(QQmlTypePrivate *);
-    void remove(const QQmlTypePrivate *type);
+    // Bool. Can only be set to 1 once.
+    QAtomicInt locked = 0;
 
     typedef QStringHash<QList<QQmlTypePrivate *> > TypeHash;
     TypeHash typeHash;
+
+    QMutex mutex;
 };
 
 QT_END_NAMESPACE