Godot: Crashes when working with Image (C++, GDNative)

[mnn]

Godot version: 3.1 64 (bindings are 05e5f5cd5e88e5d7ed408a421424443c1571eeb3)

OS/device including version: Linux 64

Issue description:

Calling get_height on Image causes a crash:

handle_crash: Program crashed with signal 11
Dumping the backtrace. Please include this when reporting the bug on https://github.com/godotengine/godot/issues
[1] /lib/x86_64-linux-gnu/libc.so.6(+0x354b0) [0x7ffff7a424b0] (??:0)
[2] ./xxx.x86_64() [0x14b08d0] (??:?)
[3] ./xxx.x86_64() [0x799ead] (??:?)
[4] godot::___godot_icall_int(godot_method_bind*, godot::Object const*) (??:0)
[5] godot::Image::get_height() const (??:0)
[6] godot::BlockTexAtlas::GenerateTexture(godot::BlockDB&) (??:0)
[7] godot::VoxWorld::_ready() (??:0)
[8] godot_variant godot::__wrapped_method<godot::VoxWorld, void>(void*, void*, void*, int, godot_variant**) (??:0)
[9] ./xxx.x86_64() [0x8713bb] (<artificial>:?)
[10] ./xxx.x86_64() [0x8e104d] (<artificial>:?)
[11] ./xxx.x86_64() [0xb5bb95] (??:?)
[12] ./xxx.x86_64() [0x65e8a6] (??:?)
[13] ./xxx.x86_64() [0x1ac60f4] (??:?)
[14] ./xxx.x86_64() [0xb0044c] (<artificial>:?)
[15] ./xxx.x86_64() [0xb003a3] (<artificial>:?)
[16] ./xxx.x86_64() [0xb003a3] (<artificial>:?)
[17] ./xxx.x86_64() [0xb5b517] (<artificial>:?)
[18] ./xxx.x86_64() [0xb5b626] (??:?)
[19] ./xxx.x86_64() [0x6504cc] (??:?)
[20] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf0) [0x7ffff7a2d830] (??:0)
[21] ./xxx.x86_64() [0x65db2e] (??:?)

Steps to reproduce:

    auto blockImg = Image::_new();
    String texName = texNames[i];
    auto loadErr = blockImg->load(texName);
    if (loadErr != Error::OK) {
      G::print("Failed to load block texture '{0}', error {1}.", texName, static_cast<int>( loadErr));
    } else {
      G::print("Successfully loaded block texture '{0}'.", texName);
    }
    G::print("is nullptr = {0}", blockImg == nullptr);
    G::print("size = {0} x {1}", blockImg->get_width(), blockImg->get_height());

where G is Godot alias. Output (followed by backtrace):

Successfully loaded block texture 'res://mods/core/textures/blocks/stone.png'.
is nullptr = False

I think it is related to load, because this:

  auto img = Image::_new();
  img->create(AtlasSideSize, AtlasSideSize, false, Image::FORMAT_RGBA8);
  G::print("img size = {0} x {1}", img->get_width(), img->get_height());

works without crashing.

Also it is not isolated to just get_height, several operations I tried also lead to a crash - get_width, using it as a parameter for blit_rect of other image (via Ref - crash on godot::Reference::init_ref()).

get_height and get_width was working in 3.1rc1, but broke after updating to 3.1. The Ref crash was present on 3.1rc1 and older too.

Being able to load (dynamically) textures is crucial for our project.

[bojidar-bg]

Does the bug also occur without GDNative (maybe test with C# as it also uses ptrcall).

[mnn]

Found a solution:

    auto blockImg = Ref<Image>(Image::_new());
    auto loadErr = blockImg->load(texName);

So wrapping late into Ref was causing the crashes. I didn't found any documentation to Ref (nothing in bindings source and online docs, or maybe I missed it?), so it is possible I am just using it wrong...

Please, if this isn't a bug, feel free to close it.

[akien-mga]

Ref gives you a valid pointer, but it doesn't necessarily mean that the data it points to is valid. You should use blockImg.is_valid() instead of comparing to nullptr.

[mnn]

The comparison to null is just for testing purposes, because blit_rect was crashing in 3.1rc1 claiming the pointer in Ref was null. There is something going on with how Ref works which escapes me. I didn't think it matters when I do the wrapping (as long as pointer/object is ok), but obviously it does matter.

[karroffel]

I think this is expected. Maybe the bindings need to be changed to make this more explicit.

Image is a reference type, when it's created via GDNative directly the refcount is 0.
If inside of the create() code somewhere a new Ref<Image> is made from this then the Ref<Image> destructor will see that it's the last one with a reference to it, so it will destroy the image.

Maybe the Image::_new() can return a Ref<Image> instead of an Image *, that should solve the issue I think.

[mnn]

I believe it should be prominently stated in docs how Ref is supposed to be used with plenty of exmples. Just fixed a mysterious bug, where sometimes (1 in 100 cases or so) it caused a crash and is also probably responsible for a crash on ending the game.

Wrong:

  auto *shape = ConcavePolygonShape::_new();
...
    shape->set_faces(collisionPool);
    collision->set_shape(Ref<ConcavePolygonShape>(shape));

Correct:

    auto shape = Ref<ConcavePolygonShape>(ConcavePolygonShape::_new());
    shape->set_faces(collisionPool);
    collision->set_shape(shape);

The worst thing is how stealthy the adverse effect of this bug/misuse is. Just a warning in a console which doesn't make any sense unless you know about engine internals (broken asserts in physics engine), and then sometimes in unrelated places in code it causes a crash.

I can image hunting this bug in a non-tiny codebase (unlike mine, where I had literally one or two suspect places) could easily take hours or more.