Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug introduced by #5657 #5982

Merged
merged 5 commits into from Apr 21, 2021

Conversation

IanCaio
Copy link
Contributor

@IanCaio IanCaio commented Apr 12, 2021

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

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

Co-authored-by: Dominic Clark <mrdomclark@gmail.com>
@LmmsBot
Copy link

LmmsBot commented Apr 13, 2021

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

Linux

macOS

Windows

🤖
{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://13596-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.104%2Bg487fb3f-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/13596?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://13599-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.104%2Bg487fb3f0f-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/13599?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://13605-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.104%2Bg487fb3f0f-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/13605?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/sjy3grqjmc2g4dkh/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/38799424"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/1i2q43vr7uunt7qv/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/38799424"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://13603-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.104%2Bg487fb3f0f-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/13603?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "04f5c63b1a6a2985b297c703c63ccfb7fcac4a94"}

@qnebra
Copy link

qnebra commented Apr 15, 2021

One liner, fixes what is supposed to fix. Test project is reloaded and played this time without crash.

	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.
@IanCaio
Copy link
Contributor Author

IanCaio commented Apr 17, 2021

Last commit introduces some changes to the behavior of #5657 that remove the need to hold a pointer to the controlled model on the controller object.

@qnebra
Copy link

qnebra commented Apr 17, 2021

Looks like those changes broke behaviour of 5657. Be more corrent, it doesn't work like previously. User needs to mute automation track to regain control with MIDI controller.

@IanCaio
Copy link
Contributor Author

IanCaio commented Apr 17, 2021

Looks like those changes broke behaviour of 5657. Be more corrent, it doesn't work like previously. User needs to mute automation track to regain control with MIDI controller.

Can you ellaborate? I tested here with a LFO controller and it behaved as previously (LFO has control when automation is not affecting model, when it's recording and when music is stopped). Could you describe step by step what you did and how it behaved differently?

@qnebra
Copy link

qnebra commented Apr 17, 2021

Looks like it could be a quickest revocation of the sentence ever. I did more testing, and thinking about it. It wasn't broken, and I was just an idiot.

Comment on lines 408 to 416
// Checks if an automated model stopped being automated by automation patterns
// so we can move the control back to any connected controller again
// Moves the control of the models that were processed earlier to their controllers.
// If they get processed again, setAutomatedValue() will move the control back
// to the automation.
if (!m_oldAutomatedValues.isEmpty())
{
for (auto it = m_oldAutomatedValues.begin(); it != m_oldAutomatedValues.end(); it++)
{
AutomatableModel * am = it.key();
if (am->controllerConnection() && !values.contains(am))
{
am->setUseControllerValue(true);
}
am->setUseControllerValue(true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setUseControllerValue causes the model to emit its dataChanged signal, so this change means that all automated models will emit that signal twice per buffer (plus an additional third time if their value actually changes). I'm not sure what sort of impact this has, but it may be worth looking at.

src/core/Song.cpp Outdated Show resolved Hide resolved
	Adds mixer model change request to avoid race condition.
	Revert changes to processAutomations.
Comment on lines 695 to 702
if (!m_oldAutomatedValues.isEmpty())
{
for (auto it = m_oldAutomatedValues.begin(); it != m_oldAutomatedValues.end(); it++)
{
AutomatableModel * am = it.key();
am->setUseControllerValue(true);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The if statement here isn't necessary.

	Since we are iterating from QVector::begin() to QVector::end()
there's no need to check if it's empty first.
@Veratil
Copy link
Contributor

Veratil commented Apr 21, 2021

Random pipelines seem to be failing, reran once and now linux.gcc is failing instead of mingw32. Travis failure seems to be the same problem. Assuming there's a hiccup in the system somewhere on github side, so merging since good.

@Veratil Veratil merged commit fbea789 into LMMS:master Apr 21, 2021
sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
* 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 <mrdomclark@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants