[Revision 10309] Error details for segfault on GetSpellProto and Holder insight.

[kamikazetg]

Erasing comment. I see no point in contributing to a 'community' that does nothing but sit around waiting to insult people

and their intelligence.

[zergtmn]

Occasionally with the new spellauraholder changes, you can get a Segmentation fault when accessing GetSpellProto() and here is one of the reasons why:

Spells like http://www.wowhead.com/spell=31224 <- Cloak of Shadows can have a trigger spell that is not really a spell and has no DBC entry (Trigger Spell #35729). Therefore it has no Proto and can error out when trying to Call functions in it's non-existent Proto.

I fixed this just by checking if it has a Proto before actually using the Proto. Though, I would like to mention that, when programming, one should always check if a pointer is null before trying to use said pointer. This latest revision has hundreds of issues in regard to this.

FAIL. Have you seen code of EffectTriggerSpell?

    // normal case
   SpellEntry const *spellInfo = sSpellStore.LookupEntry( triggered_spell_id );
   if (!spellInfo)
   {
       sLog.outError("EffectTriggerSpell of spell %u: triggering unknown spell id %i", m_spellInfo->Id,triggered_spell_id);
       return;
   }

[kamikazetg]

Erasing Comment

[LordJZ]

Hmm, I can't see a place in the code where m_deletedAuras used without removing Aura from m_modAuras.

See for yourself:

void Unit::RemoveSpellAuraHolder(SpellAuraHolder *holder, AuraRemoveMode mode)
{
   // Statue unsummoned at holder remove
   SpellEntry const* AurSpellInfo = holder->GetSpellProto();
   Totem* statue = NULL;
   Unit* caster = holder->GetCaster();
   if(IsChanneledSpell(AurSpellInfo) && caster)
       if(caster->GetTypeId()==TYPEID_UNIT && ((Creature*)caster)->isTotem() && ((Totem*)caster)->GetTotemType()==TOTEM_STATUE)
           statue = ((Totem*)caster);

   if (m_spellAuraHoldersUpdateIterator != m_spellAuraHolders.end() && m_spellAuraHoldersUpdateIterator->second == holder)
       ++m_spellAuraHoldersUpdateIterator;

   SpellAuraHolderBounds bounds = GetSpellAuraHolderBounds(holder->GetId());
   for (SpellAuraHolderMap::iterator itr = bounds.first; itr != bounds.second; ++itr)
   {
       if (itr->second == holder)
       {
           m_spellAuraHolders.erase(itr);
           break;
       }
   }

   holder->SetRemoveMode(mode);
   holder->UnregisterSingleCastHolder();

   for (int32 i = 0; i < MAX_EFFECT_INDEX; ++i)
   {
       Aura *aura = holder->m_auras[i];
       if (aura)
[b][color=red]            RemoveAura(aura, mode);[/color][/b]
   }

   holder->_RemoveSpellAuraHolder();

   if (mode != AURA_REMOVE_BY_DELETE)
       holder->HandleSpellSpecificBoosts(false);

   if(statue)
       statue->UnSummon();

   // 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())
[b][color=red]        m_deletedHolders.push_back(holder);[/color][/b]
   else
       delete holder;

   if (mode != AURA_REMOVE_BY_EXPIRE && IsChanneledSpell(AurSpellInfo) && !IsAreaOfEffectSpell(AurSpellInfo) && caster && caster->GetGUID() != GetGUID())
       caster->InterruptSpell(CURRENT_CHANNELED_SPELL);
}

void Unit::RemoveAura(Aura *Aur, AuraRemoveMode mode)
{
   SpellEntry const* AurSpellInfo = Aur->GetSpellProto();

   // remove from list before mods removing (prevent cyclic calls, mods added before including to aura list - use reverse order)
   if (Aur->GetModifier()->m_auraname < TOTAL_AURAS)
[b][color=red]        m_modAuras[Aur->GetModifier()->m_auraname].remove(Aur);[/color][/b]
   else
       sLog.outError("Found aura with Id > TOTAL_AURAS: %u", Aur->GetModifier()->m_auraname);

   // Set remove mode
   Aur->SetRemoveMode(mode);

   // some ShapeshiftBoosts at remove trigger removing other auras including parent Shapeshift aura
   // remove aura from list before to prevent deleting it before
   ///m_Auras.erase(i);

   DEBUG_FILTER_LOG(LOG_FILTER_SPELL_CAST, "Aura %u now is remove mode %d",Aur->GetModifier()->m_auraname, mode);

   // aura _MUST_ be remove from holder before unapply.
   // un-apply code expected that aura not find by diff searches
   // in another case it can be double removed for example, if target die/etc in un-apply process.
   Aur->GetHolder()->RemoveAura(Aur->GetEffIndex());

   // some auras also need to apply modifier (on caster) on remove
   if (mode == AURA_REMOVE_BY_DELETE)
   {
       switch (Aur->GetModifier()->m_auraname)
       {
           // need properly undo any auras with player-caster mover set (or will crash at next caster move packet)
           case SPELL_AURA_MOD_POSSESS:
           case SPELL_AURA_MOD_POSSESS_PET:
           case SPELL_AURA_CONTROL_VEHICLE:
               Aur->ApplyModifier(false,true);
               break;
           default: break;
       }
   }
   else
       Aur->ApplyModifier(false,true);

   // If aura in use (removed from code that plan access to it data after return)
   // store it in aura list with delayed deletion
   if (Aur->IsInUse())
[b][color=red]        m_deletedAuras.push_back(Aur);[/color][/b]
   else
       delete Aur;
}

