Correctly free relevant scripts when closing scene tabs

[Delsin-Yu]

Fixes #85983
EditorNode::_remove_scene from editor/editor_node.cpp calls editor_data.clear_script_icon_cache() for cleaning all script icon cache, previously, the implementation inside _remove_edited_scene(p_change_tab) loops through all scene tabs including the one we are closing, results in a call to EditorData::get_script_icon() on the closing scene tab, and create Ref<Script>s to that unloading scene lives inside _script_icon_cache.

This issue causes, primarily, the c-sharp scripts not to get freed when closing scene tabs, and might be a cause of #78513.
These wild script references may live inside the ScriptManagerBridge.ScriptTypeBiMap, resulting in the duplicate key issue when unloading the assebly.

modules/mono/glue/runtime_interop.cpp:1324 - System.ArgumentException: An item with the same key has already been added. Key: Game.WeaponAbilityData
     at System.Collections.Generic.Dictionary`2.TryInsert(TKey key, TValue value, InsertionBehavior behavior)
     at System.Collections.Generic.Dictionary`2.Add(TKey key, TValue value)
     at Godot.Bridge.ScriptManagerBridge.ScriptTypeBiMap.Add(IntPtr scriptPtr, Type scriptType) in /root/godot/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.types.cs:line 23
     at Godot.Bridge.ScriptManagerBridge.AddScriptBridge(IntPtr scriptPtr, godot_string* scriptPath) in /root/godot/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.cs:line 419

[AThousandShips]

You should target master not 4.2, changes can then be cherry picked.

If you need more information about the update process please see: here. If wanted I can fix the branch for you.

[AThousandShips]

Some style fixes (will help as you need to push again to cause CI to run)

[Delsin-Yu]

I don't think adding this removing_scene field inside editor_data is healthy, a better approach would directly modify the logic inside EditorNode::_remove_edited_scene(bool p_change_tab) since we just need to find a way for calling editor_data.remove_scene(old_index) before _set_current_scene(new_index). However, the program started crashing after modifying the logic there.

Original Logic

void EditorNode::_remove_edited_scene(bool p_change_tab) {
	// When scene gets closed no node is edited anymore, so make sure the editors are notified before nodes are freed.
	hide_unused_editors(SceneTreeDock::get_singleton());

	int new_index = editor_data.get_edited_scene();
	int old_index = new_index;

	if (new_index > 0) {
		new_index = new_index - 1;
	} else if (editor_data.get_edited_scene_count() > 1) {
		new_index = 1;
	} else {
		editor_data.add_edited_scene(-1);
		new_index = 1;
	}

	if (p_change_tab) {
		_set_current_scene(new_index);
	}
	editor_data.remove_scene(old_index);
	_update_title();
	scene_tabs->update_scene_tabs();
}

New Logic that Causes Crashing

void EditorNode::_remove_edited_scene(bool p_change_tab) {
	// When scene gets closed no node is edited anymore, so make sure the editors are notified before nodes are freed.
	hide_unused_editors(SceneTreeDock::get_singleton());

	int old_index = editor_data.get_edited_scene();

	editor_data.remove_scene(old_index);

	if (p_change_tab) {
		int new_index = editor_data.get_edited_scene();

		if (new_index == 0) {
			if (editor_data.get_edited_scene_count() == 0) {
				editor_data.add_edited_scene(-1);
			}
		} else {
			new_index = new_index - 1;
		}

		_set_current_scene(new_index);
	}

	_update_title();
	scene_tabs->update_scene_tabs();
}

The Crash Message

