[Insight-developers] FMR in RemoveObserver()

[William A. Hoffman]

I guess it should
be :
Observer* o = *i;
m_Observers.remove(*i);
delete o;


At 10:26 PM 7/23/2001 -0400, William A. Hoffman wrote:
>I think it is just the order that is wrong:
>
>  if((*i)->m_Tag == tag)
>       {
>       delete (*i);  // this should be done after the remove
>       m_Observers.remove(*i);
>       return;  // this return will stop all iteration on the list
>}
>
>So, the only thing that needs to be done, is to move the delete one line down.
>Remove only removes the first element in the list.
>
>
>At 01:49 PM 7/23/2001 -0700, Lydia Ng wrote:
>>Hi,
>>
>>I am getting free memory read (FMR) errors in my app
>>due to RemoveObserver():
>>
>>void
>>SubjectImplementation::
>>RemoveObserver(unsigned long tag)
>>{
>>   for(std::list<Observer* >::iterator i = m_Observers.begin();
>>       i != m_Observers.end(); ++i)
>>     {
>>     if((*i)->m_Tag == tag)
>>       {
>>       delete (*i);
>>       m_Observers.remove(*i);
>>       return;
>>       }
>>     }
>>}
>>
>>The problem seems to be with using an iterator as the argument to
>>list::remove( const T & ).
>>
>>remove() tries to remove *all* occurences of the element in the list.
>>After it removes the element associated with the iterator, the iterator
>>becomes invalid. But the function still needs to do comparisions on the
>>rest of the list and it tries to dereference the invalid iterator.
>>This results in FMR errors (at least in the VC++ 6.0 sp 5 implementation
>>anyway) and the possibility that all occurences are not not
>>been removed.
>>
>>See also MSDN note on the subject:
>>http://support.microsoft.com/support/kb/articles/Q250/8/48.ASP
>>
>>
>>There are two solutions
>>
>>(1) make a copy of the element and pass that to remove:
>>
>>Observer * observerPtr = *i;
>>m_Observers.remove( observerPtr );
>>
>>(2) Since all the observers should be uniquely
>>identified by m_Tag we could use erase( iterator )
>>which only removes the one occurence specified
>>by an iterator:
>>
>>m_Observers.erase( i );
>>
>>-----------------------------------
>>I prefer (2) since erase is O(1) and it also doesn't
>>require the extra copying.
>>
>>Any objections to the change?
>>
>>Cheers,
>>Lydia
>>
>>_______________________________________________
>>Insight-developers mailing list
>>Insight-developers@public.kitware.com
>>http://public.kitware.com/mailman/listinfo/insight-developers
>
>
>_______________________________________________
>Insight-developers mailing list
>Insight-developers@public.kitware.com
>http://public.kitware.com/mailman/listinfo/insight-developers