P.S. Glad at least someone tried to dig deep enough into Laise's aura code

[zergtmn]

Thanks for actually taking the time to not read my post. I can and have recreated this error on multiple occasions.

Not-existing spells are NOT casted and so aura holders are NOT created for them. I think it's simple to understand.

Maybe you have seen this in SpellAuraHolder contructor:

ASSERT(spellproto && spellproto == sSpellStore.LookupEntry( spellproto->Id ) && "`info` must be pointer to sSpellStore element");

Think before replying, please.

[LordJZ]

zergtmn, at least the OP is right about spellproto being null for some aura holders...

[kamikazetg]

Erasing comment

[zergtmn]

zergtmn, at least the OP is right about spellproto being null for some aura holders...

Hm, I see in SpellAuraHolder constructor:

ASSERT(spellproto && spellproto == sSpellStore.LookupEntry( spellproto->Id ) && "`info` must be pointer to sSpellStore element");

then few lines below

m_spellProto = spellproto;

So m_spellProto can't be null if you didn't removed that assert.

[zergtmn]

Actually, I did think about my post before replying. Search delete holder; and look at it. I thought this was a learning project? Not a place for children to attempt to rip on people trying to help. If you do not have anything constructive to say, do not post in my threads.

I should also note that I have solved the spellauraholder crashes on my server. 200+ users with no crashes for 20+hrs. I was just trying to give insight for the devs to go on in order to properly fix this in their build. My method is slightly hacky and I would prefer the person that designed the spellauraholder system to write a more efficient method for others to use.

If you doubt that holders are not being deleted, take a look at all the crash logs reported on Segfaults while trying to access spellauraholder maps. You cannot get a spellauraholder segfault without a null pointer being in the map.

As for Cloak of Shadows, I have SEEN this happen and have FIXED it by checking for an existing proto first. I would not bother mentioning it if I had not already fixed it.

Cloak of Shadows works fine for me. Am I doing something wrong?

I think you are trolling.

[Schmoozerd]

custom fixes that cause other problems, most likely

though he might have a very correct point about problems with aura removal

[kamikazetg]

Erasing comment

[zergtmn]

Sigh, I give up. I'll leave you guys to your crashing. This is why I do not bother trying to contribute. Bunch of spoiled children who always think they know better. This community has gone downhill. And obviously I have a point with m_spellauraholders as the problem is solved here.

I'm 21 years old, lol. Reading first part of your post I started thinking you haven't looked at code at all.

I gave you proofs and where are yours?

[Schmoozerd]

have to agree with Zergtmn,

perhaps the OP has a valid point about aura removal, but his example is just stupid and not reproducable.

btw, 6 posts in 1.5 years and then flaming about the community when you get actuall content based answers doesn't put you into the best light

[kamikazetg]

Alright, so much for the community. Erasing it.

P.S. If you had actually read my post, you'd see the example was not even related to the spellauraholder problem. It's obvious this

place is nothing but people waiting around to flame others and boost their egos.

I'll just continue to do what I've been doing. Fixing my own problems since every post I've read where people try to lend insight is

flamed for being stupid or wrong. That is why I have no posts in 1.5years. Would you bother submitting content if people flamed you

about it everytime you did?

I honestly don't know why people bother to contribute at all anymore and I'm seriously wondering why I bothered to do so now. I knew

how it was going to turn out: People sifting through to find anything wrong and flaming about it. Seriously, go read some posts sometime.

It's a giant grouping of flamers and insulters.

As for 'content based answers', there were not any real content answers. Only people answering showing it's obvious they did not bother

to read the entire post.

It's also fun to read edits done by Moderators with insulting and rude comments in them. Very professional.

Shrug, I've wasted enough of my time here. Back to fixing the hundreds of null pointer errors.

[LordJZ]

zergtmn:

Hmm I guess I explained my thoughts a little bad. I meant that function GetSpellProto actually CAN return NULL if an Aura object was somehow deleted improperly. Crash dumps posted here at forums prove that.

P.S. kamikazetg: hate to say that, but you previous post is a fail. You _have_ mentioned spellauraholder problem in CoS description. You haven't submitted any content in this thread. You haven't given an adequate answer to zergtmn's 1st post in this topic.

[QSOS]

I honestly don't know why people bother to contribute at all anymore and I'm seriously wondering why I bothered to do so now. I knew

how it was going to turn out: People sifting through to find anything wrong and flaming about it. Seriously, go read some posts sometime.

It's a giant grouping of flamers and insulters.

You are quick to slag others and generalize the entirety of the community based on the unconstructive comments of a few. You even go to the extent to wonder "why people bother to contribute at all anymore" - yet the fact that you remain active here is an indicator that you could not operate your server without the selfless contributions by people such as TOM_RUS. Do yourself a favor and grow up. The real world is filled with assholes that should be burnt at the stake, but they're not going to go away anytime soon.

[zergtmn]

Hmm I guess I explained my thoughts a little bad. I meant that function GetSpellProto actually CAN return NULL if an Aura object was somehow deleted improperly. Crash dumps posted here at forums prove that.

Yea, I know. In such case GetSpellProto can return NULL or invalid pointer or even crash before return depending on configuration. Checking result of GetSpellProto every time as was recommended in first post is just stupid. Better find real double deletion bug if it exists instead of filling code with hacks.