Fix crash when activating non-initial song then opening document with expansion chip(s) #148
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nyanpasu64
changed the title
… expansion chip CFamiTrackerDoc::OpenDocumentNew() and other functions can call CFamiTrackerDoc::SetupChannels() -> CFamiTrackerDoc::SetMachine (), during the middle of updating the document. Previously this could call CDocument::UpdateAllViews() (a method which has no right to exist), trying to redraw the pattern editor mid-load, when the number of songs has already been reset to 1 but the active song/track is still unchanged. If you previously picked a non-initial song, CPatternEditor::GetFrameCount() calls m_pDocument->GetFrameCount (GetSelectedTrack()), which indexes out of bounds. The "solution" is to add a boolean parameter to CFamiTrackerDoc::SetMachine(), to skip redrawing if the document is in an inconsistent state. Of course the real fix is to create a StateTransaction-style type which reloads program state exactly once, after all changes have been made and the program is in a consistent state. But FamiTracker is too complex to rewrite state management entirely.
nyanpasu64
force-pushed
the
fix-open-song-crash
branch
from
a525a86 to
bff3dc4
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
CFamiTrackerDoc::OpenDocumentNew()and other functions can callCFamiTrackerDoc::SetupChannels(). If the new document contains expansion chips, it callsCFamiTrackerDoc::SetMachine()during the middle of updating the document. Previously this could callCDocument::UpdateAllViews()(a method which has no right to exist), trying to redraw the pattern editor mid-load, when the number of songs has already been reset to 1 but the active song/track is still unchanged. If you previously picked a non-initial song,CPatternEditor::GetFrameCount()callsm_pDocument->GetFrameCount(GetSelectedTrack()), which indexes out of bounds.The "solution" is to add a boolean parameter to
CFamiTrackerDoc::SetMachine(), to skip redrawing if the document is in an inconsistent state. I would not be surprised if there are other crashes lurking undiscovered.Of course the real fix is to make observing inconsistent state difficult to impossible by design, by creating a StateTransaction-style type which reloads program state exactly once, after all changes have been made and the program is in a consistent state. But FamiTracker is too complex to rewrite state management entirely.
Fixes #147.