I am still seeing crashes with the damage extension when using the generic rootless layer. (This bug is
related to #1032, which is also about damage-rootless incompatibility, but is a different problem with a
different cause.) Here is a sample back trace:
#0 0x001d4ca8 in damageCopyArea (pSrc=0x21ae000, pDst=0x5a94760, pGC=0x5a743a0, srcx=0,
srcy=0, width=5, height=15, dstx=4, dsty=2) at damage.c:744
#1 0x00038ce8 in ProcCopyArea (client=0x5a704f0) at dispatch.c:1758
#2 0x00034868 in Dispatch () at dispatch.c:455
#3 0x0000a624 in main (argc=3, argv=0xbffffb0c, envp=0xbffffb1c) at main.c:442
Notice that RootlessCopyArea() is not in the backtrace. This is because the destination drawable is an
unviewable window. Damage is crashing on checkGCDamage(pDst, pGC). Rootless does not do anything
with windows until they are realized. It looks like Damage is crashing because it is trying to find a valid
pixmap for an unrealized window. Should Damage be tracking unrealized windows? Any ideas on how
to resolve this?
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
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
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]
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
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
Workaround patch checked in after Torrey reviewed it.