[SRU] Xorg crashes when performing gesture

For future reference, the core dump is generated from xserver-xorg-core=2:1.9.0-0ubuntu7.

This is the correct core dump.
I had some problems trying to make Xorg generate core dump. I had to start Xorg manually, as describe in lp #666601.

According to the core dump, the gesture code was doing window hit-testing, but the gesture code runs in a signal handler. If a gesture is started while the window tree is being modified the server may crash.

My related branch above includes a new patch to block the SIGIO signal while the window tree is being modified.

Please disregard the above package in comment #6. It has an incorrect version.

I'm still having X crashes.
Attached is a core dump against xserver-xorg-core 2:1.9.0-0ubuntu8~test1

The backtrace from the core dump shows:

#0 0xb75bcb69 in ?? () from /lib/libc.so.6
#1 0xb75be682 in calloc () from /lib/libc.so.6
#2 0x080abb2c in AllocateOutputBuffer (who=0x978ae48, count=32, __buf=0xbfda255c) at ../../os/io.c:1044
#3 WriteToClient (who=0x978ae48, count=32, __buf=0xbfda255c) at ../../os/io.c:758
#4 0x0806515f in WriteEventsToClient (pClient=0x978ae48, count=1, events=0xbfda255c) at ../../dix/events.c:5705
#5 0x08067b72 in TryClientEvents (client=0x978ae48, dev=0xbfda21e8, pEvents=0xbfda255c, count=1, mask=4325376, filter=131072, grab=0x0)
    at ../../dix/events.c:1953
#6 0x0806ab60 in DeliverEventsToWindow (pDev=0xbfda21e8, pWin=0x98cae40, pEvents=0xbfda255c, count=1, filter=131072, grab=0x0) at ../../dix/events.c:2053
#7 0x0806b728 in DeliverEvents (pWin=0x98cae40, xE=0xbfda255c, count=1, otherParent=0x0) at ../../dix/events.c:2530
#8 0x080987ab in DeleteWindow (value=0x98cae40, wid=88080385) at ../../dix/window.c:943
#9 0x080711ad in FreeClientResources (client=0x9c07a60) at ../../dix/resource.c:859
#10 0x08072bc9 in CloseDownClient (client=0x9c07a60) at ../../dix/dispatch.c:3490
#11 0x08077b65 in Dispatch () at ../../dix/dispatch.c:442
#12 0x080625ba in main (argc=4, argv=0xbfda2794, envp=0xbfda27a8) at ../../dix/main.c:291

The odd thing here is that it segfaulted from AllocateOutputBuffer. The function takes no arguments (it's inlined, so it has args in the backtrace, but it doesn't really) and returns a pointer to freshly allocated data. I also added debugging symbols for calloc from libc6-dbg=2.12.1-0ubuntu10 and everything looks sane in terms of the stack. I think it's unlikely this is a stack corruption based on this data. However, it's even more unlikely, imo, that this is a bug in eglibc. Unfortunately, it's hard to tell what is going on through remote debugging.

Gustavo, can you try to reproduce again and see if the backtrace is the same? If so, it would point the finger towards eglibc. If not, then there really may be some weird stack corruption going on. Maybe something odd is occurring due to input handling in a signal handler?

I've linked this bug with two branches with a workaround that has been working for me.

The code in these branches are definitely not suitable for production. I did it in a quick and dirty way as a proof of concept. There are many clear improvements still to be done.

I didn't find exactly where is the bug. My suspects are we are doing something that shouldn't be done from a signal handler, although I don't know exactly what. So I moved the code that sends gestures to the client from the signal handler to the xserver main loop, passing the gestures through the event queue. As it has been working, probably the WriteToClient function is not (or uses a function that is not) async-signal-safe.

Gustavo, thank you very much for this! Looks very sane. We will continue to process this to see what can be done for Maverick. As you are probably aware, this problem will go away automatically in Natty. Thanks again, very nice.

Gustavo,

Awesome work! How long have you been using this code, presumably without any crashes? If things are working better, I think we'll probably try to use your work. I think the server patch looks good, and the evdev patch just needs to remove the unused code.

Thanks a bunch!

I've been using this code for a week without any crashes.
The crash frequency before this code was around 2 crashes a day.

I will remove the unused code as suggested by Chase. Any other suggestion is welcome.
I know this code is not relevant for Natty, but if you guys intend to deliver it as an update for Maverick I'd gladly help with whatever is needed.

Thank you guys for your help on doing this. I'm very happy now using gestures without X crashes :-D

Gustavo,

I began reviewing your work more closely to package it up for testing. I realized that there are areas that are a little rough, as you noted. I am in the process of cleaning it up.

As such, don't worry about updating your branches. I'll be proposing for Maverick slightly different patches based on your work.

Thanks again!

I've updated my branches and pushed new packages to ppa:utouch-team/utouch. Gustavo, could you test the packages out to ensure they work?

Thanks

Bug 687662 is preventing me from getting the xorg-server source package uploaded to the ppa :(. I've been told it should be fixed in a day or two, when I will retry the upload. Until then, industrious users could try building from the source branch I've linked to this bug :).

Chase,

I've just installed your packages (building from branch) and they are working fine. I'll report immediately if I have any crash.

The code is also great. I would just add that there is no need for including gestureproto.h from xf86Xinput.c and that evdev should be updated to depend of xserver-xorg-core (>> 2:1.9.0-0ubuntu8).

Thank you!

