Review Request: Put an end to plugin statusbar icon leaks

[David Faure]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/101653/#review7034
-----------------------------------------------------------


Good analysis, thanks for that.

I only disagree with the deprecation of the method. lxr shows cases where an icon is being removed manually. For instance (for a good or a bad reason...) http://lxr.kde.org/source/kde/kde-baseapps/konq-plugins/microformat/konqmficon.cpp#315 shows an icon being removed while the page is loading, and added only once the page is loaded.

Same thing in http://lxr.kde.org/source/extragear/base/kwebkitpart/src/kwebkitpart.cpp#759, the wallet icon is removed when the wallet is closed.

- David Faure


On Oct. 1, 2011, 7:14 p.m., Thomas Friedrichsmeier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101653/
> -----------------------------------------------------------
> 
> (Updated Oct. 1, 2011, 7:14 p.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Description
> -------
> 
> I guess this one needs some explaining...
> 
> Many KParts::Plugins appear to assume that widgets that they add to a KParts::StatusBarExtension (SE) will effectively be owned by SE, and will be deleted with the extension. E.g. the konqi W3 Validator plugin has this code:
> 
> void PluginValidators::removeStatusBarIcon()
> {
>   if (!m_icon)
>     return;
>  
>   m_statusBarExt = KParts::StatusBarExtension::childObject(m_part);
>   if (!m_statusBarExt)
>     return;
>  
>   m_statusBarExt->removeStatusBarItem(m_icon);
>   delete m_icon;
>   m_icon = 0;
> }
> 
> Note the return when no SE exists (any more). Checking for the existence of the SE is important, as it may become deleted before the plugins are deleted. In fact, at least for KHTMLPart that is the case. However, the assumption that the icon will have been deleted with the SE is wrong. It is still around. In konqueror this is not visible, because the part is always sent a de-activate event before destruction, and the SE hides the icons, then. In RKWard, parts typically do not receive a de-activate event before destruction, turning the memory leak into a visible icon leak (see this screenshot: http://reaktanz.de/stuff/R/RKWard_icon_soup.png ).
> 
> Fixing this in the plugin is simple: Only SE::removeStatusBarItem() would need to be guarded. But the problem is bound to re-surface in the next plugin. Further, I think the assumption that the widget will become owned by the SE is very reasonable.
> 
> ----- The part of the description below was changed for Version 2 of the patch -----
> 
> Basically that is what this patch does, by deleting the widget in the destructor. Double deletion of the widget in both the StatusBarExtension and the Plugin should not be a problem in this solution. In the StatusBarExtension, the widget is already stored as a QPointer, and it is destroyed using deleteLater(), rather than immediate deletion.
> 
> Deprecating StatusBarExtension::removeStatusBarItem() may be somewhat controversial. However, while I could think of valid use-cases for this in theory, I firmly believe that none exists in practice. Rather all uses that I have seen are in conjunction with the manual deletion of the added widget / icon. The deprecation is intended to serve as a prompt for removing such - now obsolete - code.
> 
> 
> Diffs
> -----
> 
>   kparts/statusbarextension.h 69691d7 
>   kparts/statusbarextension.cpp 11fdf40 
> 
> Diff: http://git.reviewboard.kde.org/r/101653/diff/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Thomas Friedrichsmeier
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20111003/a5d77901/attachment.htm>