Re: [RFC xwayland] Pointer warp emulation

[Peter Hutterer]

On Wed, Apr 13, 2016 at 06:07:51PM +0800, Jonas Ådahl wrote:
> With Wayland, the X server is no longer in control of the pointer
> position, and as such, clients warping the pointer has no effect. This
> causes many applications (mostly games) to fail to function properly, as
> they depend on the warp requests to actually take effect. The same can
> be said about pointer grabs with window confinement.
> 
> A way to make it possible to allow these applications to behave as
> expected is to introduce the concept of "pointer warp emulation".
> 
> I have implemented such a solution, which can be found here:
> https://github.com/jadahl/xserver/commits/wip/xwayland-pointer-warp

I had a look through these patches and nothing sticks out in a bad way. The
only issue (and we discussed this over IRC already) is that we shouldn't
drop the abs min/max ranges for the device, adding a separate relative-only
device is better here.

> It currently only works on mutter (explained further below), but should
> work with weston given by making some (minor) equivalent changes there.
> For reference, the mutter branch can be found here:
> https://github.com/jadahl/mutter/commits/wip/xwayland-pointer-warps
> 
> I have so far mostly tested with three applications (games): OpenTTD,
> Half Life 2, and Trine 1, which all seem to work fine with the current
> branch.
> 
> With pointer warp emulation, a client warping the pointer is translated
> into Xwayland using of the relative pointer[0] and pointer
> constraints[1] Wayland protocols, while faking the pointer position
> exposed to the rest of the X server.
> 
> When the emulator is enabled, the actual pointer position and the
> pointer position exposed to Xwayland may be different.
> 
> There are a few problems to solve here:
> 
> 1) when to enable a pointer warp emulator
> 2) when to disable a pointer warp emulator
> 3) when to enable a pointer lock
> 4) when to disable a pointer lock
> 
> Problem 3) needs attention in both Xwayland and in the compositor, but
> the rest can be partly solved completely in Xwayland.
> 
> As for problem 1) and 2), in my implementation I enable the emulator
> once the pointer is warped, but given that the pointer cursor and the exposed
> pointer position may be different, only as long as the pointer sprite is
> already hidden. The emulator is disabled once the sprite is made visible
> again.
> 
> As for problem 3), in the compositor I prototyped this with (mutter), I
> had to relax the requirements for enabling a pointer lock. For regular
> Wayland clients, a lock was only enabled if its surface was active/in
> focus. This was not possible for X11 clients, as often the surface that
> was the target of the warp happened to be an override-redirect window,
> which never will have such a state. So for X11 clients the requirements
> had to be relaxed, and the responsibility of behaving responsibly had to
> be partly moved to Xwayland.
> 
> In order to make Xwayland behave more responsibly regarding problem 3),
> some requirements were added in order to trigger a pointer lock. The
> requirements more or less tries to mimic the situations where the window
> would receive input events. For example the current implementation looks
> at the current grab, pointer focus and warp destination, etc to figure
> out whether locking is reasonable.
> 
> As for problem 4), in order to gracefully give back pointer control to
> the compositor, the lock is always disabled if the fake pointer position
> ends up outside of the given window rectangle.

I've thought through that and I haven't come up with anything better here.
I think having the relaxed rules as a special case for XWayland is
acceptable, XWayland is simply a special but important use-case. The
half-private protocol is IMO worse long-term, it's a lot harder to extend
than a private agreement.

Cheers,
   Peter

> 
> An alternative solution to using the Wayland pointer constraints
> protocols could be to introduce a half-private "wp_xwayland" protocol
> with a "warp_pointer" request, only exposed to Xwayland. This would,
> apart from making the compositors require more feature and becoming more
> complicated, largely have the same problems as mentioned above,
> requiring an "emulator" to keep track of fake positions vs real
> positions, knowing when to avoid warping, and I suspect it would work
> more or less equally well as this solution; but its worth to mention.
> 
> Apart from warping, the linked branch also adds support for confining
> pointer grabs. These requests are more directly translated to
> the equivalent Wayland protocol.
> 
> How does people feel about adding "emulation" code like this to
> XWayland? Does the approach described in this E-mail and implemented in
> the linked branch seem reasonable?
> 
> 
> 
> Jonas
> 
> 
> [0] 
> https://cgit.freedesktop.org/wayland/wayland-protocols/tree/unstable/relative-pointer/relative-pointer-unstable-v1.xml
> [1] 
> 

