Skip to content

Conversation

@zombietype
Copy link

Ref would lose count during return from plugin - either dangling when
directly returned, or double deleted if a Ref is stored.
Fixing by increasing the count before encode.

Fixes: #652

@Calinou Calinou added bug This has been identified as a bug topic:gdextension This relates to the new Godot 4 extension implementation labels Nov 23, 2021
@BastiaanOlij
Copy link
Collaborator

Just commenting here as well so it's recorded instead of just adding my feedback on godot chat.

I think your logic is sound as you've explained in 652, the core issue seems to be that we just copy the pointer into the reference object on the godot side instead of calling the proper setter logic.

What I'm wondering about is if this is good/safe enough. This will only work if the reference we're assigning is currently unset. If it is set that means we're just overwriting the current pointer and that object will become dangling.

What is should be doing is calling ref (https://github.com/godotengine/godot/blob/master/core/object/ref_counted.h#L61), but we can't call that from within Godot.

I wonder if this fix should be on the godot side instead of on the godot-cpp side, if that is even possible. Else we may have to enhance this logic to check if there is a current value set on the reference and unref that first.

Comment on lines 249 to 254
if (p_val != nullptr) {
p_val->reference();
*(void**)p_ptr = p_val->_owner;
} else {
*(void**)p_ptr = nullptr;
}
Copy link
Member

Choose a reason for hiding this comment

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

Style issue (spaces vs tabs).

@BastiaanOlij
Copy link
Collaborator

Talking some more on Godot chat we're looking at adding a method to the API that allows us to set a reference on the Godot side properly so the full logic runs.

@BastiaanOlij
Copy link
Collaborator

BastiaanOlij commented Nov 24, 2021

@kidrigger I don't have a good test case to test it with but this should improve your solution to make it a little more safer:
godotengine/godot#55277

This should change your code to:

_FORCE_INLINE_ static void encode(Ref<T> p_val, const void *p_ptr) {
	godot::internal::gdn_interface->reference_assign((GDNativeObjectPtr) p_ptr, (GDNativeObjectPtr)p_val->_owner);
}

This way we're sure that if a reference object is re-used it will properly clear the previous reference.

kb173 added a commit to boku-ilen/geodot-plugin that referenced this pull request Feb 3, 2022
These are likely related to refcounting issues.
The change in the SConstruct script fixes a crash that occured somewhere in `bind_get_argument_type` when registering classes and binding methods.
Commenting out the `delete`s in destructors fixed a `double free or corruption` crash that occured after the fix described above.

Likely related to godotengine/godot-cpp#652, but neither godotengine/godot-cpp#660 or godotengine/godot-cpp#662 seemed to help...
kb173 added a commit to boku-ilen/geodot-plugin that referenced this pull request Feb 15, 2022
These are likely related to refcounting issues.
The change in the SConstruct script fixes a crash that occured somewhere in `bind_get_argument_type` when registering classes and binding methods.
Commenting out the `delete`s in destructors fixed a `double free or corruption` crash that occured after the fix described above.

Likely related to godotengine/godot-cpp#652, but neither godotengine/godot-cpp#660 or godotengine/godot-cpp#662 seemed to help...
@zombietype zombietype closed this Jul 4, 2022
@akien-mga akien-mga added this to the 4.0 milestone Jul 22, 2022
@akien-mga akien-mga removed this from the 4.0 milestone Jul 22, 2022
@zhehangd zhehangd mentioned this pull request Aug 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

archived bug This has been identified as a bug topic:gdextension This relates to the new Godot 4 extension implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GDExtension Ref<T> object freeing error.

4 participants