SpatialEditor::_finish_grid() is freeing RIDs that don't belong to it

[Zylann]

Godot version:
Godot 3.2.4 279f4fb
Might matter too in 4.0

OS/device including version:
Windows 10 64 bits, editor, debug

Issue description:
Recently I noticed in my voxel module that the seams I place between chunks to hid LOD cracks were missing. Then sometimes, entire blocks were missing too.
Errors sometimes occurred in the console, where RID_Owner could not find a given RID, helpfully reported as:

ERROR: RID_Owner<struct VisualServerScene::Instance>::get: Condition "!id_map.has(p_rid.get_data())" is true. Returned: 0
   At: C:\Projects\Godot\Engine\godot_fork\core/rid.h:150

I use VisualServer directly to create and destroy a large number of mesh instances, so at first I thought of a mistake in my own code. But I quickly realized it didn't make any sense.
My own code only has one place where I create RIDs, and one where I deallocate them, resetting to null. There was no room for this error to happen at a glance.

Minimal reproduction project:
There is none, I'm developping a module. But here is how I found this.

After several hours of debugging, one of the lucky crime scenes led me to do the following:

• In the only place I create mesh instances, I added the RID pointer to a global map (which I had to protect with a mutex)
• In the only place I destroy these mesh instances, I removed the RID pointer from the map, just before calling free().
• In VisualServerScene::free(), I added an assert, which would break only if the pointer is still in the map.
• So if the assert breaks, it means something is freeing my mesh instances behind my back.

It asserted.
The culprit was: SpatialEditor::_finish_grid().

godot/editor/plugins/spatial_editor_plugin.cpp

Line 5704 in cddff04

void SpatialEditor::_finish_grid() {

Git blames @reduz 7 years ago :p
It was calling free on 3 rids. 2 of them were null, one was dangling, and happened to have the same pointer as my own, valid RID.

A quick glance shows that none of those RIDs are reset after being freed. "thankfully", VisualServer silently ignores the cases where the passed RID is null or dangling, so nobody noticed it (grr).
So unless they are luckily all overwritten before being used, there is a chance the rest of this plugin calls _finish_grid again on freed pointers (and it does!). Pointers that can get re-used by the OS, for new instances.

For the record, I also had the grid and axes turned off, which not many people do I guess.

Note:
if you ever find yourself or some other code doing free without resetting or overwriting the RID nearby, then it might be a dormant bug waiting to strike (I could not find any occurrence of RID() in this file).

---

This can have affected a number of uncracked bugs causing logs by the editor with similar error messages. And it's hard to tell wether these logs come from the game or the editor, since they both print in the same system console.

[lawnjelly]

I've been running exclusively with the rids=tracked_handles build (see #54907) for a few days now and I've finally got a repeatable instance of this bug. For me I just run the editor load up TPS demo, and load the level.tscn file, and it triggers every time.

Can confirm it is in SpatialEditor::_finish_grid(). This repeatability will make it possible to confirm the fix works. It probably does just need setting the RIDs to NULL in _finish_grid etc but will be able to confirm this now.

EDIT: Yes I can now confirm (in terms of the RIDs rather than just anecdotally) that setting the RIDs to NULL after freeing fixes the bug.

[akien-mga]

Fixed by #54650.