Fix segmentation fault when using Ref

[PhilWun]

This should fix #343

[BastiaanOlij]

Weird change, can you elaborate why this fixes the issue?

[victorholt]

@PhilWun @BastiaanOlij very odd, but nice, thanks for the fix..., been trying to get Ref(T::_new()) working all day. Seems like something is broken with the instancing in 3.2 for custom classes, not built-in classes which do not use the GODOT_CLASS() macro.

[API-Beast]

I have manually merged this request on local and I can confirm that it indeed fixes the issue. A quite critical issue if you ask me.

[TheSHEEEP]

Adding my voice here, too.
It is an odd fix, for sure - but as long as it works, the "oddity" isn't that relevant, really.

This should definitely be merged.

[sheepandshepherd]

The fix (making NativeScript::new_() add to the ref-count) is mostly correct, but it'd be good to move it to the call site where the early free actually happens (GODOT_CLASS in Godot.hpp):

Name *instance = godot::as<Name>(script->new_());
//                │               │
//                │               └─> returns Variant: ref-count is 1
//                └─> returns Name*: ref-count drops to 0 when the Variant is destroyed

Otherwise NativeScript::new_() will now leak memory anywhere else it might be used.

Also, the Ref itself leaks now too. The Ref(T *) constructor adds to the ref-count a second time. It will need to be fixed.
It calls init_ref(), which behaves differently for NativeScript References because their refcount_init is no longer 1:

Reference *r = Reference::_new(); // refcount = 1, refcount_init = 1
Ref<Reference> r_ref(r); // refcount = 1, refcount_init = 0

CustomResource *c = CustomResource::_new(); // refcount = 1, refcount_init = 0
Ref<CustomResource> c_ref(c); // refcount = 2, refcount_init = 0

So, the CustomResource leaks.

#350 fixes this too, but requires changes to Godot. Maybe we can find a cleaner solution that doesn't require it? But we have no access to refcount_init in GDNative, so idk how to specialize Ref(T *) based on it.

[TheSHEEEP]

You aren't wrong, but a memory leak (especially as there is seemingly a workaround for it) is preferable to an outright "you cannot use this fairly important class at all because it crashes".
At least until a more permanent solution can be found.

After all, this has to move ahead somehow and some of these issues have been out for 2+ years :/

[sheepandshepherd]

Alright, you have a good point. And this has been one of the most mentioned issues in the #gdnative_dev Discord for a long time, so it really should be fixed ASAP. I made a PR with this same fix, but moved it into the buggy _new() function as mentioned above.

Hopefully afterwards we can find a way to fix the Ref(T *) leak.

[Zylann]

Should no longer be needed after #408

[follower]

For the sake of completeness: I just noticed the following comment which may go some way to explain this commit: #343 (comment)

I have the same problem. It happened after I updated api.json. I tested around and found out, that the problem is, that the new function of NativeScript returns now a Variant instead of an Object. The commit that changed that is godotengine/godot@e2121c9.

[...]

The cause of the segmentation fault is, that the reference count hits 0 during the _new() call. The object gets deleted and a dangling pointer remains.

But as Zylann mentioned, after #408 this shouldn't be necessary.

[Zylann]

Closing since the cause of the leak was fixed.