Object: Let debug lock handle callee destruction within call chain gracefully

[RandomShaper]

Fixes #75658.

[AThousandShips]

How does this relate to:

• Player crashes randomly in Godot 4.2 on macOS #86788

And this fix:

• [GDScript] Fix lifetime of call base object #86969

[RandomShaper]

How does this relate to:

• Player crashes randomly in Godot 4.2 on macOS #86788

And this fix:

• [GDScript] Fix lifetime of call base object #86969

Wow, I had totally forgotten I was involved there.

Now I understand the issue better, I can say that GDScript already ensures the lifetime of a RefCounted base object, because it stores it on the stack, so it will survive at least up to the end of the call. Therefore, I'd say there's no need for the extra temporaries added by the compiler in #86969.

[RandomShaper]

Of course, using Variant has the side effect of deferring the destruction of the object until after the call, so I've modified the approach in a way that the refcount is not artificially incremented, while still having safety if the object is released prior to returning.

[lawnjelly]

This looks pretty good and identical to my approach in the issue (great minds think alike and all that 😁 ), so am almost there to approve.

The only thing I wasn't sure about is how often this gets called in practice with every call to an object, and how expensive the ObjectDB lookup is (hashtable?) and whether we should be looking to avoid it, even in debug. i.e. does it appear on a profile. I suspect not, but would be nice to check.

An alternative would be to optionally store e.g. a pointer to a bool on each Object, which can inform the calling code when an object has been destroyed. This is O(1) but has the disadvantage of storing an extra pointer on each object.

If the Object::call() isn't used that often it isn't worth micro-optimizing though.

UPDATE:
Actually nearly every alternative I can think up either has potential bugs (due to e.g. the chain of calls containing more than one reference to the object), or may not be O(1) themselves, so maybe we should just go for this and work further if it introduces any performance issues.

[AThousandShips]

Makes sense and looks good to me

[AThousandShips]

Would be good to add some tests if we can reliably reproduce these issues

[RandomShaper]

Regarding performance, I wouldn't worry much since ObjectDB::get_instance() is very fast, and O(1), as it's based on generational ids. On the other hand, I think there's an opportunity to revamp this debug lock feature and maybe make it work only if the debugger is attached (like certain Variant object validity checks do) and/or make a very different implementation. After all, the only goal of the debug lock is prevent free() from being called if the object is at that point involved in a call chain.

I've force-pushed with an added likely(), and also (due to the striking coincidence) including @lawnjelly as a co-author (that's the least a gentleman can do 😀).

I'm adding some tests and will push again.

[lawnjelly]

I've force-pushed with an added likely(), and also (due to the striking coincidence) including @lawnjelly as a co-author (that's the least a gentleman can do 😀).

Ah yes I'll do that on mine too. 👍

[RandomShaper]

Pushed some tests, as "black-boxey" as possible. There are other cases of OBJ_DEBUG_LOCK used in object.cpp (emit_signalp for the emitting object and set_script()), but in those it wouldn't be safe that the object destroyed itself.

[lawnjelly]

I don't know the first thing about tests, but given the MRP only crashes with asan and debug build, does the test fail with the old code (with the normal tests build)?

[RandomShaper]

I don't know the first thing about tests, but given the MRP only crashes with asan and debug build, does the test fail with the old code (with the normal tests build)?

It will only fail with asan enabled, but luckily CI runs jobs that compile with sanitizers and runs tests on such builds.

[RandomShaper]

I've been able to come up with a way of letting a crash happen despite asan not enabled. Not as guaranteed, but makes the test crash with the prior implementation of _ObjectDebugLock.

[RandomShaper]

Wow, getting the non-asan/tsan version of the crash induction to work on every platform has proven challenging.

Also, I've added tests for GDScript as its script instance and VM implementation have to guarantee the behavior. Those will only fail with asan/tsan, though.

I'm fairly happy with this now.

[akien-mga]

Thanks!

[akien-mga]

Cherry-picked for 4.3.1.