CrashHandlerException: Program crashed
Engine version: Godot Engine v4.2.1.rc.mono.custom_build (daeb1c7292cbb426fd45c5ca98b1c7da40b390ba)
Dumping the backtrace. Please include this when reporting the bug to the project developer.
[0] Node::get_configuration_warnings_as_string (D:\UnityProject\godot\scene\main\node.cpp:3067)
[1] SceneTreeEditor::_add_nodes (D:\UnityProject\godot\editor\gui\scene_tree_editor.cpp:287)
[2] SceneTreeEditor::_update_tree (D:\UnityProject\godot\editor\gui\scene_tree_editor.cpp:617)
[3] call_with_variant_args_helper<SceneTreeEditor,bool,0> (D:\UnityProject\godot\core\variant\binder_common.h:308)
[4] call_with_variant_args_dv<SceneTreeEditor,bool> (D:\UnityProject\godot\core\variant\binder_common.h:451)
[5] MethodBindT<SceneTreeEditor,bool>::call (D:\UnityProject\godot\core\object\method_bind.h:337)
[6] Object::callp (D:\UnityProject\godot\core\object\object.cpp:775)
[7] Callable::callp (D:\UnityProject\godot\core\variant\callable.cpp:69)
[8] CallQueue::_call_function (D:\UnityProject\godot\core\object\message_queue.cpp:222)
[9] CallQueue::flush (D:\UnityProject\godot\core\object\message_queue.cpp:328)
[10] SceneTree::physics_process (D:\UnityProject\godot\scene\main\scene_tree.cpp:473)
[11] Main::iteration (D:\UnityProject\godot\main\main.cpp:3596)
[12] OS_Windows::run (D:\UnityProject\godot\platform\windows\os_windows.cpp:1474)
[13] widechar_main (D:\UnityProject\godot\platform\windows\godot_windows.cpp:182)
[14] _main (D:\UnityProject\godot\platform\windows\godot_windows.cpp:204)
[15] main (D:\UnityProject\godot\platform\windows\godot_windows.cpp:218)
[16] WinMain (D:\UnityProject\godot\platform\windows\godot_windows.cpp:232)
[17] __scrt_common_main_seh (D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288)
[18] <couldn't map PC to fn name>
-- END OF BACKTRACE --

[KoBeWi]

I checked why the tab updates while deleting and seems like when closing the tab, update_scene_tabs() is called at least 3 times, for no reason. I can't reproduce the issue, so I can't test, but #83957 might fix it too.

The fix here looks like a workaround for bad tab management.

[Delsin-Yu]

I checked why the tab updates while deleting and seems like when closing the tab, update_scene_tabs() is called at least 3 times, for no reason. I can't reproduce the issue, so I can't test, but #83957 might fix it too.

The fix here looks like a workaround for bad tab management.

Detailed reproduction steps are written in issue #85983.
I just want to emphasize that this specific fix is targeting the CSharp script system (un-freed (re-referenced) script reference causing issue & module crash on assembly reload), and I agree that #83957 is a better way as long as that resolves the CSharp scripting issue as well.

[KoBeWi]

Detailed reproduction steps are written in issue

I know, I just don't have a C# setup to test. I tried with GDScript and other resources and it doesn't seem to happen.

I agree that #83957 is a better way as long as that resolves the CSharp scripting issue as well.

It's already merged, please give it a test.

[Delsin-Yu]

It's already merged, please give it a test.

The test indicates the issue persists to exist.
It looks like the core of this issue is not calling update_scene_tabs() multiple times, but calling update_scene_tabs() in the middle of deleting a tab will cause a re-reference of script dependencies.

[Delsin-Yu]

This is a flowchart for explaining this issue.

[KoBeWi]

Well the issue is still updating the tabs before the old tab was completely removed. So I think a better solution would be delaying tab update.

[Delsin-Yu]

Well the issue is still updating the tabs before the old tab was completely removed. So I think a better solution would be delaying tab update.

I will try that

[KoBeWi]

The tab update now happens after tab is fully removed, so if it fixes the issue, then it's fine.

On unrelated note, I think we should be more conservative with calling tab updates. There are cases like call to _set_current_scene() followed by update_scene_tabs(), which makes the update happen twice, because _set_current_scene() already does this internally. Tab update is now cheaper, but it's still called more than necessary.

[YuriSizov]

Could you please squash your commits into one? Make sure that the final commit has a short but descriptive message (the title of this PR is a good option). See this documentation, if you need help with squashing.

[YuriSizov]

Thanks, and congrats on your first merged Godot contribution!