Repository: godotengine/godot (language: cpp)

Issue:
Editor crash when interacting with a re-opened scene containing an animation player
### Tested versions

At least 4.4.beta1 to current head `6dc78c8aa17ad46e701e0c4e7b7632c2f0e098f1` but involves code introduced in  https://github.com/godotengine/godot/pull/99700 or a related PR cc @hpvb 

~~As far as I've seen, the crash does not happen if the editor is compiled with `dev_build=yes`~~

### System information

Godot v4.4.beta.mono (6dc78c8aa) - macOS Sequoia (15.2.0) - Multi-window, 1 monitor - Metal (Forward+) - integrated Apple M2 (Apple8) - Apple M2 (8 threads)

### Issue description

After closing and re-opening a scene containing an animation player and then either selecting a node or closing the scene tab several times, either SIGSEGV or SIGBUS crashes the editor. From lldb:
```
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x3d9ec6003f8001a8)
  * frame #0: 0x0000000102cb4df8 godot.macos.editor.arm64`Node::get_parent() const
    frame #1: 0x0000000102468e1c godot.macos.editor.arm64`SceneTreeEditor::_update_tree(bool) + 500
    frame #2: 0x0000000104bd0394 godot.macos.editor.arm64`CallQueue::_call_function(Callable const&, Variant const*, int, bool) + 336
    frame #3: 0x0000000104bd06b0 godot.macos.editor.arm64`CallQueue::flush() + 352
    frame #4: 0x0000000102ceb6d0 godot.macos.editor.arm64`SceneTree::physics_process(double) + 244
    frame #5: 0x00000001003f366c godot.macos.editor.arm64`Main::iteration() + 596
    frame #6: 0x0000000100383878 godot.macos.editor.arm64`OS_MacOS::run() + 148
    frame #7: 0x00000001003b0ad4 godot.macos.editor.arm64`main + 388
    frame #8: 0x0000000196350274 dyld`start + 2840