Re: [PATCH xserver] present: Be consistent in using window vs vblank->window in present_execute()

[Michel Dänzer]

On 19.04.2016 17:21, Chris Wilson wrote:
> Upon entering the function, we copy frequently accessed members of the
> vblank structure to locals (such as the Window). However, we then
> fluctuate between using the local window and the vblank->window. Under
> certain situations, the present_flip callback into the driver may be
> completed instantaneously and so accessing the vblank structure after a
> successful call into the driver may cause a use-after-free. This is
> trivially avoided by using the locals we took earlier.
> 
> Signed-off-by: Chris Wilson 

The shortlog is a bit long and maybe shouldn't mention window
specifically, e.g. something like "present: Use more local variables
instead of struct members in present_execute". Either way,

Reviewed-by: Michel Dänzer 


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH libICE] Enable visibility annotations

[Yury Gribov]

On 04/18/2016 06:21 PM, Adam Jackson wrote:

On Mon, 2016-04-18 at 09:23 +0300, Yury Gribov wrote:


Does below look like a sane approach?
1) get all deps via
$ apt-cache rdepends libice6 libice-dev
2) unpack each of the above .debs and scan for ELFs
3) for each ELF see if one of now hidden symbols is UND


That sounds good to me.


I've cooked a simple script (attached, reviews welcome) and applied it 
to Ubuntu 14. It showed that there are additional uses for 
_IceTransNoListen (mentioned by Alan in separate email) but nothing else.



Note that this does not handle transitive dependencies e.g. if some
weird library links static version of libICE and the re-exports it's
symbols as part of new lib's public interface.


It'd be possible to scan for this too I suspect.  Look for packages
that BuildRequire libice6-static, scan the resulting binaries for
exports. I suspect there are zero such packages.


I have postponed this activity for now (especially given that this 
behavior would be a serious packaging abuse).


-Y


find-used-symbols.sh
Description: application/shellscript
IceAcceptConnection
IceAddConnectionWatch
IceAllocScratch
IceAuthFileName
IceCloseConnection
IceComposeNetworkIdList
IceConnectionNumber
IceConnectionStatus
IceConnectionString
_IceErrorBadLength
_IceErrorBadMinor
_IceErrorBadState
_IceErrorBadValue
IceFlush
IceFreeAuthFileEntry
IceFreeListenObjs
IceGenerateMagicCookie
IceGetConnectionContext
IceGetListenConnectionNumber
IceGetListenConnectionString
IceGetPeerName
IceLastSentSequenceNumber
IceListenForConnections
IceLockAuthFile
IceOpenConnection
_IcePaMagicCookie1Proc
_IcePoMagicCookie1Proc
IceProcessMessages
IceProtocolSetup
IceProtocolShutdown
_IceRead
IceReadAuthFileEntry
_IceReadSkip
IceRegisterForProtocolReply
IceRegisterForProtocolSetup
IceRemoveConnectionWatch
IceSetErrorHandler
IceSetHostBasedAuthProc
IceSetIOErrorHandler
IceSetPaAuthData
IceSetShutdownNegotiation
_IceTransNoListen
IceUnlockAuthFile
_IceWrite
IceWriteAuthFileEntry
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH xserver] present: Be consistent in using window vs vblank->window in present_execute()

[Chris Wilson]

Upon entering the function, we copy frequently accessed members of the
vblank structure to locals (such as the Window). However, we then
fluctuate between using the local window and the vblank->window. Under
certain situations, the present_flip callback into the driver may be
completed instantaneously and so accessing the vblank structure after a
successful call into the driver may cause a use-after-free. This is
trivially avoided by using the locals we took earlier.

