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 <eike.ziller@qt.io>
This commit is contained in:
Jarek Kobus
2021-01-27 14:58:39 +01:00
parent cbac6e6d2f
commit 44b090925c

View File

@@ -98,6 +98,7 @@ void DirectoryFilter::restoreState(const QByteArray &state)
setShortcutString(shortcut); setShortcutString(shortcut);
setIncludedByDefault(defaultFilter); setIncludedByDefault(defaultFilter);
locker.unlock();
updateFileIterator(); updateFileIterator();
} }
@@ -138,6 +139,8 @@ bool DirectoryFilter::openConfigDialog(QWidget *parent, bool &needsRefresh)
&DirectoryFilter::updateOptionButtons, &DirectoryFilter::updateOptionButtons,
Qt::DirectConnection); Qt::DirectConnection);
m_ui->directoryList->clear(); 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->directoryList->addItems(m_directories);
m_ui->nameLabel->setVisible(m_isCustomFilter); m_ui->nameLabel->setVisible(m_isCustomFilter);
m_ui->nameEdit->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->setText(Utils::msgFilePatternLabel());
m_ui->filePatternLabel->setBuddy(m_ui->filePattern); m_ui->filePatternLabel->setBuddy(m_ui->filePattern);
m_ui->filePattern->setToolTip(Utils::msgFilePatternToolTip()); 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->filePattern->setText(Utils::transform(m_filters, &QDir::toNativeSeparators).join(','));
m_ui->exclusionPatternLabel->setText(Utils::msgExclusionPatternLabel()); m_ui->exclusionPatternLabel->setText(Utils::msgExclusionPatternLabel());
m_ui->exclusionPatternLabel->setBuddy(m_ui->exclusionPattern); m_ui->exclusionPatternLabel->setBuddy(m_ui->exclusionPattern);
m_ui->exclusionPattern->setToolTip(Utils::msgFilePatternToolTip()); 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( m_ui->exclusionPattern->setText(
Utils::transform(m_exclusionFilters, &QDir::toNativeSeparators).join(',')); Utils::transform(m_exclusionFilters, &QDir::toNativeSeparators).join(','));
m_ui->shortcutEdit->setText(shortcutString()); m_ui->shortcutEdit->setText(shortcutString());
@@ -222,12 +229,13 @@ void DirectoryFilter::updateOptionButtons()
void DirectoryFilter::updateFileIterator() void DirectoryFilter::updateFileIterator()
{ {
QMutexLocker locker(&m_lock);
setFileIterator(new BaseFileFilter::ListIterator(m_files)); setFileIterator(new BaseFileFilter::ListIterator(m_files));
} }
void DirectoryFilter::refresh(QFutureInterface<void> &future) void DirectoryFilter::refresh(QFutureInterface<void> &future)
{ {
QStringList directories; QStringList directories, filters, exclusionFilters;
{ {
QMutexLocker locker(&m_lock); QMutexLocker locker(&m_lock);
if (m_directories.isEmpty()) { if (m_directories.isEmpty()) {
@@ -238,8 +246,10 @@ void DirectoryFilter::refresh(QFutureInterface<void> &future)
return; return;
} }
directories = m_directories; 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()); future.setProgressRange(0, subDirIterator.maxProgress());
Utils::FilePaths filesFound; Utils::FilePaths filesFound;
auto end = subDirIterator.end(); auto end = subDirIterator.end();
@@ -273,7 +283,10 @@ void DirectoryFilter::setDirectories(const QStringList &directories)
{ {
if (directories == m_directories) if (directories == m_directories)
return; return;
{
QMutexLocker locker(&m_lock);
m_directories = directories; m_directories = directories;
}
Internal::Locator::instance()->refresh({this}); Internal::Locator::instance()->refresh({this});
} }
@@ -296,11 +309,13 @@ QStringList DirectoryFilter::directories() const
void DirectoryFilter::setFilters(const QStringList &filters) void DirectoryFilter::setFilters(const QStringList &filters)
{ {
QMutexLocker locker(&m_lock);
m_filters = filters; m_filters = filters;
} }
void DirectoryFilter::setExclusionFilters(const QStringList &exclusionFilters) void DirectoryFilter::setExclusionFilters(const QStringList &exclusionFilters)
{ {
QMutexLocker locker(&m_lock);
m_exclusionFilters = exclusionFilters; m_exclusionFilters = exclusionFilters;
} }