Commit 4f7d35d1 authored by Martin Tobias Holmedahl Sandsmark's avatar Martin Tobias Holmedahl Sandsmark
Browse files

Fix leaking ThumbnailGenerators

The logic for deleting ThumbnailGenerators was kind of broken, so fix
that.

Not sure if this is the right approach, but it doesn't seem racy (and
can't trigger any races), and doesn't leak anymore.

Test Plan: Run with asan and ubsan, open folders with many thumbnails,
scroll around.

Reviewed By: ngraham

Differential Revision: https://phabricator.kde.org/D28346
parent 2f48c626
......@@ -36,6 +36,7 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Cambridge, MA 02110-1301, USA
#include <QImageReader>
#include <QTransform>
#include <QBuffer>
#include <QCoreApplication>
namespace Gwenview
{
......@@ -178,7 +179,12 @@ bool ThumbnailContext::load(const QString &pixPath, int pixelSize)
//------------------------------------------------------------------------
ThumbnailGenerator::ThumbnailGenerator()
: mCancel(false)
{}
{
connect(qApp, &QCoreApplication::aboutToQuit, this, [=]() {
wait();
}, Qt::DirectConnection);
start();
}
void ThumbnailGenerator::load(
const QString& originalUri, time_t originalTime, KIO::filesize_t originalFileSize, const QString& originalMimeType,
......@@ -196,7 +202,6 @@ void ThumbnailGenerator::load(
mPixPath = pixPath;
mThumbnailPath = thumbnailPath;
mThumbnailGroup = group;
if (!isRunning()) start();
mCond.wakeOne();
}
......@@ -205,6 +210,12 @@ QString ThumbnailGenerator::originalUri() const
return mOriginalUri;
}
bool ThumbnailGenerator::isStopped()
{
QMutexLocker lock(&mMutex);
return mStopped;
}
time_t ThumbnailGenerator::originalTime() const
{
return mOriginalTime;
......@@ -235,21 +246,18 @@ void ThumbnailGenerator::cancel()
void ThumbnailGenerator::run()
{
LOG("");
while (!testCancel()) {
QString pixPath;
int pixelSize;
{
QMutexLocker lock(&mMutex);
// empty mPixPath means nothing to do
LOG("Waiting for mPixPath");
if (mPixPath.isNull()) {
LOG("mPixPath.isNull");
mCond.wait(&mMutex);
}
}
if (testCancel()) {
return;
break;
}
{
QMutexLocker lock(&mMutex);
......@@ -279,7 +287,7 @@ void ThumbnailGenerator::run()
mPixPath.clear(); // done, ready for next
}
if (testCancel()) {
return;
break;
}
{
QSize size(mOriginalWidth, mOriginalHeight);
......@@ -289,7 +297,12 @@ void ThumbnailGenerator::run()
LOG("Done");
}
}
LOG("Ending thread");
QMutexLocker lock(&mMutex);
mStopped = true;
deleteLater();
}
void ThumbnailGenerator::cacheThumbnail()
......
......@@ -51,6 +51,10 @@ class ThumbnailGenerator : public QThread
public:
ThumbnailGenerator();
// Because we override run(), like you're not really supposed to do, we
// can't trust isRunning()
bool isStopped();
void load(
const QString& originalUri,
time_t originalTime,
......@@ -89,6 +93,7 @@ private:
QWaitCondition mCond;
ThumbnailGroup::Enum mThumbnailGroup;
bool mCancel;
bool mStopped = false;
};
} // namespace
......
......@@ -175,11 +175,10 @@ ThumbnailProvider::ThumbnailProvider()
ThumbnailProvider::~ThumbnailProvider()
{
LOG(this);
abortSubjob();
mThumbnailGenerator->cancel();
disconnect(mThumbnailGenerator, nullptr, this, nullptr);
disconnect(mThumbnailGenerator, nullptr, sThumbnailWriter, nullptr);
connect(mThumbnailGenerator, SIGNAL(finished()), mThumbnailGenerator, SLOT(deleteLater()));
abortSubjob();
mThumbnailGenerator->cancel();
if (mPreviousThumbnailGenerator) {
disconnect(mPreviousThumbnailGenerator, nullptr, sThumbnailWriter, nullptr);
}
......@@ -193,7 +192,7 @@ void ThumbnailProvider::stop()
// startCreatingThumbnail() will take care that these two threads won't work on the same item.
mItems.clear();
abortSubjob();
if (mThumbnailGenerator->isRunning() && !mPreviousThumbnailGenerator) {
if (!mThumbnailGenerator->isStopped() && !mPreviousThumbnailGenerator) {
mPreviousThumbnailGenerator = mThumbnailGenerator;
mPreviousThumbnailGenerator->cancel();
disconnect(mPreviousThumbnailGenerator, nullptr, this, nullptr);
......@@ -524,7 +523,7 @@ void ThumbnailProvider::startCreatingThumbnail(const QString& pixPath)
// connect mPreviousThumbnailGenerator's signal "finished" to determineNextIcon
// which will load the thumbnail from sThumbnailWriter or from disk
// (because we re-add mCurrentItem to mItems).
if (mPreviousThumbnailGenerator && mPreviousThumbnailGenerator->isRunning() &&
if (mPreviousThumbnailGenerator && !mPreviousThumbnailGenerator->isStopped() &&
mOriginalUri == mPreviousThumbnailGenerator->originalUri() &&
mOriginalTime == mPreviousThumbnailGenerator->originalTime() &&
mOriginalFileSize == mPreviousThumbnailGenerator->originalFileSize() &&
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment