On Sunday, April 7th, 2024 at 6:14 PM, Okan Demirmen <o...@demirmen.com> wrote:
> On Sun 2024.04.07 at 12:11 -0400, Okan Demirmen wrote:
>
> > On Sat 2024.04.06 at 19:30 +0000, ZenitDS wrote:
> >
> > > Hello Okan,
> > >
> > > Here is a better explanation of where the issue lies, hopefully clarifies
> > > it
> > >
> > > When moving any window, a small window that is child of the client's
> > > window
> > > is created in the top-left corner and the WM works with it assuming it is
> > > kept
> > > alive for as long as the moving/resizing lasts.
> > > This means that if the client destroys its window, (thus destroying its
> > > childs),
> > > cwm will continue to do operations on the destroyed window. This leads to
> > > a
> > > crash (I assume because of X11 killing it).
> > > The idea of the sample code was to detect the creation of the child
> > > window and
> > > destroy the window in that moment, leading to a certain crash.
> > > It is also reproducible by
> > > $ sleep 5 && exit
> > > inside a terminal emulator by starting to move the window before the exit.
> > > Probably this would have illustrated better the issue.
> >
> > Ah okay, this makes sense. In fact the issue here is that we are
> > specificing the grap window on the client; instead we should grab
> > against the root for both move and resize and use coordinates relative
> > to root instead. Something like the below:
>
>
> oh and for the record, cwm doesn't crash, it just gets wedged in
> blocking XMaskEvent waiting for an event on a window that's no longer.
> sure, that's not good of course!
I see, since it was no longer responding, I believed it was a crash. Should
have done a pgrep.
Your patch fixes the issue completely (much better than my garbage code :)).
>
> > Index: kbfunc.c
> > ===================================================================
> > RCS file: /home/open/cvs/xenocara/app/cwm/kbfunc.c,v
> > diff -u -p -r1.174 kbfunc.c
> > --- kbfunc.c 20 Jul 2023 14:39:34 -0000 1.174
> > +++ kbfunc.c 7 Apr 2024 16:06:32 -0000
> > @@ -162,7 +162,7 @@ kbfunc_client_move_mb(void *ctx, struct
> >
> > client_ptr_inbound(cc, 1);
> >
> > - if (XGrabPointer(X_Dpy, cc->win, False, MOUSEMASK,
> > + if (XGrabPointer(X_Dpy, sc->rootwin, False, MOUSEMASK,
> > GrabModeAsync, GrabModeAsync, None, Conf.cursor[CF_MOVE],
> > CurrentTime) != GrabSuccess)
> > return;
> > @@ -251,7 +251,7 @@ kbfunc_client_resize_mb(void *ctx, struc
> >
> > xu_ptr_set(cc->win, cc->geom.w, cc->geom.h);
> >
> > - if (XGrabPointer(X_Dpy, cc->win, False, MOUSEMASK,
> > + if (XGrabPointer(X_Dpy, sc->rootwin, False, MOUSEMASK,
> > GrabModeAsync, GrabModeAsync, None, Conf.cursor[CF_RESIZE],
> > CurrentTime) != GrabSuccess)
> > return;
> > @@ -267,8 +267,8 @@ kbfunc_client_resize_mb(void *ctx, struc
> > continue;
> > ltime = ev.xmotion.time;
> >
> > - cc->geom.w = ev.xmotion.x;
> > - cc->geom.h = ev.xmotion.y;
> > + cc->geom.w = ev.xmotion.x - cc->geom.x - cc->bwidth;
> > + cc->geom.h = ev.xmotion.y - cc->geom.y - cc->bwidth;
> > client_apply_sizehints(cc);
> > client_resize(cc, 1);
> > screen_prop_win_draw(sc,
> >
> > > Cheers
> > >
> > > > On Sat 2024.04.06 at 15:40 +0000, ZenitDS wrote:
> > > >
> > > > > Hello,
> > > > >
> > > > > I sent this patch two months ago but got no response. I send it
> > > > > again just in case.
> > > > > Some kind of response like, "ok, we have seen it but dont want to
> > > > > merge or mess with this", would be nice.
> > > >
> > > > Hi - I saw this and was at first confused.
> > > >
> > > > > When running cwm you can destroy the client window while
> > > > > you are moving or resizing it, leading to crash of the WM
> > > > > when the prop window is accessed (the one at the top-left that
> > > > > indicates the position/scale).
> > > > >
> > > > > I include a demonstration for the crash and also a patch that
> > > > > fixes it. Hopefully someone with more experience here can implement
> > > > > a better solution.
> > > >
> > > > Looking again, I'm still confused as to why this sample is essentialy
> > > > pulling events from the x event queue, rather than using XWindowEvent();
> > > > it seems a contrived example. What is the use-case outside of this
> > > > example?
> > > >
> > > > Best regards,
> > > > Okan
> > > >
> > > > > Demonstration code for the crash:
> > > > >
> > > > > #include <X11/Xlib.h>
> > > > >
> > > > > #include <err.h>
> > > > > #include <unistd.h>
> > > > >
> > > > > int
> > > > > main(void)
> > > > > {
> > > > > Display *dpy;
> > > > > Window win;
> > > > > int scr;
> > > > > XEvent ev;
> > > > >
> > > > > dpy = XOpenDisplay(NULL);
> > > > > if (!dpy)
> > > > > err(1, "XOpenDisplay");
> > > > >
> > > > > scr = DefaultScreen(dpy);
> > > > >
> > > > > win = XCreateSimpleWindow(dpy, RootWindow(dpy, scr), 0, 0, 500, 500,
> > > > > 0, 0, WhitePixel(dpy, scr));
> > > > > if (!win)
> > > > > err(1, "XCreateSimpleWindow");
> > > > >
> > > > > XSelectInput(dpy, win, StructureNotifyMask | SubstructureNotifyMask |
> > > > > ExposureMask);
> > > > > XMapRaised(dpy, win);
> > > > >
> > > > > while (1) {
> > > > > XNextEvent(dpy, &ev);
> > > > > switch (ev.type) {
> > > > > case CreateNotify:
> > > > > goto end;
> > > > > case Expose:
> > > > > XClearWindow(dpy, win);
> > > > > break;
> > > > > default:
> > > > > break;
> > > > > }
> > > > > }
> > > > > end:
> > > > > XDestroyWindow(dpy, win);
> > > > >
> > > > > XCloseDisplay(dpy);
> > > > >
> > > > > return 0;
> > > > > }
> > > > >
> > > > > Here is the patch:
> > > > >
> > > > > Index: calmwm.h
> > > > > ===================================================================
> > > > > RCS file: /cvs/xenocara/app/cwm/calmwm.h,v
> > > > > retrieving revision 1.379
> > > > > diff -u -p -r1.379 calmwm.h
> > > > > --- calmwm.h 20 Jul 2023 14:39:34 -0000 1.379
> > > > > +++ calmwm.h 6 Apr 2024 15:26:37 -0000
> > > > > @@ -481,7 +481,7 @@ struct geom screen_area(struct screen_
> > > > > struct screen_ctx *screen_find(Window);
> > > > > void screen_init(int);
> > > > > void screen_prop_win_create(struct screen_ctx *, Window);
> > > > > -void screen_prop_win_destroy(struct screen_ctx *);
> > > > > +void screen_prop_win_destroy(struct screen_ctx *, int);
> > > > > void screen_prop_win_draw(struct screen_ctx *,
> > > > > const char *, ...)
> > > > > attribute((format (printf, 2, 3)))
> > > > > @@ -558,6 +558,7 @@ void conf_screen(struct screen_ctx *)
> > > > > void conf_group(struct screen_ctx *);
> > > > >
> > > > > void xev_process(void);
> > > > > +void xev_process_ev(XEvent *);
> > > > >
> > > > > int xu_get_prop(Window, Atom, Atom, long, unsigned char **);
> > > > > int xu_get_strprop(Window, Atom, char **);
> > > > > Index: kbfunc.c
> > > > > ===================================================================
> > > > > RCS file: /cvs/xenocara/app/cwm/kbfunc.c,v
> > > > > retrieving revision 1.174
> > > > > diff -u -p -r1.174 kbfunc.c
> > > > > --- kbfunc.c 20 Jul 2023 14:39:34 -0000 1.174
> > > > > +++ kbfunc.c 6 Apr 2024 15:26:37 -0000
> > > > > @@ -169,8 +169,8 @@ kbfunc_client_move_mb(void *ctx, struct
> > > > >
> > > > > screen_prop_win_create(sc, cc->win);
> > > > > screen_prop_win_draw(sc, "%+5d%+5d", cc->geom.x, cc->geom.y);
> > > > > - while (move) {
> > > > > - XMaskEvent(X_Dpy, MOUSEMASK, &ev);
> > > > > + while (move > 0) {
> > > > > + XMaskEvent(X_Dpy, MOUSEMASK | SubstructureNotifyMask, &ev);
> > > > > switch (ev.type) {
> > > > > case MotionNotify:
> > > > > /* not more than 60 times / second */
> > > > > @@ -197,11 +197,27 @@ kbfunc_client_move_mb(void ctx, struct
> > > > > case ButtonRelease:
> > > > > move = 0;
> > > > > break;
> > > > > + / check for destroy events, in case the client window
> > > > > + * gets destroyed, which forcefully closes the prop window.
> > > > > + /
> > > > > + case DestroyNotify:
> > > > > + / set move to -1 to specify abrupt exit /
> > > > > + if (ev.xdestroywindow.window == cc->win) {
> > > > > + screen_prop_win_destroy(sc, 1);
> > > > > + move = -1;
> > > > > + } else if (ev.xdestroywindow.window == sc->prop.win) {
> > > > > + screen_prop_win_destroy(sc, 0);
> > > > > + move = -1;
> > > > > + }
> > > > > + default: / process event anyway */
> > > > > + xev_process_ev(&ev);
> > > > > }
> > > > > }
> > > > > - if (ltime)
> > > > > - client_move(cc);
> > > > > - screen_prop_win_destroy(sc);
> > > > > + if (move != -1) {
> > > > > + if (ltime)
> > > > > + client_move(cc);
> > > > > + screen_prop_win_destroy(sc, 1);
> > > > > + }
> > > > > XUngrabPointer(X_Dpy, CurrentTime);
> > > > > }
> > > > >
> > > > > @@ -258,7 +274,7 @@ kbfunc_client_resize_mb(void *ctx, struc
> > > > >
> > > > > screen_prop_win_create(sc, cc->win);
> > > > > screen_prop_win_draw(sc, "%4d x %-4d", cc->dim.w, cc->dim.h);
> > > > > - while (resize) {
> > > > > + while (resize > 0) {
> > > > > XMaskEvent(X_Dpy, MOUSEMASK, &ev);
> > > > > switch (ev.type) {
> > > > > case MotionNotify:
> > > > > @@ -277,11 +293,26 @@ kbfunc_client_resize_mb(void ctx, struc
> > > > > case ButtonRelease:
> > > > > resize = 0;
> > > > > break;
> > > > > + / check for destroy events, in case the client window
> > > > > + * gets destroyed, which forcefully closes the prop window.
> > > > > + /
> > > > > + case DestroyNotify:
> > > > > + if (ev.xdestroywindow.window == cc->win) {
> > > > > + screen_prop_win_destroy(sc, 1);
> > > > > + resize = -1;
> > > > > + } else if (ev.xdestroywindow.window == sc->prop.win) {
> > > > > + screen_prop_win_destroy(sc, 0);
> > > > > + resize = -1;
> > > > > + }
> > > > > + default: / process event anyway */
> > > > > + xev_process_ev(&ev);
> > > > > }
> > > > > }
> > > > > - if (ltime)
> > > > > - client_resize(cc, 1);
> > > > > - screen_prop_win_destroy(sc);
> > > > > + if (resize != -1) {
> > > > > + if (ltime)
> > > > > + client_resize(cc, 1);
> > > > > + screen_prop_win_destroy(sc, 1);
> > > > > + }
> > > > > XUngrabPointer(X_Dpy, CurrentTime);
> > > > >
> > > > > /* Make sure the pointer stays within the window. */
> > > > > Index: screen.c
> > > > > ===================================================================
> > > > > RCS file: /cvs/xenocara/app/cwm/screen.c,v
> > > > > retrieving revision 1.98
> > > > > diff -u -p -r1.98 screen.c
> > > > > --- screen.c 27 Jan 2022 18:45:10 -0000 1.98
> > > > > +++ screen.c 6 Apr 2024 15:26:37 -0000
> > > > > @@ -278,10 +278,15 @@ screen_prop_win_create(struct screen_ctx
> > > > > }
> > > > >
> > > > > void
> > > > > -screen_prop_win_destroy(struct screen_ctx *sc)
> > > > > +screen_prop_win_destroy(struct screen_ctx sc, int destroy_window)
> > > > > {
> > > > > XftDrawDestroy(sc->prop.xftdraw);
> > > > > - XDestroyWindow(X_Dpy, sc->prop.win);
> > > > > + /
> > > > > + * In case the window was already destroyed
> > > > > + * (i.e. DestroyNotify event)
> > > > > + */
> > > > > + if (destroy_window)
> > > > > + XDestroyWindow(X_Dpy, sc->prop.win);
> > > > > }
> > > > >
> > > > > void
> > > > > Index: xevents.c
> > > > > ===================================================================
> > > > > RCS file: /cvs/xenocara/app/cwm/xevents.c,v
> > > > > retrieving revision 1.150
> > > > > diff -u -p -r1.150 xevents.c
> > > > > --- xevents.c 24 Mar 2020 14:47:29 -0000 1.150
> > > > > +++ xevents.c 6 Apr 2024 15:26:37 -0000
> > > > > @@ -485,9 +485,15 @@ xev_process(void)
> > > > >
> > > > > while (XPending(X_Dpy)) {
> > > > > XNextEvent(X_Dpy, &e);
> > > > > - if ((e.type - Conf.xrandr_event_base) == RRScreenChangeNotify)
> > > > > - xev_handle_randr(&e);
> > > > > - else if ((e.type < LASTEvent) && (xev_handlers[e.type] != NULL))
> > > > > - (*xev_handlers[e.type])(&e);
> > > > > + xev_process_ev(&e);
> > > > > }
> > > > > +}
> > > > > +
> > > > > +void
> > > > > +xev_process_ev(XEvent *ev)
> > > > > +{
> > > > > + if ((ev->type - Conf.xrandr_event_base) == RRScreenChangeNotify)
> > > > > + xev_handle_randr(ev);
> > > > > + else if ((ev->type < LASTEvent) && (xev_handlers[ev->type] != NULL))
> > > > > + (*xev_handlers[ev->type])(ev);
> > > > > }
