std::list::merge() leads to crash due to debug assert in CoreObjectFactory::GetFileWriters()
Closed, ResolvedPublic
Actions

Assigned To

None

Authored By

	wegner
	Nov 15 2013, 5:57 PM

Description

If I add a new file type a crash in debug mode can occur in case two unsorted lists are to be merged.
(mitkCoreObjectFactory.cpp line 402)

I don't even understand why a writer of a new datatype has to be added to m_FileWriters and to m_ExtraFactories like:

static bool already_registered = false;

if ( !already_registered ) {
    MyIOFactory::RegisterOneFactory();
    MyWriterFactory::RegisterOneFactory();

    // Member of superclass so against Stryker naming conventions.
    m_FileWriters.push_back( MyWriter::New().GetPointer() );

    mitk::CoreObjectFactory::GetInstance()->RegisterExtraFactory(this);
    CreateFileExtensionsMap();

    already_registered = true;
}

Given above mentioned section:

mitk::CoreObjectFactory::FileWriterList mitk::CoreObjectFactory::GetFileWriters() {

FileWriterList allWriters = m_FileWriters;
for (ExtraFactoriesContainer::iterator it = m_ExtraFactories.begin(); it != m_ExtraFactories.end() ; it++ ) {
  FileWriterList list2 = (*it)->GetFileWriters();
  allWriters.merge(list2);
}
return allWriters;

}

leads to an instance of MyWriter in m_FileWriters and thus as a copy in allWriters and then all extra Factories are asked for their writers so once more its added and merged to allWriters.
If I choose to only register my factory via RegisterExtraFactory it doesn't work either.

A work around would be to
sort the list prior to merging or to choose a different container.
A callstack is in T14866.

;) Best Regards,
Ingmar

Event Timeline

wegner added projects: MITK, Bugzilla.Nov 15 2013, 5:57 PM

Using a std::set to automatically sort the pointers during instert works fine.

The following implementation worked fine:

mitk::CoreObjectFactory::FileWriterList mitk::CoreObjectFactory::GetFileWriters() {

FileWriterList allWriters = m_FileWriters;
//sort to merge lists later on
typedef std::set<mitk::FileWriterWithInformation::Pointer> FileWriterSet;
FileWriterSet fileWritersSet;

for( FileWriterList::iterator iter = allWriters.begin(); 
     iter != allWriters.end(); ++iter) {
    fileWritersSet.insert(*iter);
}

//collect all extra factories
for (ExtraFactoriesContainer::iterator it = m_ExtraFactories.begin(); 
     it != m_ExtraFactories.end(); it++ ) {
    FileWriterList list2 = (*it)->GetFileWriters();
 
  //add them to the sorted set
  for( FileWriterList::iterator iter = list2.begin(); 
     iter != list2.end(); ++iter) {
      fileWritersSet.insert(*iter);
  }
}

//write back to allWriters to return a list 
allWriters.clear();
for (FileWriterSet::iterator iter = fileWritersSet.begin(); 
     iter != fileWritersSet.end(); ++iter) {
    allWriters.insert(allWriters.end(), *iter);
}

return allWriters;

}

Using std::sort didn't work because the -operator of a smartpointer is not defined.

I would propose to change the list to a set in general or to use the above workaround until io machanism is refactured.

Greetings, ;) Ingmar

I guess the suggested fix/workaround is ok. Maybe the code can be simplified a bit by using std::list::insert and std::set::insert in their "range" versions, getting rid of the for-loops.

Is this bug in progress ?

New remote branch pushed: bug-16437-MergeLeadsToCrashDueToDebugAssertInCoreObjectFactoryGetFileWriters

[ee7d49]: Merge branch 'bug-16437-MergeLeadsToCrashDueToDebugAssertInCoreObjectF

Merged commits:

2013-11-27 15:08:42 Adrian Winterstein [015699]
For loops replaced by range versions of the insert functions.

2013-11-27 14:43:48 Ingmar Wegner [be7751]
Using a std::set to automatically sort the pointers during instert.