[2/2] xlib: Call XSync() before ignoring errors

Submitted by Uli Schlachter on March 7, 2017, 10:21 a.m.

Details

Message ID 20170307102131.11666-2-psychon@znc.in
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Cairo

Not browsing as part of any series.

Commit Message

Uli Schlachter March 7, 2017, 10:21 a.m.
The code here wants to ignore errors for a specific request. To do so,
it sets a no-op error handler. However, it could happen that some
previous request caused an error and this error will also be ignored by
the no-op error handler.

To avoid this, call XSync() before setting the error handler. This makes
sure that all pending errors are handled.

Signed-off-by: Uli Schlachter <psychon@znc.in>
---
 src/cairo-xlib-surface.c | 2 ++
 1 file changed, 2 insertions(+)

Patch hide | download patch | download mbox

diff --git a/src/cairo-xlib-surface.c b/src/cairo-xlib-surface.c
index 84e7c8e49..908d76daf 100644
--- a/src/cairo-xlib-surface.c
+++ b/src/cairo-xlib-surface.c
@@ -787,6 +787,7 @@  _get_image_surface (cairo_xlib_surface_t    *surface,
 
 	    _cairo_xlib_shm_surface_get_ximage (&image->base, &shm_image);
 
+	    XSync (display->display, False);
 	    old_handler = XSetErrorHandler (_noop_error_handler);
 	    success = XShmGetImage (display->display,
 				    surface->drawable,
@@ -808,6 +809,7 @@  _get_image_surface (cairo_xlib_surface_t    *surface,
     if (surface->use_pixmap == 0) {
 	cairo_xlib_error_func_t old_handler;
 
+	XSync (display->display, False);
 	old_handler = XSetErrorHandler (_noop_error_handler);
 
 	ximage = XGetImage (display->display,

Comments

On Tue, Mar 07, 2017 at 11:21:31AM +0100, Uli Schlachter wrote:
> The code here wants to ignore errors for a specific request. To do so,
> it sets a no-op error handler. However, it could happen that some
> previous request caused an error and this error will also be ignored by
> the no-op error handler.
> 
> To avoid this, call XSync() before setting the error handler. This makes
> sure that all pending errors are handled.
> 
> Signed-off-by: Uli Schlachter <psychon@znc.in>
> ---
>  src/cairo-xlib-surface.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/src/cairo-xlib-surface.c b/src/cairo-xlib-surface.c
> index 84e7c8e49..908d76daf 100644
> --- a/src/cairo-xlib-surface.c
> +++ b/src/cairo-xlib-surface.c
> @@ -787,6 +787,7 @@ _get_image_surface (cairo_xlib_surface_t    *surface,
>  
>  	    _cairo_xlib_shm_surface_get_ximage (&image->base, &shm_image);
>  
> +	    XSync (display->display, False);
>  	    old_handler = XSetErrorHandler (_noop_error_handler);

I was thinking if only we could check for an error from this request,
and remembered this is not xcb.
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
On 07.03.2017 11:31, Chris Wilson wrote:
> On Tue, Mar 07, 2017 at 11:21:31AM +0100, Uli Schlachter wrote:
>> The code here wants to ignore errors for a specific request. To do so,
>> it sets a no-op error handler. However, it could happen that some
>> previous request caused an error and this error will also be ignored by
>> the no-op error handler.
>>
>> To avoid this, call XSync() before setting the error handler. This makes
>> sure that all pending errors are handled.
>>
>> Signed-off-by: Uli Schlachter <psychon@znc.in>
>> ---
>>  src/cairo-xlib-surface.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/src/cairo-xlib-surface.c b/src/cairo-xlib-surface.c
>> index 84e7c8e49..908d76daf 100644
>> --- a/src/cairo-xlib-surface.c
>> +++ b/src/cairo-xlib-surface.c
>> @@ -787,6 +787,7 @@ _get_image_surface (cairo_xlib_surface_t    *surface,
>>  
>>  	    _cairo_xlib_shm_surface_get_ximage (&image->base, &shm_image);
>>  
>> +	    XSync (display->display, False);
>>  	    old_handler = XSetErrorHandler (_noop_error_handler);
> 
> I was thinking if only we could check for an error from this request,
> and remembered this is not xcb.
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Even better: XSetErrorHandler() has no Display* argument. This sets a
global variable! This code is not even thread-safe and it cannot be.
Hopefully, no one uses multiple Displays in a single program...

Uli
On Tue, 2017-03-07 at 11:39 +0100, Uli Schlachter wrote:

> Even better: XSetErrorHandler() has no Display* argument. This sets a
> global variable! This code is not even thread-safe and it cannot be.
> Hopefully, no one uses multiple Displays in a single program...

Yes, it's a terrible API.

A more idiomatic approach to this kind of problem is something like:

XLockDisplay(dpy);
next = NextRequest(dpy);
// still not thread safe but can at least check the exact request
bar = XSetErrorHandler(foo);
XShmGetImage(...);
XSetErrorHandler(bar);
XUnlockDisplay(dpy);

Failing that, if you _really_ want to be correct here, there is also a
real async API in Xlib. It's not what you might call documented, and
it's moderately painful to use, but it's ABI (some extension libs rely
on it) and per-display so you don't have the thread safety problem.

- ajax