Created attachment 3645[details]
Logs for the described bug reproduce
There is something in glib 2.76 that makes Openbox crash when focusing/minimizing/switching between windows.
The scenarios that I managed to reproduce it with are the following:
1) Run conky in "desktop" own_window_type, open any 3D game (e.g. openjk, ioquake3, any Wine game), and execute ToggleShowDesktop. Openbox will try to focus conky and will crash.
Alternatively, switch the windows with default Alt-Tab action (NextWindow + Focus, Raise, Unshade) with conky active. Same result.
2) Run any VirtualBox VM in a fullscreen (it needs to be fullscreen before power on).
I am attaching logs for cases 1) and 2), but there is nothing useful in them.
Reverting to glib 2.74.6 fixes the issues.
Change in glib that might be relevant: https://gitlab.gnome.org/GNOME/glib/-/issues/2937
Openbox 3.6.1-9
Arch Linux 6.1.18-1-lts
nvidia 1:525.89.02-9
xorg-server 21.1.7-1
Hi, classic gnome developers as usual I see. I looked at your patch and the archlinux thread and see the problem with the code though. I think your patch is better than the code avoiding the list copy in the thread, which is n^2, while the copy is linear. It makes sense to me to just copy the list too since the goal is to visit every item in the list exactly once anyway. Perhaps the best thing is to just copy the items with layer FULLSCREEN, but i really doubt it matters in practice.
I've committed it in the work branch (rebaseable) in my repo. If you want to be credited under another name than "pldubouilh" in the commit, let me know.
@Mikachu, @pldubouilh: out of curiosity, could you elaborate what was the exact issue with the code and why copying the list fixes it, and how it relates to glib change with removing the slice allocator?
We just ran into this bug in Ubuntu 23.04 after a glib update - the above attachment that I just posted has a patch I'm testing for it. I believe this is why the bug is happening and how the patch fixes it.
Openbox maintains a list of windows organized by stacking order from highest to lowest. This is a doubly-linked list.
There is a function in Openbox called client_calc_layer that uses a pointer into this list to walk through it while modifying it. When client_calc_layer modifies the list, it also messes up the pointer that it is actively using to walk through the list. The next element that Openbox loads contains a dangling pointer, and that promptly results in a segfault when Openbox tries to dereference that pointer.
To solve the problem, I added a patch that makes Openbox save a pointer to the list element *before* the current element before modifying the list. When the list is then modified, the still-valid pointer to the previous list element is used to get a new (and actually usable!) pointer to the current list element. The old, broken pointer is then overwritten by the new, working pointer, and the segfaults stop.
I'm not sure if your patch is sufficient, the ->next pointer of the itPrev you save could still be the element that was removed by the client_calc_layer_internal call.
How so? From what I understand, "it" only gets corrupted because it's pointing to an object that is removed, so the pointers it points to are no longer correct. itPrev shouldn't get removed though, so the pointers it points to should still be pointing to valid places.
However, I am somewhat of a noob when it comes to C and am not deeply familiar with Openbox's inner workings, so I'm very open to being wrong and would love to have a better patch than this.
The proposed copy-the-list patch (as taken by Mikachu and Debian) ends with "g_list_free(it)", which won't have any effect, as "it" is NULL at that point. Was that meant to be "g_list_free(list)"?
Created attachment 3646 [details] patch