From d9505e796d61bee08060a2a7166bde80a3388260 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sun, 5 Jul 2020 15:03:33 +0200 Subject: Disallow copy/move for ControlObject --- src/control/controlobject.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/control/controlobject.h b/src/control/controlobject.h index 56af1aefd5..a3e247f846 100644 --- a/src/control/controlobject.h +++ b/src/control/controlobject.h @@ -180,6 +180,11 @@ class ControlObject : public QObject { void readOnlyHandler(double v); private: + ControlObject(ControlObject&&) = delete; + ControlObject(const ControlObject&) = delete; + ControlObject& operator=(ControlObject&&) = delete; + ControlObject& operator=(const ControlObject&) = delete; + inline bool ignoreNops() const { return m_pControl ? m_pControl->ignoreNops() : true; } -- cgit v1.2.3 From 042d2ebf43184096fcac245cb847015e8025ee88 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sun, 5 Jul 2020 16:57:35 +0200 Subject: Fix creation and management of ControlObject instances --- src/control/control.cpp | 60 +++++++++++++++++++++++++++++-------------------- 1 file changed, 36 insertions(+), 24 deletions(-) diff --git a/src/control/control.cpp b/src/control/control.cpp index d46fcb1d78..16d0ea3d21 100644 --- a/src/control/control.cpp +++ b/src/control/control.cpp @@ -112,39 +112,51 @@ QSharedPointer ControlDoublePrivate::getControl( VERIFY_OR_DEBUG_ASSERT(!key.isEmpty()) { qWarning() << "ControlDoublePrivate::getControl returning NULL" << "for empty ConfigKey."; - return QSharedPointer(); + return nullptr; } - QSharedPointer pControl; - // Scope for MMutexLocker. { - MMutexLocker locker(&s_qCOHashMutex); - auto it = s_qCOHash.constFind(key); - if (it != s_qCOHash.constEnd()) { - if (pCreatorCO) { - qWarning() << "ControlObject" << key.group << key.item << "already created"; - DEBUG_ASSERT(!"ControlObject already created"); + const MMutexLocker locker(&s_qCOHashMutex); + const auto it = s_qCOHash.find(key); + if (it != s_qCOHash.end()) { + auto pControl = it.value().lock(); + if (pControl) { + // Control object already exists + VERIFY_OR_DEBUG_ASSERT(!pCreatorCO) { + qWarning() + << "ControlObject" + << key.group << key.item + << "already created"; + return nullptr; + } + return pControl; } else { - pControl = it.value(); + // The weak pointer has become invalid and can be cleaned up + s_qCOHash.erase(it); } } } - if (pControl == NULL) { - if (pCreatorCO) { - pControl = QSharedPointer( - new ControlDoublePrivate(key, pCreatorCO, bIgnoreNops, - bTrack, bPersist, defaultValue)); - MMutexLocker locker(&s_qCOHashMutex); - //qDebug() << "ControlDoublePrivate::s_qCOHash.insert(" << key.group << "," << key.item << ")"; - s_qCOHash.insert(key, pControl); - } else if (!flags.testFlag(ControlFlag::NoWarnIfMissing)) { - qWarning() << "ControlDoublePrivate::getControl returning NULL for (" - << key.group << "," << key.item << ")"; - DEBUG_ASSERT(flags.testFlag(ControlFlag::NoAssertIfMissing)); - } + if (pCreatorCO) { + auto pControl = QSharedPointer( + new ControlDoublePrivate(key, + pCreatorCO, + bIgnoreNops, + bTrack, + bPersist, + defaultValue)); + const MMutexLocker locker(&s_qCOHashMutex); + //qDebug() << "ControlDoublePrivate::s_qCOHash.insert(" << key.group << "," << key.item << ")"; + s_qCOHash.insert(key, pControl); + return pControl; + } + + if (!flags.testFlag(ControlFlag::NoWarnIfMissing)) { + qWarning() << "ControlDoublePrivate::getControl returning NULL for (" + << key.group << "," << key.item << ")"; + DEBUG_ASSERT(flags.testFlag(ControlFlag::NoAssertIfMissing)); } - return pControl; + return nullptr; } // static -- cgit v1.2.3 From 55f19fffc08e369d3c49fa298048e0726bc3a854 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Mon, 6 Jul 2020 00:29:49 +0200 Subject: Make CO creator pointer management less thread-unsafe --- src/control/control.h | 10 +++++----- src/control/controlobject.cpp | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/control/control.h b/src/control/control.h index 56df7d5ae2..b3dcb0ef1e 100644 --- a/src/control/control.h +++ b/src/control/control.h @@ -110,12 +110,12 @@ class ControlDoublePrivate : public QObject { return m_defaultValue.getValue(); } - inline ControlObject* getCreatorCO() const { - return m_pCreatorCO; + ControlObject* getCreatorCO() const { + return m_pCreatorCO.loadAcquire(); } - inline void removeCreatorCO() { - m_pCreatorCO = NULL; + bool resetCreatorCO(ControlObject* pCreatorCO) { + return m_pCreatorCO.testAndSetOrdered(pCreatorCO, nullptr); } inline ConfigKey getKey() { @@ -180,7 +180,7 @@ class ControlDoublePrivate : public QObject { QSharedPointer m_pBehavior; - ControlObject* m_pCreatorCO; + QAtomicPointer m_pCreatorCO; // Hack to implement persistent controls. This is a pointer to the current // user configuration object (if one exists). In general, we do not want the diff --git a/src/control/controlobject.cpp b/src/control/controlobject.cpp index 2de1c38683..65d66a3647 100644 --- a/src/control/controlobject.cpp +++ b/src/control/controlobject.cpp @@ -54,7 +54,7 @@ ControlObject::ControlObject(ConfigKey key, bool bIgnoreNops, bool bTrack, ControlObject::~ControlObject() { if (m_pControl) { - m_pControl->removeCreatorCO(); + m_pControl->resetCreatorCO(this); } } -- cgit v1.2.3 From 602c0af806b0fc5ac741a32d93b4753118a2b233 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Mon, 6 Jul 2020 00:53:51 +0200 Subject: Fix cleanup of leaked controls on exit and in tests --- src/control/control.cpp | 14 +++++++++----- src/control/control.h | 4 +++- src/mixxx.cpp | 2 +- src/test/mixxxtest.cpp | 8 ++------ 4 files changed, 15 insertions(+), 13 deletions(-) diff --git a/src/control/control.cpp b/src/control/control.cpp index 16d0ea3d21..fb1c1cba92 100644 --- a/src/control/control.cpp +++ b/src/control/control.cpp @@ -161,16 +161,20 @@ QSharedPointer ControlDoublePrivate::getControl( // static void ControlDoublePrivate::getControls( - QList >* pControlList) { - s_qCOHashMutex.lock(); + QList >* pControlList, + bool detachLeakedControls) { pControlList->clear(); + MMutexLocker locker(&s_qCOHashMutex); + pControlList->reserve(s_qCOHash.size()); for (auto it = s_qCOHash.constBegin(); it != s_qCOHash.constEnd(); ++it) { - QSharedPointer pControl = it.value(); - if (!pControl.isNull()) { + auto pControl = it.value().lock(); + if (pControl) { pControlList->push_back(pControl); } } - s_qCOHashMutex.unlock(); + if (detachLeakedControls) { + s_qCOHash.clear(); + } } // static diff --git a/src/control/control.h b/src/control/control.h index b3dcb0ef1e..d0c83fce83 100644 --- a/src/control/control.h +++ b/src/control/control.h @@ -52,7 +52,9 @@ class ControlDoublePrivate : public QObject { double defaultValue = 0.0); // Adds all ControlDoublePrivate that currently exist to pControlList - static void getControls(QList >* pControlsList); + static void getControls( + QList >* pControlsList, + bool detachLeakedControls = false); static QHash getControlAliases(); diff --git a/src/mixxx.cpp b/src/mixxx.cpp index 0fd9dde2a6..892efca386 100644 --- a/src/mixxx.cpp +++ b/src/mixxx.cpp @@ -781,7 +781,7 @@ void MixxxMainWindow::finalize() { QList > leakedControls; QList leakedConfigKeys; - ControlDoublePrivate::getControls(&leakedControls); + ControlDoublePrivate::getControls(&leakedControls, true); if (leakedControls.size() > 0) { qDebug() << "WARNING: The following" << leakedControls.size() diff --git a/src/test/mixxxtest.cpp b/src/test/mixxxtest.cpp index f70495e225..cf8acc7b06 100644 --- a/src/test/mixxxtest.cpp +++ b/src/test/mixxxtest.cpp @@ -46,12 +46,8 @@ MixxxTest::~MixxxTest() { // Mixxx leaks a ton of COs normally. To make new tests not affected by // previous tests, we clear our all COs after every MixxxTest completion. QList> leakedControls; - ControlDoublePrivate::getControls(&leakedControls); - foreach (QSharedPointer pCDP, leakedControls) { - if (pCDP.isNull()) { - continue; - } - ConfigKey key = pCDP->getKey(); + ControlDoublePrivate::getControls(&leakedControls, true); + for (auto pCDP : qAsConst(leakedControls)) { delete pCDP->getCreatorCO(); } } -- cgit v1.2.3 From c69db9510dc17c4c8beabe5264d791e5cb5f09ac Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Mon, 6 Jul 2020 09:05:08 +0200 Subject: Controls: Sanitize code, reorder members, clarify comments --- src/control/control.cpp | 45 ++++++++++++++-------------------- src/control/control.h | 65 ++++++++++++++++++++++++++----------------------- 2 files changed, 53 insertions(+), 57 deletions(-) diff --git a/src/control/control.cpp b/src/control/control.cpp index fb1c1cba92..ea563524c0 100644 --- a/src/control/control.cpp +++ b/src/control/control.cpp @@ -1,46 +1,37 @@ -#include -#include - #include "control/control.h" #include "util/stat.h" -// Static member variable definition +//static UserSettingsPointer ControlDoublePrivate::s_pUserConfig; -QHash > ControlDoublePrivate::s_qCOHash -GUARDED_BY(ControlDoublePrivate::s_qCOHashMutex); +//static +QHash> ControlDoublePrivate::s_qCOHash + GUARDED_BY(ControlDoublePrivate::s_qCOHashMutex); +//static QHash ControlDoublePrivate::s_qCOAliasHash -GUARDED_BY(ControlDoublePrivate::s_qCOHashMutex); + GUARDED_BY(ControlDoublePrivate::s_qCOHashMutex); +//static MMutex ControlDoublePrivate::s_qCOHashMutex; -/* -ControlDoublePrivate::ControlDoublePrivate() - : m_bIgnoreNops(true), - m_bTrack(false), - m_trackType(Stat::UNSPECIFIED), - m_trackFlags(Stat::COUNT | Stat::SUM | Stat::AVERAGE | - Stat::SAMPLE_VARIANCE | Stat::MIN | Stat::MAX), - m_confirmRequired(false) { - initialize(); -} -*/ - -ControlDoublePrivate::ControlDoublePrivate(ConfigKey key, - ControlObject* pCreatorCO, - bool bIgnoreNops, bool bTrack, - bool bPersist, double defaultValue) +ControlDoublePrivate::ControlDoublePrivate( + ConfigKey key, + ControlObject* pCreatorCO, + bool bIgnoreNops, + bool bTrack, + bool bPersist, + double defaultValue) : m_key(key), + m_pCreatorCO(pCreatorCO), m_bPersistInConfiguration(bPersist), m_bIgnoreNops(bIgnoreNops), m_bTrack(bTrack), m_trackType(Stat::UNSPECIFIED), m_trackFlags(Stat::COUNT | Stat::SUM | Stat::AVERAGE | - Stat::SAMPLE_VARIANCE | Stat::MIN | Stat::MAX), - m_confirmRequired(false), - m_pCreatorCO(pCreatorCO) { + Stat::SAMPLE_VARIANCE | Stat::MIN | Stat::MAX), + m_confirmRequired(false) { initialize(defaultValue); } @@ -161,7 +152,7 @@ QSharedPointer ControlDoublePrivate::getControl( // static void ControlDoublePrivate::getControls( - QList >* pControlList, + QList>* pControlList, bool detachLeakedControls) { pControlList->clear(); MMutexLocker locker(&s_qCOHashMutex); diff --git a/src/control/control.h b/src/control/control.h index d0c83fce83..229323098f 100644 --- a/src/control/control.h +++ b/src/control/control.h @@ -1,10 +1,10 @@ -#ifndef CONTROL_H -#define CONTROL_H +#pragma once +#include #include -#include #include -#include +#include +#include #include "control/controlbehavior.h" #include "control/controlvalue.h" @@ -26,7 +26,7 @@ Q_DECLARE_OPERATORS_FOR_FLAGS(ControlFlags) class ControlDoublePrivate : public QObject { Q_OBJECT public: - virtual ~ControlDoublePrivate(); + ~ControlDoublePrivate() override; // Used to implement control persistence. All controls that are marked // "persist in user config" get and set their value on creation/deletion @@ -43,9 +43,10 @@ class ControlDoublePrivate : public QObject { // Gets the ControlDoublePrivate matching the given ConfigKey. If pCreatorCO // is non-NULL, allocates a new ControlDoublePrivate for the ConfigKey if // one does not exist. - static QSharedPointer getControl(const ConfigKey& key, + static QSharedPointer getControl( + const ConfigKey& key, ControlFlags flags = ControlFlag::None, - ControlObject* pCreatorCO = NULL, + ControlObject* pCreatorCO = nullptr, bool bIgnoreNops = true, bool bTrack = false, bool bPersist = false, @@ -80,7 +81,7 @@ class ControlDoublePrivate : public QObject { // ValueChangeRequest slot. void setAndConfirm(double value, QObject* pSender); // Gets the control value. - inline double get() const { + double get() const { return m_value.getValue(); } // Resets the control value to its default. @@ -88,8 +89,10 @@ class ControlDoublePrivate : public QObject { // Set the behavior to be used when setting values and translating between // parameter and value space. Returns the previously set behavior (if any). - // The caller must not delete the behavior at any time. The memory is managed - // by this function. + // Callers must allocate the passed behavior using new and ownership to this + // memory is passed with the function call!! + // TODO: Pass a std::unique_ptr instead of a plain pointer to ensure this + // transfer of ownership. void setBehavior(ControlNumericBehavior* pBehavior); void setParameter(double dParam, QObject* pSender); @@ -100,15 +103,15 @@ class ControlDoublePrivate : public QObject { void setValueFromMidi(MidiOpCode opcode, double dParam); double getMidiParameter() const; - inline bool ignoreNops() const { + bool ignoreNops() const { return m_bIgnoreNops; } - inline void setDefaultValue(double dValue) { + void setDefaultValue(double dValue) { m_defaultValue.setValue(dValue); } - inline double defaultValue() const { + double defaultValue() const { return m_defaultValue.getValue(); } @@ -120,7 +123,7 @@ class ControlDoublePrivate : public QObject { return m_pCreatorCO.testAndSetOrdered(pCreatorCO, nullptr); } - inline ConfigKey getKey() { + ConfigKey getKey() { return m_key; } @@ -146,25 +149,25 @@ class ControlDoublePrivate : public QObject { void valueChangeRequest(double value); private: - ControlDoublePrivate(ConfigKey key, ControlObject* pCreatorCO, - bool bIgnoreNops, bool bTrack, bool bPersist, - double defaultValue); + ControlDoublePrivate( + ConfigKey key, + ControlObject* pCreatorCO, + bool bIgnoreNops, + bool bTrack, + bool bPersist, + double defaultValue); void initialize(double defaultValue); void setInner(double value, QObject* pSender); - ConfigKey m_key; + const ConfigKey m_key; + + QAtomicPointer m_pCreatorCO; // Whether the control should persist in the Mixxx user configuration. The // value is loaded from configuration when the control is created and // written to the configuration when the control is deleted. bool m_bPersistInConfiguration; - // User-visible, i18n name for what the control is. - QString m_name; - - // User-visible, i18n description for what the control does. - QString m_description; - // Whether to ignore sets which would have no effect. bool m_bIgnoreNops; @@ -175,6 +178,12 @@ class ControlDoublePrivate : public QObject { int m_trackFlags; bool m_confirmRequired; + // User-visible, i18n name for what the control is. + QString m_name; + + // User-visible, i18n description for what the control does. + QString m_description; + // The control value. ControlValueAtomic m_value; // The default control value. @@ -182,8 +191,6 @@ class ControlDoublePrivate : public QObject { QSharedPointer m_pBehavior; - QAtomicPointer m_pCreatorCO; - // Hack to implement persistent controls. This is a pointer to the current // user configuration object (if one exists). In general, we do not want the // user configuration to be a singleton -- objects that need access to it @@ -193,7 +200,8 @@ class ControlDoublePrivate : public QObject { static UserSettingsPointer s_pUserConfig; // Hash of ControlDoublePrivate instantiations. - static QHash > s_qCOHash; + static QHash> s_qCOHash; + // Hash of aliases between ConfigKeys. Solely used for looking up the first // alias associated with a key. static QHash s_qCOAliasHash; @@ -201,6 +209,3 @@ class ControlDoublePrivate : public QObject { // Mutex guarding access to s_qCOHash and s_qCOAliasHash. static MMutex s_qCOHashMutex; }; - - -#endif /* CONTROL_H */ -- cgit v1.2.3 From e001852ac45ecfed0163ad1ce235bef10168b039 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Mon, 6 Jul 2020 09:05:29 +0200 Subject: ControlDoublePrivate: Disallow copy/move --- src/control/control.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/control/control.h b/src/control/control.h index 229323098f..d3ec3597f6 100644 --- a/src/control/control.h +++ b/src/control/control.h @@ -156,6 +156,11 @@ class ControlDoublePrivate : public QObject { bool bTrack, bool bPersist, double defaultValue); + ControlDoublePrivate(ControlDoublePrivate&&) = delete; + ControlDoublePrivate(const ControlDoublePrivate&) = delete; + ControlDoublePrivate& operator=(ControlDoublePrivate&&) = delete; + ControlDoublePrivate& operator=(const ControlDoublePrivate&) = delete; + void initialize(double defaultValue); void setInner(double value, QObject* pSender); -- cgit v1.2.3 From c344bcd25715bfbd27323daa969cdc86a1d515d8 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Mon, 6 Jul 2020 09:06:00 +0200 Subject: ControlDoublePrivate: Avoid unnecessary locking --- src/control/control.cpp | 6 ------ src/control/control.h | 5 +++++ 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/control/control.cpp b/src/control/control.cpp index ea563524c0..4dcaf0dcd8 100644 --- a/src/control/control.cpp +++ b/src/control/control.cpp @@ -168,12 +168,6 @@ void ControlDoublePrivate::getControls( } } -// static -QHash ControlDoublePrivate::getControlAliases() { - MMutexLocker locker(&s_qCOHashMutex); - return s_qCOAliasHash; -} - void ControlDoublePrivate::reset() { double defaultValue = m_defaultValue.getValue(); // NOTE: pSender = NULL is important. The originator of this action does diff --git a/src/control/control.h b/src/control/control.h index d3ec3597f6..ccf7cb9826 100644 --- a/src/control/control.h +++ b/src/control/control.h @@ -59,6 +59,11 @@ class ControlDoublePrivate : public QObject { static QHash getControlAliases(); + static QHash getControlAliases() { + // Implicitly shared classes can safely be copied across threads + return s_qCOAliasHash; + } + const QString& name() const { return m_name; } -- cgit v1.2.3 From d09f936b3756a2d3d8ea80aa9080b94a8a1e309e Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Mon, 6 Jul 2020 09:18:02 +0200 Subject: Re-add old comment --- src/control/control.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/control/control.cpp b/src/control/control.cpp index 4dcaf0dcd8..25c6552553 100644 --- a/src/control/control.cpp +++ b/src/control/control.cpp @@ -106,6 +106,7 @@ QSharedPointer ControlDoublePrivate::getControl( return nullptr; } + // Scope for MMutexLocker. { const MMutexLocker locker(&s_qCOHashMutex); const auto it = s_qCOHash.find(key); -- cgit v1.2.3 From da412742d25187394048129cb1fcd4176f3ab42a Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Mon, 6 Jul 2020 09:19:16 +0200 Subject: Add a debug assertion --- src/control/controlobject.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/control/controlobject.cpp b/src/control/controlobject.cpp index 65d66a3647..988681cc50 100644 --- a/src/control/controlobject.cpp +++ b/src/control/controlobject.cpp @@ -54,7 +54,9 @@ ControlObject::ControlObject(ConfigKey key, bool bIgnoreNops, bool bTrack, ControlObject::~ControlObject() { if (m_pControl) { - m_pControl->resetCreatorCO(this); + const bool success = m_pControl->resetCreatorCO(this); + Q_UNUSED(success); + DEBUG_ASSERT(success); } } -- cgit v1.2.3 From 3b05a477824ec58864ab656d1d56d17ec78021d0 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Mon, 6 Jul 2020 09:23:53 +0200 Subject: Make deletion of creator COs atomic --- src/control/control.cpp | 5 +++++ src/control/control.h | 1 + src/test/mixxxtest.cpp | 6 ++---- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/control/control.cpp b/src/control/control.cpp index 25c6552553..0e3a2aceed 100644 --- a/src/control/control.cpp +++ b/src/control/control.cpp @@ -1,5 +1,6 @@ #include "control/control.h" +#include "control/controlobject.h" #include "util/stat.h" //static @@ -169,6 +170,10 @@ void ControlDoublePrivate::getControls( } } +void ControlDoublePrivate::deleteCreatorCO() { + delete m_pCreatorCO.fetchAndStoreOrdered(nullptr); +} + void ControlDoublePrivate::reset() { double defaultValue = m_defaultValue.getValue(); // NOTE: pSender = NULL is important. The originator of this action does diff --git a/src/control/control.h b/src/control/control.h index ccf7cb9826..96b9ddada3 100644 --- a/src/control/control.h +++ b/src/control/control.h @@ -127,6 +127,7 @@ class ControlDoublePrivate : public QObject { bool resetCreatorCO(ControlObject* pCreatorCO) { return m_pCreatorCO.testAndSetOrdered(pCreatorCO, nullptr); } + void deleteCreatorCO(); ConfigKey getKey() { return m_key; diff --git a/src/test/mixxxtest.cpp b/src/test/mixxxtest.cpp index cf8acc7b06..a57debe7c7 100644 --- a/src/test/mixxxtest.cpp +++ b/src/test/mixxxtest.cpp @@ -45,9 +45,7 @@ MixxxTest::MixxxTest() { MixxxTest::~MixxxTest() { // Mixxx leaks a ton of COs normally. To make new tests not affected by // previous tests, we clear our all COs after every MixxxTest completion. - QList> leakedControls; - ControlDoublePrivate::getControls(&leakedControls, true); - for (auto pCDP : qAsConst(leakedControls)) { - delete pCDP->getCreatorCO(); + for (auto pControl : ControlDoublePrivate::takeAllInstances()) { + pControl->deleteCreatorCO(); } } -- cgit v1.2.3 From f1b1cdc655fc693c8b643ef0948d7991c43faccd Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Mon, 6 Jul 2020 09:25:04 +0200 Subject: Split get/take of existing/leaked controls into separate functions --- src/control/control.cpp | 32 +++++++++++++++++++++++--------- src/control/control.h | 10 ++++------ src/dialog/dlgdevelopertools.cpp | 10 +++++----- src/mixxx.cpp | 11 ++++------- 4 files changed, 36 insertions(+), 27 deletions(-) diff --git a/src/control/control.cpp b/src/control/control.cpp index 0e3a2aceed..99f72ce37f 100644 --- a/src/control/control.cpp +++ b/src/control/control.cpp @@ -153,21 +153,35 @@ QSharedPointer ControlDoublePrivate::getControl( } // static -void ControlDoublePrivate::getControls( - QList>* pControlList, - bool detachLeakedControls) { - pControlList->clear(); +QList> ControlDoublePrivate::getAllInstances() { + QList> result; MMutexLocker locker(&s_qCOHashMutex); - pControlList->reserve(s_qCOHash.size()); - for (auto it = s_qCOHash.constBegin(); it != s_qCOHash.constEnd(); ++it) { + result.reserve(s_qCOHash.size()); + for (auto it = s_qCOHash.begin(); it != s_qCOHash.end(); ++it) { auto pControl = it.value().lock(); if (pControl) { - pControlList->push_back(pControl); + result.append(std::move(pControl)); + } else { + // The weak pointer has become invalid and can be cleaned up + s_qCOHash.erase(it); } } - if (detachLeakedControls) { - s_qCOHash.clear(); + return result; +} + +// static +QList> ControlDoublePrivate::takeAllInstances() { + QList> result; + MMutexLocker locker(&s_qCOHashMutex); + result.reserve(s_qCOHash.size()); + for (auto it = s_qCOHash.begin(); it != s_qCOHash.end(); ++it) { + auto pControl = it.value().lock(); + if (pControl) { + result.append(std::move(pControl)); + } } + s_qCOHash.clear(); + return result; } void ControlDoublePrivate::deleteCreatorCO() { diff --git a/src/control/control.h b/src/control/control.h index 96b9ddada3..5ff49bcd4b 100644 --- a/src/control/control.h +++ b/src/control/control.h @@ -52,12 +52,10 @@ class ControlDoublePrivate : public QObject { bool bPersist = false, double defaultValue = 0.0); - // Adds all ControlDoublePrivate that currently exist to pControlList - static void getControls( - QList >* pControlsList, - bool detachLeakedControls = false); - - static QHash getControlAliases(); + // Returns a list of all existing instances. + static QList> getAllInstances(); + // Clears all existing instances and returns them as a list. + static QList> takeAllInstances(); static QHash getControlAliases() { // Implicitly shared classes can safely be copied across threads diff --git a/src/dialog/dlgdevelopertools.cpp b/src/dialog/dlgdevelopertools.cpp index 4642c649cc..9e5b462e1c 100644 --- a/src/dialog/dlgdevelopertools.cpp +++ b/src/dialog/dlgdevelopertools.cpp @@ -13,9 +13,9 @@ DlgDeveloperTools::DlgDeveloperTools(QWidget* pParent, m_pConfig(pConfig) { setupUi(this); - QList > controlsList; - ControlDoublePrivate::getControls(&controlsList); - QHash controlAliases = + const QList> controlsList = + ControlDoublePrivate::getAllInstances(); + const QHash controlAliases = ControlDoublePrivate::getControlAliases(); for (auto it = controlsList.constBegin(); @@ -144,8 +144,8 @@ void DlgDeveloperTools::slotControlDump() { return; } - QList > controlsList; - ControlDoublePrivate::getControls(&controlsList); + const QList> controlsList = + ControlDoublePrivate::getAllInstances(); for (auto it = controlsList.constBegin(); it != controlsList.constEnd(); ++it) { const QSharedPointer& pControl = *it; if (pControl) { diff --git a/src/mixxx.cpp b/src/mixxx.cpp index 892efca386..e9fcc6647c 100644 --- a/src/mixxx.cpp +++ b/src/mixxx.cpp @@ -777,19 +777,16 @@ void MixxxMainWindow::finalize() { delete m_pGuiTick; delete m_pVisualsManager; - // Check for leaked ControlObjects and give warnings. - QList > leakedControls; QList leakedConfigKeys; - ControlDoublePrivate::getControls(&leakedControls, true); - + // Check for leaked ControlObjects and give warnings. + QList> leakedControls = + ControlDoublePrivate::takeAllInstances(); if (leakedControls.size() > 0) { qDebug() << "WARNING: The following" << leakedControls.size() << "controls were leaked:"; + leakedConfigKeys.reserve(leakedControls.size()); foreach (QSharedPointer pCDP, leakedControls) { - if (pCDP.isNull()) { - continue; - } ConfigKey key = pCDP->getKey(); qDebug() << key.group << key.item << pCDP->getCreatorCO(); leakedConfigKeys.append(key); -- cgit v1.2.3 From 75c6c6841ca49ce10e1e50e54ce0da19b46c3922 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Tue, 7 Jul 2020 16:37:55 +0200 Subject: Fix leaked controls on shutdown --- src/mixxx.cpp | 59 ++++++++++++++-------------------- src/mixxx.h | 9 +++--- src/preferences/dialog/dlgprefdeck.cpp | 43 +++++++++++++------------ src/preferences/dialog/dlgprefdeck.h | 26 ++++++++------- 4 files changed, 67 insertions(+), 70 deletions(-) diff --git a/src/mixxx.cpp b/src/mixxx.cpp index e9fcc6647c..6503f44db3 100644 --- a/src/mixxx.cpp +++ b/src/mixxx.cpp @@ -177,7 +177,7 @@ MixxxMainWindow::MixxxMainWindow(QApplication* pApp, const CmdlineArgs& args) StatsManager::createInstance(); } - m_pSettingsManager = new SettingsManager(this, args.getSettingsPath()); + m_pSettingsManager = make_parented(this, args.getSettingsPath()); initializeKeyboard(); @@ -777,39 +777,6 @@ void MixxxMainWindow::finalize() { delete m_pGuiTick; delete m_pVisualsManager; - QList leakedConfigKeys; - - // Check for leaked ControlObjects and give warnings. - QList> leakedControls = - ControlDoublePrivate::takeAllInstances(); - if (leakedControls.size() > 0) { - qDebug() << "WARNING: The following" << leakedControls.size() - << "controls were leaked:"; - leakedConfigKeys.reserve(leakedControls.size()); - foreach (QSharedPointer pCDP, leakedControls) { - ConfigKey key = pCDP->getKey(); - qDebug() << key.group << key.item << pCDP->getCreatorCO(); - leakedConfigKeys.append(key); - } - - // Deleting leaked objects helps to satisfy valgrind. - // These delete calls could cause crashes if a destructor for a control - // we thought was leaked is triggered after this one exits. - // So, only delete so if developer mode is on. - if (CmdlineArgs::Instance().getDeveloper()) { - foreach (ConfigKey key, leakedConfigKeys) { - // A deletion early in the list may trigger a destructor - // for a control later in the list, so we check for a null - // pointer each time. - ControlObject* pCo = ControlObject::getControl(key, ControlFlag::NoAssertIfMissing); - if (pCo) { - delete pCo; - } - } - } - leakedControls.clear(); - } - // Delete the track collections after all internal track pointers // in other components have been released by deleting those components // beforehand! @@ -826,6 +793,30 @@ void MixxxMainWindow::finalize() { // at exit. m_pSettingsManager->save(); + // Check for leaked ControlObjects and give warnings. + { + QList> leakedControls = + ControlDoublePrivate::takeAllInstances(); + VERIFY_OR_DEBUG_ASSERT(leakedControls.isEmpty()) { + qWarning() + << "The following" + << leakedControls.size() + << "controls were leaked:"; + for (auto pCDP : leakedControls) { + ConfigKey key = pCDP->getKey(); + qWarning() << key.group << key.item << pCDP->getCreatorCO(); + // Deleting leaked objects helps to satisfy valgrind. + // These delete calls could cause crashes if a destructor for a control + // we thought was leaked is triggered after this one exits. + // So, only delete so if developer mode is on. + if (CmdlineArgs::Instance().getDeveloper()) { + pCDP->deleteCreatorCO(); + } + } + } + // Finally drop all shared pointers by exiting this scope + } + Sandbox::shutdown(); qDebug() << t.elapsed(false).debugMillisWithUnit() << "deleting SettingsManager"; diff --git a/src/mixxx.h b/src/mixxx.h index 765a7c065c..387a2909b3 100644 --- a/src/mixxx.h +++ b/src/mixxx.h @@ -23,13 +23,14 @@ #include #include "preferences/configobject.h" -#include "preferences/usersettings.h" #include "preferences/constants.h" +#include "preferences/usersettings.h" +#include "soundio/sounddeviceerror.h" #include "track/track.h" #include "util/cmdlineargs.h" -#include "util/timer.h" #include "util/db/dbconnectionpool.h" -#include "soundio/sounddeviceerror.h" +#include "util/parented_ptr.h" +#include "util/timer.h" class ChannelHandleFactory; class ControlPushButton; @@ -142,7 +143,7 @@ class MixxxMainWindow : public QMainWindow { QWidget* m_pWidgetParent; LaunchImage* m_pLaunchImage; - SettingsManager* m_pSettingsManager; + parented_ptr m_pSettingsManager; // The effects processing system EffectsManager* m_pEffectsManager; diff --git a/src/preferences/dialog/dlgprefdeck.cpp b/src/preferences/dialog/dlgprefdeck.cpp index 9ce38c8158..79799bd4ad 100644 --- a/src/preferences/dialog/dlgprefdeck.cpp +++ b/src/preferences/dialog/dlgprefdeck.cpp @@ -34,22 +34,29 @@ constexpr int kDefaultRateRampSensitivity = 250; // to playermanager.cpp } -DlgPrefDeck::DlgPrefDeck(QWidget * parent, MixxxMainWindow * mixxx, - PlayerManager* pPlayerManager, - UserSettingsPointer pConfig) - : DlgPreferencePage(parent), - m_pConfig(pConfig), - m_mixxx(mixxx), - m_pPlayerManager(pPlayerManager), - m_iNumConfiguredDecks(0), - m_iNumConfiguredSamplers(0) { +DlgPrefDeck::DlgPrefDeck(QWidget* parent, + MixxxMainWindow* mixxx, + PlayerManager* pPlayerManager, + UserSettingsPointer pConfig) + : DlgPreferencePage(parent), + m_mixxx(mixxx), + m_pPlayerManager(pPlayerManager), + m_pConfig(pConfig), + m_pControlTrackTimeDisplay(std::make_unique( + ConfigKey("[Controls]", "ShowDurationRemaining"))), + m_pControlTrackTimeFormat(std::make_unique( + ConfigKey("[Controls]", "TimeFormat"))), + m_pNumDecks( + make_parented("[Master]", "num_decks", this)), + m_pNumSamplers(make_parented( + "[Master]", "num_samplers", this)), + m_iNumConfiguredDecks(0), + m_iNumConfiguredSamplers(0) { setupUi(this); - m_pNumDecks = new ControlProxy("[Master]", "num_decks", this); m_pNumDecks->connectValueChanged(this, [=](double value){slotNumDecksChanged(value);}); slotNumDecksChanged(m_pNumDecks->get(), true); - m_pNumSamplers = new ControlProxy("[Master]", "num_samplers", this); m_pNumSamplers->connectValueChanged(this, [=](double value){slotNumSamplersChanged(value);}); slotNumSamplersChanged(m_pNumSamplers->get(), true); @@ -74,10 +81,10 @@ DlgPrefDeck::DlgPrefDeck(QWidget * parent, MixxxMainWindow * mixxx, connect(ComboBoxCueMode, SIGNAL(activated(int)), this, SLOT(slotCueModeCombobox(int))); // Track time display configuration - m_pControlTrackTimeDisplay = new ControlObject( - ConfigKey("[Controls]", "ShowDurationRemaining")); - connect(m_pControlTrackTimeDisplay, SIGNAL(valueChanged(double)), - this, SLOT(slotSetTrackTimeDisplay(double))); + connect(m_pControlTrackTimeDisplay.get(), + &ControlObject::valueChanged, + this, + QOverload::of(&DlgPrefDeck::slotSetTrackTimeDisplay)); double positionDisplayType = m_pConfig->getValue( ConfigKey("[Controls]", "PositionDisplay"), @@ -101,10 +108,7 @@ DlgPrefDeck::DlgPrefDeck(QWidget * parent, MixxxMainWindow * mixxx, this, SLOT(slotSetTrackTimeDisplay(QAbstractButton *))); // display time format - - m_pControlTrackTimeFormat = new ControlObject( - ConfigKey("[Controls]", "TimeFormat")); - connect(m_pControlTrackTimeFormat, + connect(m_pControlTrackTimeFormat.get(), &ControlObject::valueChanged, this, &DlgPrefDeck::slotTimeFormatChanged); @@ -351,7 +355,6 @@ DlgPrefDeck::DlgPrefDeck(QWidget * parent, MixxxMainWindow * mixxx, } DlgPrefDeck::~DlgPrefDeck() { - delete m_pControlTrackTimeDisplay; qDeleteAll(m_rateControls); qDeleteAll(m_rateDirectionControls); qDeleteAll(m_cueControls); diff --git a/src/preferences/dialog/dlgprefdeck.h b/src/preferences/dialog/dlgprefdeck.h index 41788ab9cf..dde7bff85a 100644 --- a/src/preferences/dialog/dlgprefdeck.h +++ b/src/preferences/dialog/dlgprefdeck.h @@ -1,7 +1,7 @@ -#ifndef DLGPREFDECK_H -#define DLGPREFDECK_H +#pragma once #include +#include #include "engine/controls/cuecontrol.h" #include "engine/controls/ratecontrol.h" @@ -9,6 +9,7 @@ #include "preferences/dialog/ui_dlgprefdeckdlg.h" #include "preferences/dlgpreferencepage.h" #include "preferences/usersettings.h" +#include "util/parented_ptr.h" class ControlProxy; class ControlPotmeter; @@ -58,7 +59,7 @@ class DlgPrefDeck : public DlgPreferencePage, public Ui::DlgPrefDeckDlg { DlgPrefDeck(QWidget *parent, MixxxMainWindow *mixxx, PlayerManager* pPlayerManager, UserSettingsPointer pConfig); - virtual ~DlgPrefDeck(); + ~DlgPrefDeck() override; public slots: void slotUpdate() override; @@ -101,19 +102,22 @@ class DlgPrefDeck : public DlgPreferencePage, public Ui::DlgPrefDeckDlg { void setRateRangeForAllDecks(int rangePercent); void setRateDirectionForAllDecks(bool inverted); - UserSettingsPointer m_pConfig; - ControlObject* m_pControlTrackTimeDisplay; - ControlObject* m_pControlTrackTimeFormat; - ControlProxy* m_pNumDecks; - ControlProxy* m_pNumSamplers; + MixxxMainWindow* const m_mixxx; + PlayerManager* const m_pPlayerManager; + const UserSettingsPointer m_pConfig; + + const std::unique_ptr m_pControlTrackTimeDisplay; + const std::unique_ptr m_pControlTrackTimeFormat; + + const parented_ptr m_pNumDecks; + const parented_ptr m_pNumSamplers; + QList m_cueControls; QList m_rateControls; QList m_rateDirectionControls; QList m_rateRangeControls; QList m_keylockModeControls; QList m_keyunlockModeControls; - MixxxMainWindow *m_mixxx; - PlayerManager* m_pPlayerManager; int m_iNumConfiguredDecks; int m_iNumConfiguredSamplers; @@ -143,5 +147,3 @@ class DlgPrefDeck : public DlgPreferencePage, public Ui::DlgPrefDeckDlg { double m_dRatePermCoarse; double m_dRatePermFine; }; - -#endif -- cgit v1.2.3 From b25fe09d5fd79450bd09f0baf279d868431c12fc Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Tue, 7 Jul 2020 17:15:05 +0200 Subject: Delete SettingsManager explicitly on shutdown --- src/mixxx.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/mixxx.cpp b/src/mixxx.cpp index 6503f44db3..d479435bde 100644 --- a/src/mixxx.cpp +++ b/src/mixxx.cpp @@ -142,7 +142,6 @@ const int MixxxMainWindow::kAuxiliaryCount = 4; MixxxMainWindow::MixxxMainWindow(QApplication* pApp, const CmdlineArgs& args) : m_pWidgetParent(nullptr), m_pLaunchImage(nullptr), - m_pSettingsManager(nullptr), m_pEffectsManager(nullptr), m_pEngine(nullptr), m_pSkinLoader(nullptr), @@ -820,6 +819,11 @@ void MixxxMainWindow::finalize() { Sandbox::shutdown(); qDebug() << t.elapsed(false).debugMillisWithUnit() << "deleting SettingsManager"; + { + auto* pSettingsManager = m_pSettingsManager.get(); + m_pSettingsManager = nullptr; // reset parented_ptr + delete pSettingsManager; + } delete m_pKeyboard; delete m_pKbdConfig; -- cgit v1.2.3 From 04017eb9ac296b5dcf1afa02d32929ec79a3e724 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Tue, 7 Jul 2020 22:02:20 +0200 Subject: Make SettingsManager a plain C++ class --- src/mixxx.cpp | 28 ++++++++++++++++------------ src/mixxx.h | 25 +++---------------------- src/preferences/settingsmanager.cpp | 6 ++---- src/preferences/settingsmanager.h | 14 +++----------- 4 files changed, 24 insertions(+), 49 deletions(-) diff --git a/src/mixxx.cpp b/src/mixxx.cpp index d479435bde..54b30bf3d2 100644 --- a/src/mixxx.cpp +++ b/src/mixxx.cpp @@ -176,7 +176,7 @@ MixxxMainWindow::MixxxMainWindow(QApplication* pApp, const CmdlineArgs& args) StatsManager::createInstance(); } - m_pSettingsManager = make_parented(this, args.getSettingsPath()); + m_pSettingsManager = std::make_unique(args.getSettingsPath()); initializeKeyboard(); @@ -284,10 +284,10 @@ void MixxxMainWindow::initialize(QApplication* pApp, const CmdlineArgs& args) { m_pRecordingManager = new RecordingManager(pConfig, m_pEngine); - #ifdef __BROADCAST__ - m_pBroadcastManager = new BroadcastManager(m_pSettingsManager, - m_pSoundManager); + m_pBroadcastManager = new BroadcastManager( + m_pSettingsManager.get(), + m_pSoundManager); #endif launchProgress(11); @@ -470,9 +470,17 @@ void MixxxMainWindow::initialize(QApplication* pApp, const CmdlineArgs& args) { } // Initialize preference dialog - m_pPrefDlg = new DlgPreferences(this, m_pSkinLoader, m_pSoundManager, m_pPlayerManager, - m_pControllerManager, m_pVCManager, pLV2Backend, m_pEffectsManager, - m_pSettingsManager, m_pLibrary); + m_pPrefDlg = new DlgPreferences( + this, + m_pSkinLoader, + m_pSoundManager, + m_pPlayerManager, + m_pControllerManager, + m_pVCManager, + pLV2Backend, + m_pEffectsManager, + m_pSettingsManager.get(), + m_pLibrary); m_pPrefDlg->setWindowIcon(QIcon(":/images/mixxx_icon.svg")); m_pPrefDlg->setHidden(true); @@ -819,11 +827,7 @@ void MixxxMainWindow::finalize() { Sandbox::shutdown(); qDebug() << t.elapsed(false).debugMillisWithUnit() << "deleting SettingsManager"; - { - auto* pSettingsManager = m_pSettingsManager.get(); - m_pSettingsManager = nullptr; // reset parented_ptr - delete pSettingsManager; - } + m_pSettingsManager.reset(); delete m_pKeyboard; delete m_pKbdConfig; diff --git a/src/mixxx.h b/src/mixxx.h index 387a2909b3..f34157f888 100644 --- a/src/mixxx.h +++ b/src/mixxx.h @@ -1,26 +1,9 @@ -/*************************************************************************** - mixxx.h - description - ------------------- - begin : Mon Feb 18 09:48:17 CET 2002 - copyright : (C) 2002 by Tue and Ken Haste Andersen - email : - ***************************************************************************/ - -/*************************************************************************** - * * - * This program is free software; you can redistribute it and/or modify * - * it under the terms of the GNU General Public License as published by * - * the Free Software Foundation; either version 2 of the License, or * - * (at your option) any later version. * - * * - ***************************************************************************/ - -#ifndef MIXXX_H -#define MIXXX_H +#pragma once #include #include #include +#include #include "preferences/configobject.h" #include "preferences/constants.h" @@ -143,7 +126,7 @@ class MixxxMainWindow : public QMainWindow { QWidget* m_pWidgetParent; LaunchImage* m_pLaunchImage; - parented_ptr m_pSettingsManager; + std::unique_ptr m_pSettingsManager; // The effects processing system EffectsManager* m_pEffectsManager; @@ -204,5 +187,3 @@ class MixxxMainWindow : public QMainWindow { static const int kMicrophoneCount; static const int kAuxiliaryCount; }; - -#endif diff --git a/src/preferences/settingsmanager.cpp b/src/preferences/settingsmanager.cpp index f5bb8c680f..efae608981 100644 --- a/src/preferences/settingsmanager.cpp +++ b/src/preferences/settingsmanager.cpp @@ -6,10 +6,8 @@ #include "preferences/upgrade.h" #include "util/assert.h" -SettingsManager::SettingsManager(QObject* pParent, - const QString& settingsPath) - : QObject(pParent), - m_bShouldRescanLibrary(false) { +SettingsManager::SettingsManager(const QString& settingsPath) + : m_bShouldRescanLibrary(false) { // First make sure the settings path exists. If we don't then other parts of // Mixxx (such as the library) will produce confusing errors. if (!QDir(settingsPath).exists()) { diff --git a/src/preferences/settingsmanager.h b/src/preferences/settingsmanager.h index d45ba67754..2f9dcd9dfb 100644 --- a/src/preferences/settingsmanager.h +++ b/src/preferences/settingsmanager.h @@ -1,17 +1,11 @@ -#ifndef PREFERENCES_SETTINGSMANAGER_H -#define PREFERENCES_SETTINGSMANAGER_H - -#include -#include -#include +#pragma once #include "preferences/broadcastsettings.h" #include "preferences/usersettings.h" -class SettingsManager : public QObject { - Q_OBJECT +class SettingsManager { public: - SettingsManager(QObject* pParent, const QString& settingsPath); + explicit SettingsManager(const QString& settingsPath); virtual ~SettingsManager(); UserSettingsPointer settings() const { @@ -37,5 +31,3 @@ class SettingsManager : public QObject { bool m_bShouldRescanLibrary; BroadcastSettingsPointer m_pBroadcastSettings; }; - -#endif /* PREFERENCES_SETTINGSMANAGER_H */ -- cgit v1.2.3