Review Request: Put an end to plugin statusbar icon leaks

[Thomas Friedrichsmeier]

-----------------------------------------------------------
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.


Summary (updated)
-----------------

Put an end to plugin statusbar icon leaks


Description (updated)
-------

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 (updated)
-----

  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/20111001/3a68cf5f/attachment.htm>