Review Request: Fix possible memory and D-Bus connection leak in KStatusNotifierItem

[Dawit Alemayehu]

> On June 6, 2011, 8:38 p.m., Aaron J. Seigo wrote:
> > i doubt this fix is correct. what looks more like is KStatusNotifierItemDBus::~KStatusNotifierItemDBus(). right now this is the implementation:
> > 
> > m_dbus.unregisterService(m_service);
> > 
> > and perhaps it should include:
> > 
> > m_dbus.unregisterObject("/StatusNotifierItem", this);
> > 
> > and perhaps even:
> > 
> > m_dbus.disconnectFromBus(m_service);
> > 
> > (though i expect the latter is done automatically when QDBusConnection is deleted? maybe not though...)
> 
> Shaun Reich wrote:
>     actually, looks like ~QDBusConnection() won't close the connection. apparently disconnectFromBus does indeed need to be called explicitly. /me suddenly wonders how much code doesn't do this, if need be.

Dunno if this class is using QDBusConnection::sessionBus(), but if it is then the disconnect is supposed to be automatic according to the sessionBus() API documentation. If indeed the disconnection is not occurring, then that might possibly explain the weird dbus related crashes reported in bug# 234484 as well. I hope that is not the case because I do not remember seeing anywhere where disconnectFromBus is called explicitly.


- Dawit


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


On June 6, 2011, 3:31 p.m., Christoph Feck wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101527/
> -----------------------------------------------------------
> 
> (Updated June 6, 2011, 3:31 p.m.)
> 
> 
> Review request for kdelibs, Plasma and Marco Martin.
> 
> 
> Summary
> -------
> 
> According to bug 261180 there is a D-Bus connection leak in KStatusNotifierItem. This patch potentially fixes it, but I do not know how to test it.
> 
> 
> This addresses bug 261180.
>     http://bugs.kde.org/show_bug.cgi?id=261180
> 
> 
> Diffs
> -----
> 
>   kdeui/notifications/kstatusnotifieritem.cpp c48ed50 
> 
> Diff: http://git.reviewboard.kde.org/r/101527/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Christoph
> 
>

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