After discussion with Bryce and others on #ubuntu-devel, I've rebased the xorg-server branch on top of the package in maverick-proposed. I also fixed the xserver-xorg-input-evdev depends for the correct xorg-server version.

Please fix this in natty first. Also, it seems that something went wrong with building the maverick-proposed version:
http://launchpadlibrarian.net/60607373/xorg-server_2%3A1.9.0-0ubuntu7.1_2%3A1.9.0-0ubuntu7.2.diff.gz

Holding this back for now, as the current maverick-proposed version of xorg-server is v-failed (bug 650539) due to a regression (bug 680811).

Also, I need to reject this again, as the new upload again introduces a huge and unrelated diff to upstream files, as I already pointed out in the previous comment. Please check your package with debdiff before uploading.

I'll need to follow up with the ubuntu-x team for most of these issues, but this issue will not be fixed in Natty because the code affected will be removed in a few weeks.

I've re-uploaded the package after doublechecking the debdiff. I think the stray files were my fault when I checked out cnd's branch into my working tree rather than a clean one.

Diff looks good now, thanks. Still holding back, see comment 23. Should we reject the current maverick-proposed version if this is urgent?

> Diff looks good now, thanks. Still holding back, see comment 23. Should
> we reject the current maverick-proposed version if this is urgent?

Given that this has some regression potential I think it needs to live in -proposed for a bit to get adequately tested.

Regarding the current maverick-proposed version, the only known regression seems to be fairly modest (behavioral change in how maximization works), and the issue that it fixes (x server crashes) seems to be pretty severe, and is getting confirmed by a number of people. The behavior regression has only been reported by one person, who isn't running stock packages (although he says it happens there too, but solid evidence is missing).

I'd be tempted to allow the current proposal through. The benefit seems to far outweigh the risk in this case. However, if you're not comfortable with that, then yeah reject it out of proposed and put this one in instead. We can then sharpen our pencils on seeing if the alleged regression can be independently confirmed.

There are now two xorg-server uploads in the proposed queue, for bug 575465 and bug 670016. Please either reupload 7.3 with an appropriate -v to include the two proposed versions, or better yet, just upload a 2:1.9.0-0ubuntu7.2 which has both changes? Also, there's still 7.1 in proposed which causes the regression in bug 680811. If we stack two more uploads on top of that, it'll quickly get unmanageable.

On Fri, Jan 07, 2011 at 08:57:23AM -0000, Martin Pitt wrote:
> There are now two xorg-server uploads in the proposed queue, for bug
> 575465 and bug 670016. Please either reupload 7.3 with an appropriate -v
> to include the two proposed versions, or better yet, just upload a
> 2:1.9.0-0ubuntu7.2 which has both changes?

Done

> Also, there's still 7.1 in
> proposed which causes the regression in bug 680811. If we stack two more
> uploads on top of that, it'll quickly get unmanageable.

Read my previous comment...
https://bugs.launchpad.net/ubuntu/+source/xorg-server/+bug/670016/comments/27

The 7.1 change looks fine to me despite the alleged regression.
Either push it through or kick it out, but lets not bottleneck xserver
SRUs on it.

Accepted xorg-server into maverick-proposed, the package will build now and be available in a few hours. Please test and give feedback here. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Thank you in advance!

As per the bug description the patch being proposed is for maverick only, there isn't really an associated task for natty. (For natty, once xserver 1.10 goes in, there'll be entirely new code to provide the feature so this patch isn't relevant there.) Closing the natty task, leaving a maverick task.

Uploaded

Accepted xserver-xorg-input-evdev into maverick-proposed, the package will build now and be available in a few hours. Please test and give feedback here. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Thank you in advance!

Not sure whether this is because of this patch, but since the last version of xserver-xorg-input-evdev in -proposed, left click will sometimes (about 2% of clicks maybe) act as a middle click (meaning a text will be pasted when selecting a textarea, or a tab will be closed in a browser when selecting it)...

Still there after reverting to the previous version, so that's not related to this patch. Still looking for what's causing this bug anyway.

I've been using the packages uploaded to maverick-proposed and they work great. I will report here if I have any issues.

This bug was fixed in the package xorg-server - 2:1.9.0-0ubuntu7.3

---------------
xorg-server (2:1.9.0-0ubuntu7.3) maverick-proposed; urgency=low

  * debian/patches/209_xorg-xi-usb-wireless-mouse-kb.patch:
    - Backport of upstream patch which swaps order of detection of a
      device as pointer or keyboard. Fixes certain mouse models that
      get detected as keyboards.
      (LP: #575465)

xorg-server (2:1.9.0-0ubuntu7.2) maverick-proposed; urgency=low

  * Move gesture event handling to server in non-signal context
    (LP: #670016)
    - Add debian/patches/208_gestures_through_event_queue.patch
 -- Bryce Harrington <email address hidden> Thu, 06 Jan 2011 15:07:12 -0800

This bug was fixed in the package xserver-xorg-input-evdev - 1:2.3.2-6ubuntu3.1

---------------
xserver-xorg-input-evdev (1:2.3.2-6ubuntu3.1) maverick-proposed; urgency=low

  * Add patch from Gustavo Luiz Duarte to enqueue gesture events for server-side
    processing.
    - Fixes X crash due to sending events to clients in signal context
      (LP: #670016)
 -- Chase Douglas <email address hidden> Mon, 06 Dec 2010 13:34:49 -0800