[3.x] Promote object validity checks to release builds

core/io/marshalls.cpp

@@ -786,18 +786,16 @@ Error encode_variant(const Variant &p_variant, uint8_t *r_buffer, int &r_len, bo
 			}
 		} break;
 		case Variant::OBJECT: {
-#ifdef DEBUG_ENABLED
-			// Test for potential wrong values sent by the debugger when it breaks.
+			// Test for potential wrong values sent by the debugger when it breaks or freed objects.
 			Object *obj = p_variant;
-			if (!obj || !ObjectDB::instance_validate(obj)) {
+			if (!obj) {
 				// Object is invalid, send a NULL instead.
 				if (buf) {
 					encode_uint32(Variant::NIL, buf);
 				}
 				r_len += 4;
 				return OK;
 			}
-#endif // DEBUG_ENABLED
 			if (!p_full_objects) {
 				flags |= ENCODE_FLAG_OBJECT_AS_ID;
 			}
@@ -1090,7 +1088,7 @@ Error encode_variant(const Variant &p_variant, uint8_t *r_buffer, int &r_len, bo
 				if (buf) {
 					Object *obj = p_variant;
 					ObjectID id = 0;
-					if (obj && ObjectDB::instance_validate(obj)) {
+					if (obj) {
 						id = obj->get_instance_id();
 					}
 

core/object.cpp

@@ -961,7 +961,6 @@ void Object::cancel_delete() {
 	_predelete_ok = true;
 }
 
-#ifdef DEBUG_ENABLED
 ObjectRC *Object::_use_rc() {
 	// The RC object is lazily created the first time it's requested;
 	// that way, there's no need to allocate and release it at all if this Object
@@ -989,7 +988,6 @@ ObjectRC *Object::_use_rc() {
 		rc = _rc.load(std::memory_order_acquire);
 	}
 }
-#endif
 
 void Object::set_script_and_instance(const RefPtr &p_script, ScriptInstance *p_instance) {
 	//this function is not meant to be used in any of these ways
@@ -1927,9 +1925,7 @@ Object::Object() {
 	_emitting = false;
 	memset(_script_instance_bindings, 0, sizeof(void *) * MAX_SCRIPT_INSTANCE_BINDINGS);
 	script_instance = nullptr;
-#ifdef DEBUG_ENABLED
 	_rc.store(nullptr, std::memory_order_release);
-#endif
 #ifdef TOOLS_ENABLED
 
 	_edited = false;
@@ -1942,14 +1938,12 @@ Object::Object() {
 }
 
 Object::~Object() {
-#ifdef DEBUG_ENABLED
 	ObjectRC *rc = _rc.load(std::memory_order_acquire);
 	if (rc) {
 		if (rc->invalidate()) {
 			memdelete(rc);
 		}
 	}
-#endif
 
 	if (script_instance) {
 		memdelete(script_instance);

core/object.h

@@ -41,9 +41,7 @@
 #include "core/variant.h"
 #include "core/vmap.h"
 
-#ifdef DEBUG_ENABLED
-#include <atomic> // For ObjectRC*
-#endif
+#include <atomic>
 
 #define VARIANT_ARG_LIST const Variant &p_arg1 = Variant(), const Variant &p_arg2 = Variant(), const Variant &p_arg3 = Variant(), const Variant &p_arg4 = Variant(), const Variant &p_arg5 = Variant()
 #define VARIANT_ARG_PASS p_arg1, p_arg2, p_arg3, p_arg4, p_arg5
@@ -476,9 +474,7 @@ class Object {
 	int _predelete_ok;
 	Set<Object *> change_receptors;
 	ObjectID _instance_id;
-#ifdef DEBUG_ENABLED
 	std::atomic<ObjectRC *> _rc;
-#endif
 	bool _predelete();
 	void _postinitialize();
 	bool _can_translate;
@@ -590,9 +586,7 @@ class Object {
 		return &ptr;
 	}
 
-#ifdef DEBUG_ENABLED
 	ObjectRC *_use_rc();
-#endif
 
 	bool _is_gpl_reversed() const { return false; }
 
@@ -798,6 +792,7 @@ class ObjectDB {
 	static void debug_objects(DebugFunc p_func);
 	static int get_object_count();
 
+	// This one may give false positives because a new object may be allocated at the same memory of a previously freed one
 	_FORCE_INLINE_ static bool instance_validate(Object *p_ptr) {
 		rw_lock.read_lock();
 

core/object_rc.h

@@ -31,8 +31,6 @@
 #ifndef OBJECTRC_H
 #define OBJECTRC_H
 
-#ifdef DEBUG_ENABLED
-
 #include "core/os/memory.h"
 #include "core/typedefs.h"
 
@@ -77,5 +75,3 @@ class ObjectRC {
 };
 
 #endif
-
-#endif

core/variant.cpp

@@ -817,11 +817,15 @@ ObjectID Variant::get_object_instance_id() const {
 	if (is_ref() && _get_obj().ref.is_null()) {
 		return 0;
 	} else {
-		return _get_obj().obj->get_instance_id();
+		return _get_obj().rc->get_ptr()->get_instance_id();
 	}
 #endif
 }
 
+bool Variant::is_invalid_object() const {
+	return type == OBJECT && _get_obj().rc && !_get_obj().rc->get_ptr();
+}
+
 void Variant::reference(const Variant &p_variant) {
 	switch (type) {
 		case NIL:
@@ -896,11 +900,9 @@ void Variant::reference(const Variant &p_variant) {
 		} break;
 		case OBJECT: {
 			memnew_placement(_data._mem, ObjData(p_variant._get_obj()));
-#ifdef DEBUG_ENABLED
-			if (_get_obj().rc) {
+			if (likely(_get_obj().rc)) {
 				_get_obj().rc->increment();
 			}
-#endif
 		} break;
 		case NODE_PATH: {
 			memnew_placement(_data._mem, NodePath(*reinterpret_cast<const NodePath *>(p_variant._data._mem)));
@@ -1018,18 +1020,13 @@ void Variant::clear() {
 			reinterpret_cast<NodePath *>(_data._mem)->~NodePath();
 		} break;
 		case OBJECT: {
-#ifdef DEBUG_ENABLED
 			if (likely(_get_obj().rc)) {
 				if (unlikely(_get_obj().rc->decrement())) {
 					memdelete(_get_obj().rc);
 				}
 			} else {
 				_get_obj().ref.unref();
 			}
-#else
-			_get_obj().obj = NULL;
-			_get_obj().ref.unref();
-#endif
 		} break;
 		case _RID: {
 			// not much need probably
@@ -1511,18 +1508,12 @@ String Variant::stringify(List<const void *> &stack) const {
 		} break;
 		case OBJECT: {
 			Object *obj = _OBJ_PTR(*this);
-			if (obj) {
-				if (_get_obj().ref.is_null() && !ObjectDB::get_instance(obj->get_instance_id())) {
-					return "[Deleted Object]";
-				}
-
+			if (likely(obj)) {
 				return obj->to_string();
 			} else {
-#ifdef DEBUG_ENABLED
-				if (ScriptDebugger::get_singleton() && _get_obj().rc && !ObjectDB::get_instance(_get_obj().rc->instance_id)) {
+				if (_get_obj().rc) {
 					return "[Deleted Object]";
 				}
-#endif
 				return "[Object:null]";
 			}
 		} break;
@@ -1678,20 +1669,13 @@ Variant::operator RID() const {
 		if (!_get_obj().ref.is_null()) {
 			return _get_obj().ref.get_rid();
 		} else {
-#ifdef DEBUG_ENABLED
 			Object *obj = likely(_get_obj().rc) ? _get_obj().rc->get_ptr() : nullptr;
 			if (unlikely(!obj)) {
-				if (ScriptDebugger::get_singleton() && _get_obj().rc && !ObjectDB::get_instance(_get_obj().rc->instance_id)) {
+				if (_get_obj().rc) {
 					ERR_PRINT("Attempted get RID on a deleted object.");
 				}
 				return RID();
 			}
-#else
-			Object *obj = _get_obj().obj;
-			if (unlikely(!obj)) {
-				return RID();
-			}
-#endif
 			Variant::CallError ce;
 			Variant ret = obj->call(CoreStringNames::get_singleton()->get_rid, nullptr, 0, ce);
 			if (ce.error == Variant::CallError::CALL_OK && ret.get_type() == Variant::_RID) {
@@ -1714,22 +1698,14 @@ Variant::operator Object *() const {
 }
 Variant::operator Node *() const {
 	if (type == OBJECT) {
-#ifdef DEBUG_ENABLED
 		Object *obj = _get_obj().rc ? _get_obj().rc->get_ptr() : nullptr;
-#else
-		Object *obj = _get_obj().obj;
-#endif
 		return Object::cast_to<Node>(obj);
 	}
 	return nullptr;
 }
 Variant::operator Control *() const {
 	if (type == OBJECT) {
-#ifdef DEBUG_ENABLED
 		Object *obj = _get_obj().rc ? _get_obj().rc->get_ptr() : nullptr;
-#else
-		Object *obj = _get_obj().obj;
-#endif
 		return Object::cast_to<Control>(obj);
 	}
 	return nullptr;
@@ -2182,12 +2158,7 @@ Variant::Variant(const NodePath &p_node_path) {
 Variant::Variant(const RefPtr &p_resource) {
 	type = OBJECT;
 	memnew_placement(_data._mem, ObjData);
-#ifdef DEBUG_ENABLED
 	_get_obj().rc = nullptr;
-#else
-	REF *ref = reinterpret_cast<REF *>(p_resource.get_data());
-	_get_obj().obj = ref->ptr();
-#endif
 	_get_obj().ref = p_resource;
 }
 
@@ -2204,15 +2175,10 @@ Variant::Variant(const Object *p_object) {
 	Reference *ref = Object::cast_to<Reference>(obj);
 	if (unlikely(ref)) {
 		*reinterpret_cast<Ref<Reference> *>(_get_obj().ref.get_data()) = Ref<Reference>(ref);
-#ifdef DEBUG_ENABLED
 		_get_obj().rc = nullptr;
 	} else {
 		_get_obj().rc = likely(obj) ? obj->_use_rc() : nullptr;
-#endif
 	}
-#if !defined(DEBUG_ENABLED)
-	_get_obj().obj = obj;
-#endif
 }
 
 Variant::Variant(const Dictionary &p_dictionary) {
@@ -2490,19 +2456,15 @@ void Variant::operator=(const Variant &p_variant) {
 			*reinterpret_cast<RID *>(_data._mem) = *reinterpret_cast<const RID *>(p_variant._data._mem);
 		} break;
 		case OBJECT: {
-#ifdef DEBUG_ENABLED
 			if (likely(_get_obj().rc)) {
 				if (unlikely(_get_obj().rc->decrement())) {
 					memdelete(_get_obj().rc);
 				}
 			}
-#endif
 			*reinterpret_cast<ObjData *>(_data._mem) = p_variant._get_obj();
-#ifdef DEBUG_ENABLED
 			if (likely(_get_obj().rc)) {
 				_get_obj().rc->increment();
 			}
-#endif
 		} break;
 		case NODE_PATH: {
 			*reinterpret_cast<NodePath *>(_data._mem) = *reinterpret_cast<const NodePath *>(p_variant._data._mem);

core/variant.h

@@ -73,22 +73,12 @@ typedef PoolVector<Color> PoolColorArray;
 #define GCC_ALIGNED_8
 #endif
 
-// With DEBUG_ENABLED, the pointer to a deleted object stored in ObjectRC is set to nullptr,
-// so _OBJ_PTR is not useful for checks in which we want to act as if we still believed the
-// object is alive; e.g., comparing a Variant that points to a deleted object with NIL,
-// should return false regardless DEBUG_ENABLED is defined or not.
-// So in cases like that we use _UNSAFE_OBJ_PROXY_PTR, which serves that purpose. With DEBUG_ENABLED
-// it won't be the real pointer to the object for non-Reference types, but that's fine.
-// We just need it to be unique for each object, to be comparable and not to be forced to NULL
-// when the object is freed.
-#ifdef DEBUG_ENABLED
 #define _REF_OBJ_PTR(m_variant) (reinterpret_cast<Ref<Reference> *>((m_variant)._get_obj().ref.get_data())->ptr())
 #define _OBJ_PTR(m_variant) ((m_variant)._get_obj().rc ? (m_variant)._get_obj().rc->get_ptr() : _REF_OBJ_PTR(m_variant))
+// _UNSAFE_OBJ_PROXY_PTR is needed for comparing an object Variant against NIL or compare two object Variants.
+// It's guaranteed to be unique per object, in contrast to the pointer stored in the RC structure,
+// which is set to null when the object is destroyed.
 #define _UNSAFE_OBJ_PROXY_PTR(m_variant) ((m_variant)._get_obj().rc ? reinterpret_cast<uint8_t *>((m_variant)._get_obj().rc) : reinterpret_cast<uint8_t *>(_REF_OBJ_PTR(m_variant)))
-#else
-#define _OBJ_PTR(m_variant) ((m_variant)._get_obj().obj)
-#define _UNSAFE_OBJ_PROXY_PTR(m_variant) _OBJ_PTR(m_variant)
-#endif
 
 class Variant {
 public:
@@ -149,13 +139,9 @@ class Variant {
 	Type type;
 
 	struct ObjData {
-#ifdef DEBUG_ENABLED
 		// Will be null for every type deriving from Reference as they have their
 		// own reference count mechanism
 		ObjectRC *rc;
-#else
-		Object *obj;
-#endif
 		// Always initialized, but will be null if the Ref<> assigned was null
 		// or this Variant is not even holding a Reference-derived object
 		RefPtr ref;
@@ -193,6 +179,7 @@ class Variant {
 	bool is_one() const;
 
 	ObjectID get_object_instance_id() const;
+	bool is_invalid_object() const;
 
 	operator bool() const;
 	operator signed int() const;

core/variant_call.cpp

@@ -1162,9 +1162,9 @@ void Variant::call_ptr(const StringName &p_method, const Variant **p_args, int p
 	if (type == Variant::OBJECT) {
 		//call object
 		Object *obj = _OBJ_PTR(*this);
-		if (!obj) {
+		if (unlikely(!obj)) {
 #ifdef DEBUG_ENABLED
-			if (ScriptDebugger::get_singleton() && _get_obj().rc && !ObjectDB::get_instance(_get_obj().rc->instance_id)) {
+			if (_get_obj().rc) {
 				ERR_PRINT("Attempted method call on a deleted object.");
 			}
 #endif
@@ -1374,9 +1374,9 @@ Variant Variant::construct(const Variant::Type p_type, const Variant **p_args, i
 bool Variant::has_method(const StringName &p_method) const {
 	if (type == OBJECT) {
 		Object *obj = _OBJ_PTR(*this);
-		if (!obj) {
+		if (unlikely(!obj)) {
 #ifdef DEBUG_ENABLED
-			if (ScriptDebugger::get_singleton() && _get_obj().rc && !ObjectDB::get_instance(_get_obj().rc->instance_id)) {
+			if (_get_obj().rc) {
 				ERR_PRINT("Attempted method check on a deleted object.");
 			}
 #endif