[Bugfix] Move Heaps_Free after DeinitOTR in SDL_main

[Malkierian]

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

• soh.otr.zip
• soh-linux-compatibility.zip
• soh-linux-performance.zip
• soh-mac.zip
• soh-switch.zip
• soh-wiiu.zip
• soh-windows.zip

[Archez]

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]

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]

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?

[briaguya-ai]

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

[Malkierian]

As a matter of fact, it isn't.

[Malkierian]

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]

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.

[briaguya-ai]

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]

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

[Malkierian]

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

[briaguya-ai]

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

[Malkierian]

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.