An attempt at fixing a bug involving DGA and mouse buttons
Peter Hutterer
peter.hutterer at who-t.net
Thu Aug 23 19:29:18 PDT 2012
Hi Steven,
Thanks for the analysis, very helpful.
On Sat, Aug 18, 2012 at 08:46:01PM -0500, Steven Elliott wrote:
> I'd like to describe a bug I've encountered involving mouse events and
> DGA as well as my attempt at fixing it. Hopefully either my patch is
> acceptable, or people will have ideas about how it can be improved.
> I'll mention some specifics about my setup at the end.
>
> Recently I debugged a problem where an application I used seemed to
> break the X Server so that it was no longer possible to cause windows to
> gain focus by clicking on them. The application is MCEdit. It's a
> pygame based python program that edits Minecraft maps.
>
> In MCEdit clicking the right mouse button engages a freelook mode so
> that moving the mouse changes the user's perspective instead of moving
> the mouse pointer. Engaging freelook mode in MCEdit causes the problem
> which persists after MCEdit has been shutdown.
>
> Debugging has revealed that the reason I'm no longer able cause windows
> to gain focus by clicking on them once freelook mode has been used in
> MCEdit is that device->button->buttonsDown ("buttonsDown" only occurs in
> one structure) is not decremented when the mouse button is released
> which causes CheckDeviceGrabs() to return early instead of sending the
> mouse button event to the window being clicked on.
>
> Further debugging revealed that the reason buttonsDown is not
> decremented when the mouse is released has to do with the fact that DGA
> mode is active during freelook mode. When DGA mode is active (when it
> has registered a hook that is called for events so that DGAHandleEvent()
> is called for keyboard and mouse events instead of
> ProcessKeyboardEvent() and ProcessPointerEvent()) the following seems to
> be true:
> 1) DGAProcessPointerEvent() calls UpdateDeviceState() with mouse
> button events where ev.detail.key is not set.
this is certainly a bug and your patch below fixes this issue,
button counting seems correct again now on the master device.
As for the slave device, that's a race condition. UDS doesn't get called
for slave devices at all while a DGA handler is active (note that the DGA
extension does not support per-device flags, it only works on masters
anyway).
So the net result is here that the slave device never has any button state
modified, and this should largely be fine. that is, unless you hit the
race condition that MCedit appears to trigger. If you call XDGASetMode()
after receiving a ButtonPress event (and that request gets processed before
the release event), the release event never updates the state of the slave
device, leading to the stuck button.
> 2) DGAHandleEvent() refuses to process events that are not master
> events:
> if (!IsMaster(device))
> return;
> which causes UpdateDeviceState() to ignore the master event as it
> thinks the child event has not been processed.
>
> My tentative fix can be seen in the attached patch. Even though my
> patch fixes the problem for me without any apparent side effects I
> suspect that additional research is needed. Although the part of my
> patch that addresses 1) above (the part in xf86DGA.c) is straightforward
> the part of my patch that addresses 2) above (the part in exevents.c)
> involves deleting code that was deliberately added. Unless the point of
> the code I removed was only perceived efficiency (avoiding processing
> the mouse release event because the developer thought it would have done
> nothing) then my patch is breaking something that I have not noticed
> yet.
>
> I have not yet been successful in writing a standalone test program to
> recreate the problem (to recreate it without MCEdit), but I can try
> things and report back.
you need to do the following:
issue a button grab with GrabModeSync for the pointer, then on receiving the
button press event call XDGASetMode/XDGASelectInput before calling
XAllowEvents.
I suspect that MCEdit uses GrabModeAsync which makes the window of the
trigger hard to hit, especially for short clicks.
I've attached a sample program that reproduces the issue here, please
confirm this is what you're seeing.
> I've reproduced the problem with Fedora 16 64 bit and Fedora 17 64 bit.
> In both cases (same hardware) I was only able to reproduce the problem
> with the proprietary driver for my Nvidia GeForce GTX 560 Ti, but that
> may just that it's an issue of timing rather than a Nvidia specific
> issue.
>
> The RPMs and their versions where I've been able to reproduce the
> problem:
> OS RPM Version
> -- --- -------
> Fedora 16 xorg-x11-server-Xorg 1.11.4-3
> Fedora 17 xorg-x11-server-Xorg 1.12.2-4
>
> Unfortunately it's not obvious to me how the RPM versions relate to
> X.Org versions even after reading the change log for the RPM, but Fedora
> 17 is fairly new.
the rpm versions match the server version almost 1:1,
xorg-x11-server-Xorg-1.12.2-4 is XServer 1.12.2 (with 4 rebuilds). You may
be confusing it with the X11R7.x katamari naming, some information about the
difference is available here:
http://who-t.blogspot.com.au/2009/10/x11r75-released-but-what-is-it.html
> This patch demonstrates an experimental fix for mouse buttons getting stuck
> down when DGA is engaged (has a handler). Send comments and questions to
> Steven Elliott <selliott4 at austin.rr.com>
>
> --- hw/xfree86/common/xf86DGA.c.orig 2011-03-01 22:09:32.000000000 -0600
> +++ hw/xfree86/common/xf86DGA.c 2012-08-15 01:06:47.006297764 -0500
> @@ -1077,6 +1077,7 @@
> ev.header = ET_Internal;
> ev.length = sizeof(ev);
> ev.type = event->subtype;
> + ev.detail.key = event->detail;
> ev.corestate = butc ? butc->state : 0;
> if (master && master->key)
> ev.corestate |= XkbStateFieldFromRec(&master->key->xkbInfo->state);
> --- Xi/exevents.c.orig 2012-01-07 01:53:52.000000000 -0600
> +++ Xi/exevents.c 2012-08-15 01:18:47.114628420 -0500
> @@ -834,25 +834,11 @@
>
> if (!button_is_down(device, key, BUTTON_PROCESSED))
> return DONT_PROCESS;
> - if (IsMaster(device)) {
> - DeviceIntPtr sd;
> -
> - /*
> - * Leave the button down if any slave has the
> - * button still down. Note that this depends on the
> - * event being delivered through the slave first
> - */
> - for (sd = inputInfo.devices; sd; sd = sd->next) {
> - if (IsMaster(sd) || GetMaster(sd, MASTER_POINTER) != device)
> - continue;
> - if (!sd->button)
> - continue;
> - for (i = 1; i <= sd->button->numButtons; i++)
> - if (sd->button->map[i] == key &&
> - button_is_down(sd, i, BUTTON_PROCESSED))
> - return DONT_PROCESS;
> - }
> - }
we can't drop this block, it is central to button handling with multiple
devices. right now, if you hold a button on the mouse and the touchpad, the
logical button will only be released once _both_ buttons are up. this is
behaviour we had since 1.7 (at least), I don't really want to change it.
A better fix is what Ville suggested, that sounds like the right thing to
do.
Cheers,
Peter
> + /*
> + * Continue processing regardless of whether slaves have the button
> + * down as some handler, such as DGAHandleEvent(), ignore non-master
> + * (slave) events.
> + */
> set_button_up(device, key, BUTTON_PROCESSED);
> if (device->valuator)
> device->valuator->motionHintWindow = NullWindow;
> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel
-------------- next part --------------
#include <unistd.h>
#include <stdio.h>
#include <stdlib.h>
#include <signal.h>
#include <X11/Xlib.h>
#include <X11/extensions/Xxf86dga.h>
static int stop;
void sighandler(int sig)
{
printf("Terminating\n");
stop = 1;
}
int main (void)
{
Display *dpy;
int err, event;
XDGAMode *modes;
int nmodes;
Window root, child;
int rx, ry, wx, wy;
unsigned int mask;
dpy = XOpenDisplay(NULL);
if (!XF86DGAQueryExtension(dpy, &err, &event))
goto error;
int major = 2, minor = 0;
XF86DGAQueryVersion(dpy, &major, &minor);
XGrabButton(dpy, AnyButton, AnyModifier, DefaultRootWindow(dpy), False,
ButtonPressMask | ButtonReleaseMask,
GrabModeSync, GrabModeAsync,
None, None);
modes = XDGAQueryModes(dpy, DefaultScreen(dpy), &nmodes);
if (nmodes == 0)
goto error;
signal(SIGINT, sighandler);
XQueryPointer(dpy, DefaultRootWindow(dpy), &root, &child,
&rx, &ry, &wx, &wy, &mask);
printf("Current button mask: %#x\n", mask & ~255);
printf("Waiting for button press + release\n");
while (!stop)
{
if (!XPending(dpy)) {
usleep(30000);
continue;
}
XEvent ev;
XNextEvent(dpy, &ev);
printf ("event %d\n", ev.xany.type);
if (ev.xany.type == ButtonPress) {
printf("Button press received\n");
XDGASetMode(dpy, DefaultScreen(dpy), modes[0].num);
XF86DGADirectVideo(dpy, DefaultScreen(dpy), XF86DGADirectMouse | XF86DGADirectKeyb);
XDGASelectInput(dpy, DefaultScreen(dpy),
ButtonPressMask | ButtonReleaseMask | PointerMotionMask);
XAllowEvents(dpy, AsyncBoth, CurrentTime);
XFlush(dpy);
}
XQueryPointer(dpy, DefaultRootWindow(dpy), &root, &child,
&rx, &ry, &wx, &wy, &mask);
printf("Button mask: %#x\n", mask & ~255);
}
XFree(modes);
XDGASetMode(dpy, 0, 0);
XCloseDisplay(dpy);
return 0;
error:
printf ("error occured.\n");
return 1;
}
More information about the xorg-devel
mailing list