Fix late destruction access violation with OpenXRAPIExtension object#110868
Conversation
m4gr3d
added
the
cherrypick:4.5
dsnopek
left a comment
dsnopek
left a comment
There was a problem hiding this comment.
Thanks!
I think this change is fine. If we wanted, we could probably have OpenXRAPI (or some other singleton) manage the OpenXRAPIExtension instance, and then we could still have just one of them. But having an extra instance per extension wrapper probably isn't a very big deal.
BastiaanOlij
left a comment
BastiaanOlij
left a comment
There was a problem hiding this comment.
Looks good to me too as a way to handle this. I guess we could have multiple instances of the extension wrapper class but they all point to the same singleton.
Davids suggestion for making the wrapper instance lifecycle be handled by the OpenXRAPI class itself is a good one too, but for now this seems good enough to me.
|
Could you squash your commits? See our pull request guidelines for more information |
brycehutchings
force-pushed
the
openxr_late_destruction_crash_fix
branch
from
77e3ed8 to
cd198f7
Compare
Sorry about that, I've squashed it |
|
Thanks! |
|
Cherry-picked to 4.5 |
Repiteo
removed
the
cherrypick:4.5
5 participants
I have a game using a GDExtension with an
OpenXRExtensionWrapperclass, and once I added code to callget_openxr_api()I started observing crashes when the exported game process would exit. Just to make sure it's not something I am doing wrong, I exported and ran the "godot-xr-template" demo project and saw the same thing.What I found was a change to refactor this class moved the
Ref<OpenXRAPIExtension>from a member variable to a static delay-initialized variable instead. I can see why this was done because then you don't pay for it if you don't need it, and if you do need it, you only pay for it once. But the problem is that this static variable holds the last reference to this object and so the cleanup will happen at the very end, after theWinMain()function has returned and the C runtime code runs to clean up global variables. At this point it seems like there is some "instance binding" with pointers that are no longer valid (perhaps pointers into the GDExtension DLL?).I am not familiar with this system to know what the best fix is, so I opted to revert this back to the prior behavior. The object that it creates looks light weight (no initialization code, no member variables) so I think having separate copies per
OpenXRExtensionWrapperand initializing them immediately is not bad. As I said, I don't know enough about this area to say whether something different or better is needed. See #104087 "OpenXR: Clean-up OpenXRExtensionWrapper by removing multiple inheritance and deprecating OpenXRExtensionWrapperExtension" for more details on the commit that changed this from member variable to static.Stack and crash details below: