Code::Blocks Forums

Developer forums (C::B DEVELOPMENT STRICTLY!) => Development => Topic started by: Miguel Gimenez on January 23, 2019, 02:37:17 pm

Title: Thread Search crashes by design. RFC about proposed changes to events in SDK
Post by: Miguel Gimenez on January 23, 2019, 02:37:17 pm
The Thread Search makes C::B crash when changing it to disabled (see ticket 777)

https://sourceforge.net/p/codeblocks/tickets/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.

Title: Re: Thread Search crashes by design. RFC about proposed changes to events in SDK
Post by: Miguel Gimenez on January 23, 2019, 04:43:37 pm
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.
Title: Re: Thread Search crashes by design. RFC about proposed changes to events in SDK
Post by: oBFusCATed on January 23, 2019, 07:40:31 pm
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.
Title: Re: Thread Search crashes by design. RFC about proposed changes to events in SDK
Post by: ollydbg on January 24, 2019, 08:24:06 am
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?
Title: Re: Thread Search crashes by design. RFC about proposed changes to events in SDK
Post by: Miguel Gimenez on January 24, 2019, 09:43:40 am
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
Title: Re: Thread Search crashes by design. RFC about proposed changes to events in SDK
Post by: oBFusCATed on January 24, 2019, 09:42:38 pm
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!
Title: Re: Thread Search crashes by design. RFC about proposed changes to events in SDK
Post by: Miguel Gimenez on January 25, 2019, 12:11:40 pm
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.
Title: Re: Thread Search crashes by design. RFC about proposed changes to events in SDK
Post by: oBFusCATed on January 25, 2019, 07:15:22 pm
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 :(
Title: Re: Thread Search crashes by design. RFC about proposed changes to events in SDK
Post by: Miguel Gimenez on January 29, 2019, 10:09:02 am
Thank you for the comments, last patch version added to ticket 777.
Title: Re: Thread Search crashes by design. RFC about proposed changes to events in SDK
Post by: ollydbg on January 30, 2019, 03:45:21 pm
Maybe a better name could be: cbEVT_DESTROY_LOG_WINDOW?  ;)
Title: Re: Thread Search crashes by design. RFC about proposed changes to events in SDK
Post by: Miguel Gimenez on January 30, 2019, 04:41:23 pm
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 (https://docs.wxwidgets.org/trunk/classwx_sizer.html#a362d7d556185fe9cd1b5d24004b86518)
Title: Re: Thread Search crashes by design. RFC about proposed changes to events in SDK
Post by: ollydbg on February 11, 2019, 06:41:27 am
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 (https://docs.wxwidgets.org/trunk/classwx_sizer.html#a362d7d556185fe9cd1b5d24004b86518)
Thanks for the explanation, I think Detach is a good name.
Title: Re: Thread Search crashes by design. RFC about proposed changes to events in SDK
Post by: oBFusCATed on February 11, 2019, 06:47:18 pm
@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.
Title: Re: Thread Search crashes by design. RFC about proposed changes to events in SDK
Post by: oBFusCATed on March 28, 2020, 09:41:36 pm
This bug should be fix in master. Hopefully I've not broken something else. Testing is welcome.
Title: Re: Thread Search crashes by design. RFC about proposed changes to events in SDK
Post by: Miguel Gimenez on March 28, 2020, 10:40:38 pm
Ticket 807 https://sourceforge.net/p/codeblocks/tickets/807 (https://sourceforge.net/p/codeblocks/tickets/807) has the same root causes and probably a similar solution.
Title: Re: Thread Search crashes by design. RFC about proposed changes to events in SDK
Post by: oBFusCATed on March 29, 2020, 12:20:53 am
Probably, but I don't have time for this at the moment.
Probably it will be good to introduce your patch for the new event anyway...
I'll revisit this someday...