crash on close cbp project (rev 7773)

[ollydbg]

gdb points to here:

Code

bool cbProject::CloseAllFiles(bool dontsave)
{
    // first try to close modified editors

    if (!dontsave && !QueryCloseAllFiles())
            return false;

    // now free the rest of the project files
    Manager::Get()->GetEditorManager()->HideNotebook();
    FilesList::iterator it = m_Files.begin();
    while (it != m_Files.end())
    {
        ProjectFile* f = *it;
        if (f)
        {
            Manager::Get()->GetEditorManager()->Close(f->file.GetFullPath(),true);
        }
        m_Files.erase(it);
        m_FileArray.Remove(*it); //crash here****************************
        delete f;
        it = m_Files.begin();
    }
    Manager::Get()->GetEditorManager()->ShowNotebook();

    return true;
}

No time to find the reason right now.

[ollydbg]

I test this again, but it does not crash every time I close the cbp project.

But I found another crash after I close the C::B, gdb point to the desturctor of ClassBrowser.
1, start c::b.
2, close c::b, and it crashes.

Code

// class destructor
ClassBrowser::~ClassBrowser()
{
    int pos = XRCCTRL(*this, "splitterWin", wxSplitterWindow)->GetSashPosition();
    Manager::Get()->GetConfigManager(_T("code_completion"))->Write(_T("/splitter_pos"), pos);

    SetParser(NULL);

    if (m_ClassBrowserBuilderThread)
    {
        m_ClassBrowserSemaphore.Post();
        // m_ClassBrowserBuilderThread->Delete(); --> would delete it twice and leads to a warning
        m_ClassBrowserBuilderThread->Wait();
    }
}

[oBFusCATed]

http://forums.codeblocks.org/index.php/topic,15882.0.html

gdb points to here:

Code

        m_Files.erase(it);
        m_FileArray.Remove(*it); //crash here****************************
        delete f;

The iterator is invalid, after the erase method. The correct code is m_FileArray.Remove(f); I guess. Or you can swap the two methods (erase and Remove)

[Jenna]

The iterator is invalid, after the erase method. The correct code is m_FileArray.Remove(f); I guess. Or you can swap the two methods (erase and Remove)

Definitely not, because:

Code

ProjectFile* f = *it;

The fix by Pecan and committed by Thomas is also not a fix in my opinion (or wxWidgets has a bug in the Remove-function), but it does no harm.

I'm not sure if an invalid iterator is really the cause for it (I did not dig into the sources of wxWidgets deeper, to see whether the iterator can be invalid in this case).

Just changeing the order of removal should do it, first

Code

        m_FileArray.Remove(*it);

and then

Code

        m_Files.erase(it);

In this case the iterator can not be invalid.

In most cases the Remove-method is called on an empty Array, because the array only gets filled if cbProject::GetFile was called at least once, and this is only done by script-functions at the moment.
According to the wxWidgets documentation we can get an assert in debug-mode, but in release-mode just nothing happens, if the element we try to remove does not exist.
If the cause for the crash should be, that we try to remove a not-existing element, we would need to check whether the array is empty and in this case not run the Remove-method.

[Jenna]

I hope it's finally fixed with svn r7777 (with this revision-number it must be fixed  ).

Additionally closing large projects should be much faster.
My very large test-project (linux kernel 2.6.35 with more than 30000 files) closes about 4 times faster (1200 ms instead of almost 5000 ms).
Times are measured without codecompletion-plugin, which is a little bit unstable at the moment.

[ollydbg]

I hope it's finally fixed with svn r7777 (with this revision-number it must be fixed  ).

Additionally closing large projects should be much faster.
My very large test-project (linux kernel 2.6.35 with more than 30000 files) closes about 4 times faster (1200 ms instead of almost 5000 ms).
Times are measured without codecompletion-plugin, which is a little bit unstable at the moment.

Hi, jens, I can confirm that open/close the cbp project does not crash again. Thank you!

[MortenMacFly]

Times are measured without codecompletion-plugin, which is a little bit unstable at the moment.

It shouldn't be any longer. If it is, please report. I have concluded the things I wanted to change. The next change will take some more time...

[oBFusCATed]

Is there any particular reason to remove the ';' after the locking macros? It looks pretty weird.

I'll give it a try in a minute and I'll report if there are any crashes.

[ollydbg]

Is there any particular reason to remove the ';' after the locking macros? It looks pretty weird.

One reason I can say is: It will make our cc's parser happy(they like a function call statement) .

[MortenMacFly]

Is there any particular reason to remove the ';' after the locking macros? It looks pretty weird.

Yes, because if you set the macros to nothing, they will expand to truly nothing. Otherwise they would expand to a semicolon. the latter I believe is "more weird". ;-)

[oBFusCATed]

Yes, because if you set the macros to nothing, they will expand to truly nothing. Otherwise they would expand to a semicolon. the latter I believe is "more weird". ;-)

No, it is not, because you (almost) never look at the preprocessed code
Also is there any particular reason to set the macros to nothing in this case?

[MortenMacFly]

Also is there any particular reason to set the macros to nothing in this case?

Yes, if you have followed the discussion with Thomas, instead of locking / unlocking which is excellent for debugging, the better way would be to use a locker object. In that case, the "Unlock" macro has to / will expand to nothing.

[oBFusCATed]

I doubt you'll be able the switch to a locker object with just a macro:)

[MortenMacFly]

I doubt you'll be able the switch to a locker object with just a macro:)

Why not? The first macro will expand to wxMutexLocker(BLAH) and the second to nothing. That's it.

[oBFusCATed]

But then you have no guarantee that the mutex will be unlocked at the correct place/time.

Code

CC_LOCK(s_lock);
... code 1 ...
CC_UNLOCK(s_lock);
... code 2 ...
CC_LOCK(s_lock);
... code 3 ...
CC_UNLOCK(s_lock);

Code 2 is executed under a lock and before code3 you have a double locking problem (probably a dead lock).