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

GDScript: Fix stack manipulation for await #61003

Merged
merged 1 commit into from
May 16, 2022

Conversation

vnen
Copy link
Member

@vnen vnen commented May 13, 2022

The stack now contains three special addresses that should no be copied to the state, since it contains references that creates cycles. They can be recreated when the function is resumed.

This commit also removes the clearing of stack from the GDScriptFunctionState destructor, since it should be cleared when the
function exits. The state stack should only be cleared manually if the instance is freed before the state resumes (which is already being done). Otherwise this would destruct the stack twice, causing crashes.

Fixes #57126

Note that I still see leaks after this PR. The SceneTreeTimer instances created still leak. I'm still not sure why this happens nor whether it's related to this, but I prefer to publish this fix now to avoid the crashing at least. I'll keep investigating the leak, but it's not as urgent. (This is now solved in this PR too).

@vnen vnen added this to the 4.0 milestone May 13, 2022
@vnen vnen requested a review from a team as a code owner May 13, 2022 17:38
The stack now contains three special addresses that should no be copied
to the state, since it contains references that creates cycles. They can
be recreated when the function is resumed.

This commit also removes the clearing of stack from the
GDScriptFunctionState destructor, since it should be cleared when the
function exits. The state stack should only be cleared manually if the
instance is freed before the state resumes (which is already being
done). Otherwise this would destruct the stack twice, causing crashes.
@vnen vnen force-pushed the gdscript-await-stack-fix branch from f20843c to 102c312 Compare May 13, 2022 23:15
@vnen
Copy link
Member Author

vnen commented May 13, 2022

Updated the PR, it solved the leaked SceneTreeTimers. I had an idea that it could be related to the temporary values in the stack and indeed it turned out to be the case. The problem is that those are saved in the function state and then initialized again when resumed, so they miss a destructor. I changed it so they're only initialized if not resuming from a stored state.

@vnen
Copy link
Member Author

vnen commented May 13, 2022

BTW, I still see one orphan StringName: RefCounted. It seems unrelated to GDScript though as it seems to leak even without any script.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks ok, and Fales confirmed that he tested it and it works fine.

@akien-mga akien-mga merged commit c41f62c into godotengine:master May 16, 2022
@akien-mga
Copy link
Member

Thanks!

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.

Memory leak/Stack corruption when using GDScript await in release_debug builds (GCC specific)
2 participants