[MC-121152] WorldClient leaks player and painting entities

I've found a memory leak present in all minecraft versions I know

The problem is, entity added to world by "Spawn Player" Packet (ID 0x05 for 1.12.2), is added to "entityList" HashSet in "WorldClient" (according to MCP mapping) twice with different IDs, first time it added with client-generated entity ID, then correct entity ID is set and it added to same set again (but old one is not deleted because equals() method is based on entity ID)
The entity added first time will never be removed by "Destroy Entity" packet (ID 0x35 for 1.13)

Very simple reproduce steps:
1) Start a server
2) Let 2 players join the server
3) One player then should disconnect
4) There's will be still two players is "entityList"

[media]

I didn't researched if it could cause any real problems, but it's definetely a bug
Sorry for my english, i'm not native english speaker

For moderators: I don't know if i'm first who reported bug in code itself, but i think it should be fixed like any other bug

Attachments

Comments 8

[Bot] Arisa 2017-10-15T23:18:14Z

Please do not mark unreleased versions as affected.
You don't have access to them yet.

pokechu22 2017-10-15T23:20:53Z

Bugs in the code are valid, and this definitely seems like it could be an issue. I'll try to confirm this on my machine later today.

sashok724 2017-10-17T16:30:34Z

So what...?

pokechu22 2017-10-17T17:26:31Z

Sorry for the delay (I've been busy). Can confirm.

Here's how I reproduced:

Start a new server with level-type=FLAT, spawn-monsters=false, spawn-npcs=false, spawn-animals=false.
Connect to the server from one client. (In this case I launched this client through MCP, so I have actual class names, but the same should happen in a vanilla obfuscated jar)
Connect another client to the server, and then disconnect it.
Force a crash by holding F3+C for 10 seconds.
In the crash report, you will find Forced entities: 2 total; [EntityPlayerSP['Player880'/0, l='MpServer', x=-1214.50, y=4.00, z=842.50], EntityOtherPlayerMP['TestUser'/1, l='MpServer', x=-1204.50, y=4.00, z=838.50]], even though TestUser has already left the server. (Forced entities in the crash is entityList)

pokechu22 2017-12-18T06:03:17Z

I've confirmed this happens with paintings as well, almost certainly for the same reason as with players (changing entity IDs causing issues with the HashSet).

The issue is that both players and paintings don't call setEntityId before calling addEntityToWorld. addEntityToWorld does call it, but only after it adds it to entityList. So the fix would be to either call setEntityId beforehand, or to move the call to setEntityId in addEntityToWorld before adding it to entityList.

Reproducing with paintings also works in single-player.

pokechu22 2018-07-30T01:54:01Z

addEntityToWorld now sets the entity ID before insertion into the hashset.

pokechu22 2018-08-07T17:53:10Z

Seems to be back in 18w31a, with the setEntityId being below the addition to the entity collection again.

pokechu22 2019-03-10T19:00:39Z

Fixed in 19w06a. addEntityToWorld no longer sets the entity ID, but instead the entity ID is set beforehand in both the painting and player handlers; also, there no longer is a Set of entities but instead only a list and an int2object map (and these are only stored on WorldClient; the base World doesn't have its own version of it). Or at least something like that; I can say that empirically, paintings do not leak anymore, and the code regarding it seems much nicer.