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 crash when replacing instrument via popup in instruments panel #25441

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cbjeukendrup
Copy link
Contributor

The problem is that the popup is unloaded as soon as it is closed, and it is closed as soon as the "replace instrument" dialog appears. The unloading causes the popup and its content and the model of that content to be deleted. But the "onClicked" signal handler is still being executed at that point. This is (naturally) not allowed by Qt and causes a crash.

Probably introduced by 95249f7; that commit fixed a mistake, but that mistake prevented the unloading, and thus the crash.

Resolves: #25439

The problem is that the popup is unloaded as soon as it is closed, and it is closed as soon as the "replace instrument" dialog appears. The unloading causes the popup and its content and the model of that content to be deleted. But the "onClicked" signal handler is still being executed at that point. This is (naturally) not allowed by Qt and causes a crash.

Probably introduced by 95249f7; that commit fixed a mistake, but that mistake prevented the unloading, and thus the crash.
@cbjeukendrup cbjeukendrup force-pushed the instrumentspanel/replace_instrument_crash branch from 6298a54 to 9c7258f Compare November 6, 2024 15:10
@zacjansheski
Copy link
Contributor

Tested on MacOS 14, Windows 11, Ubuntu 22.04.3. Approved
#25439 FIXED

@Eism
Copy link
Contributor

Eism commented Nov 7, 2024

@cbjeukendrup
What do you think about moving the instrument replacement from the popup to the InstrumentsPanelTreeModel model?
I remember that we have already fixed several crashes with the opening of a dialog from a popup that we had previously closed, so it looks like we shouldn't do that

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.

Nightly: Replace instrument in Instruments panel crashes MS
3 participants