Improve QLibrary global destruction

The previous solution was a global static, which got deleted at the end
of the execution of the application or at QtCore unload, whichever came
first. Unfortunately, the end of the execution often came first, which
is inconvenient: it means the global was deleted before all atexit
functions were run, including some QLibrary destructors.

Consequently, some QLibrary destructors did not reach the global data
and were thus unable to unload their libraries or delete their data
properly. The previous solution leaked.

This solution instead uses a Q_DESTRUCTOR_FUNCTION, which makes a
requirement to destroy only at QtCore unload time. Thus, we're sure that
all references have been dropped.

Additionally, during the cleanup, do try to unload the libraries that
have a single reference count left. That means either a QLibrary that
was destroyed without unload(), or a use of the static QLibrary::resolve
functions.

Change-Id: I12e0943b0c6edc27390c103b368d1b04bfe7e302
Reviewed-by: Lars Knoll <lars.knoll@digia.com>

2 files changed, 111 insertions, 39 deletions

diff --git a/src/corelib/plugin/qlibrary.cpp b/src/corelib/plugin/qlibrary.cpp
index d17067684a..7d6c3c3063 100644
--- a/src/corelib/plugin/qlibrary.cpp
+++ b/src/corelib/plugin/qlibrary.cpp
@@ -79,8 +79,6 @@ QT_BEGIN_NAMESPACE
 #  define QT_NO_DEBUG_PLUGIN_CHECK
 #endif
 
