Fix bug introduced by LMMS#5657 (LMMS#5982)

* Fix bug introduced by LMMS#5657

	There was a bug introduced by LMMS#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 <[email protected]>

include/ControllerConnection.h

@@ -37,7 +37,6 @@
 #include "JournallingObject.h"
 #include "ValueBuffer.h"
 
-class AutomatableModel;
 class ControllerConnection;
 
 typedef QVector<ControllerConnection *> 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 @@ public slots:
 
 	static ControllerConnectionVector s_connections;
 
-	AutomatableModel * m_controlledModel;
-
 signals:
 	// The value changed while the mixer isn't running (i.e: MIDI CC)
 	void valueChanged();

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() );
 		}

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 ) {

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();

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();
 	}
 }