Bug 81819 - Xwayland crashes when seat adds a capability and sends some input event
Summary: Xwayland crashes when seat adds a capability and sends some input event
Status: RESOLVED FIXED
Alias: None
Product: Wayland
Classification: Unclassified
Component: XWayland (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Wayland bug list
QA Contact: Xorg Project Team
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-07-27 21:59 UTC by Michael Forney
Modified: 2015-09-22 09:54 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Patch fixing the issue (861 bytes, patch)
2014-07-27 22:23 UTC, Michael Forney
Details | Splinter Review
Another patch (1.67 KB, patch)
2014-08-02 01:57 UTC, Boyan Ding
Details | Splinter Review

Description Michael Forney 2014-07-27 21:59:38 UTC
When Xwayland is started while the seat has limited capabilities (i.e. only pointer) and then later on more capabilities are added, any events sent from those new devices cause a crash.

This seems to be because in xwayland-input.c:add_device where the devices are created, xwayland adds them with AddInputDevice, but does not activate or enable them (with ActivateDevice and EnableDevice), so miPointerDeviceInitialize is never called on these devices.

The devices with the capabilities present when Xwayland starts do not have this problem because they are activated and enabled in InitAndStartDevices.

I think adding a call to ActivateDevice, followed by a call to EnableDevice at the end of add_device should be enough (and it does solve the problem), but I am not familiar enough with the X server architecture to know if this is the right solution.
Comment 1 Michael Forney 2014-07-27 22:22:07 UTC
This can be reproduced by simply starting an X application, unplugging all pointing devices, then plugging one in and moving the cursor into the X window.
Comment 2 Michael Forney 2014-07-27 22:23:49 UTC
Created attachment 103567 [details] [review]
Patch fixing the issue
Comment 4 Michael Forney 2014-07-29 01:09:46 UTC
Partially.

The commit you referenced fixes the case where a seat capability disappears and then reappears (such as when disconnecting then reconnecting a mouse, or switching VTs).

However, if Xwayland is started without some capability (for example, no pointing devices connected), then a mouse is plugged in, the problem remains. This is can be reproduced by starting weston with no pointing devices, switching to another VT, launching weston-terminal (by setting WAYLAND_DISPLAY), switching back to the weston VT, launching an X client in the opened terminal (by setting DISPLAY), then plugging in a mouse and moving the cursor over the X client.

The issue is that the devices created after Xwayland startup are never activated.
Comment 5 Boyan Ding 2014-08-02 01:57:43 UTC
Created attachment 103842 [details] [review]
Another patch

Actually I sent a patch similar to yours to xorg-devel a month ago, but another patch (the commit I mentioned above) was accepted, clearly people only thought about switching VTs and didn't notice the problem you described.

Now with that commit in mind, I propose this patch. Is it okay?
Comment 6 Michael Forney 2014-08-03 04:56:40 UTC
(In reply to comment #5)
> Created attachment 103842 [details] [review] [review]
> Another patch
> 
> Actually I sent a patch similar to yours to xorg-devel a month ago, but
> another patch (the commit I mentioned above) was accepted, clearly people
> only thought about switching VTs and didn't notice the problem you described.
> 
> Now with that commit in mind, I propose this patch. Is it okay?

Yep. I can confirm that your patch (with a small typo fix) fixes the issue as well.

Thanks!
Comment 7 Chang Liu 2014-12-10 22:50:02 UTC
Can confirm that this patch (with a typo fix) fixes Xwayland crashing when switching tty.

Now if anyone could get this patch upstream...
Comment 8 Pekka Paalanen 2014-12-11 12:49:36 UTC
If the patch is not already upstream, you would better not mark the bug as fixed then. Since it's not fixed upstream. :-)

Marking a bug fixed just makes developers not see it anymore.

Reopening, assuming the patch isn't upstream. Could you poke xorg-devel@, please?
Comment 9 Daniel Stone 2014-12-11 13:33:47 UTC
Please post it with the following line:
Reviewed-by: Daniel Stone <daniels@collabora.com>

as well as Cc'ing keithp@keithp.com, asking him to pull it for 1.17.
Comment 10 Boyan Ding 2014-12-11 14:26:38 UTC
The patch is at http://lists.x.org/archives/xorg-devel/2014-August/043432.html. Should I send a ping?
Comment 11 Chang Liu 2014-12-11 18:40:53 UTC
(In reply to Boyan Ding from comment #10)
> The patch is at
> http://lists.x.org/archives/xorg-devel/2014-August/043432.html. Should I
> send a ping?

Yes please. Also is there any chance that this patch can be backported to 1.16 (and be included in say, the 1.16.3 release)? It's going to be a while before 1.17 can be released. It will be even longer before it can make into most distros. Considering that this is purely a bug fix that has minimal impact on the rest of the codebase I'd say it is fairly reasonable to do so.
Comment 12 Olivier Fourdan 2015-09-22 09:54:22 UTC
Patch has been merged in master.

http://cgit.freedesktop.org/xorg/xserver/commit/?id=1ba4fde


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.