Fixed a crash when trying to switch servers

Ownership of the charInfo global variable wasn't well defined. It was
being locked, unlocked and generally modified from a lot of places, and
somewhere in this mess it ended up crashing when switching servers.

Now the CharHandler instances, for eAthena and manaserv respectively,
own this list of characters. A new class, Net::Character wraps up the
slot index in combination with the player dummy. The list is passed on
to the CharSelectDialog each time it changes.

Both related and unrelated cleanups were made as well.

Reviewed-by: Jared Adams

9 files changed, 192 insertions, 216 deletions

diff --git a/src/net/manaserv/beinghandler.cpp b/src/net/manaserv/beinghandler.cpp
index 5f9d1586..ae5bbc0e 100644
--- a/src/net/manaserv/beinghandler.cpp
+++ b/src/net/manaserv/beinghandler.cpp
@@ -114,7 +114,7 @@ Vector BeingHandler::giveSpeedInPixelsPerTicks(float speedInTilesPerSeconds)
     {
         speedInTicks.x = speedInTicks.y = 0;
         logger->log("Manaserv::BeingHandler: Speed wasn't given back"
-        " because game/Map not initialized.");
+                    " because game/Map not initialized.");
     }
     // We don't use z for now.
     speedInTicks.z = 0;
diff --git a/src/net/manaserv/charhandler.cpp b/src/net/manaserv/charhandler.cpp
index 122ee10b..7a75e38d 100644
--- a/src/net/manaserv/charhandler.cpp
+++ b/src/net/manaserv/charhandler.cpp
@@ -41,25 +41,10 @@
 
 #include "resources/colordb.h"
 
+#include "utils/dtor.h"
 #include "utils/gettext.h"
 
 extern Net::CharHandler *charHandler;
