Bug 26545

Adding xcb@lists.freedesktop.org into CC based on recent comments at xorg-devel. Nobody has commented yet so I assume this report was never noticed.

Sorry for missing this bug. Could you provide a test program? I'm getting too lost in the twisty maze of Xlib to work out the problem by inspection.

I can't find any extensions using XESetError or the corresponding entry in XExtensionHooks, which I guess explains why this bug hasn't been obvious before.

Created attachment 34959 [details]
Minimal test case to test DRI2 extension error handling

Test case attached. You are right that not many extensions define error handlers, however at least DRI2 does so.

The test case contains a minimal DRI2 implementation. I took dri2.h & dri2.c from Mesa and removed everything that isn't necessary for the test case. This was done to keep things simple so that it's possible to concentrate on DRI2 extension itself and not complicate things with GL stuff.

EXPECTED OUTCOME:
without-xcb# ./test-exterr 
dri2.c: filtered BadDrawable from DRI2DestroyDrawable

ACTUAL OUTCOME:
with-xcb# ./test-exterr 
X Error of failed request:  BadDrawable (invalid Pixmap or Window parameter)
  Major opcode of failed request:  134 (DRI2)
  Minor opcode of failed request:  4 (DRI2DestroyDrawable)
  Resource id in failed request:  0x2400001
  Serial number of failed request:  15
  Current serial number in output stream:  17

Perfect, thanks! I've pushed a fix for the bug you've observed, though there's a little more work to do.

Before I explain that: I found what looks like a long-standing bug in Xlib, or at least a misfeature. Try editing your test program to call XNextEvent before XCloseDisplay. You'll find that even Xlib --without-xcb calls _XError then.

As far as I can tell, Xlib only consulted handlers set with XESetError for errors seen by _XReply--not when looking for events, or waiting to be able to flush the output queue.

I've just gone to a tiny bit of trouble to reproduce that odd behavior in Xlib built --with-xcb. If that's not desirable, it would be nice to remove the extra code that implements it. :-)

Now, I can't close the bug yet, because there are two cases where Xlib built --with-xcb still won't consult with the extension error handlers: for requests issued while an async_handler is queued, and for connections where XSetEventQueueOwner has been used to let XCB do all event handling. (This is the "if(req && xcb_poll_for_reply..." case in process_responses.)

I can fix those cases trivially, but supporting the above odd behavior at the same time requires some refactoring.

(In reply to comment #4)
> Before I explain that: I found what looks like a long-standing bug in Xlib, or
> at least a misfeature. Try editing your test program to call XNextEvent before
> XCloseDisplay. You'll find that even Xlib --without-xcb calls _XError then.
> 
> As far as I can tell, Xlib only consulted handlers set with XESetError for
> errors seen by _XReply--not when looking for events, or waiting to be able to
> flush the output queue.
> 
> I've just gone to a tiny bit of trouble to reproduce that odd behavior in Xlib
> built --with-xcb. If that's not desirable, it would be nice to remove the extra
> code that implements it. :-)

This seems more like a misfeature than a bug because the documentation is a little bit vague.

From the documentation of XESetError: "Inside Xlib, there are times that you may want to suppress the calling of the external error handling when an error occurs. This allows status to be returned on a call at the cost of the call being synchronous (though most such routines are query operations, in any case, and are typically programmed to be synchronous). When Xlib detects a protocol error in _XReply(), it calls your procedure ..."

If I take that literally it means that I should force my call to be synchronous in order to be able to suppress errors.

I really don't know what the original intentions of Xlib designers have been. It seems a little bit silly to me that there isn't any good way to generally suppress errors in Xlib without doing some tricks. That odd behaviour is certainly not desirable in my opinion, but it's impossible to say if error detection was limited to _XReply and not to general handling of replies on purpose.

> Now, I can't close the bug yet, because there are two cases where Xlib built
> --with-xcb still won't consult with the extension error handlers: for requests
> issued while an async_handler is queued, and for connections where
> XSetEventQueueOwner has been used to let XCB do all event handling. (This is
> the "if(req && xcb_poll_for_reply..." case in process_responses.)
> 
> I can fix those cases trivially, but supporting the above odd behavior at the
> same time requires some refactoring.

Thanks for fixing this, I can live with the synchronous error handling but can't really say if the odd behaviour should be preserved.

(In reply to comment #5)
> From the documentation of XESetError: "Inside Xlib, there are times that you
> may want to suppress the calling of the external error handling when an error
> occurs. This allows status to be returned on a call at the cost of the call
> being synchronous (though most such routines are query operations, in any case,
> and are typically programmed to be synchronous). When Xlib detects a protocol
> error in _XReply(), it calls your procedure ..."

Thanks for digging up that note. It sounds pretty clear, even if it is insane. So I've pushed the rest of the fix for this bug, and made the behavior match what Xlib --without-xcb does, and what this documentation says it should do.

> If I take that literally it means that I should force my call to be synchronous
> in order to be able to suppress errors.

Have you considered using an async reply handler, instead? Those are consulted in _XError, regardless of how the error was discovered. You'd want to enqueue your handler only when issuing the request that might fail, and dequeue it when it's called on a later sequence number. A bit tricky, but feasible.

Alternatively, of course, you could use XCB directly for this request. ;-)

> I really don't know what the original intentions of Xlib designers have been.

Everyone I've talked to who worked on Xlib over the years says Xlib wasn't designed, it was just accreted. :-/

(In reply to comment #5)
> From the documentation of XESetError: "Inside Xlib, there are times that you
> may want to suppress the calling of the external error handling when an error
> occurs. This allows status to be returned on a call at the cost of the call
> being synchronous (though most such routines are query operations, in any case,
> and are typically programmed to be synchronous). When Xlib detects a protocol
> error in _XReply(), it calls your procedure ..."
> 
> If I take that literally it means that I should force my call to be synchronous
> in order to be able to suppress errors.

Well, if you take it both literally and exclusively, yeah.  It doesn't say anywhere that it won't call your extension error handler outside of _XReply; however, this is behaviour that people almost certainly rely on, so we're screwed I guess.

> I really don't know what the original intentions of Xlib designers have been.
> It seems a little bit silly to me that there isn't any good way to generally
> suppress errors in Xlib without doing some tricks. That odd behaviour is
> certainly not desirable in my opinion, but it's impossible to say if error
> detection was limited to _XReply and not to general handling of replies on
> purpose.

Xlib error handling, on the whole, is a bit of a joke.

(In reply to comment #6)
> Have you considered using an async reply handler, instead? Those are consulted
> in _XError, regardless of how the error was discovered. You'd want to enqueue
> your handler only when issuing the request that might fail, and dequeue it when
> it's called on a later sequence number. A bit tricky, but feasible.
> 
> Alternatively, of course, you could use XCB directly for this request. ;-)

Thanks for the suggestions. However, I'm happy to see that there is probably not going to be a need to work around this problem in the near future when https://bugs.freedesktop.org/show_bug.cgi?id=26394 gets fixed. That should eliminate the possibility of BadDrawables from DRI2DestroyDrawable altogether.

Patch has been already sent for review: http://lists.freedesktop.org/archives/xorg-devel/2010-April/007149.html.

(In reply to comment #8)
> Thanks for the suggestions. However, I'm happy to see that there is probably
> not going to be a need to work around this problem in the near future when
> https://bugs.freedesktop.org/show_bug.cgi?id=26394 gets fixed. That should
> eliminate the possibility of BadDrawables from DRI2DestroyDrawable altogether.

Ok, seems that I was too quick to jump to conclusions and some more work is needed to sort the DRI2 issue out. But that's another story and not related to this bug anymore.