Converting RefCounted to Variant causes premature free

[ttencate]

This code results in an unexpected panic:

eprintln!("new()");
let mut image = Image::new();
eprintln!("to_variant()");
let variant = image.to_variant();
eprintln!("drop()");
std::mem::drop(image);
eprintln!("try_from_variant()");
let image = Gd::<Image>::try_from_variant(&variant);

With --features=trace, I see this:

new()
to_variant()
drop()
Gd::drop   <godot_core::gen::classes::image::re_export::Image>
  Stat::dec   <godot_core::gen::classes::image::re_export::Image>
  +-- was last=true
try_from_variant()
  Stat::inc   <godot_core::gen::classes::image::re_export::Image>
thread '<unnamed>' panicked at 'as_ref_counted() on freed instance; maybe forgot to increment reference count?', godot-core/src/obj/gd.rs:373:9

Why does drop() see was last=true? Isn't the Variant supposed to hold another reference to the Image object?

---

Maybe related is this report that just came in on Discord:

#[func]
    fn process_img(&self, buffer: PackedByteArray) -> Gd<Image> {
        let mut img = Image::new();
        img.load_png_from_buffer(buffer);
        return img;
    }

but for some reason gdscript receives <Object#null>, not sure what i'm doing wrong here

---

I did some sleuthing:

• We call object_to_variant, which is obtained through get_variant_from_type_constructor in gdextension_interface.h.
• gdextension_get_variant_from_type_constructor returns a pointer to VariantTypeConstructor<Object *>::variant_from_type.
• This calls VariantInitializer<T>::init(variant); and VariantInternalAccessor<T>::set(variant, *((T *)p_value));. One by one:
  • VariantInitializer<T>::init(variant); calls VariantInternal::init_object(v);.
  • This calls object_assign_null(v); and then sets v->type = Variant::OBJECT;.
  • object_assign_null sets v->_get_obj().obj = nullptr; and v->_get_obj().id = ObjectID();. So that's a thing: a non-nil variant that holds a null object.
• We return to variant_from_type.
  • This calls VariantInternalAccessor<T>::set(variant, *((T *)p_value)); as mentioned.
  • This calls VariantInternal::object_assign(v, p_value);.
  • There are two overloads; we have T = Object so we end up in object_assign(Variant *v, const Object *o); // Needs RefCounted, so it's implemented elsewhere..
  • "Elsewhere" is variant_construct.cpp. If o is ref-counted, it calls init_ref() on it. On failure, it sets the variant to a null object.
  • Then it sets v->_get_obj().obj = const_cast<Object *>(o); and v->_get_obj().id = o->get_instance_id();.

So what does init_ref do, exactly? The documentation is not much help so let's turn to the source again:

• First it calls reference().
  • This calls refcount.refval(), which increments the ref count and returns the new one. Except if the ref count is zero, then it does nothing and returns zero. If that happens, reference() returns false.
  • Then reference() does some irrelevant stuff with script. Probably to allow an object to be destroyed if the only reference to it is from its script.
  • Then something with _get_extension()->reference(...) which may be related to gdextension but I'll ignore it for now.
  • Then it calls _instance_binding_reference(true); on Object. I don't understand what it does.
  • And then it returns true.

Long story short: init_ref increments the reference count. Not sure if there's a more complicated purpose here, or if it's just mis-named.

[ttencate]

Maybe related: godotengine/godot#64813

[lilizoey]

The discord issue mentioned has a workaround where you just somehow keep a shared reference to the img after returning. Which seems to support the idea that these are related somehow

[ttencate]

Smaller repro that brings us closer to the smoking gun:

eprintln!("new()");
let mut image = Image::new();
eprintln!("refcount is now {}", image.get_reference_count());
eprintln!("to_variant()");
let variant = image.to_variant();
eprintln!("refcount is now {}", image.get_reference_count());

Output:

new()
refcount is now 1
to_variant()
refcount is now 1

The last one should be 2 of course.

[Bromeon]

Thanks for the help with debugging, especially the "smaller repro" in the last comment was very useful to locate the issue. In case you're interested about what exactly went wrong, the PR contains a detailed description.

[ttencate]

Fix confirmed to work in integration tests for #101. Thank you!