Commit 3be72f82 authored by Christian Esken's avatar Christian Esken
Browse files

Fix memory leaks, especially related to the MixDevice - DBusControlWrapper reference cycle.

CCBUGS: 309464
parent 0817891c
......@@ -70,6 +70,7 @@ Mixer_ALSA::Mixer_ALSA( Mixer* mixer, int device ) : Mixer_Backend(mixer, devic
Mixer_ALSA::~Mixer_ALSA()
{
kDebug() << "Destruct " << this;
close();
}
......@@ -478,6 +479,8 @@ void Mixer_ALSA::deinitAlsaPolling()
int
Mixer_ALSA::close()
{
kDebug() << "close " << this;
int ret=0;
m_isOpen = false;
......@@ -509,7 +512,6 @@ Mixer_ALSA::close()
mixer_elem_list.clear();
mixer_sid_list.clear();
m_mixDevices.clear();
m_id2numHash.clear();
deinitAlsaPolling();
......
......@@ -45,14 +45,39 @@ m_devnum (device) , m_isOpen(false), m_recommendedMaster(), _mixer(mixer), _poll
}
int Mixer_Backend::shutdown()
{
int ret = close();
freeMixDevices();
return ret;
}
int Mixer_Backend::close()
{
kDebug() << "Implicit close on " << this << ". Please instead call close() explicitly (before destroying this object)";
// ^^^ Background. before the destructor runs, the C++ runtime changes the virtual pointers to point back
// to the common base class. So what actually runs is not run Mixer_ALSA::close(), but this method.
//
// Comment: IMO this is totally stupid and insane behavior of C++, because you cannot simply cannot call
// the overwritten (cleanup) methods in the destructor.
return 0;
}
Mixer_Backend::~Mixer_Backend()
{
kDebug() << "Destruct " << this;
// qDebug() << "Running Mixer_Backend destructor";
delete _pollingTimer;
//qDeleteAll(m_mixDevices); // TODO cesken Leak check the removed qDeleteAll()
m_mixDevices.clear();
shutdown();
}
void Mixer_Backend::freeMixDevices()
{
foreach (shared_ptr<MixDevice> md, m_mixDevices)
md->close();
m_mixDevices.clear();
}
bool Mixer_Backend::openIfValid() {
bool valid = false;
......@@ -69,9 +94,10 @@ bool Mixer_Backend::openIfValid() {
// The initial state must be read manually
QTimer::singleShot( POLL_OSS_RATE_FAST, this, SLOT(readSetFromHW()) );
}
} // cold be opened
else {
close();
} // could be opened
else
{
shutdown();
}
return valid;
}
......
......@@ -41,9 +41,25 @@ protected:
Mixer_Backend(Mixer *mixer, int devnum);
virtual ~Mixer_Backend();
/// Derived classes MUST implement this to open the mixer. Returns a KMix error code (O=OK).
/**
* Derived classes MUST implement this to open the mixer.
*
* @return a KMix error code (O=OK).
*/
virtual int open() = 0;
virtual int close() = 0;
/**
* Derived classes MUST implement this to close the mixer. Do not call this directly, but use shutdown() instead.
* The method cannot be made pure virtual, as we use close() in the destructor, and C++ does not allow this.
* http://stackoverflow.com/questions/99552/where-do-pure-virtual-function-call-crashes-come-from?lq=1
*
* @return a KMix error code (O=OK).
*/
virtual int close();
/*
* Shutdown deinitializes this MixerBackend, freeing resources and calling close()
*/
virtual int shutdown();
/*
* Returns the driver name, e.g. "ALSA" or "OSS". This virtual method is for looking up the
......@@ -148,6 +164,7 @@ public slots:
protected:
QString m_mixerName;
void freeMixDevices();
protected slots:
virtual void readSetFromHW();
......
......@@ -57,7 +57,6 @@ int Mixer_MPRIS2::open()
int Mixer_MPRIS2::close()
{
m_isOpen = false;
m_mixDevices.clear();
return 0;
}
......@@ -387,8 +386,14 @@ void Mixer_MPRIS2::newMediaPlayer(QString name, QString oldOwner, QString newOwn
int lastDot = name.lastIndexOf('.');
QString id = ( lastDot == -1 ) ? name : name.mid(lastDot+1);
apps.remove(id);
shared_ptr<MixDevice> md = m_mixDevices.get(id);
kDebug() << "MixDevice 1 useCount=" << md.use_count();
md->close();
kDebug() << "MixDevice 2 useCount=" << md.use_count();
m_mixDevices.removeById(id);
kDebug() << "MixDevice 3 useCount=" << md.use_count();
notifyToReconfigureControls();
kDebug() << "MixDevice 4 useCount=" << md.use_count();
}
else
{
......
......@@ -187,7 +187,6 @@ int Mixer_OSS::close()
_pollingTimer->stop();
m_isOpen = false;
int l_i_ret = ::close(m_fd);
m_mixDevices.clear();
return l_i_ret;
}
......
......@@ -481,7 +481,6 @@ int Mixer_OSS4::close()
{
m_isOpen = false;
int l_i_ret = ::close(m_fd);
m_mixDevices.clear();
m_recommendedMaster.reset();
return l_i_ret;
}
......
......@@ -797,9 +797,15 @@ void Mixer_PULSE::removeWidget(int index)
{
if ((*iter)->id() == id)
{
// delete *iter; // TODO cesken LET-THIS-CHECK Delete should not be required after migration to shared_ptr
shared_ptr<MixDevice> md = m_mixDevices.get(id);
kDebug() << "MixDevice 1 useCount=" << md.use_count();
md->close();
kDebug() << "MixDevice 2 useCount=" << md.use_count();
m_mixDevices.erase(iter);
kDebug() << "MixDevice 3 useCount=" << md.use_count();
emitControlsReconfigured();
kDebug() << "MixDevice 4 useCount=" << md.use_count();
return;
}
}
......@@ -814,7 +820,7 @@ void Mixer_PULSE::removeAllWidgets()
if (KMIXPA_APP_PLAYBACK == m_devnum)
outputRoles.clear();
m_mixDevices.clear();
freeMixDevices();
emitControlsReconfigured();
}
......
......@@ -222,7 +222,6 @@ int Mixer_SUN::close()
_pollingTimer->stop();
m_isOpen = false;
int l_i_ret = ::close( fd );
m_mixDevices.clear();
return l_i_ret;
}
......
......@@ -138,8 +138,24 @@ void MixDevice::init( Mixer* mixer, const QString& id, const QString& name, con
// kDebug(67100) << "MixDevice::init() _id=" << _id;
}
/*
* When a MixDevice shall be finally discarded, you must use this method to free its resources.
* You must not use this MixDevice after calling close().
* <br>
* The necessity stems from a memory leak due to object cycle (MixDevice<->DBusControlWrapper), so the reference
* counting shared_ptr has no chance to clean up. See Bug 309464 for background information.
*/
void MixDevice::close()
{
delete _dbusControlWrapper;
_dbusControlWrapper = 0;
}
shared_ptr<MixDevice> MixDevice::addToPool()
{
kDebug() << "id=" << _mixer->id() << ":" << _id;
shared_ptr<MixDevice> thisSharedPtr(this);
//shared_ptr<MixDevice> thisSharedPtr = ControlPool::instance()->add(fullyQualifiedId, this);
_dbusControlWrapper = new DBusControlWrapper( thisSharedPtr, dbusPath() );
......@@ -192,7 +208,8 @@ void MixDevice::addEnums(QList<QString*>& ref_enumList)
}
MixDevice::~MixDevice() {
MixDevice::~MixDevice()
{
_enumValues.clear(); // The QString's inside will be auto-deleted, as they get unref'ed
delete _dbusControlWrapper;
}
......
......@@ -113,6 +113,8 @@ public:
MixDevice( Mixer* mixer, const QString& id, const QString& name, const QString& iconName = "", MixSet* moveDestinationMixSet = 0 );
~MixDevice();
void close();
shared_ptr<MixDevice> addToPool();
const QString& iconName() const { return _iconName; }
......
......@@ -297,7 +297,7 @@ bool Mixer::openIfValid(int cardId)
void Mixer::close()
{
if ( _mixerBackend != 0)
_mixerBackend->close();
_mixerBackend->shutdown();
}
......
......@@ -238,6 +238,7 @@ void ViewBase::resetMdws()
while (!_mdws.isEmpty())
delete _mdws.takeFirst();
// _mixSet contains shared_ptr instances, so clear() should be enough to prevent mem leak
_mixSet.clear(); // Clean up our _mixSet so we can reapply our GUIProfile
}
......
......@@ -134,9 +134,9 @@ public:
protected:
MixSet _mixSet;
QList<Mixer*> _mixers; // this might deprecate _mixer in the future. Currently only in use by ViewDockAreaPopup
QList<Mixer*> _mixers;
KMenu *_popMenu;
KActionCollection* _actions; // -<- applciations wide action collection
KActionCollection* _actions; // -<- application wide action collection
ViewFlags _vflags;
const QString _guiProfileId;
......
......@@ -147,7 +147,7 @@ void ViewDockAreaPopup::_setMixSet()
if ( optionsLayout != 0 )
{
QLayoutItem *li2;
while ( ( li2 = optionsLayout->takeAt(0) ) ) // strangely enough, it crashes here
while ( ( li2 = optionsLayout->takeAt(0) ) )
delete li2;
}
......@@ -202,20 +202,17 @@ Application: KMix (kmix), signal: Segmentation fault
* BKO 299754: Looks like I need to explicitly delete layout(). I have no idea why
* "delete _layoutMDW" is not enough, as that is supposed to be the layout
* of this ViewDockAreaPopup
* (Hint: it might have been 0 already)
*/
kDebug() << "2 layout()=" << layout() << ", _layoutMDW=" << _layoutMDW;
delete layout(); // BKO 299754
kDebug() << "3 layout()=" << layout() << ", _layoutMDW=" << _layoutMDW;
// delete _layoutMDW;
_layoutMDW = new QGridLayout(this);
kDebug() << "4 layout()=" << layout() << ", _layoutMDW=" << _layoutMDW;
_layoutMDW->setSpacing(KDialog::spacingHint());
_layoutMDW->setMargin(0);
//_layoutMDW->setSizeConstraint(QLayout::SetMinimumSize);
_layoutMDW->setSizeConstraint(QLayout::SetMaximumSize);
_layoutMDW->setObjectName(QLatin1String("KmixPopupLayout"));
setLayout(_layoutMDW);
kDebug() << "5 layout()=" << layout() << ", _layoutMDW=" << _layoutMDW;
// Adding all mixers, as we potentially want to show all master controls. Due to hotplugging
// we have to redo the list on each _setMixSet() (instead of setting it once in the Constructor)
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment