XMonad.Util.XSelection.getSelection leaks a handle to the X server

[GoogleCodeExporter]

getSelection opens its own connection to the X server to retrieve and convert 
the current selection without impacting the main connection or run loop. It 
doesn't close the connection afterward, so repeated use will eventually cause 
the server to stop accepting connections or xmonad to run out of file 
descriptors, depending on system load etc.

Original issue reported on code.google.com by allber...@gmail.com on 31 May 2014 at 9:21

[GoogleCodeExporter]

Original comment by allber...@gmail.com on 31 May 2014 at 9:22

• Changed title: XMonad.Util.XSelection.getSelection leaks a handle to the X server

[GoogleCodeExporter]

I see. That would explain why every few weeks my X.org refuses to open up any 
more applications and I have to restart it. Would it be sufficient to simply 
add a 'closeDisplay' at the end of 'getSelection' to match the 'openDisplay' at 
the start?

Original comment by gwe...@gmail.com on 31 May 2014 at 9:28

[GoogleCodeExporter]

`closeDisplay dpy`, yes, although you need to handle both legs of the `if` at 
the end. I don't think it needs anything else, although I can think of timing 
issues which could potentially lead to it never receiving that SelectionNotify 
event it busywaits on.

Original comment by allber...@gmail.com on 31 May 2014 at 9:32

[GoogleCodeExporter]

So could we do that as:

    hunk ./XMonad/Util/XSelection.hs 66
    -    if ev_event_type ev == selectionNotify
    -       then do res <- getWindowProperty8 dpy clp win
    -               return $ decode . map fromIntegral . fromMaybe [] $ res
    -       else destroyWindow dpy win >> return ""
    +    result <- if ev_event_type ev == selectionNotify
    +                 then do res <- getWindowProperty8 dpy clp win
    +                         return $ decode . map fromIntegral . fromMaybe [] $ res
    +                 else destroyWindow dpy win >> return ""
    +    closeDisplay dpy
    +    return result

? Compiles, anyway.

Original comment by gwe...@gmail.com on 31 May 2014 at 11:53

[GoogleCodeExporter]

You may never receive an event if the selection owner dies/exits before 
responding to SelectionRequest. You may also receive things like keyboard 
mapping notifications which you'll want to ignore.

I don't think there's a simple "naïve" solution here, much as with 
putSelection. You do it right or you have the potential for deadlocks. If we're 
going to insist on not using callbacks then I guess for now we accept and 
document the deadlock potential.

A future replacement for this thing (someday! I started on it to fix 
putSelection, but there is no hope for a compatible interface because you 
simply cannot do it this way) will need to use callbacks. My proposed design 
uses a handleEventHook and some ExtensibleState.

Original comment by allber...@gmail.com on 1 Jun 2014 at 12:00

[GoogleCodeExporter]

This transcript from IRC explains what's going on here and why this (and the 
infamous putSelection previously eliminated from the module) cannot work as 
written.

[31 21:34] * geekosaur also finds another potential putSelection-style deadlock 
in the code... furrfu
[31 21:34] <geekosaur> XSelection needs to be taken out and shot. but one done 
correctly will not in any way be compatible and will annoy users
[31 21:35] <geekosaur> because it needs to use callbacks
[31 21:35] <geekosaur> people think selection manipulation is something that 
can trivially be done synchronously
[31 21:35] <geekosaur> X11 disagrees and will happily deadlock you
[31 21:36] <pharaun> ouch
[31 21:37] <kaol> XSelections are held by an X client until someone comes and 
asks them what it is.
[31 21:37] <geekosaur> sure, then there's a negotiation
[31 21:38] <geekosaur> so, one (uncommon but possible) way to deadlock xmonad 
in the current getSelection is to kill the client holding the selection before 
it can receive or respond to the SelectionRequest event
[31 21:38] <geekosaur> getSelection will loop forever looking for an event that 
will never arrive
[31 21:39] <geekosaur> a way you could trigger this practically is by trying to 
use it when the selection happens to be held by something which is advertising 
a non-text selection
[31 21:41] <geekosaur> if it has a bug where receiving a request for 
UTF8_STRING causes it to crash (either because it's not expecting it / naïvely 
assumes it will always receive an appropriate request, or because it crashes 
trying to come up with a suitable text response)
[31 21:50] <pharaun> i *noticed* the xselection being held by a xclient thing
[31 21:50] <pharaun> i used to select then close and try to paste and was like 
... its not working??
[31 21:51] <kaol> Yes. That's one effect of it.
[31 21:51] <pharaun> well that explains why
[31 21:51] <geekosaur> yes, the selection isn't a pasteboard/container, it's 
just an advertisement
[31 21:51] <pharaun> yeah
[31 21:52] <pharaun> i'm not the most familiar with x11 protocol so didn't know 
that but after noticing that effect i adjusted my flow slightly and never had a 
issue since
[31 21:52] <geekosaur> clients respond to the ad by sending a selection request 
saying "I'd like your selection in one of these formats" and then they haggle 
until they either settle on a mutually acceptable format or abort by not 
responding
[31 21:54] <geekosaur> nobody actually uses it this way, which is why you get 
copy image location / copy image in a browser instead of just a copy image and 
if you then paste it as a string you get the location
[31 21:54] <geekosaur> people don't think the way X11 selections were designed
[31 21:55] <pharaun> that seems unfortunate
[31 21:55] <pharaun> because it seems like it would be neat esp being able to 
haggle which format they want
[31 21:57] <geekosaur> the envisioned behavior was that when you go to paste 
it, it can display a menu of various formats you might want, including multiple 
string formats (so, for an HTML editor, you might want to pick between an image 
URL vs. a prebuilt <img> link)
[31 21:57] <geekosaur> ideas that never happened
[31 21:57] <geekosaur> (like Windows' "Paste Special" only done right)
[31 22:07] <pharaun> oh yeah windows paste special, i remember that
[31 22:07] <pharaun> i wonder how come it never happened?
[31 22:07] <pharaun> too complicated?
[31 22:08] <geekosaur> like I said, nobody understood it
[31 22:08] <pharaun> :\
[31 22:09] <geekosaur> people write code like XMonad.Util.XSelection that 
completely do not understand how selections work and therefore can't imagine 
those possibilities
[31 22:09] <pharaun> :\
[31 22:10] <geekosaur> people think of selections as mailboxes, not as 
haggling. the thing in the mailbox can't change form for whatever picks it up, 
it's fixed when it's mailed
[31 22:11] <pharaun> ahhh
[31 22:11] <pharaun> looking at that code now, its yeah ok
[31 22:12] <geekosaur> I don't think it's too complicated really, it's just 
that nobody thinks about it right, they expect mailboxes so that's what they 
end up with
[31 22:13] <pharaun> heh and if everyone is going to think like that, you're 
not going to be able to do the haggling anyway
[31 22:13] <geekosaur> which might be a documentation problem, maybe on the 
part of the toolkit folks --- I have not looked at how gtk handles selections 
but I bet it uses that mailbox model
[31 22:13] <pharaun> ahh
[31 22:14] <geekosaur> especially since CLIPBOARD can't seem to decide which it 
is --- it does some things like a mailbox and some like haggling
[31 22:14] <geekosaur> whereas PRIMARY is a haggling model

Original comment by allber...@gmail.com on 1 Jun 2014 at 12:05

[GoogleCodeExporter]

Alright; I've pushed a patch fixing the closeDisplay issue and adding a warning 
to the documentation.

Original comment by gwe...@gmail.com on 1 Jun 2014 at 2:52