fix crash when more than one instances of ExtractorCollection are destructed
Details
Details
- Reviewers
dfaure - Group Reviewers
Frameworks - Commits
- R286:b53a72743601: fix crash when more than one instances of ExtractorCollection are destructed
without the modification to Extractor class, the new test fails. With the fix, valgrind does not report any new memory leak.
Due to the way QPluginLoader::instance works, the plugins are implicitly shared. If the first ExtractorCollection delete them, the second one will fail due to double delete.
Diff Detail
Diff Detail
- Repository
- R286 KFileMetaData
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
According to QPluginLoader, if one wants to release the memory,
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 ?
| src/extractor.cpp | ||
|---|---|---|
| 39–46 | Make a method to call it, here and in destructor to not have duplicate code. | |
| src/extractor.cpp | ||
|---|---|---|
| 34–37 | 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. | |
Comment Actions
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.
| src/extractor.cpp | ||
|---|---|---|
| 34–37 | 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. | |
| 36 | Are you sure you want to call unload? We've had many many problems in the past with unloading plugins. | |
| src/extractor.h | ||
| 43 | add noexcept | |
| 45 | The proper syntax for a move constructor is | |
| src/extractorcollection.cpp | ||
| 110 | This lacks encapsulation. Add a setPluginFileName method to the Extract class, so d doesn't have to be public. | |
| 121 | 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. | |
Comment Actions
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
Comment Actions
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.
Comment Actions
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)
Comment Actions
add a setAutoDeletePlugin method and modify the name of the enum value to not use the shared word