-
-struct CharInfo {
-    unsigned char slot;
-    std::string name;
-    Gender gender;
-    int hs, hc;
-    unsigned short level;
-    unsigned short charPoints;
-    unsigned short corrPoints;
-    unsigned int money;
-    unsigned char attr[7];
-};
-
-typedef std::vector<CharInfo> CharInfos;
-CharInfos chars;
-
 extern ManaServ::GameHandler *gameHandler;
 
 namespace ManaServ {
@@ -71,9 +56,7 @@ extern std::string netToken;
 extern ServerInfo gameServer;
 extern ServerInfo chatServer;
 
-CharHandler::CharHandler():
-    mCharSelectDialog(0),
-    mCharCreateDialog(0)
+CharHandler::CharHandler()
 {
     static const Uint16 _messages[] = {
         APMSG_CHAR_CREATE_RESPONSE,
@@ -91,104 +74,54 @@ void CharHandler::handleMessage(Net::MessageIn &msg)
     switch (msg.getId())
     {
         case APMSG_CHAR_CREATE_RESPONSE:
-            handleCharCreateResponse(msg);
+            handleCharacterCreateResponse(msg);
             break;
 
         case APMSG_CHAR_DELETE_RESPONSE:
-        {
-            int errMsg = msg.readInt8();
-            // Character deletion successful
-            if (errMsg == ERRMSG_OK)
-            {
-                LocalPlayer *tempPlayer = mCharInfo->getEntry();
-                mCharInfo->setEntry(0);
-                mCharInfo->unlock();
-                if (mCharSelectDialog)
-                    mCharSelectDialog->update(mCharInfo->getPos());
-                new OkDialog(_("Info"), _("Player deleted."));
-                delete tempPlayer;
-            }
-            // Character deletion failed
-            else
-            {
-                std::string errorMessage = "";
-                switch (errMsg)
-                {
-                    case ERRMSG_NO_LOGIN:
-                        errorMessage = _("Not logged in.");
-                        break;
-                    case ERRMSG_INVALID_ARGUMENT:
-                        errorMessage = _("Selection out of range.");
-                        break;
-                    default:
-                        errorMessage = _("Unknown error.");
-                }
-                mCharInfo->unlock();
-                new OkDialog(_("Error"), errorMessage);
-            }
-        }
+            handleCharacterDeleteResponse(msg);
             break;
 
         case APMSG_CHAR_INFO:
-        {
-            CharInfo info;
-            info.slot = msg.readInt8(); // character slot
-            info.name = msg.readString();
-            info.gender = msg.readInt8() == GENDER_MALE ? GENDER_MALE :
-                                                          GENDER_FEMALE;
-            info.hs = msg.readInt8();
-            info.hc = msg.readInt8();
-            info.level = msg.readInt16();
-            info.charPoints = msg.readInt16();
-            info.corrPoints = msg.readInt16();
-            info.money = msg.readInt32();
-
-            for (int i = 0; i < 7; i++)
-            {
-                info.attr[i] = msg.readInt8();
-            }
-
-            chars.push_back(info);
-
-            if (mCharSelectDialog)
-            {
-                mCharInfo->select(info.slot);
-
-                LocalPlayer *tempPlayer = new LocalPlayer();
-                tempPlayer->setName(info.name);
-                tempPlayer->setGender(info.gender);
-                tempPlayer->setSprite(SPRITE_HAIR, info.hs * -1,
-                                      ColorDB::get(info.hc));
-                tempPlayer->setLevel(info.level);
-                tempPlayer->setCharacterPoints(info.charPoints);
-                tempPlayer->setCorrectionPoints(info.corrPoints);
-                tempPlayer->setMoney(info.money);
-
-                for (int i = 0; i < 7; i++)
-                {
-                    tempPlayer->setAttributeBase(i, info.attr[i], false);
-                }
-
-                mCharInfo->setEntry(tempPlayer);
-
-                mCharSelectDialog->update(info.slot);
-            }
-        }
+            handleCharacterInfo(msg);
             break;
 
         case APMSG_CHAR_SELECT_RESPONSE:
-            handleCharSelectResponse(msg);
+            handleCharacterSelectResponse(msg);
             break;
     }
 }
 
-void CharHandler::handleCharCreateResponse(Net::MessageIn &msg)
+void CharHandler::handleCharacterInfo(Net::MessageIn &msg)
 {
-    int errMsg = msg.readInt8();
+    CachedCharacterInfo info;
+    info.slot = msg.readInt8();
+    info.name = msg.readString();
+    info.gender = msg.readInt8() == GENDER_MALE ? GENDER_MALE :
+                                                  GENDER_FEMALE;
+    info.hairStyle = msg.readInt8();
+    info.hairColor = msg.readInt8();
+    info.level = msg.readInt16();
+    info.characterPoints = msg.readInt16();
+    info.correctionPoints = msg.readInt16();
+    info.money = msg.readInt32();
+
+    for (int i = 0; i < 7; i++)
+    {
+        info.attribute[i] = msg.readInt8();
+    }
+
+    mCachedCharacterInfos.push_back(info);
+
+    updateCharacters();
+}
+
+void CharHandler::handleCharacterCreateResponse(Net::MessageIn &msg)
+{
+    const int errMsg = msg.readInt8();
 
-    // Character creation failed
     if (errMsg != ERRMSG_OK)
     {
+        // Character creation failed
         std::string errorMessage = "";
         switch (errMsg)
         {
@@ -233,17 +166,48 @@ void CharHandler::handleCharCreateResponse(Net::MessageIn &msg)
     }
     else
     {
+        // Close the character create dialog
         if (mCharCreateDialog)
         {
-            mCharCreateDialog->success();
             mCharCreateDialog->scheduleDelete();
             mCharCreateDialog = 0;
         }
     }
+}
 
+void CharHandler::handleCharacterDeleteResponse(Net::MessageIn &msg)
+{
+    int errMsg = msg.readInt8();
+    if (errMsg == ERRMSG_OK)
+    {
+        // Character deletion successful
+        delete mSelectedCharacter;
+        mCharacters.remove(mSelectedCharacter);
+        updateCharSelectDialog();
+        unlockCharSelectDialog();
+        new OkDialog(_("Info"), _("Player deleted."));
+    }
+    else
+    {
+        // Character deletion failed
+        std::string errorMessage = "";
+        switch (errMsg)
+        {
+            case ERRMSG_NO_LOGIN:
+                errorMessage = _("Not logged in.");
+                break;
+            case ERRMSG_INVALID_ARGUMENT:
+                errorMessage = _("Selection out of range.");
+                break;
+            default:
+                errorMessage = strprintf(_("Unknown error (%d)."), errMsg);
+        }
+        new OkDialog(_("Error"), errorMessage);
+    }
+    mSelectedCharacter = 0;
 }
 
-void CharHandler::handleCharSelectResponse(Net::MessageIn &msg)
+void CharHandler::handleCharacterSelectResponse(Net::MessageIn &msg)
 {
     int errMsg = msg.readInt8();
 
@@ -265,31 +229,20 @@ void CharHandler::handleCharSelectResponse(Net::MessageIn &msg)
         gameServerConnection->connect(gameServer.hostname, gameServer.port);
         chatServerConnection->connect(chatServer.hostname, chatServer.port);
 
-        // Keep the selected character and delete the others
-        player_node = mCharInfo->getEntry();
-        int slot = mCharInfo->getPos();
-        mCharInfo->unlock();
-        mCharInfo->select(0);
-
-        do {
-            LocalPlayer *tmp = mCharInfo->getEntry();
-            if (tmp != player_node)
-            {
-                delete tmp;
-                mCharInfo->setEntry(0);
-            }
-            mCharInfo->next();
-        } while (mCharInfo->getPos());
-        mCharInfo->select(slot);
-
-        mCharInfo->clear(); //player_node will be deleted by ~Game
+        // Prevent the selected local player from being deleted
+        player_node = mSelectedCharacter->dummy;
+        mSelectedCharacter->dummy = 0;
+
+        mCachedCharacterInfos.clear();
+        updateCharacters();
 
         state = STATE_CONNECT_GAME;
     }
-    else if(errMsg == ERRMSG_FAILURE)
+    else if (errMsg == ERRMSG_FAILURE)
     {
         errorMessage = _("No gameservers are available.");
-        mCharInfo->clear();
+        delete_all(mCharacters);
+        mCharacters.clear();
         state = STATE_ERROR;
     }
 }
@@ -297,13 +250,15 @@ void CharHandler::handleCharSelectResponse(Net::MessageIn &msg)
 void CharHandler::setCharSelectDialog(CharSelectDialog *window)
 {
     mCharSelectDialog = window;
+    updateCharacters();
 }
 
 void CharHandler::setCharCreateDialog(CharCreateDialog *window)
 {
     mCharCreateDialog = window;
 
-    if (!mCharCreateDialog) return;
+    if (!mCharCreateDialog)
+        return;
 
     std::vector<std::string> attributes;
     attributes.push_back(_("Strength:"));
@@ -316,59 +271,34 @@ void CharHandler::setCharCreateDialog(CharCreateDialog *window)
     mCharCreateDialog->setAttributes(attributes, 60, 1, 20);
 }
 
-void CharHandler::getCharacters()
+void CharHandler::requestCharacters()
 {
     if (!accountServerConnection->isConnected())
+    {
         Net::getLoginHandler()->connect();
+    }
     else
     {
-        mCharInfo->unlock();
-        LocalPlayer *tempPlayer;
-        for (CharInfos::const_iterator it = chars.begin(); it != chars.end(); it++)
-        {
-            const CharInfo info = (CharInfo) (*it);
-            mCharInfo->select(info.slot);
-
-            tempPlayer = new LocalPlayer();
-            tempPlayer->setName(info.name);
-            tempPlayer->setGender(info.gender);
-            tempPlayer->setSprite(SPRITE_HAIR, info.hs * -1,
-                                  ColorDB::get(info.hc));
-            tempPlayer->setLevel(info.level);
-            tempPlayer->setCharacterPoints(info.charPoints);
-            tempPlayer->setCorrectionPoints(info.corrPoints);
-            tempPlayer->setMoney(info.money);
-
-            for (int i = 0; i < 7; i++)
-            {
-                tempPlayer->setAttributeBase(i, info.attr[i], false);
-            }
-
-            mCharInfo->setEntry(tempPlayer);
-        }
-
-        // Close the character create dialog
-        if (mCharCreateDialog)
-        {
-            mCharCreateDialog->scheduleDelete();
-            mCharCreateDialog = 0;
-        }
-
+        // The characters are already there, continue to character selection
         state = STATE_CHAR_SELECT;
     }
 }
 
-void CharHandler::chooseCharacter(int slot, LocalPlayer* character)
+void CharHandler::chooseCharacter(Net::Character *character)
 {
-    MessageOut msg(PAMSG_CHAR_SELECT);
-
-    msg.writeInt8(slot);
+    mSelectedCharacter = character;
 
+    MessageOut msg(PAMSG_CHAR_SELECT);
+    msg.writeInt8(mSelectedCharacter->slot);
     accountServerConnection->send(msg);
 }
 
-void CharHandler::newCharacter(const std::string &name, int slot, bool gender,
-                  int hairstyle, int hairColor, std::vector<int> stats)
+void CharHandler::newCharacter(const std::string &name,
+                               int /* slot */,
+                               bool gender,
+                               int hairstyle,
+                               int hairColor,
+                               const std::vector<int> &stats)
 {
     MessageOut msg(PAMSG_CHAR_CREATE);
 
@@ -386,12 +316,12 @@ void CharHandler::newCharacter(const std::string &name, int slot, bool gender,
     accountServerConnection->send(msg);
 }
 
-void CharHandler::deleteCharacter(int slot, LocalPlayer* character)
+void CharHandler::deleteCharacter(Net::Character *character)
 {
-    MessageOut msg(PAMSG_CHAR_DELETE);
-
-    msg.writeInt8(slot);
+    mSelectedCharacter = character;
 
+    MessageOut msg(PAMSG_CHAR_DELETE);
+    msg.writeInt8(mSelectedCharacter->slot);
     accountServerConnection->send(msg);
 }
 
@@ -400,19 +330,56 @@ void CharHandler::switchCharacter()
     gameHandler->quit(true);
 }
 
-unsigned int CharHandler::baseSprite() const
+int CharHandler::baseSprite() const
 {
     return SPRITE_BASE;
 }
 
-unsigned int CharHandler::hairSprite() const
+int CharHandler::hairSprite() const
 {
     return SPRITE_HAIR;
 }
 
-unsigned int CharHandler::maxSprite() const
+int CharHandler::maxSprite() const
 {
     return SPRITE_VECTOREND;
 }
 
+void CharHandler::updateCharacters()
+{
+    // Delete previous characters
+    delete_all(mCharacters);
+    mCharacters.clear();
+
+    if (!mCharSelectDialog)
+        return;
+
+    // Create new characters and initialize them from the cached infos
+    for (unsigned i = 0; i < mCachedCharacterInfos.size(); ++i)
+    {
+        const CachedCharacterInfo &info = mCachedCharacterInfos.at(i);
+
+        Net::Character *character = new Net::Character;
+        character->slot = info.slot;
+        LocalPlayer *player = character->dummy;
+        player->setName(info.name);
+        player->setGender(info.gender);
+        player->setSprite(SPRITE_HAIR, info.hairStyle * -1,
+                          ColorDB::get(info.hairColor));
+        player->setLevel(info.level);
+        player->setCharacterPoints(info.characterPoints);
+        player->setCorrectionPoints(info.correctionPoints);
+        player->setMoney(info.money);
+
+        for (int i = 0; i < 7; i++)
+        {
+            player->setAttributeBase(i, info.attribute[i], false);
+        }
+
+        mCharacters.push_back(character);
+    }
+
+    updateCharSelectDialog();
+}
+
 } // namespace ManaServ
diff --git a/src/net/manaserv/charhandler.h b/src/net/manaserv/charhandler.h
index 74f68f55..4a1af307 100644
--- a/src/net/manaserv/charhandler.h
+++ b/src/net/manaserv/charhandler.h
@@ -42,11 +42,6 @@ class CharHandler : public MessageHandler, public Net::CharHandler
 
         void handleMessage(Net::MessageIn &msg);
 
-        void setCharInfo(LockedArray<LocalPlayer*> *charInfo)
-        {
-            mCharInfo = charInfo;
-        }
-
         void setCharSelectDialog(CharSelectDialog *window);
 
         /**
@@ -56,32 +51,52 @@ class CharHandler : public MessageHandler, public Net::CharHandler
          */
         void setCharCreateDialog(CharCreateDialog *window);
 
-        void getCharacters();
+        void requestCharacters();
 
-        void chooseCharacter(int slot, LocalPlayer* character);
+        void chooseCharacter(Net::Character *character);
 
         void newCharacter(const std::string &name, int slot,
-                        bool gender, int hairstyle, int hairColor,
-                        std::vector<int> stats);
+                          bool gender, int hairstyle, int hairColor,
+                          const std::vector<int> &stats);
 
