_ClockMsgTargets changed while iterating

[ryzom-pipeline]

Original report by Meelis Mägi (Bitbucket: [Meelis Mägi](https://bitbucket.org/Meelis Mägi), ).

---

nel/gui/widget_manager.cpp

// and send clock tick msg to ctrl that are registered
        std::vector<CCtrlBase*> clockMsgTarget = _ClockMsgTargets;
        for(std::vector<CCtrlBase*>::iterator it = clockMsgTarget.begin(); it != clockMsgTarget.end(); ++it)
        {
            (*it)->handleEvent(clockTick);
        }

Do I understand this correctly that _ClockMsgTargets is duplicated into local variable?

handleEvent may add/remove elements and also ClockMsgTargets is modified.

Linux client crashes in there, trying to call handleEvent on deleted element.

Fix would be to check if element is still present,

if (isClockMsgTarget(*it))
  (*it)->handleEvent(clockTick);

or is there better way?

[ryzom-pipeline]

Original comment by Kishan Grimout (Bitbucket: kishan_grimout, ).

---

Hello,

You're correct that _ClockMsgTargets is duplicated on the stack. If handleEvent() only removes itself from the container (which looks true as long as I only see calls like unregisterClockMsgTarget(this)), another solution, considering the usage (no random access to elements as far as I could see), would be to use a std::list for _ClockMsgTargets. This way, iterators wouldn't be invalidated when modifying the container (except for the one being erased) and the local copy of the container would not be necessary anymore.

Something like this may work:

#!c++

        for (std::list<CCtrlBase*>::iterator it = _ClockMsgTargets.begin(); it != _ClockMsgTargets.end();)
        {
            CCtrlBase* ctrl = *it;
            ++it;
            ctrl->handleEvent(clockTick);
        }

[ryzom-pipeline]

Original comment by Meelis Mägi (Bitbucket: [Meelis Mägi](https://bitbucket.org/Meelis Mägi), ).

---

Thanks, Yes I think changing to std::list is better way.

[ryzom-pipeline]

Original comment by Meelis Mägi (Bitbucket: [Meelis Mägi](https://bitbucket.org/Meelis Mägi), ).

---

On second tought, std::list fixes it when index0 removes index2, but would still crash if index0 removes index1

[ryzom-pipeline]

Original comment by Kishan Grimout (Bitbucket: kishan_grimout, ).

---

Yes, that's what I meant when saying "only removes itself from the container", meaning index0 removes index0 (or as you mentioned more generally any index but index1). But as said, there are only calls to unregisterClockMsgTarget(this), coming from the handleEvent() call (or from handleScroll(), also coming from handleEvent()), especially when you look at this very case of the "clocktick" event.

Anyway, your fix is most likely the safest but at a cost. Maybe checking the performance hit would show this is not a big deal. It will depend on the number of controls registered in _ClockMsgTargets and the frequency of the clock tick events.

The use of std::list has some constraints that may be hard to guarantee with future changes (maybe there won't be any, who knows).

I'm just thinking about another safe solution that would be to use smart pointers and keep the local copy of the container. Thus pointers in the local copy would remain valid. But this may imply a lot of changes.

Another option: store the "unregisterClockMsgTarget", "registerClockMsgTarget", and "delCtrl" (which deletes the controls) calls as requests to postpone their executions to avoid modifying the containers while being read.

I don't have access to the source code right now so I'm not sure how manageable these solutions would be.

Hope this helps!

[ryzom-pipeline]

Original comment by Meelis Mägi (Bitbucket: [Meelis Mägi](https://bitbucket.org/Meelis Mägi), ).

---

ClockMsgTargets list is smallish with 75items or so and called each frame.

What if unregisterClockMsgTarget sets element at index to NULL and its removed when iterating over the std::list? Not too hackish?

void CWidgetManager::unregisterClockMsgTarget(CCtrlBase *vb)
{
    std::list<CCtrlBase*>::iterator it = std::find(_ClockMsgTargets.begin(), _ClockMsgTargets.end(), vb);
    if (it != _ClockMsgTargets.end())
    {
        // mark element as deleted, it will be removed in sendClockTickEvent
        (*it) = NULL;
    }
}

for(std::list<CCtrlBase*>::iterator it = _ClockMsgTargets.begin(); it != _ClockMsgTargets.end();)
{
    CCtrlBase* ctrl = *it;
    if (ctrl)
    {
        ctrl->handleEvent(clockTick);
        ++it;
        }
        else
            it = _ClockMsgTargets.erase(it);
    }
}

No performance impact with find and list erase is fast too.

[ryzom-pipeline]

Original comment by Kishan Grimout (Bitbucket: kishan_grimout, ).

---

Sounds like a good solution!

[ryzom-pipeline]

Original comment by Meelis Mägi (Bitbucket: [Meelis Mägi](https://bitbucket.org/Meelis Mägi), ).

---

thanks,

fixed in commit 8055f6b

[ryzom-pipeline]

Original comment by Cédric Ochs (Bitbucket: [Cédric OCHS](https://bitbucket.org/Cédric OCHS), ).

---

Great job @nimetu :)