[crash fix needs testing]When deleting holder, remove its aura from auralist

[Auntie Mangos]

Lynx3d, can you look at another crashfix? http://getmangos.eu/community/viewtopic.php?id=14662

Crash happens because RemoveSingleAuraFromSpellAuraHolder can remove aura holder. But then holder is accessed again...

[Auntie Mangos]

Some crashdumps related to this here -> http://getmangos.eu/community/viewtopic.php?id=14729

[tehmarto]

Recently I seen many crashes where auras are get by GetAurasByType(type) and looping them after that cause segmental fault (access violation) crashes. I think reason for that is because, when delete holder it deletes its auras too, but invalid pointer to auras still stored in m_modAuras[].

For example here.

bool Unit::AddSpellAuraHolder(SpellAuraHolder *holder)
{
   SpellEntry const* aurSpellInfo = holder->GetSpellProto();

   // ghost spell check, allow apply any auras at player loading in ghost mode (will be cleanup after load)
   if( !isAlive() && !IsDeathPersistentSpell(aurSpellInfo) &&
       !IsDeathOnlySpell(aurSpellInfo) &&
       (GetTypeId()!=TYPEID_PLAYER || !((Player*)this)->GetSession()->PlayerLoading()) )
   {
       delete holder;
       return false;
   }

Before delete holder, there should be:

for(int i=0; i < MAX_EFFECT_INDEX; ++i)
   if(holder->m_auras[i])
       m_modAuras[holder->m_auras[i]->GetModifier()->m_auraname].remove(*itr);

And everywhere where we delete holder should happen the same. I don't know how to reproduce the crash actually so i can test it with and without this thing and thats the reason why i post it here, and not sure if it fixes something actually - that needs to be tested when put on a server with many people where it crash more often and see if the crashes with getAurasbyType loops stop. I have no place to test that now.

Thanks.

[Patman128]

And everywhere where we delete holder should happen the same.

Couldn't this just be coded into SpellAuraHolder::~SpellAuraHolder()?

[Lynx3d]

m_modAuras is no member of SpellAuraHolder, so no, cannot put it in the destructor.

Also, you're talking about Unit::AddSpellAuraHolder(SpellAuraHolder *holder), if it gets deleted here, it is not yet added to Unit and its auras can't be in m_modAuras anyway...or at least i think it shouldn't.

This is also the only place besides RemoveSpellAuraHolder() where holders are deleted, and RemoveSpellAuraHolder() clearly takes care of m_modAuras removal by calling RemoveAura()

-edit-

never mind, my proposed change was not required...

[tehmarto]

for(int i=0; i < MAX_EFFECT_INDEX; ++i)
{
   if(holder->m_auras[i])
   {
       AuraList const& auras = GetAurasByType(holder->m_auras[i]->GetModifier()->m_auraname);
       for(AuraList::const_iterator itr = auras.begin(); itr != auras.end(); ++itr)
       {
           if(holder->m_auras[i] == *itr)
           {
               sLog.outError("deleting aura witch otherwise would stay in auralist and cause crash later");
               m_modAuras[(*itr)->GetModifier()->m_auraname].remove(*itr);
               itr = auras.begin();
           }
       }
   }
}

This can be placed before delete holder; everywhere where in code is delete holder; to test if pointer in auralist when delete pointer.

[laise]

explicit delete from modAuras is wrong because there is no change in general aura process of deleting - so shouldn't be any change to order or explicit calls outside of RemoveAura. Is there any crash dump when it fails on GetAurasByType call?

[darkstalker]

maybe could use a Flyweight pattern and let it handle creation/destruction of holder objects

[tehmarto]

Here some crash dumps related to this:

http://pastebin.com/D5UUZb33

http://pastebin.com/bmrfmiYW

http://pastebin.com/dJR1b4r6

http://pastebin.com/p3iZN4ts

http://pastebin.com/B4kZnhXk

This type of crashes have repeated many times. I don't think this crashes can be caused by something other than a invalid pointer into m_modAuras.

[Lynx3d]

tehmarto: Your line numbers are suspicously off compared to [10264]...what revision with which custom patches are we talking about this time?

[tehmarto]

Revision is old yes. No custom patches related to this crash

[The_Game_Master]

BUMP FOR THE LOVE OF GOD!!!!!111one

I tryed on a x64 and x32 system, just in case was system dependent. No, it's not. Crash after crash after crash no matter the system.

http://pastebin.com/6JESbHnh

Just in case it's needed.

[Xfurry]

This problem happens when there is a high number of players online (we have about 800 - 10000).

If you try to replicate this on a test realm with less than 50-100 players, it won't crash so often or it won't crash at all.

[Toinan67]

This must be fixed faster, or we risk losing players and server devs in favor to Trinity core, which isn't a good thing..

1) "This must be fixed faster" -> give us patches to fix it faster? Or if you can't, don't ask devs to work faster. They will indeed work infinitely faster than you.

2) "or we risk losing players" -> who cares about your players?...

3) "server devs in favor to Trinity core" -> Noooo please stay with us, you bring so much to the MaNGOS community.

[Forusim]

Always prepare Testserver before updating Liveserver works quiet good for us

Rev 10148 works quiet good, before all the big changes in spellsystem.

[The_Game_Master]

I just love when you all come with your air of superiority and have no idea how to solve the problem. No, i don't have a clue either. But atleast i try.

tehmarto, thanks for the extra debug info. I found only 4 places where holders where deleted http://pastebin.com/VKR9UPSw did i missed something?

[FeRkEl]

shouldn't there be a test if(holder->isinuse()) ?

if the holder is in use you shouldn't delete it because this would generate crashs too? undeleted holder would "just" be an memleak ( or am i wrong? )

[VladimirMangos]

this is wrong way: replace crashes by memory leaks...

[FeRkEl]

aktually you just searched for delete holder, didn't you?

your first holder isn't an aura holder but a SQL..holder (don't know exact class)

shouldn't be in this Function (unit.cpp 3816) bool Unit::AddSpellAuraHolder(SpellAuraHolder *holder)

a check if (holder->IsEmptyHolder() ) before delete? should this prevent crashs??

// If holder in use (removed from code that plan access to it data after return)

// store it in holder list with delayed deletion

if (holder->IsInUse())

m_deletedHolders.push_back(holder);

else

delete holder;

maybe this would be the solution?

[Woweur]

Maybe use shared_ptr ?

[The_Game_Master]

Still no progress with it?

[kamikazetg]

Why exactly would this add a memory leak? Cleaning up invalid pointers is not going to cause a memory leak. However, crashing every 40mins due to invalid pointers through 80% of the core is worse than a memory leak. His first option, perhaps, but his second one I do not see an issue with.

P.S. Thank you for pointing me in the right direction for my crashes tehmarto. While I didn't use your method to solve it (I rewrote the aura system to handle it's own deletion), it did get me thinking the correct way. After all, that's what community input is about. It's not about whining that it's not getting fixed or about how other people don't contribute (in a thread of a person trying to contribute no less). It's about sharing what you've noticed and how you went about it thereby giving the devs something to go on.

Thanks again for taking the time.

[FeRkEl]

would you share your patch?

[darkstalker]

Maybe use shared_ptr ?

was thinking the same, but someone has to do the dirty job.

[FeRkEl]

hmm i'm not very expirenced with c++/mangos core and shared_ptr's but doesn't a aura has a reference of its auraholder, and the aura holder has a reference of the aura's?

and doesnt such a cross reference cause memleaks with shared_ptr's ?

maybe i just didn't get it how it realy works