wx2.8.12, sending event cross threads are not safe in current CB source

[ollydbg]

There are two functions which can send events from a worker thread to main GUI.
wxPostEvent() and AddPendingEvent()

According to the discussions below:

• wx forum post: thread safety issue about wxCommandEvent in wx2.8.12
• our CB forum post: unnecessory locker in CClogger
  

I think there are many issues in our C::B source:

wxCommandEvent is not thread safe for its m_cmdString member, e.g.:

Code

            
            wxString message;
            wxCommandEvent evt(...);
            evt.SetString(message.c_str());
            wxPostEvent(m_Parent, evt);

The code snippet is not thread safe, because "evt" and the cloned wxCommandEvent shared the same wxString member buffer.

CodeBlocksEvent is not thread safe, e.g. in cbthreadpool.cpp

Code

      CodeBlocksEvent evt = CodeBlocksEvent(cbEVT_THREADTASK_ALLDONE, m_ID);
      wxPostEvent(m_pOwner, evt);

CodeBlocksEvent is a derived class from wxCommandEvent, and I don't see it Clone its m_strString.

3, I'm not sure PipedProcess class has such issue, because its related to send event cross processes.

Solution:
If the event need to carry wxString from worker thread to main GUI thread, we should implement a Clone method which deep copy its m_cmdString. Just like the new event type wxThreadEvent in wx 2.9.x.

Any comments.

[MortenMacFly]

Solution:
If the event need to carry wxString from worker thread to main GUI thread, we should implement a Clone method which deep copy its m_cmdString. Just like the new event type wxThreadEvent in wx 2.9.x.

Yes, we should always do. As a matter of fact, for wx events its a good thing to always implement the clone method once you are using member vars.

[ollydbg]

Ok, this is the first draft patch. I add a new class named cbThreadEvent, which replace all the cross thread sending message from wxCommandEvent to cbThreadEvent. The patch also included an improved hook measure patch from: Performance measurement of all C::B event handlers

[MortenMacFly]

Ok, this is the first draft patch.

Could you do a proper SVN based patch? This patch does not apply.

[golgepapaz]

This ,

Could you do a proper SVN based patch? This patch does not apply.

Won't it be proper that defining the event in sdk_events.h and sdk_events.cpp instead of
cclogger.cpp and it's own header?

Also about that profiling stuff, I don't want to pay for something that I don't need. This should be
optional.

[MortenMacFly]

Won't it be proper that defining the event in sdk_events.h and sdk_events.cpp instead of
cclogger.cpp and it's own header?

Its not meant to be captured form outside. CC is a plugin, not the SDK. So the event is in the correct place.

Also about that profiling stuff, I don't want to pay for something that I don't need. This should be
optional.

Its only for debugging atm and surely will be in #defines to be explicitly compiled in or even removed after all. Patches posted here are fr playing with and will be cleaned-up before application. Don't you worry...

[golgepapaz]

Its not meant to be captured form outside. CC is a plugin, not the SDK. So the event is in the correct place.

I think it more of a general-purpose event than something exclusive to CC as the name
and the purpose suggest. Since we are stuck with wx2.8.12 for some time, I consider it a utility for
for developers who cannot use wxThreadEvent (I myself need it for example). There are many utility classes in the sdk like nullptr_t
cbThreadPool, cbAssert etc. I don't see why this should be different.

[ollydbg]

Note my patch is only a draft patch, which show the direction, but it surely need all your comments. So, thanks for all your comments.
@golgepapaz
I think a good place to put the new event in sdk_events.h and sdk_events.cpp. But for me, I just put it in a new file, because once I change the sdk_events.h, then I need to rebuild all my targets..... That's some times waste a lot of time.

I believe that sending event cross thread not only happens in CC.
About the performance measure on the event hooks, that's my direction to find a "hang" bug, because sometimes, my Codeblocks will hang for 2 or 3 seconds, so I would like to see which cause this hang issue.

@Morten, this is the patch against SVN.

[MortenMacFly]

@Morten, this is the patch against SVN.

OK, thanks for that - it applied safely.

However, having this applied the Spellchecker plugin does not compile anymore. The reason is that is derives directly from HookFunctorBase and therefore your newly introduced method "GetTypeName2 is not implemented as it is pure virtual.

This is (however) most likely a design issue in the SpellChecker plugin. I don't know the rationale for deriving directly from this class (probably due to the need to override "Call"?!), but IMHO that should not be done and needed.

Maybe danselmi can enlighten us...?! ;-)

EDIT: And its the same for the OccurrenceHighlighter plugin... :-/

[oBFusCATed]

Is OccurrenceHighlighter still needed? I don't see in the contrib subdir...

[danselmi]

This is (however) most likely a design issue in the SpellChecker plugin.

Corrected in rev. 9164

[danselmi]

Is OccurrenceHighlighter still needed? I don't see in the contrib subdir...

OccurrenceHighlighter is an addition to the occurrence highlighting implemented in the core. It is able to permanently (until the string gets removed from the list again) highlight strings in all editors. Not only the currently selected.

Should I add it into contrib subdir?

[MortenMacFly]

Should I add it into contrib subdir?

I think so, yes (in fact I was under the assumption its there already...). I find it quite useful!
And thanks for the fix, btw...

[oBFusCATed]

Should I add it into contrib subdir?

I'd say core and also extract the other highlight occurrence code from the sdk in it.

[ollydbg]

Note my patch is only a draft patch, which show the direction, but it surely need all your comments. So, thanks for all your comments.
@golgepapaz
I think a good place to put the new event in sdk_events.h and sdk_events.cpp. But for me, I just put it in a new file, because once I change the sdk_events.h, then I need to rebuild all my targets..... That's some times waste a lot of time.

I believe that sending event cross thread not only happens in CC.

Now, I add the class implementation in a cpp file.
This is a new patch ( I add a new cpp file in the SDK)
See attachment, any comments?