Merge pull request #454 from godot-rust/bugfix/free-while-bound

Bugfix: prevent `Gd` destruction while user object is bound

Author: Bromeon

Cargo.toml

@@ -15,3 +15,6 @@ members = [
     # utils
     "godot-fmt",
 ]
+
+#[patch."https://github.com/godot-rust/godot4-prebuilt"]
+#godot4-prebuilt = { git = "https://github.com//godot-rust/godot4-prebuilt", branch = "4.1"}

godot-core/src/lib.rs

@@ -82,7 +82,7 @@ pub mod private {
             && *global_config.is_editor.get_or_init(is_editor)
     }
 
-    fn print_panic(err: Box<dyn std::any::Any + Send>) {
+    pub fn print_panic(err: Box<dyn std::any::Any + Send>) {
         if let Some(s) = err.downcast_ref::<&'static str>() {
             print_panic_message(s);
         } else if let Some(s) = err.downcast_ref::<String>() {

godot-core/src/obj/gd.rs

@@ -206,10 +206,12 @@ impl<T: GodotClass> Gd<T> {
         })
     }
 
-    /// ⚠️ Returns the last known, possibly invalid instance ID of this object.
+    /// Returns the last known, possibly invalid instance ID of this object.
     ///
     /// This function does not check that the returned instance ID points to a valid instance!
     /// Unless performance is a problem, use [`instance_id()`][Self::instance_id] instead.
+    ///
+    /// This method is safe and never panics.
     pub fn instance_id_unchecked(&self) -> InstanceId {
         // SAFETY:
         // A `Gd` can only be created from a non-null `RawGd`. Meaning `raw.instance_id_unchecked()` will
@@ -390,16 +392,22 @@ where
     /// example to the node tree in case of nodes.
     ///
     /// # Panics
-    /// * When the referred-to object has already been destroyed.
-    /// * When this is invoked on an upcast `Gd<Object>` that dynamically points to a reference-counted type (i.e. operation not supported).
+    /// - When the referred-to object has already been destroyed.
+    /// - When this is invoked on an upcast `Gd<Object>` that dynamically points to a reference-counted type (i.e. operation not supported).
+    /// - When the object is bound by an ongoing `bind()` or `bind_mut()` call (through a separate `Gd` pointer).
     pub fn free(self) {
+        // Note: this method is NOT invoked when the free() call happens dynamically (e.g. through GDScript or reflection).
+        // As such, do not use it for operations and validations to perform upon destruction.
+
         // TODO disallow for singletons, either only at runtime or both at compile time (new memory policy) and runtime
+        use dom::Domain;
 
         // Runtime check in case of T=Object, no-op otherwise
         let ref_counted = T::Mem::is_ref_counted(&self.raw);
         assert_ne!(
             ref_counted, Some(true),
-            "called free() on Gd<Object> which points to a RefCounted dynamic type; free() only supported for manually managed types."
+            "called free() on Gd<Object> which points to a RefCounted dynamic type; free() only supported for manually managed types\n\
+            object: {self:?}"
         );
 
         // If ref_counted returned None, that means the instance was destroyed
@@ -408,7 +416,17 @@ where
             "called free() on already destroyed object"
         );
 
-        // This destroys the Storage instance, no need to run destructor again
+        // SAFETY: object must be alive, which was just checked above. No multithreading here.
+        // Also checked in the C free_instance_func callback, however error message can be more precise here and we don't need to instruct
+        // the engine about object destruction. Both paths are tested.
+        let bound = unsafe { T::Declarer::is_currently_bound(&self.raw) };
+        assert!(
+            !bound,
+            "called free() while a bind() or bind_mut() call is active"
+        );
+
+        // SAFETY: object alive as checked.
+        // This destroys the Storage instance, no need to run destructor again.
         unsafe {
             sys::interface_fn!(object_destroy)(self.raw.obj_sys());
         }

godot-core/src/obj/guards.rs

@@ -4,6 +4,8 @@
  * file, You can obtain one at https://mozilla.org/MPL/2.0/.
  */
 
+use godot_ffi::out;
+
 #[cfg(not(feature = "experimental-threads"))]
 use std::cell;
 use std::fmt::Debug;
@@ -31,6 +33,7 @@ impl<'a, T> GdRef<'a, T> {
 
     #[cfg(feature = "experimental-threads")]
     pub(crate) fn from_cell(cell_ref: sync::RwLockReadGuard<'a, T>) -> Self {
+        out!("GdRef init: {:?}", std::any::type_name::<T>());
         Self { cell_ref }
     }
 }
@@ -43,6 +46,12 @@ impl<T> Deref for GdRef<'_, T> {
     }
 }
 
+impl<T> Drop for GdRef<'_, T> {
+    fn drop(&mut self) {
+        out!("GdRef drop: {:?}", std::any::type_name::<T>());
+    }
+}
+
 // TODO Clone or Share
 
 // ----------------------------------------------------------------------------------------------------------------------------------------------
@@ -67,6 +76,7 @@ impl<'a, T> GdMut<'a, T> {
 
     #[cfg(feature = "experimental-threads")]
     pub(crate) fn from_cell(cell_ref: sync::RwLockWriteGuard<'a, T>) -> Self {
+        out!("GdMut init: {:?}", std::any::type_name::<T>());
         Self { cell_ref }
     }
 }
@@ -84,3 +94,9 @@ impl<T> DerefMut for GdMut<'_, T> {
         self.cell_ref.deref_mut()
     }
 }
+
+impl<T> Drop for GdMut<'_, T> {
+    fn drop(&mut self) {
+        out!("GdMut drop: {:?}", std::any::type_name::<T>());
+    }
+}

godot-core/src/obj/raw.rs

@@ -461,7 +461,8 @@ impl<T: GodotClass> Drop for RawGd<T> {
     fn drop(&mut self) {
         // No-op for manually managed objects
 
-        out!("RawGd::drop   <{}>", std::any::type_name::<T>());
+        // out!("RawGd::drop   <{}>", std::any::type_name::<T>());
+
         // SAFETY: This `Gd` wont be dropped again after this.
         let is_last = unsafe { T::Mem::maybe_dec_ref(self) }; // may drop
         if is_last {

godot-core/src/obj/traits.rs

@@ -275,6 +275,15 @@ pub mod dom {
         where
             T: GodotClass<Declarer = Self>,
             F: FnOnce(&mut T) -> R;
+
+        /// Check if the object is a user object *and* currently locked by a `bind()` or `bind_mut()` guard.
+        ///
+        /// # Safety
+        /// Object must be alive.
+        #[doc(hidden)]
+        unsafe fn is_currently_bound<T>(obj: &RawGd<T>) -> bool
+        where
+            T: GodotClass<Declarer = Self>;
     }
 
     /// Expresses that a class is declared by the Godot engine.
@@ -293,6 +302,13 @@ pub mod dom {
                     .expect("scoped mut should not be called on a null object"),
             )
         }
+
+        unsafe fn is_currently_bound<T>(_obj: &RawGd<T>) -> bool
+        where
+            T: GodotClass<Declarer = Self>,
+        {
+            false
+        }
     }
 
     /// Expresses that a class is declared by the user.
@@ -309,6 +325,13 @@ pub mod dom {
             let mut guard = obj.bind_mut();
             closure(&mut *guard)
         }
+
+        unsafe fn is_currently_bound<T>(obj: &RawGd<T>) -> bool
+        where
+            T: GodotClass<Declarer = Self>,
+        {
+            obj.storage().unwrap_unchecked().is_bound()
+        }
     }
 }