Author Topic: crash on close cbp project (rev 7773) (Read 23385 times)

ollydbg · « **on:** February 05, 2012, 02:45:39 am »

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 · « **Reply #1 on:** February 05, 2012, 02:56:02 am »

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 · « **Reply #2 on:** February 05, 2012, 10:34:58 am »

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

Quote from: ollydbg on February 05, 2012, 02:45:39 am

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 · « **Reply #3 on:** February 05, 2012, 10:52:18 am »

Quote from: oBFusCATed on February 05, 2012, 10:34:58 am

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 · « **Reply #4 on:** February 05, 2012, 01:25:19 pm »

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 · « **Reply #5 on:** February 05, 2012, 02:53:45 pm »

Quote from: jens on February 05, 2012, 01:25:19 pm

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 · « **Reply #6 on:** February 05, 2012, 03:02:34 pm »

Quote from: jens on February 05, 2012, 01:25:19 pm

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 · « **Reply #7 on:** February 05, 2012, 03:47:35 pm »

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 · « **Reply #8 on:** February 05, 2012, 03:54:36 pm »

Quote from: oBFusCATed on February 05, 2012, 03:47:35 pm

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 · « **Reply #9 on:** February 05, 2012, 04:09:49 pm »

Quote from: oBFusCATed on February 05, 2012, 03:47:35 pm

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 · « **Reply #10 on:** February 05, 2012, 04:13:17 pm »

Quote from: MortenMacFly on February 05, 2012, 04:09:49 pm

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 · « **Reply #11 on:** February 05, 2012, 04:16:17 pm »

Quote from: oBFusCATed on February 05, 2012, 04:13:17 pm

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 · « **Reply #12 on:** February 05, 2012, 04:21:09 pm »

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

MortenMacFly · « **Reply #13 on:** February 05, 2012, 04:43:53 pm »

Quote from: oBFusCATed on February 05, 2012, 04:21:09 pm

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 · « **Reply #14 on:** February 05, 2012, 04:56:30 pm »

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).

MortenMacFly · « **Reply #15 on:** February 05, 2012, 05:00:45 pm »

Quote from: oBFusCATed on February 05, 2012, 04:56:30 pm

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

I know, but in the places where this happens, it doesn't matter if you only lock a single code fragment, or the whole sequence. It only changes the behaviour slightly (the mutex is locked longer), but from what I inspected this is OK.

alternatively the first macro can expand to "{ wxMutexLocker(BLAH)" and the second to "}".

MortenMacFly · « **Reply #16 on:** February 05, 2012, 05:05:37 pm »

BTW, what is more interesting than the discussion of how macros look like (in fact I cant image anything more boring), what is worth noting is why I've switched from critical sections to mutexes:

In wxWidgets, mutexes have two interesting abilities:

1.) They return for Lock() and Unlock if it was error free (and return immediately therefore)
2.) You can do a "TryLock()" to see if a "Lock()" would fail in advance

Both is really good for debugging and if you compile the CC code with a certain switch, Code::Blocks will assert if a lock leads to a deadlock and show you the exact code line, where this happens.

ollydbg · « **Reply #17 on:** February 06, 2012, 02:09:41 am »

Quote from: ollydbg on February 05, 2012, 02:56:02 am

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();
    }
}

About this issue:
I debug a while but still can't find the reason, it looks like the crash happens that the splitter window's destructor (on delete its child)

The crash call stack:

Code

