Re: cwm ornery about raisewindow requests

[Okan Demirmen]

On Thu 2013.05.02 at 00:22 -0400, sma...@panix.com wrote:
Hi,

First off, thanks for the detailed report!

> While using emacs org mode and entering a timestamp using [C-c !] the
> emacs window would drop to the bottom of the X stacking order. This
> command can be traced to the raise-frame function which sends a raise
> window request to cwm.
>
> It happened consistently for me, but when I asked another user on
> daemonforums to try it he found it only happened on his macppc and not
> his i386 machine. Also, I had it go away briefly when I fumbled in
> getting debugging information (unstripped but no -g or maybe a mix of
> object files with -g and without, not sure now). I mention this in
> case you don't see the problem yourself. There's an uninitialized auto
> involved, which I believe to be the cause of the pseudo-intermittency.
>
> With debugging info I could step through xevents.c::xev_handle_configurerequest
> and see the following:
>
> 1. The call to XConfigureWindow reuses the value mask from the
> incoming XConfigureReqestEvent.
> 2. The value of that mask was 64. i.e. CWStackMode (defined as 1 << 6 in X.h)
> 3. CWStackMode is not one of the bits looked for in the preceding
> check masks and set corresponding wc field statements.
> 4. Not involved in my scenario I don't think, but CWSibling is also missing.
> 5. The XWindowChanges variable wc used for the XConfigureWindow call
> happens to have the value 1 for its stack_mode field for me. 1 is
> defined as Below in X.h. Above is 0.
>
> When I add checks for CWStackMode and CWSibling with appropriate sets
> of the corresponding wc fields (excuse the pun) the problem seems
> fixed. If it's on top it stays on top. If it's lower it raises.

Unfortunately, I'm not able to replicate, nor I suppose understand, the
current behavior wrt to the behavior with the patch applied - basically,
there's no change in my tests. I could be missing your point...

> A couple other things puzzled me in this function that don't affect me
> and may not be important:
> 1. wc.border_width will be set if the incoming event has CWBorderWidth
> in it's value mask, but a few lines later that value will be overwritten with
> cc->bwidth.
> 2. In the else clause the CWStackMode of the value mask is cleared but
> the wc.stack_mode field is set. Is this for documentation that the
> window will be raised by other means when the window eventually gets mapped?

Yeah, those's intentional - "calm wm" is supposed to keep things in a
way that the user expects; is that always right? probably not, but
keeps the behavior sane. That said, you're right that it makes no sense
to grab it then throw it away...

Again though, I don't believe our X event handling does everything
right; something to review.

> >How-To-Repeat:
> 1. with cwm running start emacs and an xterm
> 2. place the emacs window so it overlaps the xterm window
> 3. in the scratch buffer evaluate the emacs lisp expression (raise-frame)
> 4. you may see the emacs window fall to the bottom.
>
> Or, if you don't want to run emacs, compile and run this
> program (I wanted to eliminate emacs as a culprit at one
> point). If you overlap another window with it and click on it
> you may see it fall to the back, or something.

Sorry, I'm not able to replicate the issue with this either. If there
is a missing initialization (as you mentioned), that could be why, and I
want to find that as well. The patch isn't wrong, it just doesn't seem
to change things (yet).

Cheers,
Okan

[Mike Small]

Sorry I was unclear. Annotations follow (excuse me if this is gets too
verbose or if I state obvious things)...

