Replace QHash with std::unordered_map inside ProFileCache

It looks like it happens that while holding a reference
to ProFileCache::parsed_files elements we are modifying
this container by adding / removing items.
In general it's undefined behavior.

In addition some other fixes have been made. Comments
inside the code.

Task-number: QTCREATORBUG-26351
Change-Id: I7f98c813ff6b7bd612757e7a14a5e6835affd21e
Reviewed-by: Eike Ziller <eike.ziller@qt.io>
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: Christian Kandeler <christian.kandeler@qt.io>

src/shared/proparser/qmakeparser.cpp

@@ -49,9 +49,9 @@ ProFileCache::ProFileCache()
 
 ProFileCache::~ProFileCache()
 {
-    foreach (const Entry &ent, parsed_files)
+    for (const auto &keyValuePair : parsed_files)
-        if (ent.pro)
+        if (keyValuePair.second.pro)
-            ent.pro->deref();
+            keyValuePair.second.pro->deref();
     QMakeVfs::deref();
 }
 
@@ -73,19 +73,36 @@ void ProFileCache::discardFile(int id)
     auto it = parsed_files.find(id);
     if (it != parsed_files.end()) {
 #ifdef PROPARSER_THREAD_SAFE
-        if (it->locker) {
+        Entry &entry = it->second;
-            if (!it->locker->done) {
+        Entry::Locker *locker = entry.locker;
-                ++it->locker->waiters;
+        if (locker) { // Either it's still being prepared or it's already done but someone
-                it->locker->cond.wait(&mutex);
+                      // else started waiting for it when it wasn't ready yet
-                if (!--it->locker->waiters) {
+                      // and is still waiting.
-                    delete it->locker;
+            if (!locker->done) {
-                    it->locker = 0;
+                ++locker->waiters;
+                locker->cond.wait(&mutex);          // Mutex is unlocked and relocked,
+                                                    // everything may happen in this time window
+                it = parsed_files.find(id);         // In meantime the iterator could have
+                                                    // been invalidated, search for it again
+                Q_ASSERT(it != parsed_files.end()); // This would mean that some other
+                                                    // waiter removed our entry, but only
+                                                    // the last waiter should do it
+                if (!--locker->waiters) {
+                    delete locker;
+                } else {
+                    // There are still other waiters. Do nothing now.
+                    locker->removeOnLastWaiter = true;
+                    return;
                 }
+            } else if (locker->waiters) {
+                // It's done but some waiters are still awaiting to be woken.
+                locker->removeOnLastWaiter = true; // Mark it for other waiters
+                return;
             }
         }
 #endif
-        if (it->pro)
+        if (entry.pro)
-            it->pro->deref();
+            entry.pro->deref();
         parsed_files.erase(it);
     }
 }
@@ -95,26 +112,53 @@ void ProFileCache::discardFiles(const QString &prefix, QMakeVfs *vfs)
 #ifdef PROPARSER_THREAD_SAFE
     QMutexLocker lck(&mutex);
 #endif
-    auto it = parsed_files.begin(), end = parsed_files.end();
+    auto it = parsed_files.begin();
-    while (it != end) {
+    while (it != parsed_files.end()) {
+        const int id = it->first;
         // Note: this is empty for virtual files from other VFSes.
-        QString fn = vfs->fileNameForId(it.key());
+        const QString fn = vfs->fileNameForId(id);
         if (fn.startsWith(prefix)) {
 #ifdef PROPARSER_THREAD_SAFE
-            if (it->locker) {
+            bool continueFromScratch = false;
-                if (!it->locker->done) {
+            Entry &entry = it->second;
-                    ++it->locker->waiters;
+            Entry::Locker *locker = entry.locker;
-                    it->locker->cond.wait(&mutex);
+            if (locker) { // Either it's still being prepared or it's already done but someone
-                    if (!--it->locker->waiters) {
+                          // else started waiting for it when it wasn't ready yet
-                        delete it->locker;
+                          // and is still waiting.
-                        it->locker = 0;
+                if (!locker->done) { // It's still being prepared.
+                    ++locker->waiters;
+                    locker->cond.wait(&mutex);          // Mutex is unlocked and relocked,
+                                                        // everything may happen in this time window
+                    it = parsed_files.find(id);         // In meantime the iterator could have
+                                                        // been invalidated, search for it again
+                    Q_ASSERT(it != parsed_files.end()); // This would mean that some other
+                                                        // waiter removed our entry, but only
+                                                        // the last waiter should do it
+                    if (!--locker->waiters) {
+                        delete locker;                  // No more waiters, remove locker and entry
+                        continueFromScratch = true;
+                    } else {
+                        // There are still other waiters. Do nothing now
+                        locker->removeOnLastWaiter = true;
+                        it = parsed_files.begin();      // Start from scratch, as new matching
+                                                        // entries could have appeared.
+                                                        // If we hit it again, we will skip it
+                                                        // as the locker is already marked as done
+                        continue;
                     }
+                } else if (locker->waiters) {
+                    // It's done but some waiters are still awaiting to be woken.
+                    locker->removeOnLastWaiter = true; // Mark it for other waiters
+                    ++it; // We skip it for now, other waiter will do the job
+                    continue;
                 }
             }
 #endif
-            if (it->pro)
+            if (entry.pro)
-                it->pro->deref();
+                entry.pro->deref();
             it = parsed_files.erase(it);
+            if (continueFromScratch)
+                it = parsed_files.begin();
         } else {
             ++it;
         }
@@ -187,20 +231,44 @@ ProFile *QMakeParser::parsedProFile(const QString &fileName, ParseFlags flags)
     if ((flags & ParseUseCache) && m_cache) {
         ProFileCache::Entry *ent;
 #ifdef PROPARSER_THREAD_SAFE
-        QMutexLocker locker(&m_cache->mutex);
+        QMutexLocker lck(&m_cache->mutex);
 #endif
         auto it = m_cache->parsed_files.find(id);
         if (it != m_cache->parsed_files.end()) {
-            ent = &*it;
+            ent = &it->second;
 #ifdef PROPARSER_THREAD_SAFE
-            if (ent->locker && !ent->locker->done) {
+            ProFileCache::Entry::Locker *locker = ent->locker;
-                ++ent->locker->waiters;
+            if (locker) { // Either it's still being prepared or it's already done but someone
-                QThreadPool::globalInstance()->releaseThread();
+                          // else started waiting for it when it wasn't ready yet
-                ent->locker->cond.wait(locker.mutex());
+                          // and is still waiting.
-                QThreadPool::globalInstance()->reserveThread();
+                if (!locker->done) { // It's still being prepared.
-                if (!--ent->locker->waiters) {
+                    ++locker->waiters;
-                    delete ent->locker;
+                    QThreadPool::globalInstance()->releaseThread();
-                    ent->locker = 0;
+                    locker->cond.wait(lck.mutex());  // Mutex is unlocked and relocked,
+                                                     // everything may happen in this time window
+                    QThreadPool::globalInstance()->reserveThread();
+
+                    it = m_cache->parsed_files.find(id);         // In meantime the iterator could have
+                                                                 // been invalidated, search for it again
+                    Q_ASSERT(it != m_cache->parsed_files.end()); // This would mean that some other
+                                                                 // waiter removed our entry, but only
+                                                                 // the last waiter should do it
+                    const bool remove = locker->removeOnLastWaiter; // Some waiter marked this
+                                                                    // entry for removal.
+                    if (!--locker->waiters) {                    // If we were the last waiter
+                        delete locker;                           // we do it now
+                        ent->locker = 0;
+                        if (remove) {
+                            if (ent->pro)
+                                ent->pro->deref();
+                            m_cache->parsed_files.erase(it);
+                            return nullptr;
+                        }
+                    }
+                    if (remove)
+                        return nullptr;
+                } else if (locker->removeOnLastWaiter) {
+                    return nullptr;
                 }
             }
 #endif
@@ -210,7 +278,7 @@ ProFile *QMakeParser::parsedProFile(const QString &fileName, ParseFlags flags)
             ent = &m_cache->parsed_files[id];
 #ifdef PROPARSER_THREAD_SAFE
             ent->locker = new ProFileCache::Entry::Locker;
-            locker.unlock();
+            lck.unlock(); // We unlock the mutex and open a time window
 #endif
             QString contents;
             if (readFile(id, flags, &contents)) {
@@ -222,11 +290,17 @@ ProFile *QMakeParser::parsedProFile(const QString &fileName, ParseFlags flags)
             }
             ent->pro = pro;
 #ifdef PROPARSER_THREAD_SAFE
-            locker.relock();
+            lck.relock(); // We relock it back and close the window. Someone could have added a new
-            if (ent->locker->waiters) {
+                          // entry to the parsed_files list (or removed one). However, no one
-                ent->locker->done = true;
+                          // was awoken for our id in this time window.
+            it = m_cache->parsed_files.find(id);         // In meantime the iterator could have
+                                                         // been invalidated, search for it again
+            Q_ASSERT(it != m_cache->parsed_files.end()); // This would mean that our reference
+                                                         // is also invalid
+            if (ent->locker->waiters) {             // Someone runs getter or wants to remove it
+                ent->locker->done = true;           // The structure is ready
                 ent->locker->cond.wakeAll();
-            } else {
+            } else {          // No one was interested, remove locker as everything is ready now
                 delete ent->locker;
                 ent->locker = 0;
             }

src/shared/proparser/qmakeparser.h

@@ -29,13 +29,14 @@
 #include "qmakevfs.h"
 #include "proitems.h"
 
-#include <qhash.h>
 #include <qstack.h>
 #ifdef PROPARSER_THREAD_SAFE
 # include <qmutex.h>
 # include <qwaitcondition.h>
 #endif
 
+#include <unordered_map>
+
 QT_BEGIN_NAMESPACE
 class QMAKE_EXPORT QMakeParserHandler
 {
@@ -208,16 +209,16 @@ private:
         ProFile *pro;
 #ifdef PROPARSER_THREAD_SAFE
         struct Locker {
-            Locker() : waiters(0), done(false) {}
             QWaitCondition cond;
-            int waiters;
+            int waiters = 0;
-            bool done;
+            bool done = false;
+            bool removeOnLastWaiter = false;
         };
         Locker *locker;
 #endif
     };
 
-    QHash<int, Entry> parsed_files;
+    std::unordered_map<int, Entry> parsed_files;
 #ifdef PROPARSER_THREAD_SAFE
     QMutex mutex;
 #endif