Author Topic: Thread Search crashes by design. RFC about proposed changes to events in SDK  (Read 10238 times)

Offline Miguel Gimenez

  • Developer
  • Lives here!
  • *****
  • Posts: 1551
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.

« Last Edit: January 23, 2019, 07:18:04 pm by Miguel Gimenez »

Offline Miguel Gimenez

  • Developer
  • Lives here!
  • *****
  • Posts: 1551
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.
« Last Edit: January 23, 2019, 07:18:15 pm by Miguel Gimenez »

Offline oBFusCATed

  • Developer
  • Lives here!
  • *****
  • Posts: 13413
    • Travis build status
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.
(most of the time I ignore long posts)
[strangers don't send me private messages, I'll ignore them; post a topic in the forum, but first read the rules!]

Online ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5906
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
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?
If some piece of memory should be reused, turn them to variables (or const variables).
If some piece of operations should be reused, turn them to functions.
If they happened together, then turn them to classes.

Offline Miguel Gimenez

  • Developer
  • Lives here!
  • *****
  • Posts: 1551
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

Offline oBFusCATed

  • Developer
  • Lives here!
  • *****
  • Posts: 13413
    • Travis build status
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!
(most of the time I ignore long posts)
[strangers don't send me private messages, I'll ignore them; post a topic in the forum, but first read the rules!]

Offline Miguel Gimenez

  • Developer
  • Lives here!
  • *****
  • Posts: 1551
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.

Offline oBFusCATed

  • Developer
  • Lives here!
  • *****
  • Posts: 13413
    • Travis build status
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 :(
(most of the time I ignore long posts)
[strangers don't send me private messages, I'll ignore them; post a topic in the forum, but first read the rules!]

Offline Miguel Gimenez

  • Developer
  • Lives here!
  • *****
  • Posts: 1551
Thank you for the comments, last patch version added to ticket 777.

Online ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5906
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Maybe a better name could be: cbEVT_DESTROY_LOG_WINDOW?  ;)
If some piece of memory should be reused, turn them to variables (or const variables).
If some piece of operations should be reused, turn them to functions.
If they happened together, then turn them to classes.

Offline Miguel Gimenez

  • Developer
  • Lives here!
  • *****
  • Posts: 1551
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

Online ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5906
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
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.
« Last Edit: February 11, 2019, 07:03:41 am by ollydbg »
If some piece of memory should be reused, turn them to variables (or const variables).
If some piece of operations should be reused, turn them to functions.
If they happened together, then turn them to classes.

Offline oBFusCATed

  • Developer
  • Lives here!
  • *****
  • Posts: 13413
    • Travis build status
@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.
(most of the time I ignore long posts)
[strangers don't send me private messages, I'll ignore them; post a topic in the forum, but first read the rules!]

Offline oBFusCATed

  • Developer
  • Lives here!
  • *****
  • Posts: 13413
    • Travis build status
This bug should be fix in master. Hopefully I've not broken something else. Testing is welcome.
(most of the time I ignore long posts)
[strangers don't send me private messages, I'll ignore them; post a topic in the forum, but first read the rules!]

Offline Miguel Gimenez

  • Developer
  • Lives here!
  • *****
  • Posts: 1551
Ticket 807 https://sourceforge.net/p/codeblocks/tickets/807 has the same root causes and probably a similar solution.