Page MenuHomePhabricator

impropriate way of using std::vector
Closed, DuplicatePublic

Description

The crash was cauesd by the std:vector:back() method which was used when all elements in the vector were erased
bool mitk::FocusManager::RemoveElement(FocusElement* element)
{

// Try find  
mitk::FocusManager::FocusListIterator position = std::find(m_FocusList.begin(),m_FocusList.end(),element);
if (position == m_FocusList.end())
                return false;
position = m_FocusList.erase(position);
//first delete the one on the position, and store the one afterewards into position
if (position == m_FocusList.end())//deleded was the last in row, then take the one before
                //m_FocElement = m_FocusList.back();  //this line was causing crash, after last element was erased you cant use back() method
                return false;
else
                m_FocElement = *position;//m_FocElement is equal to the next one in row
return true;

}

Related Objects

StatusAssignedTask
DuplicateNone

Event Timeline

Ingmar will take care of this.

patch that adds mitkFocusManagerTest

Hi,
I have initially tested the class mitkFocusManager with mitkFocusManagerTest.cpp (see Attachment). No errors occured!

Which version of mitkFocusManager.cpp do you use?
In Revision 17179 the lists is checked against empty() after erasing the element.:
(lighted out the comments)...

position = m_FocusList.erase(position);
if ( m_FocusList.size() == 0 )
  m_FocElement = NULL; //thus .back should not be called on removing the last element
else if ( position == m_FocusList.end() )
  m_FocElement = m_FocusList.back();
else
  m_FocElement = *position;

Jakub, can you check this?
I just checked viewsvn: this error is solved since revision 15162.

copy-paste from #1419:
From Mathias Seitel 2008-09-04 16:33:49

Additional fix: in FocusManager::RemoveElement(), when removing the last
FocusElement, the current-element pointer is set to NULL now (instead of using
the now invalid last element). This bug becomes obvious after the (correctly)
extending the destructure of BaseRenderer to remove the associated FocusElement
from FocusManager

r15162

By Jakub:

Hello

Thanks for help!
I am not able to put the comment in bugzilla so I am writing by mail:

I have this revision: 13129. I see that in the new revision you solved this problem.

Still I would suggest to update the code. In your current code:
...
position = m_FocusList.erase(position);
if ( m_FocusList.size() == 0 )

m_FocElement = NULL; //thus .back should not be called on removing the last element else if ( position == m_FocusList.end() )
m_FocElement = m_FocusList.back();

else

m_FocElement = *position;

if you check the size of the vector, and the size is 0 it means that position in the vector is equal to end(), so the condition else if ( position == m_FocusList.end() )

m_FocElement = m_FocusList.back();

does not make sense because you will never get there.

so it could be like that:
...
position = m_FocusList.erase(position);
if ( m_FocusList.size() == 0 )

m_FocElement = NULL;

else

m_FocElement = *position;

Regards

Jakub

Hi Jakub, (<<sorry for the wrong c in your name>>)
why can't you add a comment in bugtracker?

The section

...else if ( position == m_FocusList.end() )

m_FocElement = m_FocusList.back();

is for the case, that the element that was to be deleted, was the last in list but there are some elements in list. In this case, the element in front of the deleted (=list.back()) shall be taken for focus.

Does that clear things up?

Why don't you update the code? I will commit the test once the dashboard is green again.

Regards,
Ingmar

mitkFocusManager commited in revision 18185

closing bug

kislinsk closed this task as a duplicate of Restricted Maniphest Task.Aug 1 2016, 10:47 PM
kislinsk reopened subtask Restricted Maniphest Task as Open.Jun 27 2018, 3:03 PM
kislinsk closed subtask Restricted Maniphest Task as Resolved.Jun 27 2018, 3:29 PM