```
Presumably some call is getting inlined between _update_tree and get_parent?

### Steps to reproduce

Close and reopen the same scene several times in a row, optionally interacting with a node. In my tests, I saw a crash after anywhere between 1 and 4 close/reopen cycles with the last user action being any of opening the scene, closing the scene, or selecting a node in the tree.

### Minimal reproduction project (MRP)

[glb_test.zip](https://github.com/user-attachments/files/18680946/glb_test.zip)


Discussion / hints:
I tried to reproduce the issue on Linux with scenes from the 2D platformer demo in 4.4.beta2 but didn't get any crash.

It might be system specific, or scene specific. Is your scene particularly complex or using some specific node types which might be relevant?

The crash trace is a bit confusing, `SceneTreeEditor::_update_tree(bool) + 500` (if 500 is the line number) seems to be an empty line in `_update_node`, while `_update_tree` is much further down.
The 500 is in this case the function address offset. I agree the trace is weird though as it shows a get_parent call from _update_tree, which unless I missed something does not happen and I'm not sure what intermediary call might've been inlined to allow that. I don't remember exactly what conditions or versions I tested, and it might be system specific although I tested it in a pretty barebones scene before. I'll test again with beta2 on mac/linux/windows and see what comes up (also I'll change my lldb output format to maybe be clearer lol)

Edit: I'm able to consistently reproduce this on macos in beta1 and beta2, both standard and mono, for scenes containing an AnimationPlayer, although re "consistently" I'm pretty sure it's a race condition somewhere as sometimes the crash happens on 1st interaction and sometimes it takes like a minute of spam open/closing the scene and clicking randomly on nodes. I'm going to try to repro on windows + linux and then see if I can get a better trace.

[glb_test.zip](https://github.com/user-attachments/files/18680946/glb_test.zip)

Edit 2: I was able to repro on Windows although it was waaaaay less likely than on the macbook. I was only able to get it to crash twice, both times with `STATUS_HEAP_CORRUPTION`. I only tested beta2 standard since on the macbook it didn't seem to matter.

Edit 3: And I'm also able to repro on my older linux laptop also with beta2 pretty easily with SIGSEGV, although the info I get in console is completely useless.
Good news(?) it does actually happen with debugging stuff enabled, I just was unlucky when previously testing it. Looks like maybe there's a race to update the pinned node and it can end up stale in certain cases:
```
[3] SceneTreeEditor::NodeCache::mark_dirty(Node*, bool) (in godot.macos.editor.dev.arm64.mono) (/Users/adam/code/godot/editor/gui/scene_tree_editor.cpp:2420)
[4] SceneTreeEditor::_update_tree(bool) (in godot.macos.editor.dev.arm64.mono) (/Users/adam/code/godot/editor/gui/scene_tree_editor.cpp:932)
```
https://github.com/godotengine/godot/blob/master/editor/gui/scene_tree_editor.cpp#L932

https://github.com/godotengine/godot/blob/master/editor/gui/scene_tree_editor.cpp#L2420

It sorta makes sense that I didn't run into this when testing the original pr before (besides the crash being sorta rare) since I was mostly using the test scene with thousands of empty base nodes and no AnimationPlayer. At worst, the mark dirty could check for instance validity rather than null but I'll try looking for a nicer solution.
Thank you for the great report and the digging!

I want to have a look to see how a node actually gets in that state in the editor. It might need a different signal being handled. If a "bad node" can make it into that codepath then it would cause problems elsewhere as well, in code I didn't even touch.

This might have been a preexisting issue that was just even more rare before.
Maybe there's a different triggering condition but the only thing I can think of is that the only time `current_pinned_node` is set is 1) if there's a current root node and 2) if the current pin is different from the previous pin, so the old pin always sticks around and if that node has already been freed the crash occurs. My first thought was just to reset all the fields in `node_cache.clear()`, so it would always be nulled when the edited scene changes, but that didn't work. I think since the old code updated the full tree each time it would be unlikely that this same crash could happen.

Edit: ok I _did_ notice before while doing repro that I very rarely got a different offset and now it looks like `AnimationPlayerEditor::get_editing_node()` can actually give back an already freed node. So either it's freed by the time the the tree updates and it crashes or it's freed by the next time the tree updates and it crashes. Clearing the cache on the tree being reset only covers the second case.
I understand what needs to happen, a PR is incoming! I'm on a trip this weekend so it might not be until Monday.
Were you able to repro or were you going off of my description/stack trace? My impression was we needed to both clear the cache values when the tree was cleared and also update the AnimationPlayerEditor to clear its editing node when the scene was changed as well, but I wasn't sure of a good mechanism for the latter.
Sorry, it'll have to be tomorrow due to $reallife, I read through all of the code on the train and I figured out the cases you were referring to! I really do know how to fix it :)

We have signals to tell us when nodes get invalidated, but we never update the node pin cache. It should all work just fine if we make sure we update that cache also in response to the correct signals!
No worries, I was mostly asking because so far nobody else has mentioned being able to repro and it seems potentially multifaceted. I cleaned up the stashed changes I was messing around with last week here https://github.com/a-johnston/godot/commit/f230e9722b6c228d306308b69315c983825ac809 and while I think this addresses the issue I also don't know if it's just less likely (since it seems like a race) or if this goes too far and impacts some other functionality.

Edit- maybe should mention I was testing opening/closing a tab, switching between two tabs with possible pins, and having multiple pinnable nodes within the same tab

Fix patch:
diff --git a/editor/gui/scene_tree_editor.cpp b/editor/gui/scene_tree_editor.cpp
index ab834929cd58..5aedd940e482 100644
--- a/editor/gui/scene_tree_editor.cpp
+++ b/editor/gui/scene_tree_editor.cpp
@@ -924,12 +924,19 @@ void SceneTreeEditor::_update_tree(bool p_scroll_to_selected) {
 		// If pinned state changed, update the currently pinned node.
 		if (AnimationPlayerEditor::get_singleton()->is_pinned() != node_cache.current_has_pin) {
 			node_cache.current_has_pin = AnimationPlayerEditor::get_singleton()->is_pinned();
-			node_cache.mark_dirty(pinned_node);
+			if (node_cache.has(pinned_node)) {
+				node_cache.mark_dirty(pinned_node);
+			}
 		}
 		// If the current pinned node changed update both the old and new node.
 		if (node_cache.current_pinned_node != pinned_node) {
-			node_cache.mark_dirty(pinned_node);
-			node_cache.mark_dirty(node_cache.current_pinned_node);
+			// get_editing_node() will return deleted nodes. If the nodes are not in cache don't try to mark them.
+			if (node_cache.has(pinned_node)) {
+				node_cache.mark_dirty(pinned_node);
+			}
+			if (node_cache.has(node_cache.current_pinned_node)) {
+				node_cache.mark_dirty(node_cache.current_pinned_node);
+			}
 			node_cache.current_pinned_node = pinned_node;
 		}
 
@@ -2373,6 +2380,10 @@ HashMap<Node *, SceneTreeEditor::CachedNode>::Iterator SceneTreeEditor::NodeCach
 	return I;
 }
 
+bool SceneTreeEditor::NodeCache::has(Node *p_node) {
+	return get(p_node, false).operator bool();
+}
+
 void SceneTreeEditor::NodeCache::remove(Node *p_node, bool p_recursive) {
 	if (!p_node) {
 		return;
@@ -2382,6 +2393,11 @@ void SceneTreeEditor::NodeCache::remove(Node *p_node, bool p_recursive) {
 		editor->selected = nullptr;
 	}
 
+	if (p_node == current_pinned_node) {
+		current_pinned_node = nullptr;
+		current_has_pin = false;
+	}
+
 	editor->marked.erase(p_node);
 
 	HashMap<Node *, CachedNode>::Iterator I = cache.find(p_node);
@@ -2419,6 +2435,7 @@ void SceneTreeEditor::NodeCache::mark_dirty(Node *p_node, bool p_parents) {
 		if (!p_parents) {
 			break;
 		}
+
 		node = node->get_parent();
 	}
 }
@@ -2483,4 +2500,6 @@ void SceneTreeEditor::NodeCache::clear() {
 	}
 	cache.clear();
 	to_delete.clear();
+	current_pinned_node = nullptr;
+	current_has_pin = false;
 }
diff --git a/editor/gui/scene_tree_editor.h b/editor/gui/scene_tree_editor.h
index e789e9692419..d29c4df0cdb2 100644
--- a/editor/gui/scene_tree_editor.h
+++ b/editor/gui/scene_tree_editor.h
@@ -90,6 +90,7 @@ class SceneTreeEditor : public Control {
 
 		HashMap<Node *, CachedNode>::Iterator add(Node *p_node, TreeItem *p_item);
 		HashMap<Node *, CachedNode>::Iterator get(Node *p_node, bool p_deleted_ok = true);
+		bool has(Node *p_node);
 		void remove(Node *p_node, bool p_recursive = false);
 		void mark_dirty(Node *p_node, bool p_parents = true);
 		void mark_children_dirty(Node *p_node, bool p_recursive = false);


Test patch: