swallow patch causing freezes and crashes

[2084x]

I decided to try out your swallow patch today as it has some improvements over the one on suckless.org, but I've found that it causes crashes and freezes if the swallowing occurs while you are viewing a different tag. to reproduce:

1. open a terminal on tag 1 and run sleep 3; mpv somefile.mkv
2. switch to a different tag
3. switch back to tag 1 a few seconds later after the terminal has been swallowed. (mpv also oddly starts in audio mode which it shouldn't)
4. quit mpv to unswallow and observe crash.

I can reproduce this every time and with any program. sometimes programs behave a little bit different though. for example if you try to run nsxiv, the terminal will just hang until you hit ctrl+c to exit which causes dwm to crash and push you to the login screen. or in the case of a program being run from inside a script, the program tends to open fine, but on exit dwm freezes when it tries to unswallow.

[bakkeby]

Thanks for reporting. Very good explanation of the issue and replication steps.

I don't spot the cause of this looking at the patch code on its own. Can't say that I have come across such an issue before either.

I'll need to have a closer look at this in about a week's time (I'm on a short trip away from home).

[2084x]

after running some more tests on a clean build I discovered that the crashing is caused by a conflict with the pertag_with_sel patch I was using. both patches alter unmanage() and I'm not experienced enough to know how to make them compatible... I switched over to the regular pertag patch you have in this repo and everything is working fine, so I think I'll stick with that.

sorry for the trouble and enjoy your trip :)

[bakkeby]

I had a look at the pertag with sel patch and I think that it should work if the for loop (that clears the selected client) is run before the code that the swallow patch introduces.

[2084x]

it should work if the for loop (that clears the selected client) is run before the code that the swallow patch introduces.

that's how I had it when I first ran into the issues. I also tried placing the for loop after the swallow code and it made no difference.

[bakkeby]

That pertag with sel patch has a bug / flaw in that when unmanaging a client it will only search the current monitor for a matching client, so it is possible to end up in a situation where pertag on another monitor is referring to a client that no longer exist. It should really be looping through all monitors when doing that check.

I very much doubt that this has anything to do with the issue you experienced.

If it is a dangling pointer issue unrelated to the pertag with sel patch then it is possible that the issue is still present in your build even if you can't reproduce the issue at the moment.

If you'd like to share your build then I can have a look to see if I spot any problem areas.

[2084x]

you can see my build here if you'd like. the most recent commit swapped to the regular pertag patch which has no issues as far as I'm aware.

although this shouldn't really matter as I've tried dwm with only the swallow and pertag with sel patches applied and the issue is present. perhaps it's not the for loop in unmanage(), but there is definitely an incompatibility with those two patches specifically.

I've also come across some swallow + fullscreen bugs today which I'll open up a new issue for soon.

[bakkeby]

Complicated compatibility issue to work out.

The root cause appears to be that the pertag (with sel) patch keeps track of which client was the selected one, so when you move back to the tag where the window swallowing took place it implicitly focuses on the terminal window bringing that back into view (obscuring parts or the whole of the other window).

One can work around that by looping through all the pertag entries and replacing the new and old client references (or setting sel to NULL like in unmanage).

diff --git a/dwm.c b/dwm.c
index d72b1f6..1c0f350 100644
--- a/dwm.c
+++ b/dwm.c
@@ -1358,6 +1358,7 @@ recttomon(int x, int y, int w, int h)
 void
 replaceclient(Client *old, Client *new)
 {
+       int i;
        Client *c = NULL;
        Monitor *mon = old->mon;

@@ -1385,6 +1386,10 @@ replaceclient(Client *old, Client *new)
        old->next = NULL;
        old->snext = NULL;

+       for (i = 0; i < LENGTH(tags) + 1; i++)
+               if (mon->pertag->sel[i] == old)
+                       mon->pertag->sel[i] = new;
+
        XMoveWindow(dpy, old->win, WIDTH(old) * -2, old->y);

        if (ISVISIBLE(new) && !new->isfullscreen) {

This of course doesn't address the issue that client references can end up on more than one monitor with the pertag-with-sel patch.

I think one could probably get away with clearing pertag->sel when moving a client from one monitor
to another via sendmon, e.g.

@@ -1540,8 +1545,12 @@ scan(void)
 void
 sendmon(Client *c, Monitor *m)
 {
+       int i;
        if (c->mon == m)
                return;
+       for (i = 0; i < LENGTH(tags) + 1; i++)
+               if (c->mon->pertag->sel[i] == c)
+                       c->mon->pertag->sel[i] = NULL;
        unfocus(c, 1);
        detach(c);
        detachstack(c);

Alternatively one could loop through all monitors when clearing pertag->sel in unmanage (and maybe in replaceclient).

It is a bit of an odd patch. I wonder what kind of workflow one need to have for selection feature to make a difference over the default behaviour of relying on the stack order.