D7750: fix crash when more than one instances of ExtractorCollection are destructed
This revision was automatically updated to reflect the committed changes. Closed by commit R286:b53a72743601: fix crash when more than one instances of ExtractorCollection are destructed (authored by mgallien). REPOSITORY R286 KFileMetaData CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D7750?vs=20200=20259 REVISION DETAIL https://phabricator.kde.org/D7750 AFFECTED FILES autotests/extractorcollectiontest.cpp src/extractor.cpp src/extractor.h src/extractor_p.h src/extractorcollection.cpp To: mgallien, #frameworks, dfaure Cc: dfaure, anthonyfieroni
D7750: fix crash when more than one instances of ExtractorCollection are destructed
mgallien updated this revision to Diff 20200. mgallien added a comment. add a setAutoDeletePlugin method and modify the name of the enum value to not use the shared word REPOSITORY R286 KFileMetaData CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D7750?vs=19918=20200 BRANCH fixPluginDelete REVISION DETAIL https://phabricator.kde.org/D7750 AFFECTED FILES autotests/extractorcollectiontest.cpp src/extractor.cpp src/extractor.h src/extractor_p.h src/extractorcollection.cpp To: mgallien, #frameworks, dfaure Cc: dfaure, anthonyfieroni
D7750: fix crash when more than one instances of ExtractorCollection are destructed
dfaure accepted this revision. dfaure added a comment. This revision is now accepted and ready to land. Looks good. To nitpick, setAutoDeletePlugin(true) would have been clearer IMHO. (Shared sounds like shared_ptr i.e. refcounting, which isn't the case here, it's just autodelete=off) REPOSITORY R286 KFileMetaData BRANCH fixPluginDelete REVISION DETAIL https://phabricator.kde.org/D7750 To: mgallien, #frameworks, dfaure Cc: dfaure, anthonyfieroni
D7750: fix crash when more than one instances of ExtractorCollection are destructed
mgallien marked an inline comment as done. REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D7750 To: mgallien, #frameworks, dfaure Cc: dfaure, anthonyfieroni
D7750: fix crash when more than one instances of ExtractorCollection are destructed
mgallien added a comment. Reverted most of my changes and only use a private method with two parameters to set the ExtractorPlugin in Extractor class. The second parameter should indicate if the Extractor instance is owner of the plugin or not. The private class is no longer included in extractorcollection.cpp. I am not really sure this is the right approach as it is very easy to use a wrong combination for the arguments of Extractor::setExtractorPlugin. I will try to improve on that. REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D7750 To: mgallien, #frameworks, dfaure Cc: dfaure, anthonyfieroni
D7750: fix crash when more than one instances of ExtractorCollection are destructed
mgallien updated this revision to Diff 19918. mgallien marked 6 inline comments as done. mgallien added a comment. add missing noexcept REPOSITORY R286 KFileMetaData CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D7750?vs=19917=19918 BRANCH fixPluginDelete REVISION DETAIL https://phabricator.kde.org/D7750 AFFECTED FILES autotests/extractorcollectiontest.cpp src/extractor.cpp src/extractor.h src/extractor_p.h src/extractorcollection.cpp To: mgallien, #frameworks, dfaure Cc: dfaure, anthonyfieroni
D7750: fix crash when more than one instances of ExtractorCollection are destructed
mgallien updated this revision to Diff 19917. mgallien added a comment. reduce the scope of modifications, use an enum to indicate if the plugin should be deleted and use private methods instead of direct access to private class REPOSITORY R286 KFileMetaData CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D7750?vs=19450=19917 BRANCH fixPluginDelete REVISION DETAIL https://phabricator.kde.org/D7750 AFFECTED FILES autotests/extractorcollectiontest.cpp src/extractor.cpp src/extractor.h src/extractor_p.h src/extractorcollection.cpp To: mgallien, #frameworks, dfaure Cc: dfaure, anthonyfieroni
D7750: fix crash when more than one instances of ExtractorCollection are destructed
dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> mgallien wrote in extractor.cpp:34
> If the Extractor is built using the move constructor, the other instance will
> have a null d pointer. As far as I know, this is standard practice.
You are correct. However, to improve readability, move (haha) the
implementation of the move constructor to be with the default constructor, i.e.
above this freeExtractorPlugin method. This will make it clearer that there are
multiple ctors, when reading top to bottom.
> extractor.cpp:36
> +if (d->m_pluginLoader.isLoaded()) {
> +d->m_pluginLoader.unload();
> +} else {
Are you sure you want to call unload? We've had many many problems in the past
with unloading plugins.
Anything that still refers to a static object from the plugin (e.g. QMetaObject
registration, or anything else) will crash.
> extractor.h:36
> public:
> +Extractor(Extractor &);
> +
add `noexcept`
> extractor.h:38
> +
> +void operator =(Extractor &);
> +
The proper syntax for a move constructor is
` Extractor& operator=(Extractor &) noexcept`
> extractorcollection.cpp:111
> +Extractor newExtractor;
> +newExtractor.d->m_pluginLoader.setFileName(pluginPath);
>
This lacks encapsulation. Add a setPluginFileName method to the Extract class,
so d doesn't have to be public.
> extractorcollection.cpp:123
> if (plugin) {
> -Extractor* ex= new Extractor;
> +Extractor* ex= new Extractor(std::move(newExtractor));
> ex->d->m_plugin = plugin;
This whole std::move business is fun, but we could just add an Extractor ctor
that takes an ExtractorPlugin as parameter, no?
(and possibly a QPluginLoader, depending on what we decide about unloading).
Then the loading will be using QPluginLoader here, as before, without a need
for cloning the extractor, which looks strange to me, design wise.
Either move the whole loading code to Extractor, or load here and then
instanciate (as before), but creating two extractor instances (and moving one
to the other) looks weird, overkill, and recipe for trouble.
REPOSITORY
R286 KFileMetaData
REVISION DETAIL
https://phabricator.kde.org/D7750
To: mgallien, #frameworks, dfaure
Cc: dfaure, anthonyfieroni
D7750: fix crash when more than one instances of ExtractorCollection are destructed
mgallien added a comment. I forgot to say that I do not mind modifying the patch. I am just hoping to make steady progress on it since I am having crash very often with my music player. REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D7750 To: mgallien, #frameworks, dfaure Cc: dfaure, anthonyfieroni
D7750: fix crash when more than one instances of ExtractorCollection are destructed
mgallien added a reviewer: dfaure. REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D7750 To: mgallien, #frameworks, dfaure Cc: dfaure, anthonyfieroni
D7750: fix crash when more than one instances of ExtractorCollection are destructed
mgallien added inline comments. INLINE COMMENTS > anthonyfieroni wrote in extractor.cpp:34 > d *should* never be nullptr If the Extractor is built using the move constructor, the other instance will have a null d pointer. As far as I know, this is standard practice. REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D7750 To: mgallien, #frameworks Cc: dfaure, anthonyfieroni
D7750: fix crash when more than one instances of ExtractorCollection are destructed
anthonyfieroni added a subscriber: dfaure.
anthonyfieroni added a comment.
Add @dfaure to accept it.
INLINE COMMENTS
> extractor.cpp:34
> {
> -delete d->m_plugin;
> +if (d) {
> +if (d->m_pluginLoader.isLoaded()) {
d *should* never be nullptr
REPOSITORY
R286 KFileMetaData
REVISION DETAIL
https://phabricator.kde.org/D7750
To: mgallien, #frameworks
Cc: dfaure, anthonyfieroni
D7750: fix crash when more than one instances of ExtractorCollection are destructed
mgallien added a comment. Should I push the fix in its current version ? REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D7750 To: mgallien, #frameworks Cc: anthonyfieroni
D7750: fix crash when more than one instances of ExtractorCollection are destructed
mgallien updated this revision to Diff 19450. mgallien added a comment. remove duplicated code REPOSITORY R286 KFileMetaData CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D7750?vs=19416=19450 BRANCH fixPluginDelete REVISION DETAIL https://phabricator.kde.org/D7750 AFFECTED FILES autotests/extractorcollectiontest.cpp src/extractor.cpp src/extractor.h src/extractor_p.h src/extractorcollection.cpp To: mgallien, #frameworks Cc: anthonyfieroni
D7750: fix crash when more than one instances of ExtractorCollection are destructed
anthonyfieroni added inline comments.
INLINE COMMENTS
> extractor.cpp:39-46
> +if (d) {
> +if (d->m_pluginLoader.isLoaded()) {
> +d->m_pluginLoader.unload();
> +} else {
> +delete d->m_plugin;
> +}
> +}
Make a method to call it, here and in destructor to not have duplicate code.
Other looks good to me +1.
REPOSITORY
R286 KFileMetaData
REVISION DETAIL
https://phabricator.kde.org/D7750
To: mgallien, #frameworks
Cc: anthonyfieroni
D7750: fix crash when more than one instances of ExtractorCollection are destructed
mgallien updated this revision to Diff 19416. mgallien added a comment. fix another potential memory leak REPOSITORY R286 KFileMetaData CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D7750?vs=19415=19416 BRANCH fixPluginDelete REVISION DETAIL https://phabricator.kde.org/D7750 AFFECTED FILES autotests/extractorcollectiontest.cpp src/extractor.cpp src/extractor.h src/extractor_p.h src/extractorcollection.cpp To: mgallien, #frameworks Cc: anthonyfieroni
D7750: fix crash when more than one instances of ExtractorCollection are destructed
mgallien updated this revision to Diff 19415. mgallien added a comment. fix memory leak REPOSITORY R286 KFileMetaData CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D7750?vs=19342=19415 BRANCH fixPluginDelete REVISION DETAIL https://phabricator.kde.org/D7750 AFFECTED FILES autotests/extractorcollectiontest.cpp src/extractor.cpp src/extractor.h src/extractor_p.h src/extractorcollection.cpp To: mgallien, #frameworks Cc: anthonyfieroni
D7750: fix crash when more than one instances of ExtractorCollection are destructed
mgallien added a comment. In https://phabricator.kde.org/D7750#144373, @anthonyfieroni wrote: > https://phabricator.kde.org/source/kfilemetadata/browse/master/src/extractorcollection.cpp;621101fd9e9d82be3d84f2140a4bf53ea13fd3f0$137 > Look it leak now, no? Sorry, I misread your comment. Yes you are true. Sorry for that. REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D7750 To: mgallien, #frameworks Cc: anthonyfieroni
D7750: fix crash when more than one instances of ExtractorCollection are destructed
mgallien added a comment. According to QPluginLoader, if one wants to release the memory, In https://phabricator.kde.org/D7750#144373, @anthonyfieroni wrote: > https://phabricator.kde.org/source/kfilemetadata/browse/master/src/extractorcollection.cpp;621101fd9e9d82be3d84f2140a4bf53ea13fd3f0$137 > Look it leak now, no? According to documentation of QPluginLoader, memory is released at application quit. If you want to release it earlier, one should use unload. I can modify the patch to use that to release as soon as it is possible (i.e. no ExtractorCollection contains a given plugin). For that, I will probably need to make QPluginLoader a member of Extractor class. What do you think ? REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D7750 To: mgallien, #frameworks Cc: anthonyfieroni
D7750: fix crash when more than one instances of ExtractorCollection are destructed
anthonyfieroni added a comment. https://phabricator.kde.org/source/kfilemetadata/browse/master/src/extractorcollection.cpp;621101fd9e9d82be3d84f2140a4bf53ea13fd3f0$137 Look it leak now, no? REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D7750 To: mgallien, #frameworks Cc: anthonyfieroni
D7750: fix crash when more than one instances of ExtractorCollection are destructed
mgallien edited the test plan for this revision. REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D7750 To: mgallien, #frameworks
D7750: fix crash when more than one instances of ExtractorCollection are destructed
mgallien created this revision. Restricted Application added a project: Frameworks. REVISION SUMMARY fix crash when more than one instances of ExtractorCollection are destructed TEST PLAN without the modification to Extractor class, the new test fails. With the fix, valgrind does not report any new memory leak REPOSITORY R286 KFileMetaData BRANCH fixPluginDelete REVISION DETAIL https://phabricator.kde.org/D7750 AFFECTED FILES autotests/extractorcollectiontest.cpp src/extractor.cpp To: mgallien, #frameworks
