#783 Selecting "Quit Character" on character selection screen, then exiting, breaks animation

[SilverNexus]

NOTE: This used to be part of bug #782, but was moved here as it became clear the behavior was a separate bug from the client hang.

If a user is on the Character Selection screen, and chooses the "File->Quit Character" option from the menu, then exits the client without playing a character, the next time the user uses that account will not present the face saved as the character "current face". This absence extends into the gameplay, where the specific image missing in the selection window is also missing in the in-game character movement animation. This only extends to the user's own viewport, so other players still see them normally.

[Kevin Zheng]

Again, this is another issue that happens once in a while, often enough
that I forget to consider it a bug. I was not aware that it could be
easily reproduced, so this will certainly help fix it.

I believe that this problem is related to the image caching code, but I
can't be certain without taking an extensive look at the code.

[SilverNexus]

I believe this would be resolved by the trunk client not loading the client window until after character selection.

[ChristopherPH]

Not only does this affect character selection, but it also affects any faces sent to the client prior to choosing a character before the disconnect.

This includes the faces used to display knowledge types, as this comes prior to selecting a character, and any other faces manually requested.

It seems as though the faces_sent array is not getting cleaned up on a socket disconnect, if there is no associated player.

[ChristopherPH]

This might not be the right way to fix this, but adding a FREE_AND_CLEAR() to free_newsocket in init.c seems to address this issue.

It might also fix a memory leak. However, in account_play_cmd() the faces_sent array is just set to NULL instead of being free()d so this might be another leak or perhaps I've missed something (There is some array copy somewhere from the socket to the player and some evil workaround in discussed in handle_client()).

I will just leave this as a comment instead of submitting a patch as it will need someone more familiar with the server and socket code to have a more detailed look.

void free_newsocket(socket_struct *ns) {
...
    ns->fd = -1;

    /* Cleanup sent faces */
    if (ns->faces_sent) {
        FREE_AND_CLEAR(ns->faces_sent);
         ns->faces_send = NULL;
    }
    ns->faces_sent_len = 0;
 ...

[Kevin Zheng]

Digging into this Frankenstein code, it does seem that we need to tread carefully.

I can't conveniently test because the latest client can't do 'File → Quit Character' conveniently from the character selection window.

Could you tell me if this patch also corrects the issue?

[ChristopherPH]

From what I can tell, yes, it does. The tests I ran are as follows:

• Without Patch:
  • Freshly restarted server:
    • Restart Server, Connect, Disconnect, Connect, Login, Choose Character: MISSING FACES (Knowledge Types)
    • Restart Server, Connect, Login, Disconnect, Connect, Login, Choose Character: MISSING FACES (Knowledge Types, Character portraits for character list)
    • Restart Server, Connect, Login, Choose Character, Disconnect, Connect, Login, Choose Character: No missing faces
  • Not freshly restarted:
    • Connect, Disconnect, Connect, Login, Choose Character: MISSING FACES (Knowledge Types)
    • Connect, Login, Disconnect, Connect, Login, Choose Character: MISSING FACES (Knowledge Types, Character portraits for character list)
    • Connect, Login, Choose Character, Disconnect, Connect, Login, Choose Character: No missing faces
• With Patch:
  • Freshly restarted server:
    • Restart Server, Connect, Disconnect, Connect, Login, Choose Character: No missing faces
    • Restart Server, Connect, Login, Disconnect, Connect, Login, Choose Character: No missing faces
    • Restart Server, Connect, Login, Choose Character, Disconnect, Connect, Login, Choose Character: No missing faces
  • Not freshly restarted:
    • Connect, Disconnect, Connect, Login, Choose Character: No missing faces
    • Connect, Login, Disconnect, Connect, Login, Choose Character: No missing faces
    • Connect, Login, Choose Character, Disconnect, Connect, Login, Choose Character: No missing faces

As a note, this can be checked on the existing GTK client by the following steps:

• Launch Client
• Connect to Server
• Login to account
• View Character List
  • Notice all the images beside the characters in the list
• Select Quit Button
• Launch Client
• Connect to Server
• Login to account
• View Character List
  • Notice all the images beside the characters are missing

I would like to point out that this patch can replace a previously calloc'd pointer and therefore may cause a memory leak. Testing the pointer for null and freeing it be worthwhile. Either way, the issue no longer appears with the attached patch, from what I can tell.

    if (ns->faces_sent)
        free(ns->faces_sent);
    ns->faces_sent = calloc(sizeof(*ns->faces_sent), get_faces_count());

Note: Edited for formatting, failed... how do I get a blank line after a bulleted list!

[Kevin Zheng]

Thank you for doing the testing and describing the detailed steps to reproduce.

I believe that the problem is that the server depends on a player log-in (via set_player_socket()) to clear the faces_sent array. If a player does not log in (but disconnects, as you describe in your steps to reproduce), the faces_sent array is recycled for the next connection.

I'm aware of the memory leak you pointed out; I just wanted to make sure we understood what was going on. I'll plug the memory leak and propose an updated patch.

[ChristopherPH]

Super! I'm happy to test again if needed.

[Kevin Zheng]

After reviewing, I believe the only change to the patch is to fix the memory leak by de-allocating the faces_sent array on early disconnect in do_server().

Please go ahead and test this patch and let me know if anything is amiss. If not, I'll go ahead and commit it.

[ChristopherPH]

This patch looks good from what I can tell. I no longer get missing faces.

[Kevin Zheng]

Fix committed, thanks.