[debug]> bt 30
[debug]#0  0x7c90120f in ntdll!DbgUiConnectToDbg () from C:\WINDOWS\system32\ntdll.dll
[debug]#1  0x7c96ee31 in ntdll!RtlpNtMakeTemporaryKey () from C:\WINDOWS\system32\ntdll.dll
[debug]#2  0x7c96f26e in ntdll!RtlpNtMakeTemporaryKey () from C:\WINDOWS\system32\ntdll.dll
[debug]#3  0x7c970456 in ntdll!RtlpNtMakeTemporaryKey () from C:\WINDOWS\system32\ntdll.dll
[debug]#4  0x7c94bafc in ntdll!LdrFindEntryForAddress () from C:\WINDOWS\system32\ntdll.dll
[debug]#5  0x02f30000 in ?? ()
[debug]#6  0x7c91a1ba in ntdll!RtlpUnWaitCriticalSection () from C:\WINDOWS\system32\ntdll.dll
[debug]#7  0x77c2c2de in msvcrt!free () from C:\WINDOWS\system32\msvcrt.dll
[debug]#8  0x02f30000 in ?? ()
[debug]#9  0x6285c73f in wxWindowBase::DestroyChildren() () from E:\code\cb\wx\wxWidgets-2.8.12\lib\gcc_dll\wxmsw28u_gcc_custom.dll
[debug]#10 0x627af748 in wxWindow::~wxWindow() () from E:\code\cb\wx\wxWidgets-2.8.12\lib\gcc_dll\wxmsw28u_gcc_custom.dll
[debug]#11 0x628767e2 in wxSplitterWindow::~wxSplitterWindow() () from E:\code\cb\wx\wxWidgets-2.8.12\lib\gcc_dll\wxmsw28u_gcc_custom.dll
[debug]#12 0x6285c73f in wxWindowBase::DestroyChildren() () from E:\code\cb\wx\wxWidgets-2.8.12\lib\gcc_dll\wxmsw28u_gcc_custom.dll
[debug]#13 0x627af748 in wxWindow::~wxWindow() () from E:\code\cb\wx\wxWidgets-2.8.12\lib\gcc_dll\wxmsw28u_gcc_custom.dll
[debug]#14 0x6286fc12 in wxPanel::~wxPanel() () from E:\code\cb\wx\wxWidgets-2.8.12\lib\gcc_dll\wxmsw28u_gcc_custom.dll
[debug]#15 0x6285c73f in wxWindowBase::DestroyChildren() () from E:\code\cb\wx\wxWidgets-2.8.12\lib\gcc_dll\wxmsw28u_gcc_custom.dll
[debug]#16 0x627af748 in wxWindow::~wxWindow() () from E:\code\cb\wx\wxWidgets-2.8.12\lib\gcc_dll\wxmsw28u_gcc_custom.dll
[debug]#17 0x65e9d5f5 in ClassBrowser::~ClassBrowser (this=0x54ace40, __in_chrg=<optimized out>) at E:\code\cb\cb_trunk\src\plugins\codecompletion\classbrowser.cpp:173
[debug]#18 0x62859e65 in wxWindowBase::Destroy() () from E:\code\cb\wx\wxWidgets-2.8.12\lib\gcc_dll\wxmsw28u_gcc_custom.dll
[debug]#19 0x65ee5b01 in NativeParser::RemoveClassBrowser (this=0x3c2a930, appShutDown=true) at E:\code\cb\cb_trunk\src\plugins\codecompletion\nativeparser.cpp:743
[debug]#20 0x65ea8e75 in CodeCompletion::OnRelease (this=0x3c2a8e8, appShutDown=true) at E:\code\cb\cb_trunk\src\plugins\codecompletion\codecompletion.cpp:896
[debug]#21 0x00afdcab in cbPlugin::Release (this=0x3c2a8e8, appShutDown=true) at E:\code\cb\cb_trunk\src\sdk\cbplugin.cpp:64
[debug]#22 0x00abba6b in PluginManager::DetachPlugin (this=0x3bb4450, plugin=0x3c2a8e8) at E:\code\cb\cb_trunk\src\sdk\pluginmanager.cpp:207
[debug]#23 0x00ac1745 in PluginManager::UnloadPlugin (this=0x3bb4450, plugin=0x3c2a8e8) at E:\code\cb\cb_trunk\src\sdk\pluginmanager.cpp:1158
[debug]#24 0x00ac16f4 in PluginManager::UnloadAllPlugins (this=0x3bb4450) at E:\code\cb\cb_trunk\src\sdk\pluginmanager.cpp:1146
[debug]#25 0x00abb8ce in PluginManager::~PluginManager (this=0x3bb4450, __in_chrg=<optimized out>) at E:\code\cb\cb_trunk\src\sdk\pluginmanager.cpp:176
[debug]#26 0x00c8fb5c in Mgr<PluginManager>::Free () at E:/code/cb/cb_trunk/src/include/manager.h:192
[debug]#27 0x00b45554 in Manager::Shutdown () at E:\code\cb\cb_trunk\src\sdk\manager.cpp:149
[debug]#28 0x00424ada in MainFrame::OnApplicationClose (this=0x35f95d8, event=...) at E:\code\cb\cb_trunk\src\src\main.cpp:2953
[debug]#29 0x627720b6 in wxEvtHandler::ProcessEventIfMatches(wxEventTableEntryBase const&, wxEvtHandler*, wxEvent&) () from E:\code\cb\wx\wxWidgets-2.8.12\lib\gcc_dll\wxmsw28u_gcc_custom.dll
[debug](More stack frames follow...)

MortenMacFly · « **Reply #18 on:** February 06, 2012, 09:55:14 am »

Quote from: ollydbg on February 05, 2012, 02:56:02 am

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.

Doesn't happen here...?! Did you forget to update the XRC file (run update.bat)? Because it has changed!

ollydbg · « **Reply #19 on:** February 06, 2012, 12:49:04 pm »

