Bug 1168

As long as GetWindowPixmap returns NULL for unrealized windows, the damage code
should operate correctly even in this case.  However, we can easily
short-circuite the code in the unrealized window case with a simple patch.  Can
you test the diff I've attached?

Created attachment 715 [details] [review]
Check for unviewable windows

patch to skip unviewable windows

Created attachment 721 [details] [review]
Check for unviewable windows in getDrawableDamageRef()

The previously proposed patch for unviewable windows does not fix the crash
although it almost does. This is because the crash is in checkGCDamage(), which
uses getDrawableDamage() and not 
getWindowDamage(). The patch attached here does fix the crash. I think this is
how we should go,  modulo any code style changes you might prefer.

You are right that the interaction problem is that Rootless does not set the
window pixmap to NULL for 
unrealized windows. Glancing at the code it appears that if it did set these to
NULL, Damage would have 
fallen back to the screen pixmap and been happy. Changing Rootless in this way
is likely to be higher 
risk/impact, however.

Keith, is the patch suggested by Torrey correct/sufficient?

No, the suggested patch isn't correct.  DamageRegister will do "bad things" as
all unviewable windows will end up getting Damage structures hooked up to the
global variable.

I don't have a good suggestion at this point; Damage expects that
GetWindowPixmap will either return legit data or NULL; there isn't any way to
avoid calling it and still have the Damage structures connected to the right place.

(In reply to comment #5)
> I don't have a good suggestion at this point

Okay, since we are close to the release (Tuesday), we need to decide if this is
truly a blocker that must hold up the release or if we can go ahead with the
release as is and fix it later.

I can't see how to fix this in Damage, which means any fix would be in the
rootless code.  It seems too close to the release to try and fix that in scary
ways at the moment.  I suggest we disable Damage and Composite on any X servers
using the rootless layer for this release and plan on placing the
simple-but-scary fix in the rootless code after the release.

Yes, this bug is definitely a release blocker for any X servers using rootless in combination with 
damage. I don't mind disabling damage for rootless X servers for this release, but last I checked it 
looked like there were issues building without damage. I'll check....

Regardless of the short term resolution for this Damage-Rootless incompatibility, I want to clarify the 
issues so others can better understand them and/or correct my misunderstandings. The current 
rootless policy is to leave the window pixmap as rootless found it. When needed for a drawing 
operation, rootless asks the implementation what the pixmap should be. It stores the old pixmap 
(whatever it was) replaces it with the new pixmap and then lets drawing proceed. When drawing is 
finished, rootless puts the old pixmap back into place. Rootless does not care about what this "non-
drawing pixmap" is and never looks at it. At one point in development putting back the original pixmap 
was necessary, but it has been a long time now so I don't know if this is still true. It is also possible that 
non-Mac OS X rootless implementations could use this non-drawing pixmap for something so NULLing 
it out would be a bad idea. None of these are show stoppers for making the change Keith suggested, 
but it points out why it is not a good 11th hour change to make. As he mentioned, maybe after the 
release it would make sense.

On the other hand I would argue that after the release it would make more sense to have Damage not 
try to track unviewable windows at all. Requiring anything of unviewable windows does not seem like 
the best design. My patch intended to make Damage ignore unviewable windows. I see that it has side 
effects, but I think this is still a good goal. There are a number of other ways Damage could fairly easily 
be made to ignore unviewable windows. My patch just had the smallest line diff count. (Unfortunately it 
was too simple.) It is probably too late for even this kind of simple change to Damage, but it would be 
good to get consensus on what the appropriate strategy should be after the release.

Now I'm confused -- there is a putative underlying pixmap which should be able
to support Damage for unmapped windows, and you call SetWindowPixmap when you
'rootless-ify' windows which Damage will track and automatically migrate the
damage records to.

Why doesn't this work?  Is there just a tiny bug in the code which is supposed
to manage this underlying pixmap?

If we can't get this working, I suggest that we take your original patch and
enable it for rootless servers so that Damage will at least not crash.  Adding a
config file value to disable per-window pixmaps for unrealized windows seems
like a straightforward kludge-around for the moment.

Created attachment 798 [details] [review]
workaround patch

Keith, is this patch what you have in mind for the workaround?
If so, then we should probably go with it for this release.

>Now I'm confused -- there is a putative underlying pixmap which should be able
>to support Damage for unmapped windows, and you call SetWindowPixmap when you
>'rootless-ify' windows which Damage will track and automatically migrate the
>damage records to.

I'm not sure what the underlying pixmap is, rootless does not really care about it or create it. Rootless 
keeps around whatever is set by fb when windows are created. This is the screen pixmap? In rootless 
mode the screen pixmap is purposely tweaked to essentially not have any size. See 
RootlessUpdateScreenPixmap() in miext/rootless/rootlessScreen.c for details if you are curious. You 
don't want a big screen pixmap sucking up memory if you are never going to use it.

>Why doesn't this work?  Is there just a tiny bug in the code which is supposed
>to manage this underlying pixmap?

Good question. I don't know, but I do plan to look into this when I have time. FWIW, I did try the trivial 
change of setting all window pixmaps to NULL when I was not drawing to them and I got the following 
crash backtrace:

0   org.xfree86.XDarwin      	0x0005cd74 fbPutXYImage + 0x70
1   ???                      	0x007e4974 rlImageGlyphBlt + 0x2b0
2   org.xfree86.XDarwin      	0x001035dc miImageText8 + 0x70
3   org.xfree86.XDarwin      	0x000f74b4 damageImageText8 + 0xe0
4   org.xfree86.XDarwin      	0x0001a3f8 doImageText + 0x1fc
5   org.xfree86.XDarwin      	0x0001a514 ImageText + 0x74
6   org.xfree86.XDarwin      	0x0001fe4c ProcImageText8 + 0x198
7   org.xfree86.XDarwin      	0x0001b8e4 Dispatch + 0x228
8   org.xfree86.XDarwin      	0x000032f8 main + 0x5dc
9   org.xfree86.XDarwin      	0x0004a694 -[XServer run] + 0x74
10  com.apple.Foundation     	0x90a39b74 forkThreadForFunction + 0x6c
11  libSystem.B.dylib        	0x900246e8 _pthread_body + 0x28

The lack of RootlessImageText8 in the backtrace means the drawable was not a viewable window. I 
think that other layers need some trivial but real pixmap in place even when no drawing is going to 
take place. I don't know why this does not work for Damage. I can spend more time on this once the 
release is not breathing down our backs.

On today's release wranglers call, we decided to set a deadline for patches of
today at 6pm EDT.  If patches are available by then, are small and
self-contained, they can still be integreated into the release.  Otherwise, we
will move the remaining open bugs to the release notes bug 999 for documentation.

If it is possible to come up with a better solution today, that would be great;
however, if not, would the workaround patch that I added be sufficient for this
release?

Workaround patch checked in after Torrey reviewed it.
Closing.