search

vgstation-coders / vgstation13

Butts
GNU Affero General Public License v3.0
266 stars 544 forks source link

Fixes hostile mob's friends list entries getting deleted while in use #37297

Closed noirogen closed 2 weeks ago

noirogen commented 3 weeks ago

What this does

Adds a check to ensure that the lastassailant weakref isn't also being used by a hostile mob for its friends list, preventing mobs from forgetting their friends. Replaces lastassailant weakref deletion with setting to null. Closes #37240.

Why it's good

This fixes issues with tamed mobs randomly deciding that they no longer are your friend. Most notably this fixes Lazarus injected mobs from turning on you 5 seconds after you get into a fight with something.

How it was tested

How I found this issue:

  1. Noticed that lazarus mobs only became aggressive after combat in a live round.
  2. Spawned a goliath
  3. Killed it
  4. Revived it with a lazarus injector
  5. Opened the mob's friends list in VV.
  6. Spawned a basilisk to fight the goliath
  7. Noticed that my entry in the friends list's GCDestroyed variable gets set to "Goodbye world!" after the mob gets hit
  8. Noticed it gets deleted/set to null shortly afterward

How I tested it was fixed:

  1. Add the changes
  2. Using the same procedure as before, spawn, kill, and revive a goliath.
  3. Spawn a zombie
  4. Punch the zombie
  5. Let them fight
  6. Verified that the zombie's lastassailant was null
  7. Verified the goliath's frends list was intact

Changelog

🆑

noirogen commented 3 weeks ago

@SECBATON-GRIFFON I am asking this out of pure curiosity, but why does attack/lastassailant logging need to be piped into runescape_pvp.dm? Seeing that as the source of the bug when I was trying to figure this out was interesting

SECBATON-GRIFFON commented 3 weeks ago

most of my work converting lassailant stuff was just making them weakrefs wherever i found them, so ask whoever put it in that file first

noirogen commented 3 weeks ago

I've been doing more testing and this actually isn't the only thing that can delete a weakref to a player and cause a mob's friends list to get nullified.

noirogen commented 3 weeks ago

If you:

  1. Spawn, kill, revive a goliath.
  2. Spawn a basilisk
  3. Punch the basilisk so you are its lastassailant
  4. Have your goliath punch the basilisk.

Lastassailant will get deleted, and my check will fail, since the goliath isn't src. Checking M's friends list would fix this bug, but then there's another problem.

If you:

  1. Spawn, kill, revive a goliath.
  2. Spawn a basilisk
  3. Punch the basilisk so you are its lastassailant
  4. Have ANY non carbon mob hit the basilisk

Lastassailant will get deleted, and you can't check because the goliath was not involved at all.

Is it even necessary to delete the weakrefs? Could we not just set them to null? Deleting a weakref annihilates the weakref for every single instance where it is used.

noirogen commented 3 weeks ago

From some research I have found that manually deleting a weakref is improper practice. When a weakref is no longer referenced by anything, eventually the byond garbage collector will delete it. If lastassailant gets set to null, and if that was the only use of the weakref to the given player, then it will end up getting deleted anyways by the GC. Thus, there is no reason to kamikaze the weakref and everything that uses it.

I've tested all the cases listed above, and it works properly. PR should be done now unless someone finds an issue I didn't see

SonixApache commented 3 weeks ago

i told you about LAassailant @SECBATON-GRIFFON i TOLD you dog