static void
xev_handle_configurerequest(XEvent *ee)
{
XConfigureRequestEvent *e = &ee->xconfigurerequest;
struct client_ctx *cc;
struct screen_ctx *sc;

-> wc is not initialized here (of course, this isn't C++), particularly the
-> .sibling and .stack_mode fields. As matters later, these fields
-> correspond, I believe (I have an Xlib book on order -- the X man pages
-> leave me guessing slightly on this point), to the .above and .detail
-> fields in XConfigureRequestEvent. See XConfigureRequestEvent(3) and
-> XConfigureWindow(3). This latter page has problems you can see below,
-> btw. I intend to report this (to OpenBSD? to Xorg?) later along with
-> another man page problem but don't have my notes handy for that.

XWindowChanges wc;

if ((cc = client_find(e->window)) != NULL) {
sc = cc->sc;

if (e->value_mask & CWWidth)
cc->geom.w = e->width;
if (e->value_mask & CWHeight)
cc->geom.h = e->height;
if (e->value_mask & CWX)
cc->geom.x = e->x;
if (e->value_mask & CWY)
cc->geom.y = e->y;

-> This if block is useless since wc.border_width is re-assigned below.

if (e->value_mask & CWBorderWidth)
wc.border_width = e->border_width;

if (cc->geom.x == 0 && cc->geom.w >= sc->view.w)
cc->geom.x -= cc->bwidth;

if (cc->geom.y == 0 && cc->geom.h >= sc->view.h)
cc->geom.y -= cc->bwidth;

-> Next is where wc's members get their values, but two of them
-> (namely .sibling and .stack_mode) are not set so will have fairly unpredictable
-> values. No doubt when you run you happen to get a zero for
-> wc.stacking_mode, which corresponds to Above. I get 1, corresponding to
-> Below. If you try my steps again, but instead put my test program window
-> on the bottom and click on a corner hanging out from under, my guess is
-> that you'll see nothing happen. But if you look at my code you can see
-> that clicking on the window should make it request a raise in the
-> stacking order.
->
-> From XConfigureWindow(3):
->
-> /* Configure window value mask bits */
->
-> #define .el CWX (1<<0) [[what's this .el doing here?]]
-> #define .el CWY (1<<1)
-> #define .el CWWidth (1<<2)
-> #define .el CWHeight (1<<3)
-> #define .el CWBorderWidth (1<<4)
-> #define .el CWSibling (1<<5)
-> #define .el CWStackMode (1<<6)
-> /* Values */
->
-> typedef struct {
-> int x, y;
-> int width, height;
-> int border_width;
-> Window sibling;
-> int stack_mode;
-> } XWindowChanges;
->
-> From X.h:
-> #define Above 0
-> #define Below 1
-> #define TopIf 2
-> #define BottomIf 3
-> #define Opposite 4
->
wc.x = cc->geom.x;
wc.y = cc->geom.y;
wc.width = cc->geom.w;
wc.height = cc->geom.h;
wc.border_width = cc->bwidth;

-> Next we're re-using the e->value_mask that came in with the client's
-> configure request for our outgoing configure request. But e->value_mask
-> was set to tell us what we can look at on ee->xconfigurerequest.
-> XConfigureWindow needs a mask to say which fields we set on wc. The set
-> of fields we assigned on wc is not the same as the set of fields that
-> were assigned on e by the client's request (wc lacks sibling and stack_mode,
-> or rather has not very predictable values for those since we didn't
-> assign to them). When I looked in the debugger I could see that
-> e->value_mask was 64, a.k.a. 1<<6, a.k.a. CWStackMode, and that wc.stack_mode
-> happened to be 1 somehow. The result was to send a stacking order
-> request with this uninitialized value, 1 or Below, for wc.stack_mode
-> and sending emacs or my test program to the bottom. You may have 0
-> (Above) sending your window to the top. If you start with it at the top,
-> you see no action.
->
XConfigureWindow(X_Dpy, cc->win, e->value_mask, &wc);
xu_configure(cc);
} else {
/* let it do what it wants, it'll be ours when we map it. */
wc.x = e->x;
wc.y = e->y;
wc.width = e->width;
wc.height = e->height;
wc.border_width = e->border_width;
wc.stack_mode = Above;
e->value_mask &= ~CWStackMode;

XConfigureWindow(X_Dpy, e->window, e->value_mask, &wc);
...


I hope the problem is clearer now. If not, let me know and I'll try
again. I often struggle to make myself understood it seems.

So if it is clear, then about my patch. I thought it might be too bold
to say so initially, but I don't think re-using e->value_mask in
XConfigureWindow is wise here. If the changes structure ever gets new
members then we're right back to the same problem. I'd think it better
to dedicate a separate mask variable for XConfigureWindow's use that
gets its bits set as the corresponding changes struct fields are
set. e.g. something like this (untested)...

...
unsigned long wc_values_mask;
...
wc_values_mask = 0
...
if (e->value_mask & CWSibling) {
wc.sibling = e->above;
wc_values_mask |= CWSibling;
}
if (e->value_mask & CWStackMode) {
wc.stack_mode = e->detail;
wc_values_mask |= CWStackMode;
}

wc.x = cc->geom.x;
wc.y = cc->geom.y;
wc.width = cc->geom.w;
wc.height = cc->geom.h;
wc.border_width = cc->bwidth;
wc_values_mask |= CWX | CWY | CWWidth | CWHeight | CWBorderWidth;

XConfigureWindow(X_Dpy, e->window, wc_values_mask, &wc);

But I'm not an Xlib programmer, so I have no idea if it's a realistic
expectation for XConfigureRequestEvent and XWindowChanges to ever
change.

--
Mike Small
sma...@panix.com

[Mike Small]

Mike Small <sma...@panix.com> writes:

> -> Next is where wc's members get their values, but two of them
> -> (namely .sibling and .stack_mode) are not set so will have fairly unpredictable
> -> values. No doubt when you run you happen to get a zero for
> -> wc.stacking_mode, which corresponds to Above. I get 1, corresponding to
> -> Below. If you try my steps again, but instead put my test program window
> -> on the bottom and click on a corner hanging out from under, my guess is
> -> that you'll see nothing happen. But if you look at my code you can see
> -> that clicking on the window should make it request a raise in the
> -> stacking order.

This is mixed up. You would have to change the test program to make a
lower request and then click on the window while it is on top.

--
Mike Small
sma...@panix.com

[Mike Small]

Maybe a problem with my previous report was that the steps only gave a
reproducible case on macppc (and even that was surprising since the
uninitialized variable, wc.stack_mode, had to happen to pick up the
value 1 -- the value for Below -- though it consistently happened for me
and another user on daemonforums on our macppc machines). Here are
steps I can use to show the bug on an amd64 machine. This variation
should be more likely to be reproducible in general since any value
other than 1 for wc.stack_mode will yield incorrect behaviour.

I) with emacs (this time 21.3 from the 5.3 cd, but that shouldn't matter):
1. Place the emacs window above another window
2. In the *Scratch* buffer evaluate the following expression:
(lower-frame) ; eval an expression by pressing ctrl-c, ctrl-e
; after it or ctrl-meta-x in it

The frame incorrectly stays on top.

II) without emacs:
1. compile the following program
========================================================================
#include <X11/Xlib.h>
#include <X11/Xutil.h>

#include <err.h>
#include <stdlib.h>

const char msg_lower[] = "Click to lower.";
const char msg_quit[] = "Q to quit.";

int
main()
{
Display *dpy;
Screen *s;
Window win;
XEvent ev;
GC gc;
XGCValues values;
KeySym k;
unsigned long black;

dpy = XOpenDisplay(NULL);
if (!dpy)
errx(1, "XOpenDpy: Couldn't open X dpy.");
s = DefaultScreenOfDisplay(dpy);
black = BlackPixelOfScreen(s);

win = XCreateSimpleWindow(dpy, RootWindowOfScreen(s),
WidthOfScreen(s)/2 - 100, HeightOfScreen(s)/2 - 100,
180, 150, 2,
black, WhitePixelOfScreen(s));
XSelectInput(dpy, win, ButtonPressMask|KeyReleaseMask|ExposureMask);
XMapWindow(dpy, win);

values.font = XLoadFont(dpy, "fixed");
gc = XCreateGC(dpy, win, GCFont, &values);

while (1) {
XNextEvent(dpy, &ev);
switch (ev.type) {
case ButtonPress:
XLowerWindow(dpy, win);
break;
case KeyRelease:
k = XkbKeycodeToKeysym(dpy, ev.xkey.keycode, 0, 0);
if (k == XK_q || k == XK_Q) {
XCloseDisplay(dpy);
return 0;
}
break;
case Expose:
XDrawString(dpy, win, gc, 20, 20, msg_lower, sizeof(msg_lower) - 1);
XDrawString(dpy, win, gc, 20, 75, msg_quit, sizeof(msg_quit) - 1);
break;
}
}
return 1;
}

========================================================================
2. place its window above another window
3. click on its window

The window again incorrectly stays on top.

After applying the following patch, I see the right behaviour on my
amd64 machine. The windows drop to the bottom of the stacking order in
each case.

Index: xevents.c
===================================================================
RCS file: /cvs/xenocara/app/cwm/xevents.c,v
retrieving revision 1.71
diff -u -u -p -r1.71 xevents.c
--- xevents.c 5 Apr 2013 17:07:25 -0000 1.71
+++ xevents.c 24 Apr 2013 02:49:00 -0000
@@ -151,6 +151,10 @@ xev_handle_configurerequest(XEvent *ee)

cc->geom.y = e->y;

if (e->value_mask & CWBorderWidth)
wc.border_width = e->border_width;

+ if (e->value_mask & CWSibling)
+ wc.sibling = e->above;
+ if (e->value_mask & CWStackMode)
+ wc.stack_mode = e->detail;

if (cc->geom.x == 0 && cc->geom.w >= sc->view.w)
cc->geom.x -= cc->bwidth;

dmesg for this other machine (acpi disabled to get it booting -- I'll
provide a proper report for that issue sometime):

OpenBSD 5.3 (GENERIC) #53: Tue Mar 12 18:15:44 MDT 2013
der...@amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC
real mem = 1600815104 (1526MB)
avail mem = 1535799296 (1464MB)
mainbus0 at root
bios0 at mainbus0: SMBIOS rev. 2.4 @ 0xfcd90 (44 entries)
bios0: vendor American Megatrends Inc. version "306" date 09/14/2007
bios0: ASUSTeK Computer Inc. Z35FM
acpi at bios0 not configured
mpbios0 at bios0: Intel MP Specification 1.4
cpu0 at mainbus0: apid 0 (boot processor)
cpu0: Intel(R) Core(TM)2 CPU T7200 @ 2.00GHz, 1995.32 MHz
cpu0: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,DTES64,MWAIT,DS-CPL,VMX,EST,TM2,SSSE3,CX16,xTPR,PDCM,NXE,LONG,LAHF,PERF
cpu0: 4MB 64b/line 16-way L2 cache
cpu0: apic clock running at 166MHz
cpu at mainbus0: not configured
mpbios0: bus 0 is type PCI
mpbios0: bus 1 is type PCI
mpbios0: bus 2 is type PCI
mpbios0: bus 3 is type PCI
mpbios0: bus 4 is type PCI
mpbios0: bus 5 is type PCI
mpbios0: bus 6 is type ISA
ioapic0 at mainbus0: apid 2 pa 0xfec00000, version 20, 24 pins
cpu0: unknown Enhanced SpeedStep CPU, msr 0x06130c2806000c28
cpu0: using only highest and lowest power states
cpu0: Enhanced SpeedStep 1995 MHz: speeds: 2000, 1000 MHz
pci0 at mainbus0 bus 0
pchb0 at pci0 dev 0 function 0 "Intel 82945GM Host" rev 0x03
vga1 at pci0 dev 2 function 0 "Intel 82945GM Video" rev 0x03
wsdisplay0 at vga1 mux 1: console (80x25, vt100 emulation)
wsdisplay0: screen 1-5 added (80x25, vt100 emulation)
intagp0 at vga1
agp0 at intagp0: aperture at 0xd0000000, size 0x10000000
inteldrm0 at vga1: apic 2 int 16
drm0 at inteldrm0
"Intel 82945GM Video" rev 0x03 at pci0 dev 2 function 1 not configured
azalia0 at pci0 dev 27 function 0 "Intel 82801GB HD Audio" rev 0x02: msi
azalia0: codecs: Realtek ALC660, Motorola/0x3055, using Realtek ALC660
audio0 at azalia0
ppb0 at pci0 dev 28 function 0 "Intel 82801GB PCIE" rev 0x02: msi
pci1 at ppb0 bus 1
ppb1 at pci0 dev 28 function 1 "Intel 82801GB PCIE" rev 0x02
pci2 at ppb1 bus 4
wpi0 at pci2 dev 0 function 0 "Intel PRO/Wireless 3945ABG" rev 0x02: msi, MoW1, address 00:13:02:1a:b1:b4
uhci0 at pci0 dev 29 function 0 "Intel 82801GB USB" rev 0x02: apic 2 int 23
uhci1 at pci0 dev 29 function 1 "Intel 82801GB USB" rev 0x02: apic 2 int 19
uhci2 at pci0 dev 29 function 2 "Intel 82801GB USB" rev 0x02: apic 2 int 18
uhci3 at pci0 dev 29 function 3 "Intel 82801GB USB" rev 0x02: apic 2 int 16
ehci0 at pci0 dev 29 function 7 "Intel 82801GB USB" rev 0x02: apic 2 int 23
usb0 at ehci0: USB revision 2.0
uhub0 at usb0 "Intel EHCI root hub" rev 2.00/1.00 addr 1
ppb2 at pci0 dev 30 function 0 "Intel 82801BAM Hub-to-PCI" rev 0xe2
pci3 at ppb2 bus 5
"Ricoh 5C832 Firewire" rev 0x00 at pci3 dev 3 function 0 not configured
sdhc0 at pci3 dev 3 function 1 "Ricoh 5C822 SD/MMC" rev 0x19: apic 2 int 20
sdmmc0 at sdhc0
"Ricoh 5C843 MMC" rev 0x01 at pci3 dev 3 function 2 not configured
"Ricoh 5C592 Memory Stick" rev 0x0a at pci3 dev 3 function 3 not configured
"Ricoh 5C852 xD" rev 0x05 at pci3 dev 3 function 4 not configured
rl0 at pci3 dev 4 function 0 "Realtek 8139" rev 0x10: apic 2 int 20, address 00:18:f3:cf:55:06
rlphy0 at rl0 phy 0: RTL internal PHY
pcib0 at pci0 dev 31 function 0 "Intel 82801GBM LPC" rev 0x02
pciide0 at pci0 dev 31 function 1 "Intel 82801GB IDE" rev 0x02: DMA, channel 0 configured to compatibility, channel 1 configured to compatibility
wd0 at pciide0 channel 0 drive 0: <ST980825A>
wd0: 16-sector PIO, LBA48, 76319MB, 156301488 sectors
atapiscsi0 at pciide0 channel 0 drive 1
scsibus0 at atapiscsi0: 2 targets
cd0 at scsibus0 targ 0 lun 0: <TSSTcorp, CD/DVDW TS-L632D, AS05> ATAPI 5/cdrom removable
wd0(pciide0:0:0): using PIO mode 4, Ultra-DMA mode 5
cd0(pciide0:0:1): using PIO mode 4, Ultra-DMA mode 2
pciide0: channel 1 ignored (disabled)
usb1 at uhci0: USB revision 1.0
uhub1 at usb1 "Intel UHCI root hub" rev 1.00/1.00 addr 1
usb2 at uhci1: USB revision 1.0
uhub2 at usb2 "Intel UHCI root hub" rev 1.00/1.00 addr 1
usb3 at uhci2: USB revision 1.0
uhub3 at usb3 "Intel UHCI root hub" rev 1.00/1.00 addr 1
usb4 at uhci3: USB revision 1.0
uhub4 at usb4 "Intel UHCI root hub" rev 1.00/1.00 addr 1
isa0 at pcib0
isadma0 at isa0
pckbc0 at isa0 port 0x60/5
pckbd0 at pckbc0 (kbd slot)
pckbc0: using irq 1 for kbd slot
wskbd0 at pckbd0: console keyboard, using wsdisplay0
pms0 at pckbc0 (aux slot)
pckbc0: using irq 12 for aux slot
wsmouse0 at pms0 mux 0
pms0: Synaptics touchpad, firmware 6.2
pcppi0 at isa0 port 0x61
spkr0 at pcppi0
mtrr: Pentium Pro MTRR support
uhub5 at uhub2 port 1 "Chicony Generic USB Hub" rev 1.10/1.00 addr 2
uhidev0 at uhub5 port 1 configuration 1 interface 0 "Chicony PFU-65 USB Keyboard" rev 1.10/1.00 addr 3
uhidev0: iclass 3/1
ukbd0 at uhidev0: 8 variable keys, 6 key codes
wskbd1 at ukbd0 mux 1
wskbd1: connecting to wsdisplay0
uhidev1 at uhub5 port 3 configuration 1 interface 0 "Microsoft Microsoft 3-Button Mouse with IntelliEye(TM)" rev 1.10/3.00 addr 4
uhidev1: iclass 3/1
ums0 at uhidev1: 3 buttons, Z dir
wsmouse1 at ums0 mux 0
vscsi0 at root
scsibus1 at vscsi0: 256 targets
softraid0 at root
scsibus2 at softraid0: 256 targets
root on wd0a (72eb03b5e79429f1.a) swap on wd0b dump on wd0b

> -> Next is where wc's members get their values, but two of them
> -> (namely .sibling and .stack_mode) are not set so will have fairly unpredictable
> -> values. No doubt when you run you happen to get a zero for
> -> wc.stacking_mode, which corresponds to Above. I get 1, corresponding to
> -> Below. If you try my steps again, but instead put my test program window
> -> on the bottom and click on a corner hanging out from under, my guess is
> -> that you'll see nothing happen. But if you look at my code you can see
> -> that clicking on the window should make it request a raise in the
> -> stacking order.

[Okan Demirmen]

Hi Mike,

I could have replied earlier - your 4 line diff exploded into a huge
diff to fix event handling (recall I said I was sure some other stuff
was not right :)).

If I don't have a working re-write soon, I'll address just the bits
you've pointed out separately and move on from there. Thank you for
your detailed reports so far!

Thanks,
Okan