suggest to fix a crash when running thread search's "find occurance of xxx"

[ollydbg]

I get this crash from time to time, here is the crash log in codeblocks.RPT(I'm using latest nightly build rev 10136)

Code

...codeblocks.exe caused an Access Violation at location 77c46fa3 in module C:\WINDOWS\system32\msvcrt.dll Reading from location 07d81000.

Registers:
eax=0860c644 ebx=00e60056 ecx=00222d91 edx=00000002 esi=07d80ffe edi=0f054a3c
eip=77c46fa3 esp=0e8efba8 ebp=0e8efbb0 iopl=0         nv up ei pl nz ac pe nc
cs=001b  ss=0023  ds=0023  es=0023  fs=003b  gs=0000             efl=00010212

Call stack:
77C46FA3  C:\WINDOWS\system32\msvcrt.dll:77C46FA3  memcpy
6CC9C2C0  E:\code\cb\CodeBlocksUnofficial\wxmsw28u_gcc_cb.dll:6CC9C2C0  _ZN12wxStringBase8InitWithEPKwjj

I suspect this is due to the same issue as I reported in: thread issue about wxCommandEvent in wx 2.8.12

I finally fix this issue by introduce a new class named CodeBlocksThreadEvent in rev 9311, and CC now use this class for communications between threads. and later in rev 9403, FileManager plugin switched to use CodeBlocksThreadEvent instead of wxCommandEvent to avoid nasty crash (wxCommandEvent has thread safety issues).

Now, I see that trunk\src\plugins\contrib\ThreadSearch\ThreadSearchThread.h

Code

/***************************************************************
 * Name:      ThreadSearchThread
 *
 * Purpose:   This class implements the search worker thread.
 *            It sends events to the view when one or more
 *            search pattern is (are) found in a file; one
 *            event/file.
 *            It derives from wxDirTraverser to test thread
 *            cancel when searching in directory.
 *            One event/file is sent.
 *
 * Author:    Jerome ANTOINE
 * Created:   2007-10-08
 * Copyright: Jerome ANTOINE
 * License:   GPL
 **************************************************************/

So, there are many events sent from worker thread to GUI thread.

I look at the code: trunk\src\plugins\contrib\ThreadSearch\ThreadSearchEvent.h

Code

class ThreadSearchEvent : public wxCommandEvent
{
public:
    /** Constructor. */
    ThreadSearchEvent(wxEventType commandType = wxEVT_NULL, int id = 0);

    /** Copy constructor. */
    ThreadSearchEvent(const ThreadSearchEvent& event);

    /** Destructor. */
    ~ThreadSearchEvent();

    virtual wxEvent *Clone() const {return new ThreadSearchEvent(*this);}

    DECLARE_DYNAMIC_CLASS(ThreadSearchEvent);

    wxArrayString GetLineTextArray() const {return m_LineTextArray;};   // Contains a series of string containing
                                                                        // line index (starting from 1), matching line
    void SetLineTextArray(const wxArrayString& ArrayString) {m_LineTextArray = ArrayString;};

    size_t GetNumberOfMatches() const { return m_LineTextArray.GetCount() / 2; }

private:
    wxArrayString m_LineTextArray;
};

In cpp file

Code

ThreadSearchEvent::ThreadSearchEvent(const ThreadSearchEvent& Event)
                  :wxCommandEvent(Event)
{
    m_LineTextArray = Event.GetLineTextArray();
}

I think this event is NOT thread safe compared the thread safe CodeBlocksThreadEvent.

Code

// Thread event, this is basically a derived wxCommandEvent but enforce a deep copy of its
// m_cmdString member. wxEVT_COMMAND_MENU_SELECTED is reused and event handlers are matched by
// ids. This is just to conserve the old code, an alternative is use some
// new event type like: cbEVT_THREAD_LOG_MESSAGE, cbEVT_THREAD_LOGDEBUG_MESSAGE
// cbEVT_THREAD_SYSTEM_HEADER_UPDATE.

class CodeBlocksThreadEvent : public wxCommandEvent
{
public:
    CodeBlocksThreadEvent(wxEventType eventType = wxEVT_NULL, int id = wxID_ANY)
        : wxCommandEvent(eventType,id)
        { }

    CodeBlocksThreadEvent(const CodeBlocksThreadEvent& event)
        : wxCommandEvent(event)
    {
        // make sure our string member (which uses COW, aka refcounting) is not
        // shared by other wxString instances:
        SetString(GetString().c_str());
    }

    virtual wxEvent *Clone() const
    {
        return new CodeBlocksThreadEvent(*this);
    }


private:
    DECLARE_DYNAMIC_CLASS_NO_ASSIGN(CodeBlocksThreadEvent)
};

So, I think not only the wxString member should be deep cloned ( such as the code SetString(GetString().c_str()); ), but I think the m_LineTextArray (a wxArrayString, which contains many wxString instances) should also be deep cloned inside the const copy constructor.

What do you think?

[oBFusCATed]

What do you think?

Make a patch and see if stops reproducing...

[ollydbg]

...
Make a patch and see if stops reproducing...

I will do this.
I did create a simple test code against wx 2.8.12:

Code

    wxArrayString a;
    wxString xyz(wxT("xyz"));
    wxString uvw(wxT("uvw"));
    a.Add(wxT("abcdefg"));
    a.Add(wxT("1234567"));
    wxArrayString b;
    b = a;
    b.Add(xyz);
    wxArrayString c(a);
    c.Add(uvw);

Under GDB, I see that both a,b,c share some contents, this looks like I need to clone wxArrayString myself. See below:

Code

[debug]> p a
[debug]$7 = wxArray<T> = {0xb386ac L"abcdefg", 0xb38754 L"1234567"}
[debug]>>>>>>cb_gdb:

$7 = wxArray<T> = {0xb386ac L"abcdefg", 0xb38754 L"1234567"}
> p b

[debug]> p b
[debug]$8 = wxArray<T> = {0xb386ac L"abcdefg", 0xb38754 L"1234567", 0xb3860c L"xyz"}
[debug]>>>>>>cb_gdb:

$8 = wxArray<T> = {0xb386ac L"abcdefg", 0xb38754 L"1234567", 0xb3860c L"xyz"}
> p c

[debug]> p c
[debug]$9 = wxArray<T> = {0xb386ac L"abcdefg", 0xb38754 L"1234567", 0xb3865c L"uvw"}
[debug]>>>>>>cb_gdb:

$9 = wxArray<T> = {0xb386ac L"abcdefg", 0xb38754 L"1234567", 0xb3865c L"uvw"}

I will try to fix this issue.

[ollydbg]

The patch below should fix this issue, I don't see the crash for some testing. Please review it, and I will commit if no objections in a few days. Thanks.

[ollydbg]

In trunk now.