Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix TileMapEditorPlugin crash where deleted tile_map accessed #80537

Conversation

lawnjelly
Copy link
Member

@lawnjelly lawnjelly commented Aug 12, 2023

Store the ObjectID and check it is valid before access.

Fixes #80609

Notes

  • I'm not super familiar with this code so this is just a 2 minute example fix, it might be ok or something more in depth might be needed.
  • There's a number of other ways of doing this, e.g. calling a signal in the plugin on TileMap deletion etc.
  • First mentioned in TileSet-related crash when closing the editor #80088 but worth a unique issue.

Store the ObjectID and check it is valid before access.
@lawnjelly lawnjelly added this to the 4.x milestone Aug 12, 2023
@lawnjelly lawnjelly changed the title Fix TileMapEditorPlugin crash where deleted tile_map accessed Fix TileMapEditorPlugin crash where deleted tile_map accessed Aug 12, 2023
@KoBeWi
Copy link
Member

KoBeWi commented Aug 12, 2023

If we store TileMap's ID, the pointer is redundant. You can change it into local variable: TileMap* tile_map = ObjectDB::get_instance(tile_map_id).

@lawnjelly
Copy link
Member Author

If we store TileMap's ID, the pointer is redundant. You can change it into local variable: TileMap* tile_map = ObjectDB::get_instance(tile_map_id).

This is true, but it depends on what paradigm you want to use to fix this, always check ObjectID, check just in vulnerable places, or remove the reference explicitly when the TileMap is deleted. This is just an example, am happy for you to do proper fix.

You could also keep a reference in the plugin so the TileMap didn't get destroyed until it reached zero. There are a number of ways of addressing this.

I get the impression this problem might be common to many plugins, maybe there is a standard way that has been used so far for this (I'm not so familiar with master).

@KoBeWi
Copy link
Member

KoBeWi commented Aug 12, 2023

There's no reason to store both ObjectID and pointer. If you have an ObjectID, access the object using id (it's safer, since it's guaranteed to return valid instance or null). We do use this pattern sometimes.

btw it does not fix my crash. I added an extra step in the issue, you might be able to reproduce it now.

@lawnjelly
Copy link
Member Author

btw it does not fix my crash. I added an extra step in the issue, you might be able to reproduce it now.

Yes, this fix was directed at the second crash I described (maybe it should have separate issue, but I thought it might be related at the time). I can now reproduce the original issue with the extra step 👍 but I have to go away this weekend so can't look at it further.

@akien-mga
Copy link
Member

Superseded by #80610.

@akien-mga akien-mga closed this Aug 14, 2023
@AThousandShips AThousandShips removed this from the 4.2 milestone Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TileMapEditorPlugin crash when closing tab
4 participants