Bug 19574

git bisect and a stack trace seem indicated here; I'm not seeing any trouble with media keys on this build.

Created attachment 21994 [details] [review]
0001-Xi-Force-a-CopyKeyClass-on-the-VCK-when-posting-a-k.patch

Something like this should fix the crash. I think my device at home is such a combo that posts some event through the mouse so I might be able to reproduce this tonight.

(In reply to comment #2)
> Created an attachment (id=21994) [details]
> 0001-Xi-Force-a-CopyKeyClass-on-the-VCK-when-posting-a-k.patch
> 
> Something like this should fix the crash. I think my device at home is such a
> combo that posts some event through the mouse so I might be able to reproduce
> this tonight.

  As I told you in irc it corrects the crash, but it will not post
the events. To trigger the problem, it appears to be required to
have a "main" keyboard without multimedia keys, and then attach
the new keyboard with multimedia keys.

  Before using this patch I started with a patch like (don't have
access to both computers now):
-%<-
diff --git a/Xi/exevents.c b/Xi/exevents.c
index 083bb2f..d245382 100644
--- a/Xi/exevents.c
+++ b/Xi/exevents.c
@@ -192,7 +192,7 @@ CopyKeyClass(DeviceIntPtr device, DeviceIntPtr master)
     static DeviceIntPtr lastMapNotifyDevice = NULL;
     KeyClassPtr mk, dk; /* master, device */
     BOOL sendNotify = FALSE;
-    int i;
+    int i, size;
 
     if (device == master)
         return;
@@ -202,17 +202,22 @@ CopyKeyClass(DeviceIntPtr device, DeviceIntPtr master)
 
     if (device != dixLookupPrivate(&master->devPrivates,
                                    CoreDevicePrivateKey)) {
-        memcpy(mk->modifierMap, dk->modifierMap, MAP_LENGTH);
-
-        if (dk->maxKeysPerModifier)
-        {
-            mk->modifierKeyMap = xrealloc(mk->modifierKeyMap,
-                                          8 * dk->maxKeysPerModifier);
+        if (dk->maxKeysPerModifier) {
+	    if (mk == NULL) {
+		mk = master->key = xmalloc(sizeof(KeyClassRec));
+                if (!mk)
+                    FatalError("[Xi] no memory for class shift.\n");
+		memcpy(mk, dk, sizeof(KeyClassRec));
+		mk->modifierMap = NULL;
+	    }
+            size = 8 * dk->maxKeysPerModifier;
+            if (size < MAP_LENGTH)
+                size = MAP_LENGTH;
+            mk->modifierKeyMap = xrealloc(mk->modifierKeyMap, size);
             if (!mk->modifierKeyMap)
                 FatalError("[Xi] no memory for class shift.\n");
-            memcpy(mk->modifierKeyMap, dk->modifierKeyMap,
-                    (8 * dk->maxKeysPerModifier));
-        } else
+            memcpy(mk->modifierMap, dk->modifierMap, size);
+        } else if (mk)
         {
             xfree(mk->modifierKeyMap);
             mk->modifierKeyMap = NULL;
-%<-
  But it would fail a few lines below in:
            if (!XkbCopyKeymap(dk->xkbInfo->desc, mk->xkbInfo->desc, True))
                FatalError("Couldn't pivot keymap from device to core!\n");

  But I did not try a merge of the above patch with yours, neither
the remaining of the code I wrote in an attempt to "port" git master
behavior to origin/server-1.6-branch. Example, I converted
static void
ChangeMasterDeviceClasses(DeviceIntPtr device,
                          deviceClassesChangedEvent *dcce)
{...}
to something like:
static void
ChangeMasterDeviceClasses(DeviceIntPtr device)
{
    DeviceIntPtr master = device->u.master;

    if (device->isMaster)
        return;

    if (!master) /* if device was set floating between SIGIO and now */
        return;

    master->public.devicePrivate = device->public.devicePrivate;

    DeepCopyDeviceClasses(device, master);
}

mi/mieq.c was easy :-)

but still no luck in getting it to actually send the events like it
does in git master, just prevented the crash.

(In reply to comment #1)
> git bisect and a stack trace seem indicated here; I'm not seeing any trouble
> with media keys on this build.

  The crash happens when plugging a usb keyboard on a computer with
a simple/standard keyboard, and using the multimedia keys in the
usb keyboard.

  I started with tag 1.5.99.3, after the revert of mpx, etc, as in this
computer, everything, but X Server was the latest released tarball.
  1.5.99.3 is "good" in the sense that it doesn't crash, but the event
is not sent to any window, but it should be "acceptable" to require
properly configuring xkb in this case :-)

%  git bisect log
# bad: [251d0d8090322b2c9dc0c8b7bef001f338d19433] Update version to 1.5.99.901 (1.6 RC1)
# good: [523aae1fa6d8002e55e85aee49f113b7eb9a6df3] Bump version to 1.5.99.3 (1.6 beta3)
git bisect start 'origin/server-1.6-branch' 'xorg-server-1.5.99.3'
# good: [6c635faa6ff0474199f4f7375022efe1e8ffa8f1] XQuartz: update quoting in case X11.app is moved to a directory with a space. (cherry picked from commit cc805dc799efa37c8dcefa3db04d87e9b835ffbd) (cherry picked from commit ecc3a7b6090552c309fe8e264d527ddd666a5761)
git bisect good 6c635faa6ff0474199f4f7375022efe1e8ffa8f1
# good: [2ce48363b862db134624797bc071f8c45323a075] xfree86: don't call CheckMotion if a device hasn't been enabled. #19176
git bisect good 2ce48363b862db134624797bc071f8c45323a075
# good: [8cfb353078d9b5d03a9633304038141a60adc970] dix: Fix handling of do_not_propagate_mask window attribute.
git bisect good 8cfb353078d9b5d03a9633304038141a60adc970
# good: [69ddac23281534a06c0acb3005a09e4448594925] Apple: Don't use DRI2 (cherry picked from commit a1d35cee5907a76977ee43c49cb55c8f411c9794)
git bisect good 69ddac23281534a06c0acb3005a09e4448594925
# bad: [6be355b8e8cabeb5832ce9970a83782ea46fd4d1] dix: drop x/y back into last.valuators before updating the history (#19285)
git bisect bad 6be355b8e8cabeb5832ce9970a83782ea46fd4d1



cut & paste of the first bad commit (for easier visualization):
% git show 6be355b8e8cabeb5832ce9970a83782ea46fd4d1
commit 6be355b8e8cabeb5832ce9970a83782ea46fd4d1
Author: Peter Hutterer <peter.hutterer@who-t.net>
Date:   Fri Jan 9 13:46:20 2009 +1000

    dix: drop x/y back into last.valuators before updating the history (#19285)
    
    positionSprite needs to scale to screen coordinates and in the process of
    doing so alters dev->last.valuators[0:1]. Drop the real coordinates back after
    finishing and before updating the motion history. This way, we don't push the
    screen coordinates into the motion history.
    
    X.Org Bug 19285 <http://bugs.freedesktop.org/show_bug.cgi?id=19285>
    (cherry picked from commit 56efbc0986e782da45addb05ece9f456d41d7a90)

diff --git a/dix/getevents.c b/dix/getevents.c
index 707d1da..16e23dc 100644
--- a/dix/getevents.c
+++ b/dix/getevents.c
@@ -695,6 +695,9 @@ positionSprite(DeviceIntPtr dev, int *x, int *y,
                                  dev->valuator->axes + 1, scr->height);
     }
 
+    /* dropy x/y (device coordinates) back into valuators for next event */
+    dev->last.valuators[0] = *x;
+    dev->last.valuators[1] = *y;
 }
 
 /**
@@ -980,9 +983,6 @@ GetPointerEvents(EventList *events, DeviceIntPtr pDev, int type, int buttons,
     positionSprite(pDev, &x, &y, scr, &cx, &cy);
     updateHistory(pDev, first_valuator, num_valuators, ms);
 
-    /* dropy x/y (device coordinates) back into valuators for next event */
-    pDev->last.valuators[0] = x;
-    pDev->last.valuators[1] = y;
 
     /* Update the valuators with the true value sent to the client*/
     if (num_valuators >= 1 && first_valuator == 0)




Xorg.0.log from "keyboard detection" to crash:
(II) config/hal: Adding input device USB-compliant keyboard
(**) USB-compliant keyboard: always reports core events
(**) USB-compliant keyboard: Device: "/dev/input/event9"
(II) USB-compliant keyboard: Found keys
(II) USB-compliant keyboard: Configuring as keyboard
(II) XINPUT: Adding extended input device "USB-compliant keyboard" (type: KEYBOARD)
(**) Option "xkb_rules" "evdev"
(**) USB-compliant keyboard: xkb_rules: "evdev"
(**) Option "xkb_model" "evdev"
(**) USB-compliant keyboard: xkb_model: "evdev"
(**) Option "xkb_layout" "us"
(**) USB-compliant keyboard: xkb_layout: "us"
Popen: `"/usr/bin/xkbcomp" -w 1 "-R/usr/share/X11/xkb" -xkm "-" -em1 "The XKEYBOARD keymap compiler (xkbcomp) reports:" -emp "> " -eml "Errors from xkbcomp are not fatal to the X server" "/tmp/server-0.xkm"', fp = 0x8bec210
Pclose: fp = 0x8bec210
Loaded XKB keymap /tmp/server-0.xkm, defined=0x7f
FlushingSerial
(II) config/hal: Adding input device USB-compliant keyboard
(**) USB-compliant keyboard: always reports core events
(**) USB-compliant keyboard: Device: "/dev/input/event10"
(II) USB-compliant keyboard: Found 10 mouse buttons
(II) USB-compliant keyboard: Found x and y relative axes
(II) USB-compliant keyboard: Found keys
(II) USB-compliant keyboard: Configuring as mouse
(II) USB-compliant keyboard: Configuring as keyboard
(**) USB-compliant keyboard: YAxisMapping: buttons 4 and 5
(**) USB-compliant keyboard: EmulateWheelButton: 4, EmulateWheelInertia: 10, EmulateWheelTimeout: 200
(II) XINPUT: Adding extended input device "USB-compliant keyboard" (type: KEYBOARD)
(**) Option "xkb_rules" "evdev"
(**) USB-compliant keyboard: xkb_rules: "evdev"
(**) Option "xkb_model" "evdev"
(**) USB-compliant keyboard: xkb_model: "evdev"
(**) Option "xkb_layout" "us"
(**) USB-compliant keyboard: xkb_layout: "us"
Popen: `"/usr/bin/xkbcomp" -w 1 "-R/usr/share/X11/xkb" -xkm "-" -em1 "The XKEYBOARD keymap compiler (xkbcomp) reports:" -emp "> " -eml "Errors from xkbcomp are not fatal to the X server" "/tmp/server-0.xkm"', fp = 0x8bf34a8
Pclose: fp = 0x8bf34a8
Loaded XKB keymap /tmp/server-0.xkm, defined=0x7f
(**) USB-compliant keyboard: (accel) keeping acceleration scheme 1
(**) USB-compliant keyboard: (accel) filter chain progression: 2.00
(**) USB-compliant keyboard: (accel) filter stage 0: 20.00 ms
(**) USB-compliant keyboard: (accel) set acceleration profile 0
FlushingSerial

Backtrace:
0: X(xorg_backtrace+0x3b) [0x812dbbb]
1: X(xf86SigHandler+0x51) [0x80c8bc1]
2: [0xffffe400]
3: X(mieqProcessInputEvents+0x327) [0x810d1e7]
4: X(ProcessInputEvents+0x17) [0x80c9787]
5: X(Dispatch+0x6e) [0x808716e]
6: X(main+0x3bd) [0x806c47d]
7: /lib/i686/libc.so.6(__libc_start_main+0xe5) [0xb7c0e5c5]
8: X [0x806b931]

Fatal server error:
Caught signal 11.  Server aborting



  Maybe this is also related to the problem I have with 1.6 and git
master, where, if I want to see where a firefox link points to, in the
status bar, I need to right click the link first...

> diff --git a/dix/getevents.c b/dix/getevents.c
> index 707d1da..16e23dc 100644
> --- a/dix/getevents.c
> +++ b/dix/getevents.c
> @@ -695,6 +695,9 @@ positionSprite(DeviceIntPtr dev, int *x, int *y,
>                                   dev->valuator->axes + 1, scr->height);
>      }
> 
> +    /* dropy x/y (device coordinates) back into valuators for next event */
> +    dev->last.valuators[0] = *x;
> +    dev->last.valuators[1] = *y;
>  }
> 
>  /**
> @@ -980,9 +983,6 @@ GetPointerEvents(EventList *events, DeviceIntPtr pDev, int
> type, int buttons,
>      positionSprite(pDev, &x, &y, scr, &cx, &cy);
>      updateHistory(pDev, first_valuator, num_valuators, ms);
> 
> -    /* dropy x/y (device coordinates) back into valuators for next event */
> -    pDev->last.valuators[0] = x;
> -    pDev->last.valuators[1] = y;
> 
>      /* Update the valuators with the true value sent to the client*/
>      if (num_valuators >= 1 && first_valuator == 0)


