From 44b090925cc7c1ac1c1f44f23e160e7b20bab93e Mon Sep 17 00:00:00 2001 From: Jarek Kobus Date: Wed, 27 Jan 2021 14:58:39 +0100 Subject: [PATCH] Fix synchronization mechanism in DirectoryFilter Currently m_directories, m_filters and m_exclusionFilters are being modified only in the main thread, while in the other thread they are only read. The access to them is being protected with mutex when reading in the other thread. However, we don't lock the mutex when modifying them in the main thread, so it may happen that the other thread locks the mutex and reads the value while the main thread directly modifies it without a lock. As a remedy, we add lacking lock mutex when we modify it from the main thread. In case of m_files its value is being modified in the other thread, so we add a protection when we use it in the main thread. In order to avoid a deadlock we unlock the mutex by the end of restoreState(), before calling updateFileIterator() which now relocks the mutex. This patch may potentially fix a crash reported in QTCREATORBUG-25265 - the last two stack traces from two different threads, having both a call to Core::DirectoryFilter::refresh(). Task-number: QTCREATORBUG-25265 Change-Id: Ic1adfb71c2325ae06e6c2303d41b758df31d012a Reviewed-by: Eike Ziller --- .../coreplugin/locator/directoryfilter.cpp | 21 ++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/src/plugins/coreplugin/locator/directoryfilter.cpp b/src/plugins/coreplugin/locator/directoryfilter.cpp index 2a41b60d718..831fd443dbd 100644 --- a/src/plugins/coreplugin/locator/directoryfilter.cpp +++ b/src/plugins/coreplugin/locator/directoryfilter.cpp @@ -98,6 +98,7 @@ void DirectoryFilter::restoreState(const QByteArray &state) setShortcutString(shortcut); setIncludedByDefault(defaultFilter); + locker.unlock(); updateFileIterator(); } @@ -138,6 +139,8 @@ bool DirectoryFilter::openConfigDialog(QWidget *parent, bool &needsRefresh) &DirectoryFilter::updateOptionButtons, Qt::DirectConnection); m_ui->directoryList->clear(); + // Note: assuming we only change m_directories in the Gui thread, + // we don't need to protect it here with mutex m_ui->directoryList->addItems(m_directories); m_ui->nameLabel->setVisible(m_isCustomFilter); m_ui->nameEdit->setVisible(m_isCustomFilter); @@ -149,10 +152,14 @@ bool DirectoryFilter::openConfigDialog(QWidget *parent, bool &needsRefresh) m_ui->filePatternLabel->setText(Utils::msgFilePatternLabel()); m_ui->filePatternLabel->setBuddy(m_ui->filePattern); m_ui->filePattern->setToolTip(Utils::msgFilePatternToolTip()); + // Note: assuming we only change m_filters in the Gui thread, + // we don't need to protect it here with mutex m_ui->filePattern->setText(Utils::transform(m_filters, &QDir::toNativeSeparators).join(',')); m_ui->exclusionPatternLabel->setText(Utils::msgExclusionPatternLabel()); m_ui->exclusionPatternLabel->setBuddy(m_ui->exclusionPattern); m_ui->exclusionPattern->setToolTip(Utils::msgFilePatternToolTip()); + // Note: assuming we only change m_exclusionFilters in the Gui thread, + // we don't need to protect it here with mutex m_ui->exclusionPattern->setText( Utils::transform(m_exclusionFilters, &QDir::toNativeSeparators).join(',')); m_ui->shortcutEdit->setText(shortcutString()); @@ -222,12 +229,13 @@ void DirectoryFilter::updateOptionButtons() void DirectoryFilter::updateFileIterator() { + QMutexLocker locker(&m_lock); setFileIterator(new BaseFileFilter::ListIterator(m_files)); } void DirectoryFilter::refresh(QFutureInterface &future) { - QStringList directories; + QStringList directories, filters, exclusionFilters; { QMutexLocker locker(&m_lock); if (m_directories.isEmpty()) { @@ -238,8 +246,10 @@ void DirectoryFilter::refresh(QFutureInterface &future) return; } directories = m_directories; + filters = m_filters; + exclusionFilters = m_exclusionFilters; } - Utils::SubDirFileIterator subDirIterator(directories, m_filters, m_exclusionFilters); + Utils::SubDirFileIterator subDirIterator(directories, filters, exclusionFilters); future.setProgressRange(0, subDirIterator.maxProgress()); Utils::FilePaths filesFound; auto end = subDirIterator.end(); @@ -273,7 +283,10 @@ void DirectoryFilter::setDirectories(const QStringList &directories) { if (directories == m_directories) return; - m_directories = directories; + { + QMutexLocker locker(&m_lock); + m_directories = directories; + } Internal::Locator::instance()->refresh({this}); } @@ -296,11 +309,13 @@ QStringList DirectoryFilter::directories() const void DirectoryFilter::setFilters(const QStringList &filters) { + QMutexLocker locker(&m_lock); m_filters = filters; } void DirectoryFilter::setExclusionFilters(const QStringList &exclusionFilters) { + QMutexLocker locker(&m_lock); m_exclusionFilters = exclusionFilters; }