Quote from: MortenMacFly on February 06, 2012, 09:55:14 am

Quote from: ollydbg on February 05, 2012, 02:56:02 am
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.
Doesn't happen here...?! Did you forget to update the XRC file (run update.bat)? Because it has changed!

Oh, sorry I forget this step, and c::b works OK now. Thanks for the reminding.

thomas · « **Reply #20 on:** February 06, 2012, 03:58:51 pm »

Quote from: MortenMacFly on February 05, 2012, 05:00:45 pm

alternatively the first macro can expand to "{ wxMutexLocker(BLAH)" and the second to "}".

*closes eyes as Morten opens Pandora's box*

Hopefully we don't ever declare a local variable somewhere after a CC_LOCK then. Well, the world ends in 2012 anyway, so let's not worry.

But yeah, I see it like you... the scoping is not at all an issue. Usually you lock exactly once per function, so it's clear anyway. And where that isn't the case, you want to have an extra pair of curly braces either way, to make the intent clear. And lastly, hopefully you'll get rid of 90% of these locks (which is the entire point).

Jenna · « **Reply #21 on:** February 09, 2012, 09:06:46 am »

In svn r7785 I fixed a possible crash in the layout-managers update routine, if the classbrowser is set to floating (and not integrated in the management-pane.
This happened due to an error in the refactoring in r7767, where a local variable hides the member-variable.

I also removed all calls of TestDestroy(), because they are only needed with Delete ( which can/should not be used for joinable threads ) and added a termination request therefore.
Without this request the call to semaphore->Post() in the classbrowser dtor awakes the builderthread for exactly one time, it loops through the Entry-functions and reaches the wait-state again, without termination.
A call of the Wait()-function of the builderthread, did never return in this case.
We also did not delete the thread-object on the heap, what can cause a memory-leak.
This might also fix crashes on shutdown that occured from time to time since some revisions.

ollydbg · « **Reply #22 on:** February 09, 2012, 02:11:28 pm »

There are a lot of TestDestroy() in Parserthread.cpp(In IS_ALIVE macro), should they be deleted too?

Jenna · « **Reply #23 on:** February 09, 2012, 02:22:47 pm »

Quote from: ollydbg on February 09, 2012, 02:11:28 pm

There are a lot of TestDestroy() in Parserthread.cpp(In IS_ALIVE macro), should they be deleted too?

No, because ParserThread is derived from cbThreadedTask, which has an own implementation of TestDestroy() .

MortenMacFly · « **Reply #24 on:** February 09, 2012, 02:46:47 pm »

Quote from: jens on February 09, 2012, 09:06:46 am

Without this request the call to semaphore->Post() in the classbrowser dtor awakes the builderthread for exactly one time, it loops through the Entry-functions and reaches the wait-state again, without termination.

That was a good one, a relict from my trials to remove calling "directly" into the thread from the class browser UI. I had in mind to (and actually did) provide the builder thread with a job queue so that the class browser attaches jobs as needed and once they are done the UI event is fired up again if signalled from the thread. However, this didn't work reliable. My second thought was to move the UI parts out f the thread to the class browser (where they belong to) and send events every time as needed. But also this didn't work well due to the fact, that wxTreeCtrl events need to be allowed (event.Allow()) in time to be processed correctly. So if you hit a plus (to expand a tree item) "too fast" you ended up in a race condition. I gave up for the moment (due to other stuff) but I still don't like the implementation of the class browser as it is in the moment...

ollydbg · « **Reply #25 on:** February 11, 2012, 04:01:45 am »

Quote from: jens on February 09, 2012, 02:22:47 pm

Quote from: ollydbg on February 09, 2012, 02:11:28 pm
There are a lot of TestDestroy() in Parserthread.cpp(In IS_ALIVE macro), should they be deleted too?
No, because ParserThread is derived from cbThreadedTask, which has an own implementation of TestDestroy() .

Thanks for the explanation. I see the cbThreadedTask class use the much similar implementation like you did:

Code

inline bool cbThreadedTask::TestDestroy() const
{
  return m_abort;
}

inline bool cbThreadedTask::Aborted() const
{
  return m_abort;
}

inline void cbThreadedTask::Abort()
{
  m_abort = true;
}

Quote from: MortenMacFly on February 09, 2012, 02:46:47 pm

That was a good one, a relict from my trials to remove calling "directly" into the thread from the class browser UI. I had in mind to (and actually did) provide the builder thread with a job queue so that the class browser attaches jobs as needed and once they are done the UI event is fired up again if signalled from the thread. However, this didn't work reliable.

This way looks better than the current implementation.

News:

Author Topic: crash on close cbp project (rev 7773) (Read 23385 times)