I really fail to see how that patch has to do with the crash you described,
especially since GPE isn't touched at all on key events.

> Backtrace:
> 0: X(xorg_backtrace+0x3b) [0x812dbbb]
> 1: X(xf86SigHandler+0x51) [0x80c8bc1]
> 2: [0xffffe400]
> 3: X(mieqProcessInputEvents+0x327) [0x810d1e7]
> 4: X(ProcessInputEvents+0x17) [0x80c9787]
> 5: X(Dispatch+0x6e) [0x808716e]
> 6: X(main+0x3bd) [0x806c47d]
> 7: /lib/i686/libc.so.6(__libc_start_main+0xe5) [0xb7c0e5c5]
> 8: X [0x806b931]

this backtrace is unrelated to the patch above as well.

(In reply to comment #5)
[...]
> I really fail to see how that patch has to do with the crash you described,
> especially since GPE isn't touched at all on key events.
> 
> > Backtrace:
> > 0: X(xorg_backtrace+0x3b) [0x812dbbb]
> > 1: X(xf86SigHandler+0x51) [0x80c8bc1]
> > 2: [0xffffe400]
> > 3: X(mieqProcessInputEvents+0x327) [0x810d1e7]
> > 4: X(ProcessInputEvents+0x17) [0x80c9787]
> > 5: X(Dispatch+0x6e) [0x808716e]
> > 6: X(main+0x3bd) [0x806c47d]
> > 7: /lib/i686/libc.so.6(__libc_start_main+0xe5) [0xb7c0e5c5]
> > 8: X [0x806b931]
> 
> this backtrace is unrelated to the patch above as well.

  Err, you are right, me being dumb :-), I stopped in the first "bad"
