[PATCH] Don't free font closures prematurely (#31501)

[Julien Cristau]

On Fri, Jul 29, 2011 at 12:31:12 -0700, James Jones wrote:

> This patch isn't a correct fix.  It just avoids the crash.  I wish I could
> remember why.  There was some discussion about this on IRC between Adam
> Jackson and I a long time ago that basically concluded the correct fix is
> "really hard."  If anyone keeps really old IRC logs around (around 8 or 9
> months back), that would have made a good update in the bug report.  Sorry
> I didn't follow up.
> 
from Dec 10, 2010:

< ajax> oof.
< ajax> the the font server code is quite possibly my new antifavorite
< jamesjones> ajax: Did that patch I attached to the font loading crash bug look at all correct?
< ajax> oh man, i seriously just finished writing a patch for that
< ajax> if you beat me to it i'm going to feel very lame
< jamesjones> sorry
< jamesjones> I'm not at all sure it's correct.
< ajax> oh wow, i see what you did.
< jamesjones> It seems like the whole idea of bailing out if you need to sleep ruins the point of Xinerama looping
< ajax> yeah, i don't think that's right
< ajax> one second...
< jamesjones> but I was too lazy to dig deeper since that patch was enough to unblock my testing.
< ajax> jamesjones: give this one a try? http://fpaste.org/BqJf/raw/
< ajax> i'm a bit ill at how that whole thing works, honestly.
< jamesjones> Give me a second, rebuilding
< jamesjones> But yeah, it doesn't make sense to me.
< ajax> at any rate it fixes my testcase of xset fp+ unix/:7100 && xlsfonts
< jturney> heh, it removes code, so it must be right :)
< jamesjones> Hehe
< ajax> nothing like having a library where your app has to implement functions with particular names and signatures to work
< jamesjones> but why the looping, if it's OK to return Success when the operation isn't done on screen[n] where n>0?
< ajax> jamesjones: it's not quite that.
< ajax> most of the font ops don't have any screen state, so you only have to call them once.
< ajax> PolyText and ImageText do have to hit every screen, so you want to make sure that they only bump the client's sleep count once if they do sleep
< ajax> so the minimal thing would have been to only change the closures for {Poly,Image}Text to not keep a sleep count
< ajax> but if you're doing that you may as well do them all
< jamesjones> right, I figured it was something like that, but then 1) Why loop the others 2) Why is it OK to early-out in PolyText/ImageText?  Does the looping get restarted somewhere I'm not seeing when the client wakes up?
< jamesjones> I suppose I should just load up xinerama and step through it
< ajax> the others aren't actually looped by xinerama
< ajax> (which i had missed the first time around, i just assumed they were)
< jamesjones> ah, k
< jamesjones> that makes more sense.
< ajax> the text rendering routines push themselves onto the work queue if they don't have a font they need because the font server hasn't gotten back to them
< ajax> check out the context around ClientSleep in doImageText, it's quite horrific
< ajax> the way they wake up is that the font server's fd selects readable, and the handler for that notices which request tat reply was for and pokes the client back into the work queue
< ajax> i think that's right anyway?  i kind of want to not know.
< jamesjones> it sounds right if there's only one screen
< ajax> well, if there's multiple screens, you might run through the sleep pattern multiple times
< jamesjones> Yeah, but it looks like each screen would loop in and block, only sleeping once
< jamesjones> then when it woke up, only the first screen would be serviced.
< jamesjones> *loop in and detect the need to sleep rather
< ajax> ew.  yeah.
< ajax> okay, so
< jamesjones> Also, it seems like with your patch, those other screens would leak their closure structs
< ajax> i think the way to fix that is to fix PanoramiX*Text* check for sleep too
< jamesjones> which is what I was attempting to avoid in mine.
< jamesjones> Yeah, that sounds like it has a better chance at correctness.
< jamesjones> Or I was thinking something like looping at a lower level, so there would be xinerama data in teh closure, but that sounded like it would violate all kinds of design stuff
< jamesjones> Your patch does avoid the crash I was seeing, FWIW
< ajax> progress
< ajax> thanks for testing.  i'll fix the xinerama thing and send the patches off to the list
< jamesjones> np, sounds good
< ajax> every once in a while i dream about working in a language where closures aren't something you have to hand roll every time
< jamesjones> If there weren't a bunch entrenched old languages I happen to know, I'd have to do something creative with my time.  Couldn't have that.
< ajax> aaaaaargh
< ajax> if you change PanoramiX*Text* to be restartable then there's a race between each screen where some other client on the same drawable or gc could modify your state
< ajax> so there goes the atomicity invariant

Cheers,
Julien