crash on close cbp project (rev 7773)

[MortenMacFly]

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]

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]

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]

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]

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]

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]

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]

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

[Jenna]

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]

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]

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

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.