Thread Search crashes by design. RFC about proposed changes to events in SDK

[Miguel Gimenez]

The Thread Search makes C::B crash when changing it to disabled (see ticket 777)

https://sourceforge.net/p/codeblocks/tickets/777/

This is because it sends a cbEVT_REMOVE_LOG_WINDOW event (who calls DeletePage() from the infopane which in turn deletes the page) and then tries to call Reparent() of the deleted page.

So the cbEVT_REMOVE_LOG_WINDOW name is misleading, as it deletes the page instead of removing.

I suggest creating a cbEVT_DELETE_LOG_WINDOW event with the actual behaviour of cbEVT_REMOVE_LOG_WINDOW and change the latter so it calls RemoveNonLogger() (already existent but currently unused) instead of DeleteNonLogger().

There are 12 uses of cbEVT_REMOVE_LOG_WINDOW in assorted plugins that can be simply renamed, leaving Thread Search use as is.

[Miguel Gimenez]

This patch contains the proposed changes. ThreadSearch now works as it should and the other plugins are not affected because for them the change is only a rename.

I don't know if these changes affect the SDK API.

[oBFusCATed]

I'm not sure adding another event and modifying all plugins is worth it...But I don't have the time to really investigate how to properly fix this.

[ollydbg]

I'm not familiar with such part, but look at the patch:

Code

@@ -443,6 +443,9 @@
 // remove a log window
 extern EVTIMPORT const wxEventType cbEVT_REMOVE_LOG_WINDOW;
 #define EVT_REMOVE_LOG_WINDOW(fn) DECLARE_EVENT_TABLE_ENTRY( cbEVT_REMOVE_LOG_WINDOW, -1, -1, (wxObjectEventFunction)(wxEventFunction)(CodeBlocksLogEventFunction)&fn, (wxObject *) NULL ),
+// delete a log window
+extern EVTIMPORT const wxEventType cbEVT_DELETE_LOG_WINDOW;
+#define EVT_DELETE_LOG_WINDOW(fn) DECLARE_EVENT_TABLE_ENTRY( cbEVT_DELETE_LOG_WINDOW, -1, -1, (wxObjectEventFunction)(wxEventFunction)(CodeBlocksLogEventFunction)&fn, (wxObject *) NULL ),

People will get puzzled when they see "remove" and "delete" a log window. So, maybe a better event name?

[Miguel Gimenez]

I didn't invent them, the names are chosen to match wxNotebook's method names:
  - cbEVT_REMOVE_LOG_WINDOW at the end calls Notebook::RemovePage()
  - cbEVT_DELETE_LOG_WINDOW at the end calls Notebook::DeletePage()

Previously the cbEVT_REMOVE_LOG_WINDOW called DeletePage() and not RemovePage(), which is a lot confusing and fooled ThreadSearch's author to think the window was still usable after removing it.

Of course the comment is too terse, I'll change them to
  REMOVE: // removes the specified window from the log but does not delete it
  DELETE: // removes the specified window from the log and deletes it

[oBFusCATed]

A change that requires changes to multiple plugins is undesirable. Especially if building the plugin with the new sdk code results in calling the wrong event!

[Miguel Gimenez]

OK, the proposal then is:
  - leave the original event untouched
  - create a new event cbEVT_DETACH_LOG_WINDOW so ThreadSearch stops crashing

The modified patch is attached.

[oBFusCATed]

I have to take a look at why this reparenting is needed. I'm not sure this is really the case. But last time I've looked at it the code looked quite complex

[Miguel Gimenez]

Thank you for the comments, last patch version added to ticket 777.

[ollydbg]

Maybe a better name could be: cbEVT_DESTROY_LOG_WINDOW? 

[Miguel Gimenez]

I don't think so, because DESTROY suggests that Window->Destroy() will be called, and in fact the purpose of this event is removing it from the log pane without destroying the window.

Detach is the verb used in wxWidgets for this action, see for example wxSizer::Detach()

https://docs.wxwidgets.org/trunk/classwx_sizer.html#a362d7d556185fe9cd1b5d24004b86518

[ollydbg]

OK, the proposal then is:
  - leave the original event untouched
  - create a new event cbEVT_DETACH_LOG_WINDOW so ThreadSearch stops crashing

The modified patch is attached.

I'm testing your patch, and it works OK.

BTW, I see when I close C::B(without your patch), the C::B process is still seen in the Windows Task manager, so I have to manually select the C::B and kill it. But I can't find the reason under GDB, when I halt the GDB, I see backtrace contains some function call inside the wx library and waiting for the gui objects.
With your patch, I don't see this issue. (EDIT: the hang issue still happens with your patch, so this is not related)

I don't think so, because DESTROY suggests that Window->Destroy() will be called, and in fact the purpose of this event is removing it from the log pane without destroying the window.

Detach is the verb used in wxWidgets for this action, see for example wxSizer::Detach()

https://docs.wxwidgets.org/trunk/classwx_sizer.html#a362d7d556185fe9cd1b5d24004b86518

Thanks for the explanation, I think Detach is a good name.

[oBFusCATed]

@ollydbg: I still think that we don't need this new event, so I prefer if you don't add it yet.

The problem is not that critical anyway, yes it is bad, but it is not blocking the usage of C::B for many people.

[oBFusCATed]

This bug should be fix in master. Hopefully I've not broken something else. Testing is welcome.

[Miguel Gimenez]

Ticket 807 https://sourceforge.net/p/codeblocks/tickets/807 has the same root causes and probably a similar solution.