From 146fcb3948fe96268cbb26b833e35d31be001712 Mon Sep 17 00:00:00 2001 From: Owen Williams Date: Sat, 23 Oct 2021 13:06:01 -0400 Subject: [PATCH 1/5] Disconnect connections from controllers when deleting controller prefs Leaving the stale connections can cause segfaults on exit. This fixes https://bugs.launchpad.net/mixxx/+bug/1946581 --- src/controllers/dlgprefcontrollers.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/controllers/dlgprefcontrollers.cpp b/src/controllers/dlgprefcontrollers.cpp index 87671a5a87d0..bd72e1e927e3 100644 --- a/src/controllers/dlgprefcontrollers.cpp +++ b/src/controllers/dlgprefcontrollers.cpp @@ -130,6 +130,9 @@ void DlgPrefControllers::rescanControllers() { } void DlgPrefControllers::destroyControllerWidgets() { + for (auto* pController : m_pControllerManager->getControllerList(false, true)) { + pController->disconnect(); + } while (!m_controllerPages.isEmpty()) { DlgPrefController* pControllerDlg = m_controllerPages.takeLast(); m_pDlgPreferences->removePageWidget(pControllerDlg); From 2c800c3ee83879c458a368163b993cdb53db7667 Mon Sep 17 00:00:00 2001 From: Owen Williams Date: Sat, 23 Oct 2021 13:09:52 -0400 Subject: [PATCH 2/5] Controller disconnect: be more restrictive about what we disconnect --- src/controllers/dlgprefcontrollers.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/controllers/dlgprefcontrollers.cpp b/src/controllers/dlgprefcontrollers.cpp index bd72e1e927e3..624881a09267 100644 --- a/src/controllers/dlgprefcontrollers.cpp +++ b/src/controllers/dlgprefcontrollers.cpp @@ -131,7 +131,7 @@ void DlgPrefControllers::rescanControllers() { void DlgPrefControllers::destroyControllerWidgets() { for (auto* pController : m_pControllerManager->getControllerList(false, true)) { - pController->disconnect(); + pController->disconnect(SIGNAL(&Controller::openChanged)); } while (!m_controllerPages.isEmpty()) { DlgPrefController* pControllerDlg = m_controllerPages.takeLast(); From 7b595c03b1ad6cc633551a634bf43ab8f26a3be9 Mon Sep 17 00:00:00 2001 From: Owen Williams Date: Sat, 23 Oct 2021 13:16:33 -0400 Subject: [PATCH 3/5] controller disconnect: store an explicit list of connections --- src/controllers/dlgprefcontrollers.cpp | 9 +++++---- src/controllers/dlgprefcontrollers.h | 1 + 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/controllers/dlgprefcontrollers.cpp b/src/controllers/dlgprefcontrollers.cpp index 624881a09267..ef4446de3d8c 100644 --- a/src/controllers/dlgprefcontrollers.cpp +++ b/src/controllers/dlgprefcontrollers.cpp @@ -130,9 +130,10 @@ void DlgPrefControllers::rescanControllers() { } void DlgPrefControllers::destroyControllerWidgets() { - for (auto* pController : m_pControllerManager->getControllerList(false, true)) { - pController->disconnect(SIGNAL(&Controller::openChanged)); + for (auto conn : m_controllerConnections) { + disconnect(conn); } + m_controllerConnections.clear(); while (!m_controllerPages.isEmpty()) { DlgPrefController* pControllerDlg = m_controllerPages.takeLast(); m_pDlgPreferences->removePageWidget(pControllerDlg); @@ -174,11 +175,11 @@ void DlgPrefControllers::setupControllerWidgets() { m_controllerPages.append(pControllerDlg); - connect(pController, + m_controllerConnections.append(connect(pController, &Controller::openChanged, [this, pControllerDlg](bool bOpen) { slotHighlightDevice(pControllerDlg, bOpen); - }); + })); QTreeWidgetItem* pControllerTreeItem = new QTreeWidgetItem( QTreeWidgetItem::Type); diff --git a/src/controllers/dlgprefcontrollers.h b/src/controllers/dlgprefcontrollers.h index 3ebda150c867..5cf1d3800cae 100644 --- a/src/controllers/dlgprefcontrollers.h +++ b/src/controllers/dlgprefcontrollers.h @@ -52,4 +52,5 @@ class DlgPrefControllers : public DlgPreferencePage, public Ui::DlgPrefControlle QTreeWidgetItem* m_pControllersRootItem; QList m_controllerPages; QList m_controllerTreeItems; + QList m_controllerConnections; }; From ab4024307060cf1bbbcbe32119cbbf7a7f069e50 Mon Sep 17 00:00:00 2001 From: Owen Williams Date: Mon, 25 Oct 2021 17:14:50 -0400 Subject: [PATCH 4/5] controller disconnect: use the list of controllers to determine what to disconnect This is safe code for now, but may not be in the future. Added some comments as hints to future developers trying to add hotplug so they don't trip over this code. (There is no way to write this code currently that would be future-proof) --- src/controllers/controllermanager.cpp | 3 +++ src/controllers/dlgprefcontrollers.cpp | 14 +++++++++----- src/controllers/dlgprefcontrollers.h | 1 - 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/controllers/controllermanager.cpp b/src/controllers/controllermanager.cpp index 0aee05c07f98..088eb93ae20b 100644 --- a/src/controllers/controllermanager.cpp +++ b/src/controllers/controllermanager.cpp @@ -178,6 +178,9 @@ void ControllerManager::slotShutdown() { } void ControllerManager::updateControllerList() { + // NOTE: Currently this function is only called on startup. If hotplug is added, changes to the + // controller list must be synchronized with dlgprefcontrollers to avoid dangling connections + // and possible crashes. auto locker = lockMutex(&m_mutex); if (m_enumerators.isEmpty()) { qWarning() << "updateControllerList called but no enumerators have been added!"; diff --git a/src/controllers/dlgprefcontrollers.cpp b/src/controllers/dlgprefcontrollers.cpp index ef4446de3d8c..a59821ccc435 100644 --- a/src/controllers/dlgprefcontrollers.cpp +++ b/src/controllers/dlgprefcontrollers.cpp @@ -130,10 +130,14 @@ void DlgPrefControllers::rescanControllers() { } void DlgPrefControllers::destroyControllerWidgets() { - for (auto conn : m_controllerConnections) { - disconnect(conn); + // NOTE: this assumes that the list of controllers does not change during the lifetime of Mixxx. + // This is currently true, but once we support hotplug, we will need better lifecycle management + // to keep this dialog and the controllermanager consistent. + QList controllerList = + m_pControllerManager->getControllerList(false, true); + for (auto controller : controllerList) { + controller->disconnect(this); } - m_controllerConnections.clear(); while (!m_controllerPages.isEmpty()) { DlgPrefController* pControllerDlg = m_controllerPages.takeLast(); m_pDlgPreferences->removePageWidget(pControllerDlg); @@ -175,11 +179,11 @@ void DlgPrefControllers::setupControllerWidgets() { m_controllerPages.append(pControllerDlg); - m_controllerConnections.append(connect(pController, + connect(pController, &Controller::openChanged, [this, pControllerDlg](bool bOpen) { slotHighlightDevice(pControllerDlg, bOpen); - })); + }); QTreeWidgetItem* pControllerTreeItem = new QTreeWidgetItem( QTreeWidgetItem::Type); diff --git a/src/controllers/dlgprefcontrollers.h b/src/controllers/dlgprefcontrollers.h index 5cf1d3800cae..3ebda150c867 100644 --- a/src/controllers/dlgprefcontrollers.h +++ b/src/controllers/dlgprefcontrollers.h @@ -52,5 +52,4 @@ class DlgPrefControllers : public DlgPreferencePage, public Ui::DlgPrefControlle QTreeWidgetItem* m_pControllersRootItem; QList m_controllerPages; QList m_controllerTreeItems; - QList m_controllerConnections; }; From eb937bb3e28b78b820281033ba7b73d24d7e7913 Mon Sep 17 00:00:00 2001 From: Owen Williams Date: Tue, 26 Oct 2021 11:43:00 -0400 Subject: [PATCH 5/5] controller disconnect : Make sure to associate the connection with the parent object --- src/controllers/dlgprefcontrollers.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/controllers/dlgprefcontrollers.cpp b/src/controllers/dlgprefcontrollers.cpp index a59821ccc435..94d9454e238c 100644 --- a/src/controllers/dlgprefcontrollers.cpp +++ b/src/controllers/dlgprefcontrollers.cpp @@ -181,6 +181,7 @@ void DlgPrefControllers::setupControllerWidgets() { connect(pController, &Controller::openChanged, + this, [this, pControllerDlg](bool bOpen) { slotHighlightDevice(pControllerDlg, bOpen); });