Signed-off-by: Chris Wilson 
---
 present/present.c | 30 +++---
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/present/present.c b/present/present.c
index 105e2bf..a90f08d 100644
--- a/present/present.c
+++ b/present/present.c
@@ -628,6 +628,7 @@ present_execute(present_vblank_ptr vblank, uint64_t ust, 
uint64_t crtc_msc)
 {
 WindowPtr   window = vblank->window;
 ScreenPtr   screen = window->drawable.pScreen;
+PixmapPtr   pixmap = vblank->pixmap;
 present_screen_priv_ptr screen_priv = present_screen_priv(screen);
 uint8_t mode;
 
@@ -648,7 +649,7 @@ present_execute(present_vblank_ptr vblank, uint64_t ust, 
uint64_t crtc_msc)
 }
 }
 
-if (vblank->flip && vblank->pixmap && vblank->window) {
+if (vblank->flip && pixmap && window) {
 if (screen_priv->flip_pending || screen_priv->unflip_event_id) {
 DebugPresent(("\tr %lld %p (pending %p unflip %lld)\n",
   vblank->event_id, vblank,
@@ -662,13 +663,14 @@ present_execute(present_vblank_ptr vblank, uint64_t ust, 
uint64_t crtc_msc)
 xorg_list_del(>window_list);
 vblank->queued = FALSE;
 
-if (vblank->pixmap && vblank->window) {
+if (pixmap && window) {
 
 if (vblank->flip) {
+   RegionPtr damage = vblank->update;
 
 DebugPresent(("\tf %lld %p %8lld: %08lx -> %08lx\n",
   vblank->event_id, vblank, crtc_msc,
-  vblank->pixmap->drawable.id, 
vblank->window->drawable.id));
+  id, window->drawable.id));
 
 /* Prepare to flip by placing it in the flip queue and
  * and sticking it into the flip_pending field
@@ -678,8 +680,7 @@ present_execute(present_vblank_ptr vblank, uint64_t ust, 
uint64_t crtc_msc)
 xorg_list_add(>event_queue, _flip_queue);
 /* Try to flip
  */
-if (present_flip(vblank->crtc, vblank->event_id, 
vblank->target_msc, vblank->pixmap, vblank->sync_flip)) {
-RegionPtr damage;
+if (present_flip(vblank->crtc, vblank->event_id, 
vblank->target_msc, pixmap, vblank->sync_flip)) {
 
 /* Fix window pixmaps:
  *  1) Restore previous flip window pixmap
@@ -689,18 +690,17 @@ present_execute(present_vblank_ptr vblank, uint64_t ust, 
uint64_t crtc_msc)
 present_set_tree_pixmap(screen_priv->flip_window,
 screen_priv->flip_pixmap,
 
(*screen->GetScreenPixmap)(screen));
-present_set_tree_pixmap(vblank->window, NULL, vblank->pixmap);
-present_set_tree_pixmap(screen->root, NULL, vblank->pixmap);
+present_set_tree_pixmap(window, NULL, pixmap);
+present_set_tree_pixmap(screen->root, NULL, pixmap);
 
 /* Report update region as damaged
  */
-if (vblank->update) {
-damage = vblank->update;
+if (damage)
 RegionIntersect(damage, damage, >clipList);
-} else
+else
 damage = >clipList;
 
-DamageDamageRegion(>window->drawable, damage);
+DamageDamageRegion(>drawable, damage);
 return;
 }
 
@@ -710,7 +710,7 @@ present_execute(present_vblank_ptr vblank, uint64_t ust, 
uint64_t crtc_msc)
 screen_priv->flip_pending = NULL;
 vblank->flip = FALSE;
 }
-DebugPresent(("\tc %p %8lld: %08lx -> %08lx\n", vblank, crtc_msc, 
vblank->pixmap->drawable.id, vblank->window->drawable.id));
+DebugPresent(("\tc %p %8lld: %08lx -> %08lx\n", vblank, crtc_msc, 
pixmap->drawable.id, window->drawable.id));
 if (screen_priv->flip_pending) {
 
 /* Check pending flip
@@ -738,7 +738,7 @@ present_execute(present_vblank_ptr vblank, uint64_t ust, 
uint64_t crtc_msc)
 return;
 }
 
-present_copy_region(>drawable, vblank->pixmap, vblank->update, 
vblank->x_off, vblank->y_off);
+present_copy_region(>drawable, pixmap, vblank->update, 
vblank->x_off, vblank->y_off);
 
 /* present_copy_region sticks the region into a scratch GC,
  * which is then freed, freeing the region
@@ -746,13 +746,13 @@