Fix memleak in KDynamicJobTracker, KWidgetJobTracker needs QApplication

Summary:
KDynamicJobTracker was not cleaning up its internal job tracking
data structure on finished jobs.
Also was it trying to create a KWidgetJobTracker even if there
was no QApplication instance.

Reviewers: #frameworks, kfunk

Reviewed By: kfunk

Subscribers: kfunk

Tags: #frameworks

Differential Revision: https://phabricator.kde.org/D3977

Gbp-Pq: Name Fix-memleak-in-KDynamicJobTracker-KWidgetJobTracker-needs.patch

diff --git a/autotests/CMakeLists.txt b/autotests/CMakeLists.txt
index 436b5d95f0a15fe9a67cc09b18ea901338855578..0e2d252a4e9611010f2fb29c4e5e5b94288a41ae 100644 (file)
--- a/autotests/CMakeLists.txt
+++ b/autotests/CMakeLists.txt
@@ -68,6 +68,7 @@ if (TARGET KF5::KIOWidgets)
 ecm_add_tests(
  clipboardupdatertest.cpp
  dropjobtest.cpp
+ kdynamicjobtrackernowidgetstest.cpp
  krununittest.cpp
  kdirlistertest.cpp
  kdirmodeltest.cpp

diff --git a/autotests/kdynamicjobtrackernowidgetstest.cpp b/autotests/kdynamicjobtrackernowidgetstest.cpp
new file mode 100644 (file)
index 0000000..a8dbee0
--- /dev/null
+++ b/autotests/kdynamicjobtrackernowidgetstest.cpp
@@ -0,0 +1,69 @@
+/* This file is part of the KDE project
+   Copyright 2017 Friedrich W. H. Kossebau <kossebau@kde.org>
+
+   This library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Library General Public
+   License as published by the Free Software Foundation; either
+   version 2 of the License, or (at your option) any later version.
+
+   This library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Library General Public License for more details.
+
+   You should have received a copy of the GNU Library General Public License
+   along with this library; see the file COPYING.LIB.  If not, write to
+   the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
+   Boston, MA 02110-1301, USA.
+ */
+
+#include <KIO/JobTracker>
+#include <KJobTrackerInterface>
+#include <KJob>
+
+#include <QtTest>
+#include <QEventLoop>
+
+// widget is shown with hardcoded delay of 500 ms by KWidgetJobTracker
+static const int testJobRunningTime = 600;
+
+class TestJob : public KJob
+{
+    Q_OBJECT
+public:
+    void start() Q_DECL_OVERRIDE { QTimer::singleShot(testJobRunningTime, this, &TestJob::doEmit); }
+
+private Q_SLOTS:
+    void doEmit() { emitResult(); }
+};
+
+
+class KDynamicJobTrackerTest : public QObject
+{
+    Q_OBJECT
+
+private Q_SLOTS:
+    void testNoCrashWithoutQWidgetsPossible();
+};
+
+void KDynamicJobTrackerTest::testNoCrashWithoutQWidgetsPossible()
+{
+    // simply linking to KIOWidgets results in KDynamicJobTracker installing itself as KIO's jobtracker
+    KJobTrackerInterface* jobtracker = KIO::getJobTracker();
+    QCOMPARE(jobtracker->metaObject()->className(), "KDynamicJobTracker");
+
+    TestJob *job = new TestJob;
+
+    jobtracker->registerJob(job);
+
+    job->start();
+    QEventLoop loop;
+    connect(job, &KJob::result, &loop, &QEventLoop::quit);
+    loop.exec();
+    // if we are here, no crash has happened due to QWidgets tried to be used -> success
+}
+
+// GUILESS, so QWidgets are not possible
+QTEST_GUILESS_MAIN(KDynamicJobTrackerTest)
+
+#include "kdynamicjobtrackernowidgetstest.moc"

diff --git a/src/widgets/kdynamicjobtracker.cpp b/src/widgets/kdynamicjobtracker.cpp
index 14924d5b576d76cd95983a0d2c42389d76badbea..867489f6f55b904c18dd93b83d4a247116016c89 100644 (file)
--- a/src/widgets/kdynamicjobtracker.cpp
+++ b/src/widgets/kdynamicjobtracker.cpp
@@ -25,6 +25,7 @@
 #include <kjobtrackerinterface.h>
 #include <kio/jobtracker.h>
 
+#include <QApplication>
 #include <QDBusConnection>
 #include <QDBusConnectionInterface>
 #include <QDBusInterface>
@@ -68,49 +69,77 @@ KDynamicJobTracker::~KDynamicJobTracker()
 
 void KDynamicJobTracker::registerJob(KJob *job)
 {
+    if (d->trackers.contains(job)) {
+        return;
+    }
+
+    // only interested in finished() signal,
+    // so catching ourselves instead of using KJobTrackerInterface::registerJob()
+    connect(job, &KJob::finished,
+            this, &KDynamicJobTracker::unregisterJob);
+
+    const bool canHaveWidgets = (qobject_cast<QApplication *>(qApp) != nullptr);
+
+    // always add an entry, even with no trackers used at all,
+    // so unregisterJob() will work as normal
+    AllTrackers& trackers = d->trackers[job];
+
     // do not try to query kuiserver if dbus is not available
     if (!QDBusConnection::sessionBus().interface()) {
-        // fallback to widget tracker only!
-        if (!d->widgetTracker) {
-            d->widgetTracker = new KWidgetJobTracker();
+        if (canHaveWidgets) {
+            // fallback to widget tracker only!
+            if (!d->widgetTracker) {
+                d->widgetTracker = new KWidgetJobTracker();
+            }
+
+            trackers.widgetTracker = d->widgetTracker;
+            trackers.widgetTracker->registerJob(job);
+        } else {
+            trackers.widgetTracker = nullptr;
         }
-        d->trackers[job].widgetTracker = d->widgetTracker;
-        d->trackers[job].widgetTracker->registerJob(job);
+
+        trackers.kuiserverTracker = nullptr;
     } else {
         if (!d->kuiserverTracker) {
             d->kuiserverTracker = new KUiServerJobTracker();
         }
 
-        d->trackers[job].kuiserverTracker = d->kuiserverTracker;
-        d->trackers[job].kuiserverTracker->registerJob(job);
-
-        QDBusInterface interface(QStringLiteral("org.kde.kuiserver"), QStringLiteral("/JobViewServer"), QLatin1String(""),
-                                QDBusConnection::sessionBus(), this);
-        QDBusReply<bool> reply = interface.call(QStringLiteral("requiresJobTracker"));
-
-        if (reply.isValid() && reply.value()) {
-            //create a widget tracker in addition to kuiservertracker.
-            if (!d->widgetTracker) {
-                d->widgetTracker = new KWidgetJobTracker();
+        trackers.kuiserverTracker = d->kuiserverTracker;
+        trackers.kuiserverTracker->registerJob(job);
+
+        trackers.widgetTracker = nullptr;
+        if (canHaveWidgets) {
+            QDBusInterface interface(QStringLiteral("org.kde.kuiserver"), QStringLiteral("/JobViewServer"), QLatin1String(""),
+                                    QDBusConnection::sessionBus(), this);
+            QDBusReply<bool> reply = interface.call(QStringLiteral("requiresJobTracker"));
+
+            if (reply.isValid() && reply.value()) {
+                // create a widget tracker in addition to kuiservertracker.
+                if (!d->widgetTracker) {
+                    d->widgetTracker = new KWidgetJobTracker();
+                }
+                trackers.widgetTracker = d->widgetTracker;
+                trackers.widgetTracker->registerJob(job);
             }
-            d->trackers[job].widgetTracker = d->widgetTracker;
-            d->trackers[job].widgetTracker->registerJob(job);
         }
     }
-
-    Q_ASSERT(d->trackers[job].kuiserverTracker || d->trackers[job].widgetTracker);
 }
 
 void KDynamicJobTracker::unregisterJob(KJob *job)
 {
-    KUiServerJobTracker *kuiserverTracker = d->trackers[job].kuiserverTracker;
-    KWidgetJobTracker *widgetTracker = d->trackers[job].widgetTracker;
+    job->disconnect(this);
+
+    QMap<KJob*, AllTrackers>::Iterator it = d->trackers.find(job);
 
-    if (!(widgetTracker || kuiserverTracker)) {
+    if (it == d->trackers.end()) {
         qWarning() << "Tried to unregister a kio job that hasn't been registered.";
         return;
     }
 
+    const AllTrackers& trackers = it.value();
+    KUiServerJobTracker *kuiserverTracker = trackers.kuiserverTracker;
+    KWidgetJobTracker *widgetTracker = trackers.widgetTracker;
+
     if (kuiserverTracker) {
         kuiserverTracker->unregisterJob(job);
     }
@@ -118,6 +147,8 @@ void KDynamicJobTracker::unregisterJob(KJob *job)
     if (widgetTracker) {
         widgetTracker->unregisterJob(job);
     }
+
+    d->trackers.erase(it);
 }
 
 Q_GLOBAL_STATIC(KDynamicJobTracker, globalJobTracker)