Commit 06b526e8 authored by Milian Wolff's avatar Milian Wolff
Browse files

Properly cleanup FileManagerListJob when folder items are deleted

This is an alternative approach to fix the issue described in
https://phabricator.kde.org/D15899

The approach used here is generic and does not require any users
of the API to know about the potential pitfalls, it actually
decreases coupling a bit!

The old approach to cleanup the list jobs only worked when we
called AbstractFileManagerPluginPrivate::removeFolder, but that
wouldn't work when a folder item gets deleted directly - e.g.
from the CMakeManager or any other code. Now, we listen to
the model signal when a row is about to be removed and cleanup
the item pointers in the list job. While doing so, we must make
take into account that the pointer may already be partially destroyed,
since only the ProjectBaseItem dtor will call takeRow, at which point
the overriden methods in ProjectFolderItem e.g. won't be available
anymore.

Additionally, it turned out that the fallback for a similar handler
of partially destroyed classes in
AbstractFileManagerPluginPrivate::jobFinished only removed the job
from a copied list, since it was using foreach, and didn't even take
the list by reference...

Finally, the test is expanded to reliably trigger the crashy behavior
explained in the review request on phabricator.

BUG: 260741
parent 451c3868
Pipeline #713 failed with stage
in 60 minutes and 1 second
......@@ -101,8 +101,6 @@ public:
/// Common renaming function.
bool rename(ProjectBaseItem* item, const Path& newPath);
void removeFolder(ProjectFolderItem* folder);
QHash<IProject*, KDirWatch*> m_watchers;
QHash<IProject*, QList<FileManagerListJob*> > m_projectJobs;
QVector<QString> m_stoppedFolders;
......@@ -154,17 +152,12 @@ KIO::Job* AbstractFileManagerPluginPrivate::eventuallyReadFolder(ProjectFolderIt
void AbstractFileManagerPluginPrivate::jobFinished(KJob* job)
{
auto* gmlJob = qobject_cast<FileManagerListJob*>(job);
if (gmlJob) {
ifDebug(qCDebug(FILEMANAGER) << job << gmlJob << gmlJob->item();)
m_projectJobs[ gmlJob->item()->project() ].removeOne( gmlJob );
} else {
// job emitted its finished signal from its destructor
// ensure we don't keep a dangling point in our list
foreach (auto jobs, m_projectJobs) {
if (jobs.removeOne(reinterpret_cast<FileManagerListJob*>(job))) {
break;
}
// ensure we don't keep a dangling point in our list
// NOTE: job is potentially emitting its finished signal from its destructor
// or the item that was used internally may have been deleted already
for (auto& jobs : m_projectJobs) {
if (jobs.removeOne(reinterpret_cast<FileManagerListJob*>(job))) {
break;
}
}
}
......@@ -217,7 +210,7 @@ void AbstractFileManagerPluginPrivate::addJobItems(FileManagerListJob* job,
int index = folders.indexOf( f->path() );
if ( index == -1 ) {
// folder got removed or is now invalid
removeFolder(f);
delete f;
--j;
} else {
// this folder already exists in the view
......@@ -232,7 +225,7 @@ void AbstractFileManagerPluginPrivate::addJobItems(FileManagerListJob* job,
if ( index == -1 ) {
// file got removed or is now invalid
ifDebug(qCDebug(FILEMANAGER) << "removing file:" << f << f->path();)
baseItem->removeRow( j );
delete f;
--j;
} else {
// this file already exists in the view
......@@ -349,12 +342,12 @@ void AbstractFileManagerPluginPrivate::deleted(const QString& path_)
continue;
}
foreach ( ProjectFolderItem* item, p->foldersForPath(indexed) ) {
removeFolder(item);
delete item;
}
foreach ( ProjectFileItem* item, p->filesForPath(indexed) ) {
emit q->fileRemoved(item);
ifDebug(qCDebug(FILEMANAGER) << "removing file" << item;)
item->parent()->removeRow(item->row());
delete item;
}
}
}
......@@ -423,33 +416,6 @@ void AbstractFileManagerPluginPrivate::continueWatcher(ProjectFolderItem* folder
m_stoppedFolders.remove(idx);
}
}
bool isChildItem(ProjectBaseItem* parent, ProjectBaseItem* child)
{
do {
if (child == parent) {
return true;
}
child = child->parent();
} while(child);
return false;
}
void AbstractFileManagerPluginPrivate::removeFolder(ProjectFolderItem* folder)
{
ifDebug(qCDebug(FILEMANAGER) << "removing folder:" << folder << folder->path();)
foreach(FileManagerListJob* job, m_projectJobs[folder->project()]) {
if (isChildItem(folder, job->item())) {
qCDebug(FILEMANAGER) << "killing list job for removed folder" << job << folder->path();
job->abort();
Q_ASSERT(!m_projectJobs.value(folder->project()).contains(job));
} else {
job->removeSubDir(folder);
}
}
folder->parent()->removeRow( folder->row() );
}
//END Private
//BEGIN Plugin
......@@ -463,6 +429,19 @@ AbstractFileManagerPlugin::AbstractFileManagerPlugin( const QString& componentNa
{
connect(core()->projectController(), &IProjectController::projectClosing,
this, [&] (IProject* project) { d->projectClosing(project); });
connect(core()->projectController()->projectModel(), &ProjectModel::rowsAboutToBeRemoved,
this, [&] (const QModelIndex& parent, int first, int last) {
// cleanup list jobs to remove about-to-be-dangling pointers
auto* model = core()->projectController()->projectModel();
for (int i = first; i <= last; ++i) {
const auto index = model->index(i, 0, parent);
auto* item = index.data(ProjectModel::ProjectItemRole).value<ProjectBaseItem*>();
Q_ASSERT(item);
for (auto* job : d->m_projectJobs.value(item->project())) {
job->handleRemovedItem(item);
}
}
});
}
AbstractFileManagerPlugin::~AbstractFileManagerPlugin() = default;
......@@ -579,7 +558,7 @@ bool AbstractFileManagerPlugin::removeFilesAndFolders(const QList<ProjectBaseIte
Q_ASSERT(item->folder());
emit folderRemoved(item->folder());
}
item->parent()->removeRow( item->row() );
delete item;
}
d->continueWatcher(parent);
......@@ -609,7 +588,7 @@ bool AbstractFileManagerPlugin::moveFilesAndFolders(const QList< ProjectBaseItem
} else {
emit folderRemoved(item->folder());
}
oldParent->removeRow( item->row() );
delete item;
KIO::Job *readJob = d->eventuallyReadFolder(newParent);
// reload first level synchronously, deeper levels will run async
// this is required for code that expects the new item to exist after
......
......@@ -32,6 +32,19 @@
using namespace KDevelop;
namespace {
bool isChildItem(ProjectBaseItem* parent, ProjectBaseItem* child)
{
do {
if (child == parent) {
return true;
}
child = child->parent();
} while(child);
return false;
}
}
FileManagerListJob::FileManagerListJob(ProjectFolderItem* item)
: KIO::Job(), m_item(item), m_aborted(false)
{
......@@ -51,11 +64,6 @@ FileManagerListJob::FileManagerListJob(ProjectFolderItem* item)
#endif
}
ProjectFolderItem* FileManagerListJob::item() const
{
return m_item;
}
void FileManagerListJob::addSubDir( ProjectFolderItem* item )
{
Q_ASSERT(!m_listQueue.contains(item));
......@@ -64,9 +72,16 @@ void FileManagerListJob::addSubDir( ProjectFolderItem* item )
m_listQueue.enqueue(item);
}
void FileManagerListJob::removeSubDir(ProjectFolderItem* item)
void FileManagerListJob::handleRemovedItem(ProjectBaseItem* item)
{
m_listQueue.removeAll(item);
// NOTE: the item could be (partially) destroyed already, thus it's not save
// to call e.g. item->folder to cast the base item to a folder item...
auto *folder = reinterpret_cast<ProjectFolderItem*>(item);
m_listQueue.removeAll(folder);
if (isChildItem(item, m_item)) {
abort();
}
}
void FileManagerListJob::slotEntries(KIO::Job* job, const KIO::UDSEntryList& entriesIn)
......
......@@ -32,7 +32,8 @@
namespace KDevelop
{
class ProjectFolderItem;
class ProjectFolderItem;
class ProjectBaseItem;
class FileManagerListJob : public KIO::Job
{
......@@ -40,10 +41,9 @@ class FileManagerListJob : public KIO::Job
public:
explicit FileManagerListJob(ProjectFolderItem* item);
ProjectFolderItem* item() const;
void addSubDir(ProjectFolderItem* item);
void removeSubDir(ProjectFolderItem* item);
void handleRemovedItem(ProjectBaseItem* item);
void abort();
void start() override;
......
......@@ -339,6 +339,8 @@ void TestProjectLoad::raceJob()
IProject *project = ICore::self()->projectController()->projectAt(0);
QCOMPARE(project->projectFile().toUrl(), p.file);
ProjectFolderItem* root = project->projectItem();
QCOMPARE(root->project(), project);
QVERIFY(root->model());
QCOMPARE(root->rowCount(), 1);
ProjectBaseItem* testItem = root->child(0);
QVERIFY(testItem->folder());
......@@ -348,16 +350,32 @@ void TestProjectLoad::raceJob()
ProjectBaseItem* asdfItem = testItem->children().at(last);
QVERIFY(asdfItem->folder());
// reload to trigger new list job
project->projectFileManager()->reload(testItem->folder());
// move dir
QVERIFY(dir.rename(QStringLiteral("test"), QStringLiteral("test2")));
// move sub dir
QVERIFY(dir.rename(QStringLiteral("test2/zzzzz"), QStringLiteral("test2/bla")));
QTest::qWait(500);
QCOMPARE(root->rowCount(), 1);
testItem = root->child(0);
QVERIFY(testItem->folder());
QCOMPARE(testItem->baseName(), QStringLiteral("test2"));
// reload full model and then move dir
project->reloadModel();
QVERIFY(dir.rename(QStringLiteral("test2"), QStringLiteral("test3")));
QTest::qWait(500);
// note: this actually invalidates the root, so query that again
root = project->projectItem();
QVERIFY(root);
QCOMPARE(root->rowCount(), 1);
testItem = root->child(0);
QVERIFY(testItem->folder());
QCOMPARE(testItem->baseName(), QStringLiteral("test3"));
}
void TestProjectLoad::addDuringImport()
......
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