Fix TileMapEditorPlugin crash by storing tilemap ID instead of pointer

[lawnjelly]

Store the tilemap ObjectID instead of raw pointer, and check it is valid before access.

Fixes #80609
Alternative to #80537

Notes

• Stores ObjectID for all accesses rather than raw pointer
• Ref counting would have also maybe worked for this, but I don't think Nodes are ref counted.

[akien-mga]

Is this relevant for 4.1.x too, or fixing a regression from 4.2-specific changes?

[lawnjelly]

Amusingly, it looks like 4.1 already has a similar fix already applied. Maybe it got broken doing 4.2, or someone else noticed the same problem there in 4.1. 😀

Yeah @groud I think originally did this with ObjectID (2 years ago) and perhaps @KoBeWi added the raw pointer (in #74717 ? ) which caused the regression.

[lawnjelly]

Failed the GodotTest CI, not sure why, am rerunning in case spurious, it's kind of hard to pin down where it is failing.

If it is genuine maybe there's an access of ObjectDB outside the lifetime or something. 🤷‍♂️ Will look if it fails again.

[KoBeWi]

I can't reproduce the crash, but I'm getting an error that should be crashing 🤔

EDIT:
This PR fixes it though.

[lawnjelly]

I can't reproduce the crash, but I'm getting an error that should be crashing thinking

Both of these are consistent with TileMap being in the process of being destroyed (possibly at the same time on another thread?) before TileMapEditorPlugin::edit() is called with NULL to remove the reference.

The interesting bit is why the CI is failing (assuming not a false flag, I've run it twice), possibly memory corruption / double free going on. I'm not familiar enough with ObjectDB to know offhand if there's an order of destruction problem that could cause this, and I'm not really familiar with running the GodotTest locally.

I'm happy for you @KoBeWi to investigate, or use #80537 which is simpler and doesn't seem to cause this (at least temporarily), or some other solution.

[KoBeWi]

Ah I missed that this is a separate PR from #80537. I though you just applied my requested changes 🙃
This one is more complex, but IMO more correct.

[lawnjelly]

Also just to note, if TileMapEditorPlugin::edit() is being called on a separate thread to TileMap destructor, then this whole sequence would need some thread protection. I'm assuming here both are on the same thread (as is I think the original code).

[KoBeWi]

Editor works on single thread, only specific tasks (like importing or baking) are multithreaded.

[lawnjelly]

I can't reproduce the crash, but I'm getting an error that should be crashing thinking

Actually, thinking about it, this could come down to differences in our debuggers. Maybe mine is catching the access to deleted object early, and yours is running the function on the remnants of the deleted object.

To check the lifetime was the problem when I wrote this, I put a bool in the TileMap for whether it was active, and set it to false in the destructor. The data still existed in the object remnants, but the destructor had been called.

[lawnjelly]

Sanitizer build has now passed, and the only change was removing that space.
🤷‍♂️

[akien-mga]

Thanks!