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

[DRAFT] GDExtension: Fix crash by copying GDExtensionScriptInstanceInfo2 on the Godot side #81205

Conversation

dsnopek
Copy link
Contributor

@dsnopek dsnopek commented Aug 31, 2023

Fixes a crash that was introduced in PR #78634

Previously, Godot was keeping a pointer to the GDExtensionScriptInstanceInfo or GDExtensionScriptInstanceInfo2 created on the GDExtension side. The crash is caused because we provided compatibility by copying the data from provided a GDExtensionScriptInstanceInfo into a new GDExtensionScriptInstanceInfo2 on the stack, which is, of course, temporary. We could certainly allocate a new GDExtensionScriptInstanceInfo2 on the heap, but then we need to keep track of whether it was created on the GDExtension side or the Godot side so we know if we should free it or not.

This PR side steps that whole problem by instead making a copy of the struct on the Godot side, which we should probably do anyway, so that the GDExtension can't accidentally modify or free the struct on its side.

This is currently marked as a draft because I haven't had a chance to test it yet!

@dsnopek dsnopek requested a review from Sauermann August 31, 2023 19:21
@dsnopek dsnopek requested review from a team as code owners August 31, 2023 19:21
@dsnopek dsnopek marked this pull request as draft August 31, 2023 19:21
@dsnopek dsnopek mentioned this pull request Aug 31, 2023
4 tasks
@akien-mga akien-mga added this to the 4.2 milestone Aug 31, 2023
@dsnopek
Copy link
Contributor Author

dsnopek commented Aug 31, 2023

@maiself brought up over on #78634 (comment) that this is copying this struct once per script instance which means each instance will take up 23 pointers more memory (92 or 184 bytes depending if it's a 32-bit or 64-bit architecture). That's not ideal...

So, perhaps this will remain a draft until we can figure out if there's a better solution.

@dsnopek
Copy link
Contributor Author

dsnopek commented Aug 31, 2023

So, originally, I didn't like the idea of having the compatibility function allocate a new GDExtensionScriptInstanceInfo2 on the heap (and then, I guess, set a bool on ScriptInstanceExtension to tell it to free it at the end). But thinking about it some more now, that would mean that only GDExtensions using the compatibility function are hit with the extra copy and memory allocation, whereas GDExtensions using the new function keep working basically the same as they did before.

Maybe that is a better way afterall?

@maiself
Copy link
Contributor

maiself commented Aug 31, 2023

Tested this, fixes all crashing I was seeing.

@dsnopek
Copy link
Contributor Author

dsnopek commented Aug 31, 2023

@maiself Thanks for testing!

@dsnopek
Copy link
Contributor Author

dsnopek commented Aug 31, 2023

PR #81206 has an alternative approach in line with my previous comment

Copy link
Contributor

@Sauermann Sauermann left a comment

Choose a reason for hiding this comment

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

I'm inclined to find #81206 a better approach than this.

@dsnopek
Copy link
Contributor Author

dsnopek commented Aug 31, 2023

Superseded by #81206

@dsnopek dsnopek closed this Aug 31, 2023
@AThousandShips AThousandShips removed this from the 4.2 milestone Sep 1, 2023
@dsnopek dsnopek deleted the script-instance-extension-memory-bug branch July 22, 2024 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants