Fix crash due to concurrent access in MidiController (Alternative)

src/controllers/bulk/bulkcontroller.cpp

@@ -97,14 +97,22 @@ QString BulkController::mappingExtension() {
 }
 
 void BulkController::setMapping(std::shared_ptr<LegacyControllerMapping> pMapping) {
+    m_pMutableMapping = pMapping;
     m_pMapping = downcastAndClone<LegacyHidControllerMapping>(pMapping.get());
 }
 
-std::shared_ptr<LegacyControllerMapping> BulkController::cloneMapping() {
+QList<LegacyControllerMapping::ScriptFileInfo> BulkController::getMappingScriptFiles() {
     if (!m_pMapping) {
-        return nullptr;
+        return {};
     }
-    return std::make_shared<LegacyHidControllerMapping>(*m_pMapping);
+    return m_pMapping->getScriptFiles();
+}
+
+QList<std::shared_ptr<AbstractLegacyControllerSetting>> BulkController::getMappingSettings() {
+    if (!m_pMapping) {
+        return {};
+    }
+    return m_pMapping->getSettings();
 }
 
 bool BulkController::matchMapping(const MappingInfo& mapping) {
@@ -202,7 +210,6 @@ int BulkController::open() {
     }
 #endif
 
-    setOpen(true);
     startEngine();
 
     if (m_pReader != nullptr) {
@@ -223,7 +230,8 @@ int BulkController::open() {
         // audio directly, like when scratching
         m_pReader->start(QThread::HighPriority);
     }
-
+    applyMapping();
+    setOpen(true);
     return 0;
 }
 

src/controllers/bulk/bulkcontroller.h

@@ -41,9 +41,11 @@ class BulkController : public Controller {
 
     QString mappingExtension() override;
 
-    virtual std::shared_ptr<LegacyControllerMapping> cloneMapping() override;
     void setMapping(std::shared_ptr<LegacyControllerMapping> pMapping) override;
 
+    QList<LegacyControllerMapping::ScriptFileInfo> getMappingScriptFiles() override;
+    QList<std::shared_ptr<AbstractLegacyControllerSetting>> getMappingSettings() override;
+
     bool isMappable() const override {
         if (!m_pMapping) {
             return false;
@@ -56,11 +58,10 @@ class BulkController : public Controller {
   protected:
     void send(const QList<int>& data, unsigned int length) override;
 
-  private slots:
+  private:
     int open() override;
     int close() override;
 
-  private:
     // For devices which only support a single report, reportID must be set to
     // 0x0.
     void sendBytes(const QByteArray& data) override;

src/controllers/controller.cpp

@@ -70,8 +70,7 @@ bool Controller::applyMapping() {
         return false;
     }
 
-    const std::shared_ptr<LegacyControllerMapping> pMapping = cloneMapping();
-    QList<LegacyControllerMapping::ScriptFileInfo> scriptFiles = pMapping->getScriptFiles();
+    QList<LegacyControllerMapping::ScriptFileInfo> scriptFiles = getMappingScriptFiles();
     if (scriptFiles.isEmpty()) {
         qCWarning(m_logBase)
                 << "No script functions available! Did the XML file(s) load "
@@ -81,7 +80,7 @@ bool Controller::applyMapping() {
 
     m_pScriptEngineLegacy->setScriptFiles(scriptFiles);
 
-    m_pScriptEngineLegacy->setSettings(pMapping->getSettings());
+    m_pScriptEngineLegacy->setSettings(getMappingSettings());
     return m_pScriptEngineLegacy->initialize();
 }
 

src/controllers/controller.h

@@ -3,6 +3,7 @@
 #include <QElapsedTimer>
 
 #include "controllers/controllermappinginfo.h"
+#include "controllers/legacycontrollermapping.h"
 #include "util/duration.h"
 #include "util/runtimeloggingcategory.h"
 
@@ -28,10 +29,15 @@ class Controller : public QObject {
     /// the controller (type.)
     virtual QString mappingExtension() = 0;
 
-    virtual std::shared_ptr<LegacyControllerMapping> cloneMapping() = 0;
-    /// WARNING: LegacyControllerMapping is not thread safe!
-    /// Clone the mapping before passing to setMapping for use in the controller polling thread.
     virtual void setMapping(std::shared_ptr<LegacyControllerMapping> pMapping) = 0;
+    std::shared_ptr<LegacyControllerMapping> getMapping() {
+        // return the unused mutable copy of the mapping, that can be edited in the GUI thread
+        // and than adopted again via setMapping()
+        return m_pMutableMapping;
+    }
+
+    virtual QList<LegacyControllerMapping::ScriptFileInfo> getMappingScriptFiles() = 0;
+    virtual QList<std::shared_ptr<AbstractLegacyControllerSetting>> getMappingSettings() = 0;
 
     inline bool isOpen() const {
         return m_bIsOpen;
@@ -69,15 +75,15 @@ class Controller : public QObject {
     // function that is assumed to exist. (Sub-classes may want to reimplement
     // this if they have an alternate way of handling such data.)
     virtual void receive(const QByteArray& data, mixxx::Duration timestamp);
-
-    virtual bool applyMapping();
     virtual void slotBeforeEngineShutdown();
 
     // Puts the controller in and out of learning mode.
     void startLearning();
     void stopLearning();
 
   protected:
+    virtual bool applyMapping();
+
     template<typename SpecificMappingType>
         requires(std::is_final_v<SpecificMappingType> == true)
     std::unique_ptr<SpecificMappingType> downcastAndClone(const LegacyControllerMapping* pMapping) {
@@ -135,6 +141,7 @@ class Controller : public QObject {
     const RuntimeLoggingCategory m_logBase;
     const RuntimeLoggingCategory m_logInput;
     const RuntimeLoggingCategory m_logOutput;
+    std::shared_ptr<LegacyControllerMapping> m_pMutableMapping;
 
   private: // but used by ControllerManager
 

src/controllers/controllermanager.cpp

@@ -11,6 +11,7 @@
 #include "util/cmdlineargs.h"
 #include "util/compatibility/qmutex.h"
 #include "util/duration.h"
+#include "util/thread_affinity.h"
 #include "util/time.h"
 
 #ifdef __PORTMIDI__
@@ -105,7 +106,7 @@ ControllerManager::ControllerManager(UserSettingsPointer pConfig)
     }
 
     m_pollTimer.setInterval(kPollInterval.toIntegerMillis());
-    connect(&m_pollTimer, &QTimer::timeout, this, &ControllerManager::pollDevices);
+    connect(&m_pollTimer, &QTimer::timeout, this, &ControllerManager::slotPollDevices);
 
     m_pThread = new QThread;
     m_pThread->setObjectName("Controller");
@@ -300,7 +301,6 @@ void ControllerManager::slotSetUpDevices() {
             qWarning() << "There was a problem opening" << name;
             continue;
         }
-        pController->applyMapping();
     }
 
     pollIfAnyControllersOpen();
@@ -337,7 +337,7 @@ void ControllerManager::stopPolling() {
     qDebug() << "Controller polling stopped.";
 }
 
-void ControllerManager::pollDevices() {
+void ControllerManager::slotPollDevices() {
     // Note: this function is called from a high priority thread which
     // may stall the GUI or may reduce the available CPU time for other
     // High Priority threads like caching reader or broadcasting more
@@ -380,6 +380,7 @@ void ControllerManager::pollDevices() {
 }
 
 void ControllerManager::openController(Controller* pController) {
+    DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this);
     if (!pController) {
         return;
     }
@@ -392,15 +393,14 @@ void ControllerManager::openController(Controller* pController) {
     // If successfully opened the device, apply the mapping and save the
     // preference setting.
     if (result == 0) {
-        pController->applyMapping();
-
         // Update configuration to reflect controller is enabled.
         m_pConfig->setValue(
                 ConfigKey("[Controller]", sanitizeDeviceName(pController->getName())), 1);
     }
 }
 
 void ControllerManager::closeController(Controller* pController) {
+    DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this);
     if (!pController) {
         return;
     }
@@ -419,9 +419,9 @@ void ControllerManager::slotApplyMapping(Controller* pController,
         return;
     }
 
+    closeController(pController);
     ConfigKey key(kSettingsGroup, sanitizeDeviceName(pController->getName()));
     if (!pMapping) {
-        closeController(pController);
         // Unset the controller mapping for this controller
         pController->setMapping(nullptr);
         m_pConfig->remove(key);
@@ -438,14 +438,12 @@ void ControllerManager::slotApplyMapping(Controller* pController,
     // startup next time
     m_pConfig->set(key, pMapping->filePath());
 
-    // This runs on the main thread but LegacyControllerMapping is not thread safe, so clone it.
     pController->setMapping(std::move(pMapping));
 
     if (bEnabled) {
         openController(pController);
         emit mappingApplied(pController->isMappable());
     } else {
-        closeController(pController);
         emit mappingApplied(false);
     }
 }

src/controllers/controllermanager.h

@@ -52,13 +52,9 @@ class ControllerManager : public QObject {
     void mappingApplied(bool applied);
 
   public slots:
-    void updateControllerList();
-
     void slotApplyMapping(Controller* pController,
             std::shared_ptr<LegacyControllerMapping> pMapping,
             bool bEnabled);
-    void openController(Controller* pController);
-    void closeController(Controller* pController);
 
   private slots:
     /// Perform initialization that should be delayed until the ControllerManager
@@ -70,12 +66,16 @@ class ControllerManager : public QObject {
     void slotSetUpDevices();
     void slotShutdown();
     /// Calls poll() on all devices that have isPolling() true.
-    void pollDevices();
+    void slotPollDevices();
+
+  private:
+    void updateControllerList();
     void startPolling();
     void stopPolling();
     void pollIfAnyControllersOpen();
+    void openController(Controller* pController);
+    void closeController(Controller* pController);
 
-  private:
     UserSettingsPointer m_pConfig;
     ControllerLearningEventFilter* m_pControllerLearningEventFilter;
     QTimer m_pollTimer;

src/controllers/dlgprefcontroller.cpp

@@ -61,8 +61,8 @@ DlgPrefController::DlgPrefController(
     initTableView(m_ui.m_pInputMappingTableView);
     initTableView(m_ui.m_pOutputMappingTableView);
 
-    std::shared_ptr<LegacyControllerMapping> pMapping = m_pController->cloneMapping();
-    slotShowMapping(pMapping);
+    std::shared_ptr<LegacyControllerMapping> pMapping = m_pController->getMapping();
+    showMapping(pMapping);
 
     m_ui.labelDeviceName->setText(m_pController->getName());
     QString category = m_pController->getCategory();
@@ -103,7 +103,11 @@ DlgPrefController::DlgPrefController(
     connect(this,
             &DlgPrefController::applyMapping,
             m_pControllerManager.get(),
-            &ControllerManager::slotApplyMapping);
+            &ControllerManager::slotApplyMapping,
+            Qt::BlockingQueuedConnection);
+    // Wait until the mapping has been cloned in the controller thread
+    // and we can continue to edit our copy
+
     // Update GUI
     connect(m_pControllerManager.get(),
             &ControllerManager::mappingApplied,
@@ -193,10 +197,10 @@ void DlgPrefController::showLearningWizard() {
     slotApply();
 
     if (!m_pMapping) {
-        m_pMapping = std::shared_ptr<LegacyControllerMapping>(new LegacyMidiControllerMapping());
+        m_pMapping = std::make_shared<LegacyMidiControllerMapping>();
         emit applyMapping(m_pController, m_pMapping, true);
         // shortcut for creating and assigning required I/O table models
-        slotShowMapping(m_pMapping);
+        showMapping(m_pMapping);
     }
 
     // Note that DlgControllerLearning is set to delete itself on close using
@@ -685,7 +689,7 @@ void DlgPrefController::slotMappingSelected(int chosenIndex) {
         // the preset combobox.
         enumerateMappings(mappingFilePath);
     }
-    slotShowMapping(pMapping);
+    showMapping(pMapping);
 }
 
 bool DlgPrefController::saveMapping() {
@@ -853,7 +857,7 @@ void DlgPrefController::initTableView(QTableView* pTable) {
     pTable->setAlternatingRowColors(true);
 }
 
-void DlgPrefController::slotShowMapping(std::shared_ptr<LegacyControllerMapping> pMapping) {
+void DlgPrefController::showMapping(std::shared_ptr<LegacyControllerMapping> pMapping) {
     m_ui.labelLoadedMapping->setText(mappingName(pMapping));
     m_ui.labelLoadedMappingDescription->setText(mappingDescription(pMapping));
     m_ui.labelLoadedMappingAuthor->setText(mappingAuthor(pMapping));

src/controllers/dlgprefcontroller.h

@@ -55,9 +55,6 @@ class DlgPrefController : public DlgPreferencePage {
   private slots:
     /// Called when the user selects another mapping in the combobox
     void slotMappingSelected(int index);
-    /// Used to selected the current mapping in the combobox and display the
-    /// mapping information.
-    void slotShowMapping(std::shared_ptr<LegacyControllerMapping> mapping);
     void slotInputControlSearch();
     void slotOutputControlSearch();
     /// Called when the Controller Learning Wizard is closed.
@@ -78,6 +75,9 @@ class DlgPrefController : public DlgPreferencePage {
     void midiInputMappingsLearned(const MidiInputMappings& mappings);
 
   private:
+    /// Used to selected the current mapping in the combobox and display the
+    /// mapping information.
+    void showMapping(std::shared_ptr<LegacyControllerMapping> mapping);
     QString mappingShortName(const std::shared_ptr<LegacyControllerMapping> pMapping) const;
     QString mappingName(const std::shared_ptr<LegacyControllerMapping> pMapping) const;
     QString mappingAuthor(const std::shared_ptr<LegacyControllerMapping> pMapping) const;

src/controllers/dlgprefcontrollers.cpp

@@ -176,7 +176,7 @@ void DlgPrefControllers::setupControllerWidgets() {
     std::sort(controllerList.begin(), controllerList.end(), controllerCompare);
 
     for (auto* pController : std::as_const(controllerList)) {
-        DlgPrefController* pControllerDlg = new DlgPrefController(
+        auto pControllerDlg = make_parented<DlgPrefController>(
                 this, pController, m_pControllerManager, m_pConfig);
         connect(pControllerDlg,
                 &DlgPrefController::mappingStarted,
@@ -187,18 +187,18 @@ void DlgPrefControllers::setupControllerWidgets() {
                 m_pDlgPreferences,
                 &DlgPreferences::show);
         // Recreate the control picker menus when decks or samplers are added
-        m_pNumDecks->connectValueChanged(pControllerDlg,
+        m_pNumDecks->connectValueChanged(pControllerDlg.get(),
                 &DlgPrefController::slotRecreateControlPickerMenu);
-        m_pNumSamplers->connectValueChanged(pControllerDlg,
+        m_pNumSamplers->connectValueChanged(pControllerDlg.get(),
                 &DlgPrefController::slotRecreateControlPickerMenu);
 
         m_controllerPages.append(pControllerDlg);
 
         connect(pController,
                 &Controller::openChanged,
                 this,
-                [this, pControllerDlg](bool bOpen) {
-                    slotHighlightDevice(pControllerDlg, bOpen);
+                [this, pDlg = pControllerDlg.get()](bool bOpen) {
+                    slotHighlightDevice(pDlg, bOpen);
                 });
 
         QTreeWidgetItem* pControllerTreeItem = new QTreeWidgetItem(