Fix RefCounted releasing early and not clearing reference

[rune-scape]

+organized the code +Ref now releases After aquiring other reference +fixed case where reference returns false should clear the reference

kinda maybe a major bug thats been here since the first commit probly related to alot of crashes when closing the editor fixes many cases of ~GDScript() crashing when cleaning up

(ive been using this code for months now)

[RandomShaper]

It'd be great that the refactor and the fixes were in separate commits. Do you think you could split the changes like that?

[rune-scape]

ive separated one of the fixes into its own commit, but making references release after acquiring is tied into the refactor and hard to separate

ive also refactored the referencing code in variant.cpp into a set of functions that mirror the ones in ref_counted.h, to make it easier compare, as they should be doing the exact same thing

[RandomShaper]

Thanks. I will need more time to review this properly, but so far I can acknowledge this fixes the race condition were an object being released by another thread is assigned to a Ref, making it dangling.

[rune-scape]

i didnt realize there was another set of functions manipulating refcounts,, i refactored/modified VariantInternal::object_assign and other related functions to use the same ones as variant

theres still refcounting code in CallableCustom that releases before aquiring, not sure if that should be in this PR ..

[rune-scape]

1. i like this, will make this change, but Variant::ObjData::ref() and ref_pointer() are too specialized
2. imo this is definitely an over optimization, null references are common and not an error, enforcing cpu branch prediction on common cases can make performance worse in some cases for little to no benefit
3. i think ive been told ptr == nullptr is preferred and its what i prefer
4. true
5. ya, i don't think the separate commits makes sense

[RandomShaper]

1. Any comments on my suggestion about using reference->unreference() instead of the cleanup ref approach?
2. OK. - (We'd actually have to judge on each condition separately. Maybe some of the identity checks would benefit? Well, since I can't back this with data, I agree that'd be premature optimization and it's also out of the scope of this PR.)
3. OK. - (My suggestion was more about the not-null checks. I find it harder to parse != nullptr as checking for a positive fact. However, yeah, there's no much point in discussing about that. It's your call.)

[akien-mga]

i think ive been told ptr == nullptr is preferred and its what i prefer

It's not necessarily consistent throughout the codebase, but I do believe the "preferred" usage currently is if (ptr) and if (!ptr). We even used to have some scripts that would remove != nullptr automatically from the codebase, though I think we stopped doing that as it was a bit of a pain and overly pedantic.

I believe what we indicated was that for Ref<T> types, we prefer using the is_null() and is_valid() methods (which was now made consistent with #96532 to enforce it). But for actual raw pointer types using them as bool is the most widespread convention among contributors.

[rune-scape]

1. Any comments on my suggestion about using reference->unreference() instead of the cleanup ref approach?

manually calling it doesn't deal with refcount any more than the cleanup_ref way, the cleanup_ref is default constructed and the data copied its then destructed (unrefed) at the end of the scope, and by using the object lifetime to cleanup i was trying to convey that it should be the last thing done in the function (also the way youve written it would release the reference first, and id call unref() instead of reference->unreference())

switched to using if (ptr) rebased to master i also made the change to Callable to release after aquiring, bc it's very related and could have similar effects

[akien-mga]

Thanks!