Review Request: One step towards fixing plugin statusbar icon leak

[Thomas Friedrichsmeier]

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

Review request for kdelibs.


Summary
-------

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. Yet, simply changing that could cause double deletion in plugins which do not make this (currently false) assumption.

Therefore, the only clean way towards fixing this in the mid term, that I could think of is:
1. Keep the current behavior by default, but:
2. Add a parameter "autodelete" to SE::addStatusBarItem(), and deprecate the old version. With autodelete set to true (encouraged by the documentation), icons will be deleted with the SE.

That's what the patch does. Note that this does not actually fix anything, yet, but simply causes deprecation warnings in code that may need appropriate fixing.


Diffs
-----

  kparts/statusbarextension.h 69691d7f80c55a6193c345f7f514d35af2b20b15 
  kparts/statusbarextension.cpp 11fdf40681c708bc0f031797083c7f6b6b638f60 

Diff: http://git.reviewboard.kde.org/r/101653/diff


Testing
-------


Thanks,

Thomas

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