D3977: Fix memleak in KDynamicJobTracker, KWidgetJobTracker needs QApplication
kossebau added a comment. In https://phabricator.kde.org/D3977#155973, @elvisangelaccio wrote: > From what I can see, after this change any job registered with > > KIO::getJobTracker()->registerJob(job); > > > no longer needs to be manually unregistered with > > KIO::getJobTracker()->unregisterJob(job); Looking at the history, `KJobTrackerInterface::registerJob(KJob *job)`has connected the job's `finished(KJob*)` signal to the tracker's `unregisterJob(KJob*)` since ages, 2008 and possibly longer (cmp. https://phabricator.kde.org/R446:7308fa7e6b756f5c6fe1b8adbcc6095e3bb5b995) And this commit here now uncovered that somehow for Ark as well. > This is a minor thing, but should probably be documented somewhere. Agreed, by just looking at the API (dox) one would assume one had to possibly also unregister manually. You proposed it -> you do it? :) REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D3977 To: kossebau, #frameworks, kfunk Cc: elvisangelaccio, kfunk
D3977: Fix memleak in KDynamicJobTracker, KWidgetJobTracker needs QApplication
elvisangelaccio added a comment. In https://phabricator.kde.org/D3977#155762, @kossebau wrote: > On a quick look the automatic unregistering based on the finished signal seems to make sense for any KJob subclass. Perhaps the change in this commit conflicts with some older code in Ark to work around the old behaviour and which does some additional manual unregistration? Or perhaps some slot in accidentally invoked 2x so the unregistration happens more than once, with the second try than failing? Wild guesses is all I have to offer, sorry. I cannot remember anything questionable about this patch, so no pointers into it, instead my initial reaction would be to look at the consumer side. Yes Ark has a manual unregistration, which I can easily fix. From what I can see, after this change any job registered with KIO::getJobTracker()->registerJob(job); no longer needs to be manually unregistered with KIO::getJobTracker()->unregisterJob(job); This is a minor thing, but should probably be documented somewhere. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D3977 To: kossebau, #frameworks, kfunk Cc: elvisangelaccio, kfunk
D3977: Fix memleak in KDynamicJobTracker, KWidgetJobTracker needs QApplication
kossebau added a comment. In https://phabricator.kde.org/D3977#155721, @elvisangelaccio wrote: > @kossebau > After this change we get the > > Tried to unregister a kio job that hasn't been registered. > > warning with a `KCompositeJob` in Ark (batchextract.cpp). > > > The job is now automatically unregistered and the `unregisterJob(this)` call in the job destructor is causing this warning. > Are KCompositeJobs still supposed to unregister themselves? KJob related knowledge sadly completely swapped out of my brain, would need to investigate myself. No idea if there should be done anything special for KCompositeJobs. On a quick look the automatic unregistering based on the finished signal seems to make sense for any KJob subclass. Perhaps the change in this commit conflicts with some older code in Ark to work around the old behaviour and which does some additional manual unregistration? Or perhaps some slot in accidentally invoked 2x so the unregistration happens more than once, with the second try than failing? Wild guesses is all I have to offer, sorry. I cannot remember anything questionable about this patch, so no pointers into it, instead my initial reaction would be to look at the consumer side. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D3977 To: kossebau, #frameworks, kfunk Cc: elvisangelaccio, kfunk
D3977: Fix memleak in KDynamicJobTracker, KWidgetJobTracker needs QApplication
elvisangelaccio added a comment. @kossebau After this change we get the Tried to unregister a kio job that hasn't been registered. warning with a `KCompositeJob` in Ark (batchextract.cpp). The job is now automatically unregistered and the `unregisterJob(this)` call in the job destructor is causing this warning. Are KCompositeJobs still supposed to unregister themselves? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D3977 To: kossebau, #frameworks, kfunk Cc: elvisangelaccio, kfunk
[Differential] [Closed] D3977: Fix memleak in KDynamicJobTracker, KWidgetJobTracker needs QApplication
This revision was automatically updated to reflect the committed changes. Closed by commit R241:3261c842a2a9: Fix memleak in KDynamicJobTracker, KWidgetJobTracker needs QApplication (authored by kossebau). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D3977?vs=9781=10113 REVISION DETAIL https://phabricator.kde.org/D3977 AFFECTED FILES autotests/CMakeLists.txt autotests/kdynamicjobtrackernowidgetstest.cpp src/widgets/kdynamicjobtracker.cpp EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kossebau, #frameworks, kfunk Cc: kfunk
[Differential] [Updated] D3977: Fix memleak in KDynamicJobTracker, KWidgetJobTracker needs QApplication
kossebau marked an inline comment as done. kossebau added a comment. Will commit in a few days, if noone objects. So it has some weeks for testing outside my system before the next KF release. REPOSITORY R241 KIO BRANCH fixKDynamicJobTracker REVISION DETAIL https://phabricator.kde.org/D3977 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kossebau, #frameworks, kfunk Cc: kfunk
[Differential] [Updated, 147 lines] D3977: Fix memleak in KDynamicJobTracker, KWidgetJobTracker needs QApplication
kossebau updated this revision to Diff 9781. kossebau marked an inline comment as done. kossebau added a comment. Use Q_DECL_OVERRIDE & nullpt REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D3977?vs=9775=9781 BRANCH fixKDynamicJobTracker REVISION DETAIL https://phabricator.kde.org/D3977 AFFECTED FILES autotests/CMakeLists.txt autotests/kdynamicjobtrackernowidgetstest.cpp src/widgets/kdynamicjobtracker.cpp EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kossebau, #frameworks, kfunk Cc: kfunk
[Differential] [Accepted] D3977: Fix memleak in KDynamicJobTracker, KWidgetJobTracker needs QApplication
kfunk accepted this revision.
kfunk added a reviewer: kfunk.
kfunk added a comment.
This revision is now accepted and ready to land.
Rest LGTM, but let's wait for another review
INLINE COMMENTS
> kdynamicjobtrackernowidgetstest.cpp:34
> +public:
> +virtual void start() { QTimer::singleShot(testJobRunningTime, this,
> ::doEmit); }
> +
`Q_DECL_OVERRIDE`
> kdynamicjobtracker.cpp:99
> +} else {
> +trackers.widgetTracker = 0;
> }
Here & below: Use `nullptr`?
REPOSITORY
R241 KIO
BRANCH
fixKDynamicJobTracker
REVISION DETAIL
https://phabricator.kde.org/D3977
EMAIL PREFERENCES
https://phabricator.kde.org/settings/panel/emailpreferences/
To: kossebau, #frameworks, kfunk
Cc: kfunk
[Differential] [Updated, 147 lines] D3977: Fix memleak in KDynamicJobTracker, KWidgetJobTracker needs QApplication
kossebau updated this revision to Diff 9775. kossebau added a comment. add unittest for crash by qwidgets without qapplication instance REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D3977?vs=9740=9775 BRANCH fixKDynamicJobTracker REVISION DETAIL https://phabricator.kde.org/D3977 AFFECTED FILES autotests/CMakeLists.txt autotests/kdynamicjobtrackernowidgetstest.cpp src/widgets/kdynamicjobtracker.cpp EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kossebau, #frameworks
[Differential] [Request, 77 lines] D3977: Fix memleak in KDynamicJobTracker, KWidgetJobTracker needs QApplication
kossebau created this revision. kossebau added a reviewer: Frameworks. Restricted Application added a project: Frameworks. REVISION 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. REPOSITORY R241 KIO BRANCH fixKDynamicJobTracker REVISION DETAIL https://phabricator.kde.org/D3977 AFFECTED FILES src/widgets/kdynamicjobtracker.cpp EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kossebau, #frameworks
