Skip to content

[Bugfix] Move Heaps_Free after DeinitOTR in SDL_main#3279

Merged
garrettjoecox merged 1 commit intoHarbourMasters:developfrom
Malkierian:fix-junk-gplaystate
Oct 20, 2023
Merged

[Bugfix] Move Heaps_Free after DeinitOTR in SDL_main#3279
garrettjoecox merged 1 commit intoHarbourMasters:developfrom
Malkierian:fix-junk-gplaystate

Conversation

@Malkierian
Copy link
Contributor

@Malkierian Malkierian commented Oct 12, 2023 •
edited by github-actions bot
Loading

Anchor was having some issues with accessing a non-null, garbage gPlayState from DeinitOTR as part of cleanup when closing while the game was running, causing a crash on closing. This was caused by Heaps_Free in Main deallocating the memory for gPlayState without making its pointer null. This PR moves Heaps_Free after DeinitOTR in SDL_main so all closing/cleanup operations can be done without worrying about non-null garbage pointers/references.

I took this approach instead of moving DeinitOTR inside Main before Heaps_Free as I figured there was probably a reason Deinit wasn't added inside Main in the first place, and this provides a better model for adding anything else that we might need in the future before Heaps_Free.

Build Artifacts

Archez
Archez approved these changes Oct 16, 2023
View reviewed changes
Copy link
Contributor

@Archez Archez left a comment

Choose a reason for hiding this comment

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

Seems good enough to me 👍
I wonder if maybe we should also set system globals to NULL like gPlayState, but I don't know what all is managed by the system heap and that doesn't need to happen in this PR.

@GaryOderNichts
Copy link
Contributor

GaryOderNichts commented Oct 16, 2023

gPlayState is set to NULL by Play_Destroy. After Main returned the state should've been freed and no longer be used anyways.
Where is DeinitOTR using it?

@Malkierian
Copy link
Contributor Author

Malkierian commented Oct 16, 2023 •
edited
Loading

It doesn't right now, the problem was exposed when DeinitOTR was calling a destruct function in Anchor using the IsSaveLoaded method of detecting a loaded save game, and that's where it was trying to reference gPlayState. Maybe there's an issue with thread waiting that I'm missing?

@briaguya0
Copy link
Contributor

briaguya0 commented Oct 16, 2023

Is Play_Destroy not getting called when closing the application while it's running?

@Malkierian
Copy link
Contributor Author

Malkierian commented Oct 16, 2023

As a matter of fact, it isn't.

@Malkierian
Copy link
Contributor Author

Malkierian commented Oct 16, 2023

Knowing this now, this might also be related to my issues with certain hooks not getting called in the process of closing SoH in the middle of a game. OnExitGame was one of them.

@Malkierian
Copy link
Contributor Author

Malkierian commented Oct 16, 2023

Heck, that might even be why Proxy felt the need to call the Anchor deinit function in DeinitOTR, as otherwise I think that would fit perfectly in OnExitGame. I still think we could get this merged to prevent those issues while we investigated the underlying problem, though, unless it was a quick fix itself.

@briaguya0
Copy link
Contributor

briaguya0 commented Oct 16, 2023

I agree this can still be merged since it doesn't break anything and will continue to not break anything after a better fix is found.

@GaryOderNichts
Copy link
Contributor

GaryOderNichts commented Oct 16, 2023 •
edited
Loading

Should Heaps_Alloc be moved at the start of main as well to be in the same place?

@Malkierian
Copy link
Contributor Author

Malkierian commented Oct 16, 2023

I could see doing that just for consistency's sake.

@briaguya0
Copy link
Contributor

briaguya0 commented Oct 16, 2023

Does Heaps_Alloc rely on any of the things that happen before it in Main?

@Malkierian
Copy link
Contributor Author

Malkierian commented Oct 16, 2023

Doesn't look like it. It only uses defines for sizes, and they're assigned to variables in heaps.c, and moving it to the top of Main() didn't prevent it from running.

@garrettjoecox garrettjoecox merged commit 35b4357 into HarbourMasters:develop Oct 20, 2023
@Malkierian Malkierian deleted the fix-junk-gplaystate branch October 23, 2023 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@garrettjoecox garrettjoecox garrettjoecox approved these changes

@Archez Archez Archez approved these changes

@briaguya0 briaguya0 briaguya0 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Malkierian @GaryOderNichts @briaguya0 @garrettjoecox @Archez