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 opening/closing mixer very quickly during playback #14790

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

Conversation

cbjeukendrup
Copy link
Contributor

@cbjeukendrup cbjeukendrup commented Nov 27, 2022

Resolves: #13903
Resolves: #9467

What happened:

  1. The Mixer is subscribed to the audio signal changes, and does this from the main thread.
  2. In the audio thread, in AudioSignalsNotifier::updateSignalValues(…), we call audioSignalChanges.send(audioChNumber, signalVal);.
  3. Because the mixer is subscribed from the main thread but "we" are on the audio thread, this results in the allocation of a QInvoker in AbstractInvoker::invoke(int type, const NotifyData& data). Also, a call to this QInvoker's invoke() method, followed by the deletion of this QInvoker is queued to QueuedInvoker.
  4. In the constructor of this QInvoker, it adds itself to the AbstractInvoker's list of m_qInvokers by calling AbstractInvoker::addQInvoker(…).

  1. Then, for some reason, AbstractInvoker::removeCallBack(…) is called, which "invalidates" the QInvoker; that means: the QInvoker's pointer to the AbstractInvoker is set to nullptr.

  1. Then, the QInvoker's invoke() method is still called (because that had been queued already, see point 2). This doesn't do much :)
  2. And, the QInvoker is deleted, because that was also queued.
  3. Normally, the destructor would remove the QInvoker from the AbstractInvoker's list of m_qInvokers by calling AbstractInvoker::removeQInvoker(…). But in this case, the QInvoker was already invalidated, so it has no reference to the AbstractInvoker anymore, so it doesn't remove itself from the list of m_qInvokers. So that list now contains a pointer to deleted memory! ⚠️⚠️⚠️

  1. Later, AbstractInvoker::removeCallBack(…) is called again, for some callback (which one exactly isn't relevant).
  2. This method loops through the list of m_qInvokers, and reads a bit of all of them, to look whether they belong to the callback that is being removed (and thus need to be invalidated).
  3. And thus it also reads from the deleted QInvoker that was still in the list! 💥

("very quickly" as in "keeping the keyboard shortcut pressed so that it fires repeatedly")

Or when quitting app during playback with mixer open
@cbjeukendrup cbjeukendrup force-pushed the crash_mixer_open_close_during_playback branch from 296c21f to 242c2b8 Compare February 7, 2024 17:23
@cbjeukendrup
Copy link
Contributor Author

Rebased again :)

@DmitryArefiev
Copy link
Contributor

DmitryArefiev commented Feb 8, 2024

@cbjeukendrup Tested on Win/Mac with Mvt.4_Beethoven__Symphony_No._9__Op._125.zip

on Win11, after some iterations, Mixer stops to open:

bandicam.2024-02-08.16-45-36-531.mp4

on Mac13.6, it still crashes:

Screen.Recording.2024-02-08.at.16.50.24.mov

Here is the crash output mixer crash.zip

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants