Using "Save As" in script editor crashes with DEV_ENABLED (reference count below zero)

[vnen]

Godot version

4.0.rc (8208060)

System information

Arch Linux

Issue description

Using the Save As... function in the script editor crashes with DEV_ENABLED builds because the reference count of the script goes below zero.

Steps to reproduce

• Open the script editor.
• Create a new GDScript file.
• Click on the menu File > Save As....
• Change the name and click on Save.

Minimal reproduction project

N/A

[YuriSizov]

So, I'm not entirely sure how the ref count gets to zero, because it's impossible to place breakpoints this low level. But what I understand about what's happening is all sorts of problematic (though not necessarily related to the crash).

1. When we save a resource, it doesn't really matter if we use "Save" or "Save as", as both end up going the same path of saving a resource at a specified file path (EditorNode::save_resource_in_path). Naturally, when you pick "Save as" and selected non-existing name, the editor creates a new file at the specified path.

2. When the editor creates a new file, it immediately tells the editor filesystem to fetch information about it. Since this happens right after the file was created, the ResourceCache doesn't know about this file path yet. This means that the resource at that path gets loaded as if it was new. This resources also gets stored in ResourceCache as it gets loaded (through Resource::set_path).

3. As a GDScript resource is loaded, GDScriptCache entry is generated. So for this new file path and this newly loaded resource cache entries are created at the same time.

4. Then the editor proceeds with the resource saving routine, which is still ongoing. The next step is to call Resource::set_path on the resource that we are trying to save, with the new path. But since in step 2. we've already loaded a new resource from that same path and stored it in the cache, we fail to do so with the "Another resource is loaded from path" error. The old path of the loaded resource is cleared in the meantime.

5. GDScript resources have their own set_path routine defined, though (starting with Fix built-in script path of GDScript to prevent crash #67849, modified by GDScript compiler inner class bugfixes #68374 and Fix cyclic references in GDScript 2.0 #67714). This routine continues despite the error, and tries to update GDScript cache to change all the references (GDScriptCache::move_script).

6. Things get confusing at this point, as we now have two loaded resources representing the same thing. One associated with the new path, and the other associated with no path. One cached by Godot, and one is free of such chains. Both cached by GDScriptCache, though. And so what we attempt to do is to take an entry in cache's HashMap that has the new path and the newly loaded resource and assign to it the old loaded resource.

7. operator= for RefCounted is overloaded to do unref on the current reference before assigning the new one. And this is where we crash.

The reason why the counter is going below zero is not clear to me, and maybe it's because of the combination of the steps described, but maybe not. In either case, there seems to be a bigger problem at hand, with saving, loading, and updating resources in memory. While GDScript specifically crashes (other resources I've tried do not), it's not an issue that has to be solved on GDScript's side, IMO.

In addition, I have another suspicion about an erroneous behavior related to this process. The editor assumes that when it saves a resource, the currently loaded copy is going to be associated with the new path (which doesn't happen, but the editor expects it to happen). Even if it worked correctly, this logic doesn't seem to handle UIDs in any way. According to KoBeWi, when we save a resource at a new path, a new UID is supposed to be generated. But the UID for the resource in memory is never changed. So successfully updating the path would not be enough for most resources (except for scripts, which don't have UIDs), and UIDs also need to be updated.

Not entirely sure what the solution is here, and I'll try to think of one, but here's my findings anyway.

[YuriSizov]

So I made a PR that partially addresses the issues I've identified. It makes sure that the editor does not update its information about resources too early, and thus prevents weird duplication from incorrect loading. And it seems to solve the crash.