Engine crash when removing node in specific situation

[KoBeWi]

Godot version

3.5.2

System information

Windows 10 x64

Issue description

I found out that you can delete a node in a specific situation that should be blocked. This causes invalid node to stay in the scene tree, which is basically a ticking bomb:

When the parent node is being freed, the broken node causes hard crash with very obscure stack trace (assuming you use C++ debugger, otherwise there is no error log). This can happen e.g. when changing scenes and also leads to random bugs like null during get_children() iteration.

The issue does not happen in 4.x, as this situation is handled and results in an error.

Steps to reproduce

1. Attach this script to a node

extends Node

func _ready() -> void:
	var ui = Node.new()
	add_child(ui)
	
	var menu = Node.new()
	ui.add_child(menu)
	
	var interface = Node.new()
	ui.add_child(interface)
	interface.connect("tree_exiting", menu.get_parent(), "remove_child", [menu])
	
	get_tree().create_timer(0.5).connect("timeout", interface, "queue_free") # invalid node here (and cow_data error)
	get_tree().create_timer(1).connect("timeout", self, "queue_free") # crash

2. Run

Minimal reproduction project

N/A

[lawnjelly]

Am investigating. This isn't a new bug as it is present in 3.4 at least, so is not due to the new queue free behaviour.

Ah it seems like during the Node::remove_child() it gets the id of the child to remove, then calls p_child->_set_tree(nullptr) which is causing the other child of the parent to get removed from the list BEFORE the first child is deleted, thus the ID is now no longer valid.

No idea how this is supposed to work, will keep looking. 😀

UPDATE:
It does seem to fix it by rechecking the idx of the child to be deleted after calling p_child->_set_tree(nullptr). So on the right track.

Ok the problem seems to be the timer, on deleting calls remove_child(). This in turn causes remove_child() to be called in _set_tree() to remove the menu node, which invalidates the idx for the timer. My solution is to simply recheck the idx after doing the _set_tree().

This is kind of a special circumstance bug but I think this should fix it, at least for this MRP. There could conceivably be some other more convoluted circumstance this same bug mechanism could cause problems, but it's difficult to guess ahead of time. I'll do a PR.