-static QBasicMutex qt_library_mutex;
-
 /*!
     \class QLibrary
     \inmodule QtCore
@@ -339,46 +337,126 @@ static void installCoverageTool(QLibraryPrivate *libPrivate)
 #endif
 }
 
-typedef QMap<QString, QLibraryPrivate*> LibraryMap;
+class QLibraryStore
+{
+public:
+    inline ~QLibraryStore();
+    static inline QLibraryPrivate *findOrCreate(const QString &fileName, const QString &version);
+    static inline void releaseLibrary(QLibraryPrivate *lib);
+
+    static inline void cleanup();
 
-struct LibraryData {
+private:
+    static inline QLibraryStore *instance();
+
+    // all members and instance() are protected by qt_library_mutex
+    typedef QMap<QString, QLibraryPrivate*> LibraryMap;
     LibraryMap libraryMap;
-    QSet<QLibraryPrivate*> loadedLibs;
 };
 
-Q_GLOBAL_STATIC(LibraryData, libraryData)
+static QBasicMutex qt_library_mutex;
+static QLibraryStore *qt_library_data = 0;
+
+QLibraryStore::~QLibraryStore()
+{
+    qt_library_data = 0;
+}
+
+inline void QLibraryStore::cleanup()
+{
+    QLibraryStore *data = qt_library_data;
+    if (!data)
+        return;
+
+    // find any libraries that are still loaded but have a no one attached to them
+    LibraryMap::Iterator it = data->libraryMap.begin();
+    for (; it != data->libraryMap.end(); ++it) {
+        QLibraryPrivate *lib = it.value();
+        if (lib->libraryRefCount.load() == 1) {
+            Q_ASSERT(lib->pHnd);
+            Q_ASSERT(lib->libraryUnloadCount.load() > 0);
+            lib->libraryUnloadCount.store(1);
+            lib->unload();
+            delete lib;
+            it.value() = 0;
+        }
+    }
 
-static LibraryMap *libraryMap()
+    if (qt_debug_component()) {
+        // dump all objects that remain
+        foreach (QLibraryPrivate *lib, data->libraryMap) {
+            if (lib)
+                qDebug() << "On QtCore unload," << lib->fileName << "was leaked, with"
+                         << lib->libraryRefCount.load() << "users";
+        }
+    }
+
+    delete data;
+}
+
+static void qlibraryCleanup()
+{
+    QLibraryStore::cleanup();
+}
+Q_DESTRUCTOR_FUNCTION(qlibraryCleanup)
+
+// must be called with a locked mutex
+QLibraryStore *QLibraryStore::instance()
 {
-    LibraryData *data = libraryData();
-    return data ? &data->libraryMap : 0;
+    if (Q_UNLIKELY(!qt_library_data))
+        qt_library_data = new QLibraryStore;
+    return qt_library_data;
+}
+
+inline QLibraryPrivate *QLibraryStore::findOrCreate(const QString &fileName, const QString &version)
+{
+    QMutexLocker locker(&qt_library_mutex);
+    QLibraryStore *data = instance();
+
+    // check if this library is already loaded
+    QLibraryPrivate *lib = data->libraryMap.value(fileName);
+    if (!lib)
+        lib = new QLibraryPrivate(fileName, version);
+
+    // track this library
+    data->libraryMap.insert(fileName, lib);
+
+    lib->libraryRefCount.ref();
+    return lib;
+}
+
+inline void QLibraryStore::releaseLibrary(QLibraryPrivate *lib)
+{
+    QMutexLocker locker(&qt_library_mutex);
+    QLibraryStore *data = instance();
+
+    if (lib->libraryRefCount.deref()) {
+        // still in use
+        return;
+    }
+
+    // no one else is using
+    Q_ASSERT(lib->libraryUnloadCount.load() == 0);
+
+    QLibraryPrivate *that = data->libraryMap.take(lib->fileName);
+    Q_ASSERT(lib == that);
+    Q_UNUSED(that);
+    delete lib;
 }
 
 QLibraryPrivate::QLibraryPrivate(const QString &canonicalFileName, const QString &version)
     : pHnd(0), fileName(canonicalFileName), fullVersion(version), instance(0),
       loadHints(0),
-      libraryRefCount(1), libraryUnloadCount(0), pluginState(MightBeAPlugin)
-{ libraryMap()->insert(canonicalFileName, this); }
+      libraryRefCount(0), libraryUnloadCount(0), pluginState(MightBeAPlugin)
+{ }
 
 QLibraryPrivate *QLibraryPrivate::findOrCreate(const QString &fileName, const QString &version)
 {
-    QMutexLocker locker(&qt_library_mutex);
-    if (QLibraryPrivate *lib = libraryMap()->value(fileName)) {
-        lib->libraryRefCount.ref();
-        return lib;
-    }
-
-    return new QLibraryPrivate(fileName, version);
+    return QLibraryStore::findOrCreate(fileName, version);
 }
 
 QLibraryPrivate::~QLibraryPrivate()
 {
-    LibraryMap * const map = libraryMap();
-    if (map) {
-        QLibraryPrivate *that = map->take(fileName);
-        Q_ASSERT(this == that);
-	Q_UNUSED(that);
-    }
 }
 
 QFunctionPointer QLibraryPrivate::resolve(const char *symbol)
@@ -391,9 +469,10 @@ QFunctionPointer QLibraryPrivate::resolve(const char *symbol)
 
 bool QLibraryPrivate::load()
 {
-    libraryUnloadCount.ref();
-    if (pHnd)
+    if (pHnd) {
+        libraryUnloadCount.ref();
         return true;
+    }
     if (fileName.isEmpty())
         return false;
 
@@ -403,11 +482,8 @@ bool QLibraryPrivate::load()
     if (ret) {
         //when loading a library we add a reference to it so that the QLibraryPrivate won't get deleted
         //this allows to unload the library at a later time
-        if (LibraryData *lib = libraryData()) {
-            lib->loadedLibs += this;
-            libraryRefCount.ref();
-        }
-
+        libraryUnloadCount.ref();
+        libraryRefCount.ref();
         installCoverageTool(this);
     }
 
@@ -425,10 +501,7 @@ bool QLibraryPrivate::unload()
                 qWarning() << "QLibraryPrivate::unload succeeded on" << fileName;
             //when the library is unloaded, we release the reference on it so that 'this'
             //can get deleted
-            if (LibraryData *lib = libraryData()) {
-                if (lib->loadedLibs.remove(this))
-                    libraryRefCount.deref();
-            }
+            libraryRefCount.deref();
             pHnd = 0;
             instance = 0;
         }
@@ -439,9 +512,7 @@ bool QLibraryPrivate::unload()
 
 void QLibraryPrivate::release()
 {
-    QMutexLocker locker(&qt_library_mutex);
-    if (!libraryRefCount.deref())
-        delete this;
+    QLibraryStore::releaseLibrary(this);
 }
 
 bool QLibraryPrivate::loadPlugin()
diff --git a/src/corelib/plugin/qlibrary_p.h b/src/corelib/plugin/qlibrary_p.h
index 28bec300eb..42172167b7 100644
--- a/src/corelib/plugin/qlibrary_p.h
+++ b/src/corelib/plugin/qlibrary_p.h
@@ -71,6 +71,7 @@ QT_BEGIN_NAMESPACE
 
 bool qt_debug_component();
 
+class QLibraryStore;
 class QLibraryPrivate
 {
 public:
@@ -128,7 +129,7 @@ private:
     QAtomicInt libraryUnloadCount;
 
     enum { IsAPlugin, IsNotAPlugin, MightBeAPlugin } pluginState;
-    friend class QLibraryPrivateHasFriends;
+    friend class QLibraryStore;
 };
 
 QT_END_NAMESPACE