-        void deleteCharacter(int slot, LocalPlayer* character);
+        void deleteCharacter(Net::Character *character);
 
         void switchCharacter();
 
-        unsigned int baseSprite() const;
-
-        unsigned int hairSprite() const;
-
-        unsigned int maxSprite() const;
+        int baseSprite() const;
 
-    protected:
-        void handleCharCreateResponse(Net::MessageIn &msg);
+        int hairSprite() const;
 
-        void handleCharSelectResponse(Net::MessageIn &msg);
+        int maxSprite() const;
 
-        LockedArray<LocalPlayer*> *mCharInfo;
-        CharSelectDialog *mCharSelectDialog;
-        CharCreateDialog *mCharCreateDialog;
+    private:
+        /**
+         * Character information needs to be cached since we receive it before
+         * we have loaded the dynamic data, so we can't resolve load any
+         * sprites yet.
+         */
+        struct CachedCharacterInfo {
+            int slot;
+            std::string name;
+            Gender gender;
+            int hairStyle;
+            int hairColor;
+            int level;
+            int characterPoints;
+            int correctionPoints;
+            int money;
+            int attribute[7];
+        };
+
+        void handleCharacterInfo(Net::MessageIn &msg);
+        void handleCharacterCreateResponse(Net::MessageIn &msg);
+        void handleCharacterDeleteResponse(Net::MessageIn &msg);
+        void handleCharacterSelectResponse(Net::MessageIn &msg);
+
+        void updateCharacters();
+
+        /** Cached character information */
+        std::vector<CachedCharacterInfo> mCachedCharacterInfos;
 };
 
 } // namespace ManaServ
