diff options
author | Thiago Macieira <thiago.macieira@intel.com> | 2019-12-18 18:17:38 -0800 |
---|---|---|
committer | Thiago Macieira <thiago.macieira@intel.com> | 2020-03-03 14:36:28 -0800 |
commit | f6c1cebe42193a62fa0b9c6a881bb1a973b1b8a9 (patch) | |
tree | 73d32c1350b7bd278b4f6c2fdcc7716d5f383263 | |
parent | d9ca61bf0f6e544ad1fe2908e84c4a4966e5d0a1 (diff) | |
download | qtbase-f6c1cebe42193a62fa0b9c6a881bb1a973b1b8a9.tar.gz |
QPluginLoader: rework the loading and the caching of instance
There was a race condition in accessing the cached instance factory
member, so rework loadPlugin() to return the cached or newly discovered
instance, with proper, atomic caching. Because I had to change that, I
took the opportunity to fix the QFactoryLoader code that calls
loadPlugin().
Note that QLibraryPrivate::loadPlugin() returns non-nullptr now if the
instance is known, which means the last return in QPluginLoader::load()
will convert to true, not false, if the instance got cached between the
earlier check and the call to loadPlugin(). That's probably what was
intended.
Task-number: QTBUG-39642
Change-Id: I46bf1f65e8db46afbde5fffd15e1a42d2b6cbf2c
Reviewed-by: David Faure <david.faure@kdab.com>
-rw-r--r-- | src/corelib/plugin/qfactoryloader.cpp | 15 | ||||
-rw-r--r-- | src/corelib/plugin/qlibrary.cpp | 45 | ||||
-rw-r--r-- | src/corelib/plugin/qlibrary_p.h | 6 |
3 files changed, 44 insertions, 22 deletions
diff --git a/src/corelib/plugin/qfactoryloader.cpp b/src/corelib/plugin/qfactoryloader.cpp index 18f10c9b43..9a6a69676d 100644 --- a/src/corelib/plugin/qfactoryloader.cpp +++ b/src/corelib/plugin/qfactoryloader.cpp @@ -389,17 +389,12 @@ QObject *QFactoryLoader::instance(int index) const QMutexLocker lock(&d->mutex); if (index < d->libraryList.size()) { QLibraryPrivate *library = d->libraryList.at(index); - if (library->instance || library->loadPlugin()) { - if (!library->inst) - library->inst = library->instance(); - QObject *obj = library->inst.data(); - if (obj) { - if (!obj->parent()) - obj->moveToThread(QCoreApplicationPrivate::mainThread()); - return obj; - } + if (QObject *obj = library->pluginInstance()) { + if (!obj->parent()) + obj->moveToThread(QCoreApplicationPrivate::mainThread()); + return obj; } - return 0; + return nullptr; } index -= d->libraryList.size(); lock.unlock(); diff --git a/src/corelib/plugin/qlibrary.cpp b/src/corelib/plugin/qlibrary.cpp index eeaa3c18ec..7ce959cc8f 100644 --- a/src/corelib/plugin/qlibrary.cpp +++ b/src/corelib/plugin/qlibrary.cpp @@ -498,7 +498,7 @@ inline void QLibraryStore::releaseLibrary(QLibraryPrivate *lib) } QLibraryPrivate::QLibraryPrivate(const QString &canonicalFileName, const QString &version, QLibrary::LoadHints loadHints) - : pHnd(0), fileName(canonicalFileName), fullVersion(version), instance(0), + : pHnd(0), fileName(canonicalFileName), fullVersion(version), libraryRefCount(0), libraryUnloadCount(0), pluginState(MightBeAPlugin) { loadHintsInt.storeRelaxed(loadHints); @@ -539,6 +539,32 @@ void QLibraryPrivate::setLoadHints(QLibrary::LoadHints lh) mergeLoadHints(lh); } +QObject *QLibraryPrivate::pluginInstance() +{ + // first, check if the instance is cached and hasn't been deleted + QObject *obj = inst.data(); + if (obj) + return obj; + + // We need to call the plugin's factory function. Is that cached? + // skip increasing the reference count (why? -Thiago) + QtPluginInstanceFunction factory = instanceFactory.loadAcquire(); + if (!factory) + factory = loadPlugin(); + + if (!factory) + return nullptr; + + obj = factory(); + + // cache again + if (inst) + obj = inst; + else + inst = obj; + return obj; +} + bool QLibraryPrivate::load() { if (pHnd) { @@ -585,7 +611,7 @@ bool QLibraryPrivate::unload(UnloadFlag flag) //can get deleted libraryRefCount.deref(); pHnd = 0; - instance = 0; + instanceFactory.storeRelaxed(nullptr); } } @@ -597,22 +623,23 @@ void QLibraryPrivate::release() QLibraryStore::releaseLibrary(this); } -bool QLibraryPrivate::loadPlugin() +QtPluginInstanceFunction QLibraryPrivate::loadPlugin() { - if (instance) { + if (auto ptr = instanceFactory.loadAcquire()) { libraryUnloadCount.ref(); - return true; + return ptr; } if (pluginState == IsNotAPlugin) - return false; + return nullptr; if (load()) { - instance = (QtPluginInstanceFunction)resolve("qt_plugin_instance"); - return instance; + auto ptr = reinterpret_cast<QtPluginInstanceFunction>(resolve("qt_plugin_instance")); + instanceFactory.storeRelease(ptr); // two threads may store the same value + return ptr; } if (qt_debug_component()) qWarning() << "QLibraryPrivate::loadPlugin failed on" << fileName << ":" << errorString; pluginState = IsNotAPlugin; - return false; + return nullptr; } /*! diff --git a/src/corelib/plugin/qlibrary_p.h b/src/corelib/plugin/qlibrary_p.h index db5afac98e..29724655a3 100644 --- a/src/corelib/plugin/qlibrary_p.h +++ b/src/corelib/plugin/qlibrary_p.h @@ -86,7 +86,7 @@ public: QString fullVersion; bool load(); - bool loadPlugin(); // loads and resolves instance + QtPluginInstanceFunction loadPlugin(); // loads and resolves instance bool unload(UnloadFlag flag = UnloadSys); void release(); QFunctionPointer resolve(const char *); @@ -100,8 +100,8 @@ public: static QStringList suffixes_sys(const QString &fullVersion); static QStringList prefixes_sys(); - QPointer<QObject> inst; - QtPluginInstanceFunction instance; + QPointer<QObject> inst; // used by QFactoryLoader + QAtomicPointer<std::remove_pointer<QtPluginInstanceFunction>::type> instanceFactory; QJsonObject metaData; QString errorString; |