No qdel callbacks (#2248)

* Adds error on qdeling callback, fixes errors this causes (#77850) You shouldn't ever qdel a callback. If you don't want to own it free your ref (remove it from a list/set it to null). When all refs are cleared it'll get cleaned up by byond itself * Fixes a bunch of callbacks that were being qdeleted, and code cleanup (#77904) ![image](https://github.com/tgstation/tgstation/assets/13398309/559eb50a-461c-4220-b628-55412baaffc3) Continuing the work of tgstation/tgstation#77850. it started with finding one that was being missed and causing a runtime...then I noticed a whole lot more. While I was doing this I found callbacks that weren't being nulled in `Destroy()`, so I added that wherever I found these spots as well as some general code cleanup. There were a lot more of these than I initially hoped to encounter so I'm labeling it as a refactor. Fixes lots of runtimes, improves code resiliency. :cl: refactor: fixed a bunch of instances of callbacks being qdeleted and cleaned up related code /:cl: * Cleans up some more callback qdels that unit tests caught --------- Co-authored-by: LemonInTheDark <[email protected]> Co-authored-by: Bloop <[email protected]>
Monkestation · Jun 13, 2024 · 0e936ee · 0e936ee
1 parent a2d1e3d
commit 0e936ee
Show file tree

Hide file tree

Showing 62 changed files with 175 additions and 90 deletions.
diff --git a/code/_onclick/hud/radial.dm b/code/_onclick/hud/radial.dm
@@ -350,7 +350,7 @@ GLOBAL_LIST_EMPTY(radial_menus)
 /datum/radial_menu/Destroy()
 	Reset()
 	hide()
-	QDEL_NULL(custom_check_callback)
+	custom_check_callback = null
 	. = ..()
 
 /*

diff --git a/code/_onclick/hud/radial_persistent.dm b/code/_onclick/hud/radial_persistent.dm
@@ -49,7 +49,7 @@
 	set_choices(newchoices,tooltips, set_page = target_page)
 
 /datum/radial_menu/persistent/Destroy()
-	QDEL_NULL(select_proc_callback)
+	select_proc_callback = null
 	GLOB.radial_menus -= uniqueid
 	Reset()
 	hide()

diff --git a/code/controllers/subsystem/circuit_component.dm b/code/controllers/subsystem/circuit_component.dm
@@ -28,8 +28,6 @@ SUBSYSTEM_DEF(circuit_component)
 
 		to_call.user = null
 		to_call.InvokeAsync()
-		qdel(to_call)
-
 
 		if(MC_TICK_CHECK)
 			return
@@ -76,7 +74,6 @@ SUBSYSTEM_DEF(circuit_component)
 			instant_run_currentrun.Cut(1,2)
 			to_call.user = null
 			to_call.InvokeAsync(received_inputs)
-			qdel(to_call)
 
 	if(length(instant_run_stack))
 		instant_run_callbacks_to_run = pop(instant_run_stack)

diff --git a/code/controllers/subsystem/movement/movement_types.dm b/code/controllers/subsystem/movement/movement_types.dm
@@ -436,7 +436,7 @@
 
 /datum/move_loop/has_target/jps/Destroy()
 	avoid = null
-	on_finish_callbacks = null
+	on_finish_callbacks.Cut()
 	return ..()
 
 ///Tries to calculate a new path for this moveloop.

diff --git a/code/controllers/subsystem/throwing.dm b/code/controllers/subsystem/throwing.dm
@@ -115,8 +115,7 @@ SUBSYSTEM_DEF(throwing)
 	thrownthing = null
 	thrower = null
 	initial_target = null
-	if(callback)
-		QDEL_NULL(callback) //It stores a reference to the thrownthing, its source. Let's clean that.
+	callback = null
 	return ..()
 
 ///Defines the datum behavior on the thrownthing's qdeletion event.

diff --git a/code/datums/callback.dm b/code/datums/callback.dm
@@ -68,6 +68,21 @@
 	if(usr)
 		user = WEAKREF(usr)
 
+/**
+ * Qdel a callback datum
+ * This is not allowed and will stack trace. callback datums are structs, if they are referenced they exist
+ *
+ * Arguments
+ * * force set to true to force the deletion to be allowed.
+ * * ... an optional list of extra arguments to pass to the proc
+ */
+/datum/callback/Destroy(force=FALSE, ...)
+	SHOULD_CALL_PARENT(FALSE)
+	if (force)
+		return ..()
+	stack_trace("Callbacks can not be qdeleted. If they are referenced, they must exist. ([object == GLOBAL_PROC ? GLOBAL_PROC : object.type] [delegate])")
+	return QDEL_HINT_LETMELIVE
+
 /**
  * Invoke this callback
  *

diff --git a/code/datums/cinematics/_cinematic.dm b/code/datums/cinematics/_cinematic.dm
@@ -54,7 +54,7 @@
 
 /datum/cinematic/Destroy()
 	QDEL_NULL(screen)
-	QDEL_NULL(special_callback)
+	special_callback = null
 	watching.Cut()
 	locked.Cut()
 	return ..()

diff --git a/code/datums/components/action_item_overlay.dm b/code/datums/components/action_item_overlay.dm
@@ -26,7 +26,7 @@
 
 /datum/component/action_item_overlay/Destroy(force, silent)
 	item_ref = null
-	QDEL_NULL(item_callback)
+	item_callback = null
 	item_appearance = null
 	return ..()
 

diff --git a/code/datums/components/ai_retaliate_advanced.dm b/code/datums/components/ai_retaliate_advanced.dm
@@ -16,6 +16,10 @@
 
 	ADD_TRAIT(parent, TRAIT_SUBTREE_REQUIRED_OPERATIONAL_DATUM, type)
 
+/datum/component/ai_retaliate_advanced/Destroy(force, silent)
+	post_retaliate_callback = null
+	return ..()
+
 /datum/component/ai_retaliate_advanced/RegisterWithParent()
 	RegisterSignal(parent, COMSIG_ATOM_WAS_ATTACKED, PROC_REF(on_attacked))
 

diff --git a/code/datums/components/anti_magic.dm b/code/datums/components/anti_magic.dm
@@ -58,8 +58,8 @@
 	src.expiration = expiration
 
 /datum/component/anti_magic/Destroy(force, silent)
-	QDEL_NULL(drain_antimagic)
-	QDEL_NULL(expiration)
+	drain_antimagic = null
+	expiration = null
 	return ..()
 
 /datum/component/anti_magic/proc/on_equip(datum/source, mob/equipper, slot)

diff --git a/code/datums/components/basic_mob_attack_telegraph.dm b/code/datums/components/basic_mob_attack_telegraph.dm
@@ -25,6 +25,13 @@
 	src.telegraph_duration = telegraph_duration
 	src.on_began_forecast = on_began_forecast
 
+/datum/component/basic_mob_attack_telegraph/Destroy(force, silent)
+	if(current_target)
+		forget_target(current_target)
+	target_overlay = null
+	on_began_forecast = null
+	return ..()
+
 /datum/component/basic_mob_attack_telegraph/RegisterWithParent()
 	. = ..()
 	RegisterSignal(parent, COMSIG_HOSTILE_PRE_ATTACKINGTARGET, PROC_REF(on_attack))

diff --git a/code/datums/components/bullet_intercepting.dm b/code/datums/components/bullet_intercepting.dm
@@ -26,7 +26,8 @@
 	RegisterSignal(parent, COMSIG_ITEM_PRE_UNEQUIP, PROC_REF(on_unequipped))
 
 /datum/component/bullet_intercepting/Destroy(force, silent)
-	QDEL_NULL(on_intercepted)
+	wearer = null
+	on_intercepted = null
 	return ..()
 
 /// Called when item changes slots, check if we're in a valid location to take bullets

diff --git a/code/datums/components/butchering.dm b/code/datums/components/butchering.dm
@@ -34,6 +34,10 @@
 	if(isitem(parent))
 		RegisterSignal(parent, COMSIG_ITEM_ATTACK, PROC_REF(onItemAttack))
 
+/datum/component/butchering/Destroy(force, silent)
+	butcher_callback = null
+	return ..()
+
 /datum/component/butchering/proc/onItemAttack(obj/item/source, mob/living/M, mob/living/user)
 	SIGNAL_HANDLER
 

diff --git a/code/datums/components/cleaner.dm b/code/datums/components/cleaner.dm
@@ -30,10 +30,8 @@
 	src.on_cleaned_callback = on_cleaned_callback
 
 /datum/component/cleaner/Destroy(force, silent)
-	if(pre_clean_callback)
-		QDEL_NULL(pre_clean_callback)
-	if(on_cleaned_callback)
-		QDEL_NULL(on_cleaned_callback)
+	pre_clean_callback = null
+	on_cleaned_callback = null
 	return ..()
 
 /datum/component/cleaner/RegisterWithParent()

diff --git a/code/datums/components/combo_attacks.dm b/code/datums/components/combo_attacks.dm
@@ -32,6 +32,10 @@
 	src.leniency_time = leniency_time
 	src.can_attack_callback = can_attack_callback
 
+/datum/component/combo_attacks/Destroy(force, silent)
+	can_attack_callback = null
+	return ..()
+
 /datum/component/combo_attacks/RegisterWithParent()
 	RegisterSignal(parent, COMSIG_ATOM_EXAMINE, PROC_REF(on_examine))
 	RegisterSignal(parent, COMSIG_ATOM_EXAMINE_MORE, PROC_REF(on_examine_more))

diff --git a/code/datums/components/deadchat_control.dm b/code/datums/components/deadchat_control.dm
@@ -51,6 +51,7 @@
 	ckey_to_cooldown = null
 	if(generated_point_of_interest)
 		SSpoints_of_interest.remove_point_of_interest(parent)
+	on_removal = null
 	return ..()
 
 /datum/component/deadchat_control/proc/deadchat_react(mob/source, message)

diff --git a/code/datums/components/effect_remover.dm b/code/datums/components/effect_remover.dm
@@ -42,7 +42,7 @@
 	src.time_to_remove = time_to_remove
 
 /datum/component/effect_remover/Destroy(force, silent)
-	QDEL_NULL(on_clear_callback)
+	on_clear_callback = null
 	return ..()
 
 /datum/component/effect_remover/RegisterWithParent()

diff --git a/code/datums/components/egg_layer.dm b/code/datums/components/egg_layer.dm
@@ -50,6 +50,7 @@
 /datum/component/egg_layer/Destroy(force, silent)
 	. = ..()
 	STOP_PROCESSING(SSobj, src)
+	egg_laid_callback = null
 
 /datum/component/egg_layer/proc/feed_food(datum/source, obj/item/food, mob/living/attacker, params)
 	SIGNAL_HANDLER

diff --git a/code/datums/components/food/edible.dm b/code/datums/components/food/edible.dm
@@ -198,9 +198,9 @@ Behavior that's still missing from this component that original food items had t
 	setup_initial_reagents(initial_reagents)
 
 /datum/component/edible/Destroy(force, silent)
-	QDEL_NULL(after_eat)
-	QDEL_NULL(on_consume)
-	QDEL_NULL(check_liked)
+	after_eat = null
+	on_consume = null
+	check_liked = null
 	return ..()
 
 /// Sets up the initial reagents of the food.

diff --git a/code/datums/components/ghost_direct_control.dm b/code/datums/components/ghost_direct_control.dm
@@ -48,8 +48,8 @@
 	return ..()
 
 /datum/component/ghost_direct_control/Destroy(force, silent)
-	QDEL_NULL(extra_control_checks)
-	QDEL_NULL(after_assumed_control)
+	extra_control_checks = null
+	after_assumed_control = null
 
 	var/mob/mob_parent = parent
 	var/list/spawners = GLOB.joinable_mobs[format_text("[initial(mob_parent.name)]")]

diff --git a/code/datums/components/growth_and_differentiation.dm b/code/datums/components/growth_and_differentiation.dm
@@ -68,6 +68,8 @@
 /datum/component/growth_and_differentiation/Destroy(force, silent)
 	STOP_PROCESSING(SSdcs, src)
 	deltimer(timer_id)
+	optional_checks = null
+	optional_grow_behavior = null
 	return ..()
 
 /// Wrapper for qdel() so we can pass it in RegisterSignals(). I hate it here too.

diff --git a/code/datums/components/healing_touch.dm b/code/datums/components/healing_touch.dm
@@ -104,7 +104,7 @@
 	return ..()
 
 /datum/component/healing_touch/Destroy(force, silent)
-	QDEL_NULL(extra_checks)
+	extra_checks = null
 	return ..()
 
 /// Validate our target, and interrupt the attack chain to start healing it if it is allowed

diff --git a/code/datums/components/health_scaling_effects.dm b/code/datums/components/health_scaling_effects.dm
@@ -54,7 +54,7 @@
 	return ..()
 
 /datum/component/health_scaling_effects/Destroy(force, silent)
-	QDEL_NULL(additional_status_callback)
+	additional_status_callback = null
 	return ..()
 
 /// Called when mob health changes, recalculates the ratio between maximum and minimum

diff --git a/code/datums/components/interaction_booby_trap.dm b/code/datums/components/interaction_booby_trap.dm
@@ -59,8 +59,8 @@
 /datum/component/interaction_booby_trap/Destroy(force, silent)
 	UnregisterSignal(parent, list(COMSIG_ATOM_ATTACK_HAND, COMSIG_ATOM_TOOL_ACT(defuse_tool), COMSIG_ATOM_EXAMINE_MORE) + additional_triggers)
 	QDEL_NULL(active_sound_loop)
-	QDEL_NULL(on_triggered_callback)
-	QDEL_NULL(on_defused_callback)
+	on_triggered_callback = null
+	on_defused_callback = null
 	return ..()
 
 /// Called when someone touches the parent atom with their hands, we want to blow up

diff --git a/code/datums/components/jetpack.dm b/code/datums/components/jetpack.dm
@@ -67,7 +67,8 @@
 
 /datum/component/jetpack/Destroy()
 	QDEL_NULL(trail)
-	QDEL_NULL(check_on_move)
+	check_on_move = null
+	get_mover = null
 	return ..()
 
 /datum/component/jetpack/proc/setup_trail()

diff --git a/code/datums/components/keep_me_secure.dm b/code/datums/components/keep_me_secure.dm
@@ -22,6 +22,11 @@
 	src.secured_callback = secured_callback
 	src.unsecured_callback = unsecured_callback
 
+/datum/component/keep_me_secure/Destroy(force, silent)
+	secured_callback = null
+	unsecured_callback = null
+	return ..()
+
 /datum/component/keep_me_secure/RegisterWithParent()
 	last_move = world.time
 	if (secured_callback || unsecured_callback)

diff --git a/code/datums/components/lock_on_cursor.dm b/code/datums/components/lock_on_cursor.dm
@@ -62,12 +62,14 @@
 	mouse_tracker.assign_to_mob(owner)
 	START_PROCESSING(SSfastprocess, src)
 
-/datum/component/lock_on_cursor/Destroy()
+/datum/component/lock_on_cursor/Destroy(force, silent)
 	clear_visuals()
 	STOP_PROCESSING(SSfastprocess, src)
 	mouse_tracker = null
 	var/mob/owner = parent
 	owner.clear_fullscreen("lock_on")
+	on_lock = null
+	can_target_callback = null
 	return ..()
 
 /// Adds overlays to all targets

diff --git a/code/datums/components/material_container.dm b/code/datums/components/material_container.dm
@@ -73,12 +73,9 @@
 /datum/component/material_container/Destroy(force, silent)
 	materials = null
 	allowed_materials = null
-	if(insertion_check)
-		QDEL_NULL(insertion_check)
-	if(precondition)
-		QDEL_NULL(precondition)
-	if(after_insert)
-		QDEL_NULL(after_insert)
+	insertion_check = null
+	precondition = null
+	after_insert = null
 	return ..()
 
 

diff --git a/code/datums/components/mind_linker.dm b/code/datums/components/mind_linker.dm
@@ -80,7 +80,7 @@
 	linked_mobs.Cut()
 	QDEL_NULL(linker_action)
 	QDEL_NULL(master_speech)
-	QDEL_NULL(post_unlink_callback)
+	post_unlink_callback = null
 	return ..()
 
 /datum/component/mind_linker/RegisterWithParent()

diff --git a/code/datums/components/nuclear_bomb_operator.dm b/code/datums/components/nuclear_bomb_operator.dm
@@ -50,6 +50,8 @@
 
 /datum/component/nuclear_bomb_operator/Destroy(force, silent)
 	QDEL_NULL(disky)
+	on_disk_collected = null
+	add_disk_overlays = null
 	return ..()
 
 /// Drop the disk on the floor, if we have it

diff --git a/code/datums/components/puzzgrid.dm b/code/datums/components/puzzgrid.dm
@@ -55,6 +55,12 @@
 		addtimer(CALLBACK(src, PROC_REF(out_of_time)), timer)
 		time_to_finish = world.time + timer
 
+/datum/component/puzzgrid/Destroy(force, silent)
+	puzzgrid = null
+	on_victory_callback = null
+	on_fail_callback = null
+	return ..()
+
 /datum/component/puzzgrid/RegisterWithParent()
 	RegisterSignal(parent, COMSIG_ATOM_ATTACK_HAND, PROC_REF(on_attack_hand))
 

diff --git a/code/datums/components/reagent_refiller.dm b/code/datums/components/reagent_refiller.dm
@@ -28,6 +28,10 @@
 
 	return ..()
 
+/datum/component/reagent_refiller/Destroy(force, silent)
+	power_draw_callback = null
+	return ..()
+
 /datum/component/reagent_refiller/RegisterWithParent()
 	RegisterSignal(parent, COMSIG_ITEM_AFTERATTACK, PROC_REF(refill))
 	RegisterSignal(parent, COMSIG_ATOM_EXITED, PROC_REF(delete_self))

diff --git a/code/datums/components/reflection.dm b/code/datums/components/reflection.dm
@@ -55,9 +55,10 @@
 	var/list/reflect_update_signals = list(COMSIG_MOVABLE_MOVED) + update_signals
 	RegisterSignals(parent, reflect_update_signals, PROC_REF(get_reflection_targets))
 
-/datum/component/reflection/Destroy()
+/datum/component/reflection/Destroy(force, silent)
 	QDEL_LIST_ASSOC_VAL(reflected_movables)
 	QDEL_NULL(reflection_holder)
+	can_reflect = null
 	return ..()
 
 ///Called when the parent changes its direction.

diff --git a/code/datums/components/religious_tool.dm b/code/datums/components/religious_tool.dm
@@ -28,6 +28,13 @@
 	if(override_catalyst_type)
 		catalyst_type = override_catalyst_type
 
+/datum/component/religious_tool/Destroy(force, silent)
+	easy_access_sect = null
+	performing_rite = null
+	catalyst_type = null
+	after_sect_select_cb = null
+	return ..()
+
 /datum/component/religious_tool/RegisterWithParent()
 	RegisterSignal(parent, COMSIG_ATOM_ATTACKBY, PROC_REF(AttemptActions))
 	RegisterSignal(parent, COMSIG_ATOM_EXAMINE, PROC_REF(on_examine))