Unexpected crash when removing file from the project

[comsytec]

Please review this (from trunk):

Code

bool cbProject::RemoveFile(ProjectFile* pf)
{
if (!pf)
return false;
m_ProjectFilesMap.erase(UnixFilename(pf->relativeFilename)); // remove from hashmap
Manager::Get()->GetEditorManager()->Close(pf->file.GetFullPath());
{
FilesList::iterator it = m_Files.find(pf);
if (it == m_Files.end())
Manager::Get()->GetLogManager()->DebugLog(_T("Can't locate node for ProjectFile* !"));
else
m_Files.erase(it);
if ( m_FileArray.GetCount() > 0 )
// m_FileArray.Remove(*it); // -------------------- wrong !! --------------------
                            // -------------------- it cannot be accessed -------
m_FileArray.Remove(pf);     // -------------------- correct !? -------------------
} 

[thomas]

Yes.

For a moment you had me scared. That looked almost like the fix I did 3 days ago (this). Running cppcheck over our code brought that one up. Now your post made me wonder if I broke more than I fixed  

This is the same issue, iterator being erased and thus invalidated, and subsequently dereferenced when removed from some list/array/map. Interesting that cppcheck didn't complain about this one...

[thomas]

I don't have time now to test, but this should be the proper implementation:

Code

	{
		FilesList::iterator it = m_Files.find(pf);

		if (it == m_Files.end())
		{
			Manager::Get()->GetLogManager()->DebugLog(_T("Can't locate node for ProjectFile* !"));
		}
		else
		{
			if (!m_FileArray.IsEmpty())
				m_FileArray.Remove(*it);

			m_Files.erase(it);
		}
	}

I am somewhat undecided about that checking for emptiness of the array. Actually it is kind of useless code since you won't ever remove a file from a project that isn't part of the project (at least, you shouldn't ever need to?). Unless someone writes a plugin that removes random filenames from a project or something the like, that should never happen.
On the other hand, wxArray does not silently ignore removing an element that is not in the array as you would expect, but triggers an assertion, so if that happens... bang, you're dead. That's somewhat uncool.

[comsytec]

If find() gives != end() then *it == pf should be true or I'm thinking bad. Of course inside else block but if already fixed then ok.

[Pecan]

I've test Thomas' code and solve a crash I've been having when an automated system deleted a file previously added to the project.

I'd like to commit it. Any objections?

[oBFusCATed]

Nope...

[MortenMacFly]

Crash fixes are always welcome, of course....