commit in the binary search, cannot test it right now, but the proper
one is in the range:
% git shortlog 10c0287232eab1b93d078774f52e65efa0c03607..681cc0f38b0b96c5e41c93d6944e4fa58c950eda
Jeremy Huddleston (2):
      XQuartz: Make debugging output for invalid depths a bit more detailed
      XQuartz: Use depth=24 instead of FatalError if we can't figure out our depth

Keith Packard (1):
      Merge commit 'whot/server-1.6-branch' into server-1.6-branch

Peter Hutterer (4):
      Let the DDX decide on the XkbRulesDefaults.
      xfree86: Only use the evdev ruleset on linux.
      dix: Fix handling of do_not_propagate_mask window attribute.
      mi: force CopyKeyClass for key events. (#19048)


Paulo

>       mi: force CopyKeyClass for key events. (#19048)

my money is on this one.

Created attachment 22028 [details] [review]
0001-mi-force-the-paired-kbd-device-before-CopyKeyClass.patch

This fixes the bug on my box, but more testing would be appreciated.

(In reply to comment #8)
> Created an attachment (id=22028) [details]
> 0001-mi-force-the-paired-kbd-device-before-CopyKeyClass.patch
> 
> This fixes the bug on my box, but more testing would be appreciated.

  Thanks. This corrects the problem, and sends the proper events,
like in git master.

  Keeping the bug open as it should be closed when this patch is
pushed to 1.6 branch.

I can confirm the patch works! X doesn't crash anymore using multimedia keyboard.
Maybe related, but still some keys are not working properly.
Key 'm' does generate KP_Insert
After pressing Numlock once:
Key 'm' does generate KP_0
After pressing Numlock again:
Key 'm' does generate m.
Same is valid for other keypad keys.

*** Bug 19600 has been marked as a duplicate of this bug. ***

Reassigning to keith for 1.6 picking.

I've applied this patch to the 1.6 branch, marking it as fixed.