diff --git a/src/net/manaserv/inventoryhandler.cpp b/src/net/manaserv/inventoryhandler.cpp
index 0020a4e7..3ce1aa6f 100644
--- a/src/net/manaserv/inventoryhandler.cpp
+++ b/src/net/manaserv/inventoryhandler.cpp
@@ -61,7 +61,7 @@ void InventoryHandler::handleMessage(Net::MessageIn &msg)
     {
         case GPMSG_INVENTORY_FULL:
             player_node->clearInventory();
-            player_node->mEquipment->setBackend(&mEqiups);
+            player_node->mEquipment->setBackend(&mEquips);
             // no break!
 
         case GPMSG_INVENTORY:
@@ -77,7 +77,7 @@ void InventoryHandler::handleMessage(Net::MessageIn &msg)
                 int id = msg.readInt16();
                 if (slot < EQUIPMENT_SIZE)
                 {
-                    mEqiups.setEquipment(slot, id);
+                    mEquips.setEquipment(slot, id);
                 }
                 else if (slot >= 32 && slot < 32 + getSize(INVENTORY))
                 {
@@ -105,7 +105,7 @@ void InventoryHandler::unequipItem(const Item *item)
     // Tidy equipment directly to avoid weapon still shown bug, for instance
     int equipSlot = item->getInvIndex();
     logger->log("Unequipping %d", equipSlot);
-    mEqiups.setEquipment(equipSlot, 0);
+    mEquips.setEquipment(equipSlot, 0);
 }
 
 void InventoryHandler::useItem(const Item *item)
diff --git a/src/net/manaserv/inventoryhandler.h b/src/net/manaserv/inventoryhandler.h
index 362e392a..402af3b5 100644
--- a/src/net/manaserv/inventoryhandler.h
+++ b/src/net/manaserv/inventoryhandler.h
@@ -30,7 +30,8 @@
 
 namespace ManaServ {
 
-class EquipBackend : public Equipment::Backend {
+class EquipBackend : public Equipment::Backend
+{
     public:
         EquipBackend()
         { memset(mEquipment, 0, sizeof(mEquipment)); }
@@ -97,7 +98,7 @@ class InventoryHandler : public MessageHandler, Net::InventoryHandler
         size_t getSize(StorageType type) const;
 
     private:
-        EquipBackend mEqiups;
+        EquipBackend mEquips;
 };
 
 } // namespace ManaServ
diff --git a/src/net/manaserv/itemhandler.cpp b/src/net/manaserv/itemhandler.cpp
index 802f9303..c939589e 100644
--- a/src/net/manaserv/itemhandler.cpp
+++ b/src/net/manaserv/itemhandler.cpp
@@ -58,12 +58,9 @@ void ItemHandler::handleMessage(Net::MessageIn &msg)
 
                 if (itemId)
                 {
-                    Game *game = Game::instance();
-                    Map *map = 0;
-                    if (game)
+                    if (Game *game = Game::instance())
                     {
-                        map = game->getCurrentMap();
-                        if (map)
+                        if (Map *map = game->getCurrentMap())
                         {
                             floorItemManager->create(id,
                                                      itemId,
@@ -71,9 +68,11 @@ void ItemHandler::handleMessage(Net::MessageIn &msg)
                                                      y / map->getTileHeight());
                         }
                         else
-                          logger->log(
-                          "ItemHandler: An item wasn't created because of"
-                          "Game/Map not initialized...");
+                        {
+                            logger->log(
+                                    "ItemHandler: An item wasn't created "
+                                    "because of Game/Map not initialized...");
+                        }
                     }
                 }
                 else if (FloorItem *item = floorItemManager->findById(id))
diff --git a/src/net/manaserv/loginhandler.cpp b/src/net/manaserv/loginhandler.cpp
index 63195816..9d8bc0e2 100644
--- a/src/net/manaserv/loginhandler.cpp
+++ b/src/net/manaserv/loginhandler.cpp
@@ -64,9 +64,11 @@ void LoginHandler::handleMessage(Net::MessageIn &msg)
         case APMSG_LOGIN_RESPONSE:
             handleLoginResponse(msg);
             break;
+
         case APMSG_REGISTER_RESPONSE:
             handleRegisterResponse(msg);
             break;
+
         case APMSG_RECONNECT_RESPONSE:
         {
             int errMsg = msg.readInt8();
diff --git a/src/net/manaserv/playerhandler.cpp b/src/net/manaserv/playerhandler.cpp
index bff9512e..56502ec1 100644
--- a/src/net/manaserv/playerhandler.cpp
+++ b/src/net/manaserv/playerhandler.cpp
@@ -306,14 +306,14 @@ void PlayerHandler::emote(int emoteId)
     // TODO
 }
 
-void PlayerHandler::increaseAttribute(size_t attr)
+void PlayerHandler::increaseAttribute(int attr)
 {
     MessageOut msg(PGMSG_RAISE_ATTRIBUTE);
     msg.writeInt8(attr);
     gameServerConnection->send(msg);
 }
 
-void PlayerHandler::decreaseAttribute(size_t attr)
+void PlayerHandler::decreaseAttribute(int attr)
 {
     MessageOut msg(PGMSG_LOWER_ATTRIBUTE);
     msg.writeInt8(attr);
diff --git a/src/net/manaserv/playerhandler.h b/src/net/manaserv/playerhandler.h
index 6edc96bb..65170d63 100644
--- a/src/net/manaserv/playerhandler.h
+++ b/src/net/manaserv/playerhandler.h
@@ -44,31 +44,23 @@ class PlayerHandler : public MessageHandler, public Net::PlayerHandler
         void handleMessage(Net::MessageIn &msg);
 
         void attack(int id);
-
         void emote(int emoteId);
 
-        void increaseAttribute(size_t attr);
-
-        void decreaseAttribute(size_t attr);
-
+        void increaseAttribute(int attr);
+        void decreaseAttribute(int attr);
         void increaseSkill(int skillId);
 
         void pickUp(FloorItem *floorItem);
-
         void setDirection(char direction);
-
         void setDestination(int x, int y, int direction = -1);
-
         void changeAction(Being::Action action);
 
         void respawn();
 
         void ignorePlayer(const std::string &player, bool ignore);
-
         void ignoreAll(bool ignore);
 
         bool canUseMagic();
-
         bool canCorrectAttributes();
 
         int getJobLocation();