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
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
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)
mgallien marked an inline comment as done.
REPOSITORY
R286 KFileMetaData
REVISION DETAIL
https://phabricator.kde.org/D7750
To: mgallien, #frameworks, dfaure
Cc: dfaure, anthonyfieroni
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
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
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
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,
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,
mgallien added a reviewer: dfaure.
REPOSITORY
R286 KFileMetaData
REVISION DETAIL
https://phabricator.kde.org/D7750
To: mgallien, #frameworks, dfaure
Cc: dfaure, anthonyfieroni
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
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
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
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
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
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
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
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
mgallien added a comment.
According to QPluginLoader, if one wants to release the memory,
In https://phabricator.kde.org/D7750#144373, @anthonyfieroni wrote:
>
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,
mgallien edited the test plan for this revision.
REPOSITORY
R286 KFileMetaData
REVISION DETAIL
https://phabricator.kde.org/D7750
To: mgallien, #frameworks
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
22 matches
Mail list logo
