From fbea78945b6da832bc54f400c40e3cece291f778 Mon Sep 17 00:00:00 2001 From: IanCaio Date: Wed, 21 Apr 2021 02:26:29 -0300 Subject: [PATCH] Fix bug introduced by #5657 (#5982) * Fix bug introduced by #5657 There was a bug introduced by #5657 where reloading a project and playing it could cause a Segmentation Fault crash. After some debugging, @DomClark tracked the issue to be likely a use-after-free being caused by m_oldAutomatedValues not being cleared when the project was loaded again. This commit adds a line to clear the m_oldAutomatedValues map on Song::clearProject(), which is called from Song::loadProject(). Now, instead of using a Signal/Slot connection to move the control of the models back to the controllers, every time the song is processing the automations, the control of the models that were processed in the last cycle are moved back to the controller. The same is done under Song::stop(), so the last cycle models control is moved back to the controller. That removes the need to have a pointer to the controlled model in the controller object. Adds mixer model change request to avoid race condition. Co-authored-by: Dominic Clark --- include/ControllerConnection.h | 5 +---- src/core/AutomatableModel.cpp | 2 +- src/core/ControllerConnection.cpp | 11 ++--------- src/core/Song.cpp | 28 +++++++++++++++++++++------- src/gui/AutomatableModelView.cpp | 7 +------ 5 files changed, 26 insertions(+), 27 deletions(-) diff --git a/include/ControllerConnection.h b/include/ControllerConnection.h index 8b60b01b6..6b44e6723 100644 --- a/include/ControllerConnection.h +++ b/include/ControllerConnection.h @@ -37,7 +37,6 @@ #include "JournallingObject.h" #include "ValueBuffer.h" -class AutomatableModel; class ControllerConnection; typedef QVector ControllerConnectionVector; @@ -48,7 +47,7 @@ class LMMS_EXPORT ControllerConnection : public QObject, public JournallingObjec Q_OBJECT public: - ControllerConnection(Controller * _controller, AutomatableModel * contmod); + ControllerConnection(Controller * _controller); ControllerConnection( int _controllerId ); virtual ~ControllerConnection(); @@ -112,8 +111,6 @@ protected: static ControllerConnectionVector s_connections; - AutomatableModel * m_controlledModel; - signals: // The value changed while the mixer isn't running (i.e: MIDI CC) void valueChanged(); diff --git a/src/core/AutomatableModel.cpp b/src/core/AutomatableModel.cpp index 9e6cb3cef..2c1162e0d 100644 --- a/src/core/AutomatableModel.cpp +++ b/src/core/AutomatableModel.cpp @@ -215,7 +215,7 @@ void AutomatableModel::loadSettings( const QDomElement& element, const QString& } if( thisConnection.isElement() ) { - setControllerConnection(new ControllerConnection((Controller*)NULL, this)); + setControllerConnection(new ControllerConnection(nullptr)); m_controllerConnection->loadSettings( thisConnection.toElement() ); //m_controllerConnection->setTargetName( displayName() ); } diff --git a/src/core/ControllerConnection.cpp b/src/core/ControllerConnection.cpp index b8fe4b302..7a7365591 100644 --- a/src/core/ControllerConnection.cpp +++ b/src/core/ControllerConnection.cpp @@ -36,11 +36,10 @@ ControllerConnectionVector ControllerConnection::s_connections; -ControllerConnection::ControllerConnection(Controller * _controller, AutomatableModel * contmod) : +ControllerConnection::ControllerConnection(Controller * _controller) : m_controller( NULL ), m_controllerId( -1 ), - m_ownsController(false), - m_controlledModel(contmod) + m_ownsController(false) { if( _controller != NULL ) { @@ -124,12 +123,6 @@ void ControllerConnection::setController( Controller * _controller ) m_ownsController = (_controller->type() == Controller::MidiController); - connect(Engine::getSong(), SIGNAL(stopped()), - m_controlledModel, SLOT(setUseControllerValue()), - Qt::UniqueConnection); - - m_controlledModel->setUseControllerValue(true); - // If we don't own the controller, allow deletion of controller // to delete the connection if( !m_ownsController ) { diff --git a/src/core/Song.cpp b/src/core/Song.cpp index ac6fed6b3..2800f829b 100644 --- a/src/core/Song.cpp +++ b/src/core/Song.cpp @@ -407,15 +407,12 @@ void Song::processAutomations(const TrackList &tracklist, TimePos timeStart, fpp // Checks if an automated model stopped being automated by automation patterns // so we can move the control back to any connected controller again - if (!m_oldAutomatedValues.isEmpty()) + for (auto it = m_oldAutomatedValues.begin(); it != m_oldAutomatedValues.end(); it++) { - for (auto it = m_oldAutomatedValues.begin(); it != m_oldAutomatedValues.end(); it++) + AutomatableModel * am = it.key(); + if (am->controllerConnection() && !values.contains(am)) { - AutomatableModel * am = it.key(); - if (am->controllerConnection() && !values.contains(am)) - { - am->setUseControllerValue(true); - } + am->setUseControllerValue(true); } } m_oldAutomatedValues = values; @@ -639,6 +636,9 @@ void Song::stop() return; } + // To avoid race conditions with the processing threads + Engine::mixer()->requestChangeInModel(); + TimeLineWidget * tl = m_playPos[m_playMode].m_timeLine; m_paused = false; m_recording = true; @@ -687,8 +687,19 @@ void Song::stop() // remove all note-play-handles that are active Engine::mixer()->clear(); + // Moves the control of the models that were processed on the last frame + // back to their controllers. + for (auto it = m_oldAutomatedValues.begin(); it != m_oldAutomatedValues.end(); it++) + { + AutomatableModel * am = it.key(); + am->setUseControllerValue(true); + } + m_oldAutomatedValues.clear(); + m_playMode = Mode_None; + Engine::mixer()->doneChangeInModel(); + emit stopped(); emit playbackStateChanged(); } @@ -886,6 +897,9 @@ void Song::clearProject() m_masterPitchModel.reset(); m_timeSigModel.reset(); + // Clear the m_oldAutomatedValues AutomatedValueMap + m_oldAutomatedValues.clear(); + AutomationPattern::globalAutomationPattern( &m_tempoModel )->clear(); AutomationPattern::globalAutomationPattern( &m_masterVolumeModel )-> clear(); diff --git a/src/gui/AutomatableModelView.cpp b/src/gui/AutomatableModelView.cpp index 1f26580a4..8cf3a4ab0 100644 --- a/src/gui/AutomatableModelView.cpp +++ b/src/gui/AutomatableModelView.cpp @@ -33,7 +33,6 @@ #include "embed.h" #include "GuiApplication.h" #include "MainWindow.h" -#include "Song.h" #include "StringPairDrag.h" #include "Clipboard.h" @@ -229,7 +228,7 @@ void AutomatableModelViewSlots::execConnectionDialog() // New else { - ControllerConnection* cc = new ControllerConnection(d.chosenController(), m); + ControllerConnection* cc = new ControllerConnection(d.chosenController()); m->setControllerConnection( cc ); //cc->setTargetName( m->displayName() ); } @@ -251,12 +250,8 @@ void AutomatableModelViewSlots::removeConnection() if( m->controllerConnection() ) { - disconnect(Engine::getSong(), SIGNAL(stopped()), - m, SLOT(setUseControllerValue())); - delete m->controllerConnection(); m->setControllerConnection( NULL ); - emit m->dataChanged(); } }